| Message ID | 20181221031053.240161-3-yuzhao@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/amd: fix race in page flip job | expand |
On 2018-12-21 4:10 a.m., Yu Zhao wrote: > When creating frame buffer, userspace may request to attach to a > previously allocated GEM object that is smaller than what GPU > requires. Validation must be done to prevent out-of-bound DMA, > which could not only corrupt memory but also reveal sensitive data. > > This fix is not done in a common code path because individual > driver might have different requirement. > > Signed-off-by: Yu Zhao <yuzhao@google.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index 755daa332f8a..bb48b016cc68 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -527,6 +527,7 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev, > struct drm_gem_object *obj; > struct amdgpu_framebuffer *amdgpu_fb; > int ret; > + int height; > struct amdgpu_device *adev = dev->dev_private; > int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0); > int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false); > @@ -550,6 +551,13 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev, > return ERR_PTR(-EINVAL); > } > > + height = ALIGN(mode_cmd->height, 8); > + if (obj->size < pitch * height) { > + dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n", > + pitch * height, obj->size); Same comment as patch 2, and maybe write "expecting >= %d but got %d" for clarity.
Hi Yu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yu-Zhao/drm-amd-fix-race-in-page-flip-job/20181222-072937
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=ia64
All warnings (new ones prefixed by >>):
In file included from include/linux/cdev.h:8,
from include/drm/drmP.h:36,
from drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:26:
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c: In function 'amdgpu_display_user_framebuffer_create':
>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:28: warning: format '%d' expects argument of type 'int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/device.h:1370:22: note: in definition of macro 'dev_fmt'
#define dev_fmt(fmt) fmt
^~~
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:3: note: in expansion of macro 'dev_err'
dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
^~~~~~~
vim +556 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
521
522 struct drm_framebuffer *
523 amdgpu_display_user_framebuffer_create(struct drm_device *dev,
524 struct drm_file *file_priv,
525 const struct drm_mode_fb_cmd2 *mode_cmd)
526 {
527 struct drm_gem_object *obj;
528 struct amdgpu_framebuffer *amdgpu_fb;
529 int ret;
530 int height;
531 struct amdgpu_device *adev = dev->dev_private;
532 int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
533 int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
534
535 if (mode_cmd->pitches[0] != pitch) {
536 dev_err(&dev->pdev->dev, "Invalid pitch: expecting %d but got %d\n",
537 pitch, mode_cmd->pitches[0]);
538 return ERR_PTR(-EINVAL);
539 }
540
541 obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
542 if (obj == NULL) {
543 dev_err(&dev->pdev->dev, "No GEM object associated to handle 0x%08X, "
544 "can't create framebuffer\n", mode_cmd->handles[0]);
545 return ERR_PTR(-ENOENT);
546 }
547
548 /* Handle is imported dma-buf, so cannot be migrated to VRAM for scanout */
549 if (obj->import_attach) {
550 DRM_DEBUG_KMS("Cannot create framebuffer from imported dma_buf\n");
551 return ERR_PTR(-EINVAL);
552 }
553
554 height = ALIGN(mode_cmd->height, 8);
555 if (obj->size < pitch * height) {
> 556 dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
557 pitch * height, obj->size);
558 return ERR_PTR(-EINVAL);
559 }
560
561 amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
562 if (amdgpu_fb == NULL) {
563 drm_gem_object_put_unlocked(obj);
564 return ERR_PTR(-ENOMEM);
565 }
566
567 ret = amdgpu_display_framebuffer_init(dev, amdgpu_fb, mode_cmd, obj);
568 if (ret) {
569 kfree(amdgpu_fb);
570 drm_gem_object_put_unlocked(obj);
571 return ERR_PTR(ret);
572 }
573
574 return &amdgpu_fb->base;
575 }
576
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Yu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc7 next-20181221]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yu-Zhao/drm-amd-fix-race-in-page-flip-job/20181222-072937
config: x86_64-randconfig-s5-12221153 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/linux/cdev.h:8:0,
from include/drm/drmP.h:36,
from drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:26:
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c: In function 'amdgpu_display_user_framebuffer_create':
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:28: warning: format '%d' expects argument of type 'int', but argument 4 has type 'size_t {aka long unsigned int}' [-Wformat=]
dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
^
include/linux/device.h:1370:22: note: in definition of macro 'dev_fmt'
#define dev_fmt(fmt) fmt
^~~
>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:3: note: in expansion of macro 'dev_err'
dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
^~~~~~~
vim +/dev_err +556 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
521
522 struct drm_framebuffer *
523 amdgpu_display_user_framebuffer_create(struct drm_device *dev,
524 struct drm_file *file_priv,
525 const struct drm_mode_fb_cmd2 *mode_cmd)
526 {
527 struct drm_gem_object *obj;
528 struct amdgpu_framebuffer *amdgpu_fb;
529 int ret;
530 int height;
531 struct amdgpu_device *adev = dev->dev_private;
532 int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
533 int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
534
535 if (mode_cmd->pitches[0] != pitch) {
536 dev_err(&dev->pdev->dev, "Invalid pitch: expecting %d but got %d\n",
537 pitch, mode_cmd->pitches[0]);
538 return ERR_PTR(-EINVAL);
539 }
540
541 obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
542 if (obj == NULL) {
543 dev_err(&dev->pdev->dev, "No GEM object associated to handle 0x%08X, "
544 "can't create framebuffer\n", mode_cmd->handles[0]);
545 return ERR_PTR(-ENOENT);
546 }
547
548 /* Handle is imported dma-buf, so cannot be migrated to VRAM for scanout */
549 if (obj->import_attach) {
550 DRM_DEBUG_KMS("Cannot create framebuffer from imported dma_buf\n");
551 return ERR_PTR(-EINVAL);
552 }
553
554 height = ALIGN(mode_cmd->height, 8);
555 if (obj->size < pitch * height) {
> 556 dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
557 pitch * height, obj->size);
558 return ERR_PTR(-EINVAL);
559 }
560
561 amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
562 if (amdgpu_fb == NULL) {
563 drm_gem_object_put_unlocked(obj);
564 return ERR_PTR(-ENOMEM);
565 }
566
567 ret = amdgpu_display_framebuffer_init(dev, amdgpu_fb, mode_cmd, obj);
568 if (ret) {
569 kfree(amdgpu_fb);
570 drm_gem_object_put_unlocked(obj);
571 return ERR_PTR(ret);
572 }
573
574 return &amdgpu_fb->base;
575 }
576
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index 755daa332f8a..bb48b016cc68 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -527,6 +527,7 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev, struct drm_gem_object *obj; struct amdgpu_framebuffer *amdgpu_fb; int ret; + int height; struct amdgpu_device *adev = dev->dev_private; int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0); int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false); @@ -550,6 +551,13 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev, return ERR_PTR(-EINVAL); } + height = ALIGN(mode_cmd->height, 8); + if (obj->size < pitch * height) { + dev_err(&dev->pdev->dev, "Invalid GEM size: expecting %d but got %d\n", + pitch * height, obj->size); + return ERR_PTR(-EINVAL); + } + amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL); if (amdgpu_fb == NULL) { drm_gem_object_put_unlocked(obj);
When creating frame buffer, userspace may request to attach to a previously allocated GEM object that is smaller than what GPU requires. Validation must be done to prevent out-of-bound DMA, which could not only corrupt memory but also reveal sensitive data. This fix is not done in a common code path because individual driver might have different requirement. Signed-off-by: Yu Zhao <yuzhao@google.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 ++++++++ 1 file changed, 8 insertions(+)