Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com --- drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
+ if (info->hsub == 0) { + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub); + return -EINVAL; + } + + if (info->vsub == 0) { + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub); + return -EINVAL; + } + for (i = 0; i < info->num_planes; i++) { unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i);
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
- for (i = 0; i < info->num_planes; i++) { unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i);
-- 1.8.3.1
On 10/28/2021 10:04 AM, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Adding the authors and reviewer of the patch in question to CC.
94b292b277343190175d39172c903c0c5fb814f1 drm: drm_fourcc: add NV15, Q410, Q401 YUV formats
Asking if you have any input on how to deal with hsub and vsub = zero?
The sanity checks of hsub and vsub in the proposed patch are similar to other error checking done in framebuffer_check() preceding the proposed patch.
Thank you, George
- for (i = 0; i < info->num_planes; i++) { unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i);
-- 1.8.3.1
Hi,
On Fri, Oct 29, 2021 at 09:15:28AM -0400, George Kennedy wrote:
Asking if you have any input on how to deal with hsub and vsub = zero?
That's just a straight mistake on those formats - they should be 1. My bad for not spotting it in review.
On the one hand, having formats in this table is a nice machine-readable way to describe them. On the other, as drm_fourcc is being used as the canonical repository for formats, including ones not used in DRM, we can end up with situations like this. (R10/R12 being another example of formats not used in DRM: 20211027233140.12268-1-laurent.pinchart@ideasonboard.com)
Thanks, -Brian
On 10/29/2021 10:14 AM, Brian Starkey wrote:
Hi,
On Fri, Oct 29, 2021 at 09:15:28AM -0400, George Kennedy wrote:
Asking if you have any input on how to deal with hsub and vsub = zero?
That's just a straight mistake on those formats - they should be 1. My bad for not spotting it in review.
On the one hand, having formats in this table is a nice machine-readable way to describe them. On the other, as drm_fourcc is being used as the canonical repository for formats, including ones not used in DRM, we can end up with situations like this. (R10/R12 being another example of formats not used in DRM: 20211027233140.12268-1-laurent.pinchart@ideasonboard.com)
Wondering if there is an alternate fix to the one proposed?
Thank you, George
Thanks, -Brian
On Thu, Nov 18, 2021 at 03:00:15PM -0500, George Kennedy wrote:
On 10/29/2021 10:14 AM, Brian Starkey wrote:
Hi,
On Fri, Oct 29, 2021 at 09:15:28AM -0400, George Kennedy wrote:
Asking if you have any input on how to deal with hsub and vsub = zero?
That's just a straight mistake on those formats - they should be 1. My bad for not spotting it in review.
On the one hand, having formats in this table is a nice machine-readable way to describe them. On the other, as drm_fourcc is being used as the canonical repository for formats, including ones not used in DRM, we can end up with situations like this. (R10/R12 being another example of formats not used in DRM: 20211027233140.12268-1-laurent.pinchart@ideasonboard.com)
Wondering if there is an alternate fix to the one proposed?
I think if the cost of defining formats correctly for everyone is that drm carries a bunch of nice machine-readable entries in its tables that it never uses, then we should do that. The very few bytes saved aren't worth any headaches (because on any soc system you anyway have tons more formats than what your driver is using). -Daniel
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest. -Daniel
- for (i = 0; i < info->num_planes; i++) { unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i);
-- 1.8.3.1
-- Ville Syrjälä Intel
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really ugly. -Daniel
On Fri, 19 Nov 2021, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really ugly.
Kbuild does have support for hostprogs which are typically used in the build. The obvious idea is to use that for code generation, but it would also be interesting to see how that could be used for compile-time evaluation of sorts. Kind of like compile-time selftests? And, of course, how badly that would be frowned upon.
git grep says there are only four hostprogs users in drivers/, so it certainly isn't a popularity contest winner. (One of them is "mkregtable" in radeon.)
BR, Jani.
On 11/19/2021 9:25 AM, Jani Nikula wrote:
On Fri, 19 Nov 2021, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
Do a sanity check on struct drm_format_info hsub and vsub values to avoid divide by zero.
Syzkaller reported a divide error in framebuffer_check() when the DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". fb_plane_width() uses hsub as a divisor and fb_plane_height() uses vsub as a divisor. These divisors need to be sanity checked for zero before use.
divide error: 0000 [#1] SMP KASAN NOPTI CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 Hardware name: Red Hat KVM, BIOS 1.13.0-2 RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 drivers/gpu/drm/drm_framebuffer.c:317
Call Trace: drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline] __se_sys_ioctl fs/ioctl.c:860 [inline] __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x44/0xae
Signed-off-by: George Kennedy george.kennedy@oracle.com
drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc..a146e4b 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, /* now let the driver pick its own format info */ info = drm_get_format_info(dev, r);
- if (info->hsub == 0) {
DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub);return -EINVAL;- }
- if (info->vsub == 0) {
DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub);return -EINVAL;- }
Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really ugly.
Kbuild does have support for hostprogs which are typically used in the build. The obvious idea is to use that for code generation, but it would also be interesting to see how that could be used for compile-time evaluation of sorts. Kind of like compile-time selftests? And, of course, how badly that would be frowned upon.
git grep says there are only four hostprogs users in drivers/, so it certainly isn't a popularity contest winner. (One of them is "mkregtable" in radeon.)
So, can someone suggest a fix? A cpp type of approach does not seem feasible.
Adding the sanity checks that are in the patch, which are similar to the sanity checks preceding them in framebuffer_check(), along with a self-test that ran through all the table entries, might address all the concerns brought up in this thread.
Thank you, George
BR, Jani.
On Mon, Nov 22, 2021 at 10:29:05AM -0500, George Kennedy wrote:
On 11/19/2021 9:25 AM, Jani Nikula wrote:
On Fri, 19 Nov 2021, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote: > Do a sanity check on struct drm_format_info hsub and vsub values to > avoid divide by zero. > > Syzkaller reported a divide error in framebuffer_check() when the > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses > vsub as a divisor. These divisors need to be sanity checked for > zero before use. > > divide error: 0000 [#1] SMP KASAN NOPTI > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 > Hardware name: Red Hat KVM, BIOS 1.13.0-2 > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 > drivers/gpu/drm/drm_framebuffer.c:317 > > Call Trace: > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:874 [inline] > __se_sys_ioctl fs/ioctl.c:860 [inline] > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > Signed-off-by: George Kennedy george.kennedy@oracle.com > --- > drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 07f5abc..a146e4b 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, > /* now let the driver pick its own format info */ > info = drm_get_format_info(dev, r); > + if (info->hsub == 0) { > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub); > + return -EINVAL; > + } > + > + if (info->vsub == 0) { > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub); > + return -EINVAL; > + } Looks like duct tape to me. I think we need to either fix those formats to have valid format info, or just revert the whole patch that added such broken things.
Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really ugly.
Kbuild does have support for hostprogs which are typically used in the build. The obvious idea is to use that for code generation, but it would also be interesting to see how that could be used for compile-time evaluation of sorts. Kind of like compile-time selftests? And, of course, how badly that would be frowned upon.
git grep says there are only four hostprogs users in drivers/, so it certainly isn't a popularity contest winner. (One of them is "mkregtable" in radeon.)
So, can someone suggest a fix? A cpp type of approach does not seem feasible.
Adding the sanity checks that are in the patch, which are similar to the sanity checks preceding them in framebuffer_check(), along with a self-test that ran through all the table entries, might address all the concerns brought up in this thread.
drm selftest sounds like a reasonable approach to me. -Daniel
On 11/25/2021 10:27 AM, Daniel Vetter wrote:
On Mon, Nov 22, 2021 at 10:29:05AM -0500, George Kennedy wrote:
On 11/19/2021 9:25 AM, Jani Nikula wrote:
On Fri, 19 Nov 2021, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote: > On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote: >> Do a sanity check on struct drm_format_info hsub and vsub values to >> avoid divide by zero. >> >> Syzkaller reported a divide error in framebuffer_check() when the >> DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via >> the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for >> the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". >> fb_plane_width() uses hsub as a divisor and fb_plane_height() uses >> vsub as a divisor. These divisors need to be sanity checked for >> zero before use. >> >> divide error: 0000 [#1] SMP KASAN NOPTI >> CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 >> Hardware name: Red Hat KVM, BIOS 1.13.0-2 >> RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] >> RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 >> drivers/gpu/drm/drm_framebuffer.c:317 >> >> Call Trace: >> drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 >> drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 >> drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 >> drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 >> vfs_ioctl fs/ioctl.c:51 [inline] >> __do_sys_ioctl fs/ioctl.c:874 [inline] >> __se_sys_ioctl fs/ioctl.c:860 [inline] >> __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 >> do_syscall_x64 arch/x86/entry/common.c:50 [inline] >> do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 >> entry_SYSCALL_64_after_hwframe+0x44/0xae >> >> Signed-off-by: George Kennedy george.kennedy@oracle.com >> --- >> drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >> index 07f5abc..a146e4b 100644 >> --- a/drivers/gpu/drm/drm_framebuffer.c >> +++ b/drivers/gpu/drm/drm_framebuffer.c >> @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, >> /* now let the driver pick its own format info */ >> info = drm_get_format_info(dev, r); >> + if (info->hsub == 0) { >> + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub); >> + return -EINVAL; >> + } >> + >> + if (info->vsub == 0) { >> + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub); >> + return -EINVAL; >> + } > Looks like duct tape to me. I think we need to either fix those formats > to have valid format info, or just revert the whole patch that added such > broken things. Yeah maybe even a compile-time check of the format table(s) to validate them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really ugly.
Kbuild does have support for hostprogs which are typically used in the build. The obvious idea is to use that for code generation, but it would also be interesting to see how that could be used for compile-time evaluation of sorts. Kind of like compile-time selftests? And, of course, how badly that would be frowned upon.
git grep says there are only four hostprogs users in drivers/, so it certainly isn't a popularity contest winner. (One of them is "mkregtable" in radeon.)
So, can someone suggest a fix? A cpp type of approach does not seem feasible.
Adding the sanity checks that are in the patch, which are similar to the sanity checks preceding them in framebuffer_check(), along with a self-test that ran through all the table entries, might address all the concerns brought up in this thread.
drm selftest sounds like a reasonable approach to me.
In the meantime, should a bugzilla bug be opened to track the issue? From this thread it does not seem as though there is a drm selftest in the works.
Thanks, George
-Daniel
On Thu, Dec 02, 2021 at 09:17:37AM -0500, George Kennedy wrote:
On 11/25/2021 10:27 AM, Daniel Vetter wrote:
On Mon, Nov 22, 2021 at 10:29:05AM -0500, George Kennedy wrote:
On 11/19/2021 9:25 AM, Jani Nikula wrote:
On Fri, 19 Nov 2021, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote: > On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote: > > > Do a sanity check on struct drm_format_info hsub and vsub values to > > > avoid divide by zero. > > > > > > Syzkaller reported a divide error in framebuffer_check() when the > > > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via > > > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for > > > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0". > > > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses > > > vsub as a divisor. These divisors need to be sanity checked for > > > zero before use. > > > > > > divide error: 0000 [#1] SMP KASAN NOPTI > > > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1 > > > Hardware name: Red Hat KVM, BIOS 1.13.0-2 > > > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline] > > > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90 > > > drivers/gpu/drm/drm_framebuffer.c:317 > > > > > > Call Trace: > > > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355 > > > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391 > > > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795 > > > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898 > > > vfs_ioctl fs/ioctl.c:51 [inline] > > > __do_sys_ioctl fs/ioctl.c:874 [inline] > > > __se_sys_ioctl fs/ioctl.c:860 [inline] > > > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860 > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80 > > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > > > Signed-off-by: George Kennedy george.kennedy@oracle.com > > > --- > > > drivers/gpu/drm/drm_framebuffer.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > > index 07f5abc..a146e4b 100644 > > > --- a/drivers/gpu/drm/drm_framebuffer.c > > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev, > > > /* now let the driver pick its own format info */ > > > info = drm_get_format_info(dev, r); > > > + if (info->hsub == 0) { > > > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n", info->hsub); > > > + return -EINVAL; > > > + } > > > + > > > + if (info->vsub == 0) { > > > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n", info->vsub); > > > + return -EINVAL; > > > + } > > Looks like duct tape to me. I think we need to either fix those formats > > to have valid format info, or just revert the whole patch that added such > > broken things. > Yeah maybe even a compile-time check of the format table(s) to validate > them properly and scream ... Or at least a selftest. I really wish C had (even very limited) compile time evaluation so one could actually loop over arrays like at compile time to check each element. As it stands you either have to check each array element by hand, or you do some cpp macro horrors to pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really ugly.
Kbuild does have support for hostprogs which are typically used in the build. The obvious idea is to use that for code generation, but it would also be interesting to see how that could be used for compile-time evaluation of sorts. Kind of like compile-time selftests? And, of course, how badly that would be frowned upon.
git grep says there are only four hostprogs users in drivers/, so it certainly isn't a popularity contest winner. (One of them is "mkregtable" in radeon.)
So, can someone suggest a fix? A cpp type of approach does not seem feasible.
Adding the sanity checks that are in the patch, which are similar to the sanity checks preceding them in framebuffer_check(), along with a self-test that ran through all the table entries, might address all the concerns brought up in this thread.
drm selftest sounds like a reasonable approach to me.
In the meantime, should a bugzilla bug be opened to track the issue? From this thread it does not seem as though there is a drm selftest in the works.
I think if you don't end up typing it, it's just not going to happen. We're not really doing a great job in tracking these kinds of tasks unfortunately :-/
Best we have is Documentation/gpu/todo.rst, but that's kinda high-overhead. -Daniel
dri-devel@lists.freedesktop.org
-
Brian Starkey -
Daniel Vetter -
George Kennedy -
Jani Nikula -
Simon Ser -
Ville Syrjälä