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

[InKi Dae]

Hi Laurent,

2012/7/19, Laurent Pinchart <laurent.pinchart at 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 at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at 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.
>

Right, it's my missing point. this part will be fixed for merge window.

>> +
>> +			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 ?
>

I agree with you.

Thanks,
Inki Dae

>>  	return fb;
>>  }
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>