this patch sets fixes some minor issues to exynos drm framebuffer module. when user sent wrong gem memory type, framebuffer size and so on to driver side, the driver has to check if those data are valid or not properly.
Thanks.
Inki Dae (4): drm/exynos: fixed a comment to gem size. drm/exynos: add packed_size not aligned in page unit. drm/exynos: check if gem type is valid or not for fb. drm/exynos: check if framebuffer and gem size are valid or not.
drivers/gpu/drm/exynos/exynos_drm_fb.c | 86 ++++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 + drivers/gpu/drm/exynos/exynos_drm_gem.h | 6 ++- 3 files changed, 91 insertions(+), 3 deletions(-)
Signed-off-by: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_gem.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h index 14d038b..085b2a5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h @@ -63,7 +63,8 @@ struct exynos_drm_gem_buf { * by user request or at framebuffer creation. * continuous memory region allocated by user request * or at framebuffer creation. - * @size: total memory size to physically non-continuous memory region. + * @size: size requested from user, in bytes and this size is aligned + * in page unit. * @flags: indicate memory type to allocated buffer and cache attruibute. * * P.S. this object would be transfered to user as kms_bo.handle so
this patch adds packed_size variable in exynos_drm_gem_obj struct and this variable is used to check for valid framebuffer and gem 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_gem.c | 2 ++ drivers/gpu/drm/exynos/exynos_drm_gem.h | 3 +++ 2 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 411d82b..94e8137 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -345,6 +345,7 @@ struct exynos_drm_gem_obj *exynos_drm_gem_create(struct drm_device *dev, { struct exynos_drm_gem_obj *exynos_gem_obj; struct exynos_drm_gem_buf *buf; + unsigned long packed_size = size; int ret;
if (!size) { @@ -369,6 +370,7 @@ struct exynos_drm_gem_obj *exynos_drm_gem_create(struct drm_device *dev, goto err_fini_buf; }
+ exynos_gem_obj->packed_size = packed_size; exynos_gem_obj->buffer = buf;
/* set memory type and cache attribute from user side. */ diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h index 085b2a5..1d80cb2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h @@ -65,6 +65,8 @@ struct exynos_drm_gem_buf { * or at framebuffer creation. * @size: size requested from user, in bytes and this size is aligned * in page unit. + * @packed_size: real size of the gem object, in bytes and + * this size isn't aligned in page unit. * @flags: indicate memory type to allocated buffer and cache attruibute. * * P.S. this object would be transfered to user as kms_bo.handle so @@ -74,6 +76,7 @@ struct exynos_drm_gem_obj { struct drm_gem_object base; struct exynos_drm_gem_buf *buffer; unsigned long size; + unsigned long packed_size; unsigned int flags; };
physically non-contiguous memory can't be used for framebuffer yet. so this patch checks if the gem memory type is valid or not for the framebuffer.
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 | 38 ++++++++++++++++++++++++++++++++ 1 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 4ccfe43..6aba1e5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -31,6 +31,7 @@ #include "drm_crtc_helper.h" #include "drm_fb_helper.h"
+#include "exynos_drm.h" #include "exynos_drm_drv.h" #include "exynos_drm_fb.h" #include "exynos_drm_gem.h" @@ -48,6 +49,22 @@ struct exynos_drm_fb { struct exynos_drm_gem_obj *exynos_gem_obj[MAX_FB_BUFFER]; };
+static int check_fb_gem_memory_type(struct drm_device *drm_dev, + struct exynos_drm_gem_obj *exynos_gem_obj) +{ + unsigned int flags; + + flags = exynos_gem_obj->flags; + + /* not support physically non-continuous memory for fb yet. TODO */ + if (IS_NONCONTIG_BUFFER(flags)) { + DRM_ERROR("cannot use this gem memory type for fb.\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); @@ -107,8 +124,17 @@ exynos_drm_framebuffer_init(struct drm_device *dev, struct drm_gem_object *obj) { struct exynos_drm_fb *exynos_fb; + struct exynos_drm_gem_obj *exynos_gem_obj; int ret;
+ exynos_gem_obj = to_exynos_gem_obj(obj); + + ret = check_fb_gem_memory_type(dev, exynos_gem_obj); + if (ret < 0) { + DRM_ERROR("cannot use this gem memory type for fb.\n"); + return ERR_PTR(-EINVAL); + } + exynos_fb = kzalloc(sizeof(*exynos_fb), GFP_KERNEL); if (!exynos_fb) { DRM_ERROR("failed to allocate exynos drm framebuffer\n"); @@ -155,6 +181,9 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, nr = exynos_drm_format_num_buffers(fb->pixel_format);
for (i = 1; i < nr; i++) { + struct exynos_drm_gem_obj *exynos_gem_obj; + int ret; + obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handles[i]); if (!obj) { @@ -163,6 +192,15 @@ exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv, return ERR_PTR(-ENOENT); }
+ exynos_gem_obj = to_exynos_gem_obj(obj); + + ret = check_fb_gem_memory_type(dev, exynos_gem_obj); + if (ret < 0) { + DRM_ERROR("cannot use this gem memory type for fb.\n"); + exynos_drm_fb_destroy(fb); + return ERR_PTR(ret); + } + exynos_fb->exynos_gem_obj[i] = to_exynos_gem_obj(obj); }
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.
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 | 48 ++++++++++++++++++++++++++++++- 1 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 6aba1e5..ce0f18d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -65,6 +65,45 @@ static int check_fb_gem_memory_type(struct drm_device *drm_dev, return 0; }
+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->width * ((fb->bits_per_pixel + 7) >> 3) + * 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); @@ -160,8 +199,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__);
@@ -204,6 +242,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; }
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; + + 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; }
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; }
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
dri-devel@lists.freedesktop.org
-
Inki Dae -
InKi Dae -
Laurent Pinchart