Hi Laurent,
2012/7/19, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> 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@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
