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

[Javier Martinez Canillas <javierm@redhat.com>]

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.

> +	/*
> +	 * 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