pitch alignment questions
On Fri, Jan 11, 2019 at 04:27:44PM +0100, Michel Dänzer wrote: > On 2019-01-10 6:56 p.m., Przemek Socha wrote: > > > > [ 147.846148] [drm:amdgpu_display_user_framebuffer_create [amdgpu]] > > Invalid > > pitch: expecting 10752 but got 10624 > > [ 147.846155] [drm:drm_internal_framebuffer_create] could not create > > framebuffer" > > Thanks, this confirms that the check is too strict. I've sent a patch > reverting this as well. > > > Yu, I like the idea behind your changes, but unfortunately it's more > complicated than that. If you want to work on similar checks which > accurately reflect the hardware constraints, people on the amd-gfx list > should be able to help with that. Hi Michel, sorry for the troubles. Background: after we turned on iommu with amd_iommu=force_isolation, we saw io page faults from amd gpu (stoney ridge). We tracked it down to userspace using 32-pixel pitch alignment, which seems smaller than the minimum alignment supported by the hw. Instead of rejecting the alignment, we suspect, it uses 64-pixel alignment to do dma. The larger alignment sometimes causes out of bound memory accesses, thus the io page faults. I created the following patch and hoped it could fix the problem: https://lore.kernel.org/patchwork/patch/1029656/ Well, it does on our stoney ridge based chromebook but also breaks other platforms. So my questions to the amd gpu experts here: 1) how do properly validate pitch alignment passed to kernel space? 2) if it's not easy, what would invalid alignment cause at worst? Thank you. ___ 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
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v6 2/2] drm/amd: validate user GEM object size
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/amd: validate user pitch alignment
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: sta...@vger.kernel.org # v4.2+
> > Signed-off-by: Yu Zhao
> > ---
> > 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?
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v5 1/2] drm/amd: validate user pitch alignment
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v5 2/2] drm/amd: validate user GEM object size
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
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
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 > > --- > > 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). ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v4 2/2] drm/amd: validate user GEM object size
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v4 1/2] drm/amd: validate user pitch alignment
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v3 2/2] drm/amd: validate user GEM object size
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v3 1/2] drm/amd: validate user pitch alignment
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: sta...@vger.kernel.org # v4.2+
Signed-off-by: Yu Zhao
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2 2/2] drm/amd: validate user GEM object size
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
---
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2 1/2] drm/amd: validate user pitch alignment
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
---
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
___
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
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 > > --- > > 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. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 3/3] drm/amd: validate user GEM object size
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
---
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(>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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/3] drm/amd: fix race in page flip job
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 --- 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(>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(>dev->event_lock, flags); -- 2.20.1.415.g653613c723-goog ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/3] drm/amd: validate user pitch alignment
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
---
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(>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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
