Re: [PATCH] drm/amdgpu/psp: prevent page fault by checking write_frame address(v2) -v2: update the ring_buffer_end address

[Alex Deucher]

On Mon, Oct 16, 2017 at 11:26 PM, Evan Quan  wrote:
> Change-Id: If3b79428b32ffab57b4e75f9c20c2b2d7e600223
> Signed-off-by: Evan Quan 

Please include a better commit message.  Something like:
Prevent a a possible buffer overflow when updating the ring buffer by
bounds checking the command frame against the available space in the
ring buffer.


> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 16 ++--
>  drivers/gpu/drm/amd/amdgpu/psp_v3_1.c  | 16 ++--
>  2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c 
> b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> index 77cab1f..7d31c74 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> @@ -257,6 +257,9 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
> unsigned int psp_write_ptr_reg = 0;
> struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
> struct psp_ring *ring = >km_ring;
> +   struct psp_gfx_rb_frame *ring_buffer_start = ring->ring_mem;
> +   struct psp_gfx_rb_frame *ring_buffer_end = ring_buffer_start +
> +   ring->ring_size / sizeof(struct psp_gfx_rb_frame) - 1;
> struct amdgpu_device *adev = psp->adev;
> uint32_t ring_size_dw = ring->ring_size / 4;
> uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
> @@ -266,9 +269,18 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
>
> /* Update KM RB frame pointer to new frame */
> if ((psp_write_ptr_reg % ring_size_dw) == 0)
> -   write_frame = ring->ring_mem;
> +   write_frame = ring_buffer_start;
> else
> -   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
> rb_frame_size_dw);
> +   write_frame = ring_buffer_start + (psp_write_ptr_reg / 
> rb_frame_size_dw);
> +   /* Check invalid write_frame ptr address */
> +   if ((write_frame < ring_buffer_start) || (ring_buffer_end < 
> write_frame)) {
> +   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
> write_frame = %x\n",
> +   ring_buffer_start,
> +   ring_buffer_end,
> +   write_frame);
> +   DRM_ERROR("write_frame is pointing to address out of 
> bounds\n");
> +   return -EINVAL;
> +   }
>
> /* Initialize KM RB frame */
> memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c 
> b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> index bcbe30d..3f0eecf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> @@ -367,6 +367,9 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
> unsigned int psp_write_ptr_reg = 0;
> struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
> struct psp_ring *ring = >km_ring;
> +   struct psp_gfx_rb_frame *ring_buffer_start = ring->ring_mem;
> +   struct psp_gfx_rb_frame *ring_buffer_end = ring_buffer_start +
> +   ring->ring_size / sizeof(struct psp_gfx_rb_frame) - 1;
> struct amdgpu_device *adev = psp->adev;
> uint32_t ring_size_dw = ring->ring_size / 4;
> uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
> @@ -378,9 +381,18 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
> /* write_frame ptr increments by size of rb_frame in bytes */
> /* psp_write_ptr_reg increments by size of rb_frame in DWORDs */
> if ((psp_write_ptr_reg % ring_size_dw) == 0)
> -   write_frame = ring->ring_mem;
> +   write_frame = ring_buffer_start;
> else
> -   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
> rb_frame_size_dw);
> +   write_frame = ring_buffer_start + (psp_write_ptr_reg / 
> rb_frame_size_dw);
> +   /* Check invalid write_frame ptr address */
> +   if ((write_frame < ring_buffer_start) || (ring_buffer_end < 
> write_frame)) {
> +   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
> write_frame = %x\n",
> +   ring_buffer_start,
> +   ring_buffer_end,
> +   write_frame);
> +   DRM_ERROR("write_frame is pointing to address out of 
> bounds\n");
> +   return -EINVAL;
> +   }
>
> /* Initialize KM RB frame */
> memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
> --
> 2.7.4
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu/psp: prevent page fault by checking write_frame address(v2) -v2: update the ring_buffer_end address

[Evan Quan]

Change-Id: If3b79428b32ffab57b4e75f9c20c2b2d7e600223
Signed-off-by: Evan Quan 
---
 drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 16 ++--
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.c  | 16 ++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
index 77cab1f..7d31c74 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
@@ -257,6 +257,9 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
unsigned int psp_write_ptr_reg = 0;
struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
struct psp_ring *ring = >km_ring;
+   struct psp_gfx_rb_frame *ring_buffer_start = ring->ring_mem;
+   struct psp_gfx_rb_frame *ring_buffer_end = ring_buffer_start +
+   ring->ring_size / sizeof(struct psp_gfx_rb_frame) - 1;
struct amdgpu_device *adev = psp->adev;
uint32_t ring_size_dw = ring->ring_size / 4;
uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
@@ -266,9 +269,18 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
 
/* Update KM RB frame pointer to new frame */
if ((psp_write_ptr_reg % ring_size_dw) == 0)
-   write_frame = ring->ring_mem;
+   write_frame = ring_buffer_start;
else
-   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   write_frame = ring_buffer_start + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   /* Check invalid write_frame ptr address */
+   if ((write_frame < ring_buffer_start) || (ring_buffer_end < 
write_frame)) {
+   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
write_frame = %x\n",
+   ring_buffer_start,
+   ring_buffer_end,
+   write_frame);
+   DRM_ERROR("write_frame is pointing to address out of bounds\n");
+   return -EINVAL;
+   }
 
/* Initialize KM RB frame */
memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
index bcbe30d..3f0eecf 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
@@ -367,6 +367,9 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
unsigned int psp_write_ptr_reg = 0;
struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
struct psp_ring *ring = >km_ring;
+   struct psp_gfx_rb_frame *ring_buffer_start = ring->ring_mem;
+   struct psp_gfx_rb_frame *ring_buffer_end = ring_buffer_start +
+   ring->ring_size / sizeof(struct psp_gfx_rb_frame) - 1;
struct amdgpu_device *adev = psp->adev;
uint32_t ring_size_dw = ring->ring_size / 4;
uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
@@ -378,9 +381,18 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
/* write_frame ptr increments by size of rb_frame in bytes */
/* psp_write_ptr_reg increments by size of rb_frame in DWORDs */
if ((psp_write_ptr_reg % ring_size_dw) == 0)
-   write_frame = ring->ring_mem;
+   write_frame = ring_buffer_start;
else
-   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   write_frame = ring_buffer_start + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   /* Check invalid write_frame ptr address */
+   if ((write_frame < ring_buffer_start) || (ring_buffer_end < 
write_frame)) {
+   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
write_frame = %x\n",
+   ring_buffer_start,
+   ring_buffer_end,
+   write_frame);
+   DRM_ERROR("write_frame is pointing to address out of bounds\n");
+   return -EINVAL;
+   }
 
/* Initialize KM RB frame */
memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
-- 
2.7.4

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

Re: [PATCH] drm/amdgpu/psp: prevent page fault by checking write_frame address

[Alex Deucher]

On Mon, Oct 16, 2017 at 5:09 AM, Evan Quan  wrote:
> Change-Id: If3b79428b32ffab57b4e75f9c20c2b2d7e600223
> Signed-off-by: Evan Quan 

Please provide a more detailed commit message.  Seems like we should
look at converting the psp to use the common ring helpers to avoid
issues like this is the future.  At the very least a ring_alloc/commit
sort of interface.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 31 ---
>  drivers/gpu/drm/amd/amdgpu/psp_v3_1.c  | 31 ---
>  2 files changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c 
> b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> index 77cab1f..487e17b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
> @@ -255,10 +255,10 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
> int index)
>  {
> unsigned int psp_write_ptr_reg = 0;
> -   struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
> -   struct psp_ring *ring = >km_ring;
> +   struct psp_gfx_rb_frame *ring_buffer_start = psp->km_ring.ring_mem;
> +   struct psp_gfx_rb_frame *write_frame_ptr;
> struct amdgpu_device *adev = psp->adev;
> -   uint32_t ring_size_dw = ring->ring_size / 4;
> +   uint32_t ring_size_dw = psp->km_ring.ring_size / 4;
> uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
>
> /* KM (GPCOM) prepare write pointer */
> @@ -266,19 +266,28 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
>
> /* Update KM RB frame pointer to new frame */
> if ((psp_write_ptr_reg % ring_size_dw) == 0)
> -   write_frame = ring->ring_mem;
> +   write_frame_ptr = ring_buffer_start;
> else
> -   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
> rb_frame_size_dw);
> +   write_frame_ptr = ring_buffer_start + (psp_write_ptr_reg / 
> rb_frame_size_dw);
> +   if ((write_frame_ptr < ring_buffer_start) ||
> +   ((ring_buffer_start + psp->km_ring.ring_size / sizeof(struct 
> psp_gfx_rb_frame)) < write_frame_ptr)) {
> +   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
> write_frame_ptr = %x\n",
> +   ring_buffer_start,
> +   ring_buffer_start + psp->km_ring.ring_size / 
> sizeof(struct psp_gfx_rb_frame),
> +   write_frame_ptr);
> +   DRM_ERROR("write_frame_ptr is pointing to address out of 
> bounds\n");
> +   return -EINVAL;
> +   }
>
> /* Initialize KM RB frame */
> -   memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
> +   memset(write_frame_ptr, 0, sizeof(struct psp_gfx_rb_frame));
>
> /* Update KM RB frame */
> -   write_frame->cmd_buf_addr_hi = upper_32_bits(cmd_buf_mc_addr);
> -   write_frame->cmd_buf_addr_lo = lower_32_bits(cmd_buf_mc_addr);
> -   write_frame->fence_addr_hi = upper_32_bits(fence_mc_addr);
> -   write_frame->fence_addr_lo = lower_32_bits(fence_mc_addr);
> -   write_frame->fence_value = index;
> +   write_frame_ptr->cmd_buf_addr_hi = upper_32_bits(cmd_buf_mc_addr);
> +   write_frame_ptr->cmd_buf_addr_lo = lower_32_bits(cmd_buf_mc_addr);
> +   write_frame_ptr->fence_addr_hi = upper_32_bits(fence_mc_addr);
> +   write_frame_ptr->fence_addr_lo = lower_32_bits(fence_mc_addr);
> +   write_frame_ptr->fence_value = index;
>
> /* Update the write Pointer in DWORDs */
> psp_write_ptr_reg = (psp_write_ptr_reg + rb_frame_size_dw) % 
> ring_size_dw;
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c 
> b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> index bcbe30d..ec472e5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
> @@ -365,10 +365,10 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
> int index)
>  {
> unsigned int psp_write_ptr_reg = 0;
> -   struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
> -   struct psp_ring *ring = >km_ring;
> +   struct psp_gfx_rb_frame *ring_buffer_start = psp->km_ring.ring_mem;
> +   struct psp_gfx_rb_frame *write_frame_ptr;
> struct amdgpu_device *adev = psp->adev;
> -   uint32_t ring_size_dw = ring->ring_size / 4;
> +   uint32_t ring_size_dw = psp->km_ring.ring_size / 4;
> uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
>
> /* KM (GPCOM) prepare write pointer */
> @@ -378,19 +378,28 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
> /* write_frame ptr increments by size of rb_frame in bytes */
> /* psp_write_ptr_reg increments by size of rb_frame in DWORDs */
> if ((psp_write_ptr_reg % ring_size_dw) == 0)
> -   write_frame = ring->ring_mem;
> +   write_frame_ptr = 

[PATCH] drm/amdgpu/psp: prevent page fault by checking write_frame address

[Evan Quan]

Change-Id: If3b79428b32ffab57b4e75f9c20c2b2d7e600223
Signed-off-by: Evan Quan 
---
 drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 31 ---
 drivers/gpu/drm/amd/amdgpu/psp_v3_1.c  | 31 ---
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
index 77cab1f..487e17b 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v10_0.c
@@ -255,10 +255,10 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
int index)
 {
unsigned int psp_write_ptr_reg = 0;
-   struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
-   struct psp_ring *ring = >km_ring;
+   struct psp_gfx_rb_frame *ring_buffer_start = psp->km_ring.ring_mem;
+   struct psp_gfx_rb_frame *write_frame_ptr;
struct amdgpu_device *adev = psp->adev;
-   uint32_t ring_size_dw = ring->ring_size / 4;
+   uint32_t ring_size_dw = psp->km_ring.ring_size / 4;
uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
 
/* KM (GPCOM) prepare write pointer */
@@ -266,19 +266,28 @@ int psp_v10_0_cmd_submit(struct psp_context *psp,
 
/* Update KM RB frame pointer to new frame */
if ((psp_write_ptr_reg % ring_size_dw) == 0)
-   write_frame = ring->ring_mem;
+   write_frame_ptr = ring_buffer_start;
else
-   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   write_frame_ptr = ring_buffer_start + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   if ((write_frame_ptr < ring_buffer_start) ||
+   ((ring_buffer_start + psp->km_ring.ring_size / sizeof(struct 
psp_gfx_rb_frame)) < write_frame_ptr)) {
+   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
write_frame_ptr = %x\n",
+   ring_buffer_start,
+   ring_buffer_start + psp->km_ring.ring_size / 
sizeof(struct psp_gfx_rb_frame),
+   write_frame_ptr);
+   DRM_ERROR("write_frame_ptr is pointing to address out of 
bounds\n");
+   return -EINVAL;
+   }
 
/* Initialize KM RB frame */
-   memset(write_frame, 0, sizeof(struct psp_gfx_rb_frame));
+   memset(write_frame_ptr, 0, sizeof(struct psp_gfx_rb_frame));
 
/* Update KM RB frame */
-   write_frame->cmd_buf_addr_hi = upper_32_bits(cmd_buf_mc_addr);
-   write_frame->cmd_buf_addr_lo = lower_32_bits(cmd_buf_mc_addr);
-   write_frame->fence_addr_hi = upper_32_bits(fence_mc_addr);
-   write_frame->fence_addr_lo = lower_32_bits(fence_mc_addr);
-   write_frame->fence_value = index;
+   write_frame_ptr->cmd_buf_addr_hi = upper_32_bits(cmd_buf_mc_addr);
+   write_frame_ptr->cmd_buf_addr_lo = lower_32_bits(cmd_buf_mc_addr);
+   write_frame_ptr->fence_addr_hi = upper_32_bits(fence_mc_addr);
+   write_frame_ptr->fence_addr_lo = lower_32_bits(fence_mc_addr);
+   write_frame_ptr->fence_value = index;
 
/* Update the write Pointer in DWORDs */
psp_write_ptr_reg = (psp_write_ptr_reg + rb_frame_size_dw) % 
ring_size_dw;
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
index bcbe30d..ec472e5 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v3_1.c
@@ -365,10 +365,10 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
int index)
 {
unsigned int psp_write_ptr_reg = 0;
-   struct psp_gfx_rb_frame * write_frame = psp->km_ring.ring_mem;
-   struct psp_ring *ring = >km_ring;
+   struct psp_gfx_rb_frame *ring_buffer_start = psp->km_ring.ring_mem;
+   struct psp_gfx_rb_frame *write_frame_ptr;
struct amdgpu_device *adev = psp->adev;
-   uint32_t ring_size_dw = ring->ring_size / 4;
+   uint32_t ring_size_dw = psp->km_ring.ring_size / 4;
uint32_t rb_frame_size_dw = sizeof(struct psp_gfx_rb_frame) / 4;
 
/* KM (GPCOM) prepare write pointer */
@@ -378,19 +378,28 @@ int psp_v3_1_cmd_submit(struct psp_context *psp,
/* write_frame ptr increments by size of rb_frame in bytes */
/* psp_write_ptr_reg increments by size of rb_frame in DWORDs */
if ((psp_write_ptr_reg % ring_size_dw) == 0)
-   write_frame = ring->ring_mem;
+   write_frame_ptr = ring_buffer_start;
else
-   write_frame = ring->ring_mem + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   write_frame_ptr = ring_buffer_start + (psp_write_ptr_reg / 
rb_frame_size_dw);
+   if ((write_frame_ptr < ring_buffer_start) ||
+   ((ring_buffer_start + psp->km_ring.ring_size / sizeof(struct 
psp_gfx_rb_frame)) < write_frame_ptr)) {
+   DRM_ERROR("ring_buffer_start = %x; ring_buffer_end = %x; 
write_frame_ptr =