Re: [Intel-gfx] [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 05, 2023 at 12:52:12PM +0200, Javier Martinez Canillas wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 
> > Apparently drivers need to check all this stuff themselves, which for
> > most things makes sense I guess. And for everything else we luck out,
> > because modern distros stopped supporting any other fbdev drivers than
> > drm ones and I really don't want to argue anymore about who needs to
> > check stuff. Therefore fixing all this just for drm fbdev emulation is
> > good enough.
> >
> 
> Agreed.
> 
> > Note that var->active is not set or validated. This is just control
> > flow for fbmem.c and needs to be validated in there as needed.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> 
> [...]
> 
> >  
> > +static void __fill_var(struct fb_var_screeninfo *var,
> > +		       struct drm_framebuffer *fb)
> > +{
> > +	int i;
> > +
> > +	var->xres_virtual = fb->width;
> > +	var->yres_virtual = fb->height;
> > +	var->accel_flags = FB_ACCELF_TEXT;
> > +	var->bits_per_pixel = drm_format_info_bpp(fb->format, 0);
> > +
> > +	var->height = var->width = 0;
> > +	var->left_margin = var->right_margin = 0;
> > +	var->upper_margin = var->lower_margin = 0;
> > +	var->hsync_len = var->vsync_len = 0;
> > +	var->sync = var->vmode = 0;
> > +	var->rotate = 0;
> > +	var->colorspace = 0;
> > +	for (i = 0; i < 4; i++)
> > +		var->reserved[i] = 0;
> > +}
> > +
> >  /**
> >   * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var
> >   * @var: screeninfo to check
> > @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >  		return -EINVAL;
> >  	}
> >  
> > -	var->xres_virtual = fb->width;
> > -	var->yres_virtual = fb->height;
> > +	__fill_var(var, fb);
> > +
> 
> [...]
> 
> There is the following here (in latest drm-misc/drm-misc-next at least):
> 
> 	/*
> 	 * Changes struct fb_var_screeninfo are currently not pushed back
> 	 * to KMS, hence fail if different settings are requested.
> 	 */
> 	bpp = drm_format_info_bpp(format, 0);
> 	if (var->bits_per_pixel > bpp ||
> 	    var->xres > fb->width || var->yres > fb->height ||
> 	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> 		drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
> 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> 			  var->xres, var->yres, var->bits_per_pixel,
> 			  var->xres_virtual, var->yres_virtual,
> 			  fb->width, fb->height, bpp);
> 		return -EINVAL;
> 	}
> 
> but only the 'var->xres > fb->width || var->yres > fb->height' from the
> conditions checked could be false after your __fill_var() call above.
> 
> You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual >
> fb->width' and 'var->yres_virtual > fb->height' checks I believe since
> those will always be true.

The __fill_var is after this. I'm honestly not sure what the exact
semantics are supposed to be, but essentially if userspace asks for too
big virtual size, we reject it. And for anything else we then tell it
(with __fill_var) how big the actually available space is.

What I'm wondering now is whether too small x/yres won't lead to problems
of some sorts ... For multi-screen we set the virtual size to be big
enough for all crtc, and then just set x/yres to be the smallest output.
That way fbcon knows to only draw as much as is visible on all screens.
But if you then pan that too much, the bigger screens might not have a big
enough buffer anymore and things fail (but shouldn't).

Not sure how to fix that tbh.
-Daniel

> 
> > +	/*
> > +	 * fb_pan_display() validates this, but fb_set_par() doesn't and just
> > +	 * falls over. Note that __fill_var above adjusts y/res_virtual.
> > +	 */
> > +	if (var->yoffset > var->yres_virtual - var->yres ||
> > +	    var->xoffset > var->xres_virtual - var->xres)
> > +		return -EINVAL;
> > +
> > +	/* We neither support grayscale nor FOURCC (also stored in here). */
> > +	if (var->grayscale > 0)
> > +		return -EINVAL;
> > +
> > +	if (var->nonstd)
> > +		return -EINVAL;
> >  
> >  	/*
> >  	 * Workaround for SDL 1.2, which is known to be setting all pixel format
> > @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >  		drm_fb_helper_fill_pixel_fmt(var, format);
> >  	}
> >  
> 
> Other than what I mentioned, the patch makes sense to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch