Messages in this thread | | | Date | Mon, 22 Nov 2021 10:29:05 -0500 | Subject | Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero | From | George Kennedy <> |
| |
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. >
| |