Re: [PATCH v2] drm/exynos: check if framebuffer and gem size are valid or not.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Inki,

On Monday 09 July 2012 14:23:23 Inki Dae wrote:
> with addfb request by user, wrong framebuffer or gem size could be sent
> to kernel side so this could induce invalid memory access by dma of a
> device. this patch checks if framebuffer and gem size are valid or not to
> avoid this issue.
> 
> Changelog v2:
> use fb->pitches instead of caculating it with fb->width and fb->bpp
> as line size.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c |   47 ++++++++++++++++++++++++++++-
>  1 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 4ccfe43..f1b1008 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -48,6 +48,44 @@ struct exynos_drm_fb {
>  	struct exynos_drm_gem_obj	*exynos_gem_obj[MAX_FB_BUFFER];
>  };
> 
> +static int check_fb_gem_size(struct drm_device *drm_dev,
> +				struct drm_framebuffer *fb,
> +				unsigned int nr)
> +{
> +	unsigned long fb_size;
> +	struct drm_gem_object *obj;
> +	struct exynos_drm_gem_obj *exynos_gem_obj;
> +	struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb);
> +
> +	/* in case of RGB format, only one plane is used. */
> +	if (nr < 2) {
> +		exynos_gem_obj = exynos_fb->exynos_gem_obj[0];
> +		obj = &exynos_gem_obj->base;
> +		fb_size = fb->pitches[0] * fb->height;
> +
> +		if (fb_size != exynos_gem_obj->packed_size) {
> +			DRM_ERROR("invalid fb or gem size.\n");
> +			return -EINVAL;
> +		}
> +	/* in case of NV12MT, YUV420M and so on, two and three planes. */
> +	} else {
> +		unsigned int i;
> +
> +		for (i = 0; i < nr; i++) {
> +			exynos_gem_obj = exynos_fb->exynos_gem_obj[i];
> +			obj = &exynos_gem_obj->base;
> +			fb_size = fb->pitches[i] * fb->height;

I think you need to take vertical chroma subsampling into account here, as 
well as fb->offsets[i]. See https://patchwork.kernel.org/patch/1147061/ for an 
ongoing discussion on this subject.

> +
> +			if (fb_size != exynos_gem_obj->packed_size) {
> +				DRM_ERROR("invalid fb or gem size.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void exynos_drm_fb_destroy(struct drm_framebuffer *fb)
>  {
>  	struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb);
> @@ -134,8 +172,7 @@ exynos_user_fb_create(struct drm_device *dev, struct
> drm_file *file_priv, struct drm_gem_object *obj;
>  	struct drm_framebuffer *fb;
>  	struct exynos_drm_fb *exynos_fb;
> -	int nr;
> -	int i;
> +	int nr, i, ret;
> 
>  	DRM_DEBUG_KMS("%s\n", __FILE__);
> 
> @@ -166,6 +203,12 @@ exynos_user_fb_create(struct drm_device *dev, struct
> drm_file *file_priv, exynos_fb->exynos_gem_obj[i] = to_exynos_gem_obj(obj);
>  	}
> 
> +	ret = check_fb_gem_size(dev, fb, nr);
> +	if (ret < 0) {
> +		exynos_drm_fb_destroy(fb);
> +		return ERR_PTR(ret);
> +	}
> +

What about checking the size before creating the frame buffer ?

>  	return fb;
>  }
-- 
Regards,

Laurent Pinchart