[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 <evan.quan at amd.com> wrote:
> Change-Id: If3b79428b32ffab57b4e75f9c20c2b2d7e600223
> Signed-off-by: Evan Quan <evan.quan at amd.com>

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 = &psp->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 = &psp->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 = %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;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx