Re: [PATCH] drm: Reject unknown legacy bpp and depth for drm_mode_addfb ioctl

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 05, 2018 at 11:22:05AM +0100, Chris Wilson wrote:
> Since this is handling user provided bpp and depth, we need to sanity
> check and propagate the EINVAL back rather than assume what the insane
> client intended and fill the logs with DRM_ERROR.
> 
> v2: Check both bpp and depth match the builtin pixel format, and
> introduce a canonical DRM_FORMAT_INVALID to reserve 0 against any future
> fourcc.
> 
> Testcase: igt/kms_addfb_basic/legacy-format
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

I checked a bunch of randomly selected userspace pieces, and if there's
hairy stuff going on then it's mis-selected bpp.
> ---
>  drivers/gpu/drm/drm_fourcc.c      | 33 +++++++++++++++++++++----------
>  drivers/gpu/drm/drm_framebuffer.c |  7 ++++++-
>  include/uapi/drm/drm_fourcc.h     |  3 +++
>  3 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 35c1e2742c27..d9dadbc43327 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -45,32 +45,45 @@ static char printable_char(int c)
>   */
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  {
> -	uint32_t fmt;
> +	uint32_t fmt = DRM_FORMAT_INVALID;
>  
>  	switch (bpp) {
>  	case 8:
> -		fmt = DRM_FORMAT_C8;
> +		if (depth == 0)
> +			fmt = DRM_FORMAT_C8;

Michel Dänzer thinks this should be depth == 8 here. I grepped around in
-modesetting, seems outright not supported. Apparently amd drivers do
support it, and set 8/8. With that addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


>  		break;
>  	case 16:
> -		if (depth == 15)
> +		switch (depth) {
> +		case 15:
>  			fmt = DRM_FORMAT_XRGB1555;
> -		else
> +			break;
> +		case 16:
>  			fmt = DRM_FORMAT_RGB565;
> +			break;
> +		default:
> +			break;
> +		}
>  		break;
>  	case 24:
> -		fmt = DRM_FORMAT_RGB888;
> +		if (depth == 24)
> +			fmt = DRM_FORMAT_RGB888;
>  		break;
>  	case 32:
> -		if (depth == 24)
> +		switch (depth) {
> +		case 24:
>  			fmt = DRM_FORMAT_XRGB8888;
> -		else if (depth == 30)
> +			break;
> +		case 30:
>  			fmt = DRM_FORMAT_XRGB2101010;
> -		else
> +			break;
> +		case 32:
>  			fmt = DRM_FORMAT_ARGB8888;
> +			break;
> +		default:
> +			break;
> +		}
>  		break;
>  	default:
> -		DRM_ERROR("bad bpp, assuming x8r8g8b8 pixel format\n");
> -		fmt = DRM_FORMAT_XRGB8888;
>  		break;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 781af1d42d76..636f626c5828 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -112,12 +112,17 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
>  	struct drm_mode_fb_cmd2 r = {};
>  	int ret;
>  
> +	r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
> +	if (r.pixel_format == DRM_FORMAT_INVALID) {
> +		DRM_DEBUG("bad (bpp:%d, depth:%d)\n", or->bpp, or->depth);
> +		return -EINVAL;
> +	}
> +
>  	/* convert to new format and call new ioctl */
>  	r.fb_id = or->fb_id;
>  	r.width = or->width;
>  	r.height = or->height;
>  	r.pitches[0] = or->pitch;
> -	r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
>  	r.handles[0] = or->handle;
>  
>  	if (r.pixel_format == DRM_FORMAT_XRGB2101010 &&
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 2ed46e9ae16a..139632b87181 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -71,6 +71,9 @@ extern "C" {
>  
>  #define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */
>  
> +/* Reserve 0 for the invalid format specifier */
> +#define DRM_FORMAT_INVALID	0
> +
>  /* color index */
>  #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>  
> -- 
> 2.19.0.rc1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx