[PATCH 1/3] drm/amd: fix race in page flip job

[PATCH 1/3] drm/amd: fix race in page flip job

[Yu Zhao]

Fix race between page flip job submission and completion. We invoke
page_flip callback to submit page flip job to GPU first and then set
pflip_status. If GPU fires page flip done irq in between, its handler
won't see the correct pflip_status thus will refuse to notify the job
completion. The job will eventually times out.

Reverse the order of calling page_flip and setting pflip_status to
fix the race.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 686a26de50f9..e309d26170db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -105,11 +105,11 @@ static void amdgpu_display_flip_work_func(struct work_struct *__work)
 	/* We borrow the event spin lock for protecting flip_status */
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 
+	/* Set the flip status */
+	amdgpu_crtc->pflip_status = AMDGPU_FLIP_SUBMITTED;
 	/* Do the flip (mmio) */
 	adev->mode_info.funcs->page_flip(adev, work->crtc_id, work->base, work->async);
 
-	/* Set the flip status */
-	amdgpu_crtc->pflip_status = AMDGPU_FLIP_SUBMITTED;
 	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 
 
-- 
2.20.1.415.g653613c723-goog

[PATCH 2/3] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

For GPU that does frame buffer compression, DMA writing out of bound
memory will cause memory corruption.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index e309d26170db..755daa332f8a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	struct amdgpu_framebuffer *amdgpu_fb;
 	int ret;
+	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);
+
+	if (mode_cmd->pitches[0] != pitch) {
+		dev_err(&dev->pdev->dev, "Invalid pitch: expecting %d but got %d\n",
+			pitch, mode_cmd->pitches[0]);
+		return ERR_PTR(-EINVAL);
+	}
 
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

[PATCH 3/3] drm/amd: validate user GEM object size

[Yu Zhao]

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);
+		return ERR_PTR(-EINVAL);
+	}
+
 	amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
 	if (amdgpu_fb == NULL) {
 		drm_gem_object_put_unlocked(obj);
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH 1/3] drm/amd: fix race in page flip job

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Fix race between page flip job submission and completion. We invoke
> page_flip callback to submit page flip job to GPU first and then set
> pflip_status. If GPU fires page flip done irq in between, its handler
> won't see the correct pflip_status thus will refuse to notify the job
> completion. The job will eventually times out.
> 
> Reverse the order of calling page_flip and setting pflip_status to
> fix the race.

There is no race, amdgpu_crtc->pflip_status is protected by dev->event_lock.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH 1/3] drm/amd: fix race in page flip job

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Fix race between page flip job submission and completion. We invoke
> page_flip callback to submit page flip job to GPU first and then set
> pflip_status. If GPU fires page flip done irq in between, its handler
> won't see the correct pflip_status thus will refuse to notify the job
> completion. The job will eventually times out.
> 
> Reverse the order of calling page_flip and setting pflip_status to
> fix the race.

There is no race, amdgpu_crtc->pflip_status is protected by dev->event_lock.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> For GPU that does frame buffer compression, DMA writing out of bound
> memory will cause memory corruption.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index e309d26170db..755daa332f8a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	struct amdgpu_framebuffer *amdgpu_fb;
>  	int ret;
> +	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);
> +
> +	if (mode_cmd->pitches[0] != pitch) {
> +		dev_err(&dev->pdev->dev, "Invalid pitch: expecting %d but got %d\n",
> +			pitch, mode_cmd->pitches[0]);

This message shouldn't be printed by default, or buggy / malicious
userspace can spam dmesg. I suggest using DRM_DEBUG_KMS or DRM_DEBUG_DRIVER.


> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
>  	if (obj ==  NULL) {
> 

Other than that, looks good to me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> For GPU that does frame buffer compression, DMA writing out of bound
> memory will cause memory corruption.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index e309d26170db..755daa332f8a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	struct amdgpu_framebuffer *amdgpu_fb;
>  	int ret;
> +	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);
> +
> +	if (mode_cmd->pitches[0] != pitch) {
> +		dev_err(&dev->pdev->dev, "Invalid pitch: expecting %d but got %d\n",
> +			pitch, mode_cmd->pitches[0]);

This message shouldn't be printed by default, or buggy / malicious
userspace can spam dmesg. I suggest using DRM_DEBUG_KMS or DRM_DEBUG_DRIVER.


> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
>  	if (obj ==  NULL) {
> 

Other than that, looks good to me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> For GPU that does frame buffer compression, DMA writing out of bound
> memory will cause memory corruption.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index e309d26170db..755daa332f8a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	struct amdgpu_framebuffer *amdgpu_fb;
>  	int ret;
> +	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);

Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
otherwise it'll spuriously fail for larger but well-aligned pitches.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> For GPU that does frame buffer compression, DMA writing out of bound
> memory will cause memory corruption.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index e309d26170db..755daa332f8a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	struct amdgpu_framebuffer *amdgpu_fb;
>  	int ret;
> +	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);

Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
otherwise it'll spuriously fail for larger but well-aligned pitches.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Yu Zhao]

On Fri, Dec 21, 2018 at 10:07:26AM +0100, Michel Dänzer wrote:
> On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> > Userspace may request pitch alignment that is not supported by GPU.
> > Some requests 32, but GPU ignores it and uses default 64 when cpp is
> > 4. If GEM object is allocated based on the smaller alignment, GPU
> > DMA will go out of bound.
> > 
> > For GPU that does frame buffer compression, DMA writing out of bound
> > memory will cause memory corruption.
> > 
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index e309d26170db..755daa332f8a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
> >  	struct drm_gem_object *obj;
> >  	struct amdgpu_framebuffer *amdgpu_fb;
> >  	int ret;
> > +	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);
> 
> Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
> otherwise it'll spuriously fail for larger but well-aligned pitches.

Good point, thanks.

[PATCH v2 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

For GPU that does frame buffer compression, DMA writing out of bound
memory will cause memory corruption.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 686a26de50f9..883a4df2386d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	struct amdgpu_framebuffer *amdgpu_fb;
 	int ret;
+	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->pitches[0], cpp, false);
+
+	if (mode_cmd->pitches[0] != pitch) {
+		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
+			      pitch, mode_cmd->pitches[0]);
+		return ERR_PTR(-EINVAL);
+	}
 
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

[PATCH v2 2/2] drm/amd: validate user GEM object size

[Yu Zhao]

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 883a4df2386d..098d6a816ee1 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->pitches[0], 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) {
+		DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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);
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3887 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52395 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29156 bytes --]

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29156 bytes --]

[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/2] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]

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-validate-user-pitch-alignment/20181222-153630
config: i386-allmodconfig (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=i386 

All warnings (new ones prefixed by >>):

   In file included from include/drm/drm_mm.h:49:0,
                    from include/drm/drmP.h:73,
                    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:17: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
      DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\n",
                    ^
             pitch * height, obj->size);
                             ~~~~~~
   include/drm/drm_print.h:362:22: note: in definition of macro 'DRM_DEBUG_KMS'
     drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
                         ^~~

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->pitches[0], cpp, false);
   534	
   535		if (mode_cmd->pitches[0] != pitch) {
   536			DRM_DEBUG_KMS("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			DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66078 bytes --]

Re: [PATCH v2 2/2] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]

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-validate-user-pitch-alignment/20181222-153630
config: i386-allmodconfig (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=i386 

All warnings (new ones prefixed by >>):

   In file included from include/drm/drm_mm.h:49:0,
                    from include/drm/drmP.h:73,
                    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:17: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
      DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\n",
                    ^
             pitch * height, obj->size);
                             ~~~~~~
   include/drm/drm_print.h:362:22: note: in definition of macro 'DRM_DEBUG_KMS'
     drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
                         ^~~

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->pitches[0], cpp, false);
   534	
   535		if (mode_cmd->pitches[0] != pitch) {
   536			DRM_DEBUG_KMS("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			DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66078 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

For GPU that does frame buffer compression, DMA writing out of bound
memory will cause memory corruption.

Cc: stable@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 686a26de50f9..883a4df2386d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	struct amdgpu_framebuffer *amdgpu_fb;
 	int ret;
+	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->pitches[0], cpp, false);
+
+	if (mode_cmd->pitches[0] != pitch) {
+		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
+			      pitch, mode_cmd->pitches[0]);
+		return ERR_PTR(-EINVAL);
+	}
 
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

[PATCH v3 2/2] drm/amd: validate user GEM object size

[Yu Zhao]

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.

Cc: stable@vger.kernel.org # v4.2+
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 883a4df2386d..a58fb8e021c6 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->pitches[0], 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) {
+		DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %zu\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);
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH v2 2/2] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

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-validate-user-pitch-alignment/20181222-153630
config: i386-randconfig-h1-12231406 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/gpu//drm/amd/amdgpu/amdgpu_display.c: In function 'amdgpu_display_user_framebuffer_create':
>> drivers/gpu//drm/amd/amdgpu/amdgpu_display.c:556:3: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' [-Wformat=]
      DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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->pitches[0], cpp, false);
   534	
   535		if (mode_cmd->pitches[0] != pitch) {
   536			DRM_DEBUG_KMS("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			DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30056 bytes --]

Re: [PATCH v2 2/2] drm/amd: validate user GEM object size

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

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-validate-user-pitch-alignment/20181222-153630
config: i386-randconfig-h1-12231406 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/gpu//drm/amd/amdgpu/amdgpu_display.c: In function 'amdgpu_display_user_framebuffer_create':
>> drivers/gpu//drm/amd/amdgpu/amdgpu_display.c:556:3: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' [-Wformat=]
      DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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->pitches[0], cpp, false);
   534	
   535		if (mode_cmd->pitches[0] != pitch) {
   536			DRM_DEBUG_KMS("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			DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30056 bytes --]

[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Yu Zhao]

On Fri, Dec 21, 2018 at 10:07:26AM +0100, Michel Dänzer wrote:
> On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> > Userspace may request pitch alignment that is not supported by GPU.
> > Some requests 32, but GPU ignores it and uses default 64 when cpp is
> > 4. If GEM object is allocated based on the smaller alignment, GPU
> > DMA will go out of bound.
> > 
> > For GPU that does frame buffer compression, DMA writing out of bound
> > memory will cause memory corruption.
> > 
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index e309d26170db..755daa332f8a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
> >  	struct drm_gem_object *obj;
> >  	struct amdgpu_framebuffer *amdgpu_fb;
> >  	int ret;
> > +	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);
> 
> Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
> otherwise it'll spuriously fail for larger but well-aligned pitches.

Actually mode_cmd->pitches[0] is aligned mode_cmd->width multiplied
by cpp. So we can't just use mode_cmd->pitches[0]. And I'm not sure
if the hardware works with larger alignment (it certainly ignores
smaller alignment).

[PATCH v4 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

For GPU that does frame buffer compression, DMA writing out of bound
memory will cause memory corruption.

Cc: stable@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 686a26de50f9..af0626a2b528 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	struct amdgpu_framebuffer *amdgpu_fb;
 	int ret;
+	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);
+
+	if (mode_cmd->pitches[0] != pitch) {
+		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
+			      pitch, mode_cmd->pitches[0]);
+		return ERR_PTR(-EINVAL);
+	}
 
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

[PATCH v4 2/2] drm/amd: validate user GEM object size

[Yu Zhao]

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.

Cc: stable@vger.kernel.org # v4.2+
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 af0626a2b528..9aa23cb20873 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) {
+		DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %zu\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);
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-23 10:44 p.m., Yu Zhao wrote:
> On Fri, Dec 21, 2018 at 10:07:26AM +0100, Michel Dänzer wrote:
>> On 2018-12-21 4:10 a.m., Yu Zhao wrote:
>>> Userspace may request pitch alignment that is not supported by GPU.
>>> Some requests 32, but GPU ignores it and uses default 64 when cpp is
>>> 4. If GEM object is allocated based on the smaller alignment, GPU
>>> DMA will go out of bound.
>>>
>>> For GPU that does frame buffer compression, DMA writing out of bound
>>> memory will cause memory corruption.
>>>
>>> Signed-off-by: Yu Zhao <yuzhao@google.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index e309d26170db..755daa332f8a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>>>  	struct drm_gem_object *obj;
>>>  	struct amdgpu_framebuffer *amdgpu_fb;
>>>  	int ret;
>>> +	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);
>>
>> Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
>> otherwise it'll spuriously fail for larger but well-aligned pitches.
> 
> Actually mode_cmd->pitches[0] is aligned mode_cmd->width multiplied
> by cpp. So we can't just use mode_cmd->pitches[0].

Use mode_cmd->pitches[0] / cpp then?


> And I'm not sure if the hardware works with larger alignment

It does.

> (it certainly ignores smaller alignment).

Yeah, pitch must be >= width. Maybe this patch could check that as well.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-23 10:44 p.m., Yu Zhao wrote:
> On Fri, Dec 21, 2018 at 10:07:26AM +0100, Michel Dänzer wrote:
>> On 2018-12-21 4:10 a.m., Yu Zhao wrote:
>>> Userspace may request pitch alignment that is not supported by GPU.
>>> Some requests 32, but GPU ignores it and uses default 64 when cpp is
>>> 4. If GEM object is allocated based on the smaller alignment, GPU
>>> DMA will go out of bound.
>>>
>>> For GPU that does frame buffer compression, DMA writing out of bound
>>> memory will cause memory corruption.
>>>
>>> Signed-off-by: Yu Zhao <yuzhao@google.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index e309d26170db..755daa332f8a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>>>  	struct drm_gem_object *obj;
>>>  	struct amdgpu_framebuffer *amdgpu_fb;
>>>  	int ret;
>>> +	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);
>>
>> Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
>> otherwise it'll spuriously fail for larger but well-aligned pitches.
> 
> Actually mode_cmd->pitches[0] is aligned mode_cmd->width multiplied
> by cpp. So we can't just use mode_cmd->pitches[0].

Use mode_cmd->pitches[0] / cpp then?


> And I'm not sure if the hardware works with larger alignment

It does.

> (it certainly ignores smaller alignment).

Yeah, pitch must be >= width. Maybe this patch could check that as well.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

Cc: stable@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 15ce7e681d67..16af80ccd0a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,22 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	struct amdgpu_framebuffer *amdgpu_fb;
 	int ret;
+	struct amdgpu_device *adev = dev->dev_private;
+	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
+	int pitch = mode_cmd->pitches[0] / cpp;
+
+	if (pitch < mode_cmd->width) {
+		DRM_DEBUG_KMS("Expecting pitch(%d)/cpp(%d) >= width(%d)\n",
+			      mode_cmd->pitches[0], cpp, mode_cmd->width);
+		return ERR_PTR(-EINVAL);
+	}
+
+	pitch = amdgpu_align_pitch(adev, pitch, cpp, false);
+	if (mode_cmd->pitches[0] != pitch) {
+		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
+			      pitch, mode_cmd->pitches[0]);
+		return ERR_PTR(-EINVAL);
+	}
 
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

[PATCH v5 2/2] drm/amd: validate user GEM object size

[Yu Zhao]

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,
otherwise it could be exploited to reveal sensitive data.

This fix is not done in a common code path because individual
driver might have different requirement.

Cc: stable@vger.kernel.org # v4.2+
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 16af80ccd0a0..61c075a78ee1 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 = mode_cmd->pitches[0] / cpp;
@@ -557,6 +558,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) {
+		DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %zu\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);
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH v5 1/2] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-30 2:00 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> Cc: stable@vger.kernel.org # v4.2+
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 15ce7e681d67..16af80ccd0a0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,22 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	struct amdgpu_framebuffer *amdgpu_fb;
>  	int ret;
> +	struct amdgpu_device *adev = dev->dev_private;
> +	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> +	int pitch = mode_cmd->pitches[0] / cpp;
> +
> +	if (pitch < mode_cmd->width) {
> +		DRM_DEBUG_KMS("Expecting pitch(%d)/cpp(%d) >= width(%d)\n",
> +			      mode_cmd->pitches[0], cpp, mode_cmd->width);
> +		return ERR_PTR(-EINVAL);
> +	}

This should possibly be tested in drm_internal_framebuffer_create instead.


> +	pitch = amdgpu_align_pitch(adev, pitch, cpp, false);
> +	if (mode_cmd->pitches[0] != pitch) {
> +		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
> +			      pitch, mode_cmd->pitches[0]);
> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
>  	if (obj ==  NULL) {
> 

Apart from the above, the v5 series looks good to me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH v5 1/2] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-30 2:00 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> Cc: stable@vger.kernel.org # v4.2+
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 15ce7e681d67..16af80ccd0a0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,22 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	struct amdgpu_framebuffer *amdgpu_fb;
>  	int ret;
> +	struct amdgpu_device *adev = dev->dev_private;
> +	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> +	int pitch = mode_cmd->pitches[0] / cpp;
> +
> +	if (pitch < mode_cmd->width) {
> +		DRM_DEBUG_KMS("Expecting pitch(%d)/cpp(%d) >= width(%d)\n",
> +			      mode_cmd->pitches[0], cpp, mode_cmd->width);
> +		return ERR_PTR(-EINVAL);
> +	}

This should possibly be tested in drm_internal_framebuffer_create instead.


> +	pitch = amdgpu_align_pitch(adev, pitch, cpp, false);
> +	if (mode_cmd->pitches[0] != pitch) {
> +		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
> +			      pitch, mode_cmd->pitches[0]);
> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
>  	if (obj ==  NULL) {
> 

Apart from the above, the v5 series looks good to me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

On Thu, Jan 03, 2019 at 05:33:19PM +0100, Michel Dänzer wrote:
> On 2018-12-30 2:00 a.m., Yu Zhao wrote:
> > Userspace may request pitch alignment that is not supported by GPU.
> > Some requests 32, but GPU ignores it and uses default 64 when cpp is
> > 4. If GEM object is allocated based on the smaller alignment, GPU
> > DMA will go out of bound.
> > 
> > Cc: stable@vger.kernel.org # v4.2+
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index 15ce7e681d67..16af80ccd0a0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -527,6 +527,22 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
> >  	struct drm_gem_object *obj;
> >  	struct amdgpu_framebuffer *amdgpu_fb;
> >  	int ret;
> > +	struct amdgpu_device *adev = dev->dev_private;
> > +	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> > +	int pitch = mode_cmd->pitches[0] / cpp;
> > +
> > +	if (pitch < mode_cmd->width) {
> > +		DRM_DEBUG_KMS("Expecting pitch(%d)/cpp(%d) >= width(%d)\n",
> > +			      mode_cmd->pitches[0], cpp, mode_cmd->width);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> 
> This should possibly be tested in drm_internal_framebuffer_create instead.

It seems to me drm_format_info_min_pitch() in framebuffer_check()
already does it. We could just remove the check here?

Re: [PATCH v5 1/2] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2019-01-07 5:00 a.m., Yu Zhao wrote:
> On Thu, Jan 03, 2019 at 05:33:19PM +0100, Michel Dänzer wrote:
>> On 2018-12-30 2:00 a.m., Yu Zhao wrote:
>>> Userspace may request pitch alignment that is not supported by GPU.
>>> Some requests 32, but GPU ignores it and uses default 64 when cpp is
>>> 4. If GEM object is allocated based on the smaller alignment, GPU
>>> DMA will go out of bound.
>>>
>>> Cc: stable@vger.kernel.org # v4.2+
>>> Signed-off-by: Yu Zhao <yuzhao@google.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index 15ce7e681d67..16af80ccd0a0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -527,6 +527,22 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>>>  	struct drm_gem_object *obj;
>>>  	struct amdgpu_framebuffer *amdgpu_fb;
>>>  	int ret;
>>> +	struct amdgpu_device *adev = dev->dev_private;
>>> +	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
>>> +	int pitch = mode_cmd->pitches[0] / cpp;
>>> +
>>> +	if (pitch < mode_cmd->width) {
>>> +		DRM_DEBUG_KMS("Expecting pitch(%d)/cpp(%d) >= width(%d)\n",
>>> +			      mode_cmd->pitches[0], cpp, mode_cmd->width);
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>
>> This should possibly be tested in drm_internal_framebuffer_create instead.
> 
> It seems to me drm_format_info_min_pitch() in framebuffer_check()
> already does it. We could just remove the check here?

Yeah, looks like that should be fine.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH v5 1/2] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2019-01-07 5:00 a.m., Yu Zhao wrote:
> On Thu, Jan 03, 2019 at 05:33:19PM +0100, Michel Dänzer wrote:
>> On 2018-12-30 2:00 a.m., Yu Zhao wrote:
>>> Userspace may request pitch alignment that is not supported by GPU.
>>> Some requests 32, but GPU ignores it and uses default 64 when cpp is
>>> 4. If GEM object is allocated based on the smaller alignment, GPU
>>> DMA will go out of bound.
>>>
>>> Cc: stable@vger.kernel.org # v4.2+
>>> Signed-off-by: Yu Zhao <yuzhao@google.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index 15ce7e681d67..16af80ccd0a0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -527,6 +527,22 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>>>  	struct drm_gem_object *obj;
>>>  	struct amdgpu_framebuffer *amdgpu_fb;
>>>  	int ret;
>>> +	struct amdgpu_device *adev = dev->dev_private;
>>> +	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
>>> +	int pitch = mode_cmd->pitches[0] / cpp;
>>> +
>>> +	if (pitch < mode_cmd->width) {
>>> +		DRM_DEBUG_KMS("Expecting pitch(%d)/cpp(%d) >= width(%d)\n",
>>> +			      mode_cmd->pitches[0], cpp, mode_cmd->width);
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>
>> This should possibly be tested in drm_internal_framebuffer_create instead.
> 
> It seems to me drm_format_info_min_pitch() in framebuffer_check()
> already does it. We could just remove the check here?

Yeah, looks like that should be fine.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v6 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

Cc: stable@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 15ce7e681d67..de9f198d5371 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,16 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	struct amdgpu_framebuffer *amdgpu_fb;
 	int ret;
+	struct amdgpu_device *adev = dev->dev_private;
+	int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
+	int pitch = mode_cmd->pitches[0] / cpp;
+
+	pitch = amdgpu_align_pitch(adev, pitch, cpp, false);
+	if (mode_cmd->pitches[0] != pitch) {
+		DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
+			      pitch, mode_cmd->pitches[0]);
+		return ERR_PTR(-EINVAL);
+	}
 
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (obj ==  NULL) {
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v6 2/2] drm/amd: validate user GEM object size

[Yu Zhao]

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,
otherwise it could be exploited to reveal sensitive data.

This fix is not done in a common code path because individual
driver might have different requirement.

Cc: stable@vger.kernel.org # v4.2+
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 de9f198d5371..32b7648b7ef4 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 = mode_cmd->pitches[0] / cpp;
@@ -551,6 +552,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) {
+		DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %zu\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);
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH v6 1/2] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2019-01-07 11:51 p.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> Cc: stable@vger.kernel.org # v4.2+
> Signed-off-by: Yu Zhao <yuzhao@google.com>

Both patches applied to amd-staging-drm-next (should land in 5.0), thanks!


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH v6 1/2] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2019-01-07 11:51 p.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> Cc: stable@vger.kernel.org # v4.2+
> Signed-off-by: Yu Zhao <yuzhao@google.com>

Both patches applied to amd-staging-drm-next (should land in 5.0), thanks!


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx