diff mbox

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

Message ID 1341811403-4642-1-git-send-email-inki.dae@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Inki Dae July 9, 2012, 5:23 a.m. UTC 
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(-)

Comments

Laurent Pinchart July 19, 2012, 1:49 p.m. UTC | #1 
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;
>  }
Inki Dae July 20, 2012, 7:28 a.m. UTC | #2 
Hi Laurent,

2012/7/19, 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.
>

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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
diff mbox

Patch

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;
+
+			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);
+	}
+
 	return fb;
 }