Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch