diff mbox series

[5.4,105/132] drm/msm: Split the a5xx preemption record

Message ID 20200915140649.400517956@linuxfoundation.org
State Superseded
Headers show
Series None | expand

Commit Message

Greg Kroah-Hartman Sept. 15, 2020, 2:13 p.m. UTC 
From: Jordan Crouse <jcrouse@codeaurora.org>

commit 34221545d2069dc947131f42392fd4cebabe1b39 upstream.

The main a5xx preemption record can be marked as privileged to
protect it from user access but the counters storage needs to be
remain unprivileged. Split the buffers and mark the critical memory
as privileged.

Cc: stable@vger.kernel.org
Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
Signed-off-by: Rob Clark <robdclark@chromium.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.h     |    1 +
 drivers/gpu/drm/msm/adreno/a5xx_preempt.c |   25 ++++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

Comments

Jordan Crouse Sept. 15, 2020, 3:54 p.m. UTC | #1 
On Tue, Sep 15, 2020 at 04:13:27PM +0200, Greg Kroah-Hartman wrote:
> From: Jordan Crouse <jcrouse@codeaurora.org>
> 
> commit 34221545d2069dc947131f42392fd4cebabe1b39 upstream.
> 
> The main a5xx preemption record can be marked as privileged to
> protect it from user access but the counters storage needs to be
> remain unprivileged. Split the buffers and mark the critical memory
> as privileged.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Hi. The MSM_BO_MAP_PRIV feature was added after 5.4. 

Since we are pulling in 7b3f3948c8b7 ("drm/msm: Disable preemption on all
5xx targets)" preemption will be disabled in the 5.4 stable tree which is enough
to cover the security concern that this patch helped address.

This patch can be dropped.

Jordan

> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.h     |    1 +
>  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |   25 ++++++++++++++++++++-----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.h
> @@ -31,6 +31,7 @@ struct a5xx_gpu {
>  	struct msm_ringbuffer *next_ring;
>  
>  	struct drm_gem_object *preempt_bo[MSM_GPU_MAX_RINGS];
> +	struct drm_gem_object *preempt_counters_bo[MSM_GPU_MAX_RINGS];
>  	struct a5xx_preempt_record *preempt[MSM_GPU_MAX_RINGS];
>  	uint64_t preempt_iova[MSM_GPU_MAX_RINGS];
>  
> --- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
> @@ -226,19 +226,31 @@ static int preempt_init_ring(struct a5xx
>  	struct adreno_gpu *adreno_gpu = &a5xx_gpu->base;
>  	struct msm_gpu *gpu = &adreno_gpu->base;
>  	struct a5xx_preempt_record *ptr;
> -	struct drm_gem_object *bo = NULL;
> -	u64 iova = 0;
> +	void *counters;
> +	struct drm_gem_object *bo = NULL, *counters_bo = NULL;
> +	u64 iova = 0, counters_iova = 0;
>  
>  	ptr = msm_gem_kernel_new(gpu->dev,
>  		A5XX_PREEMPT_RECORD_SIZE + A5XX_PREEMPT_COUNTER_SIZE,
> -		MSM_BO_UNCACHED, gpu->aspace, &bo, &iova);
> +		MSM_BO_UNCACHED | MSM_BO_MAP_PRIV, gpu->aspace, &bo, &iova);
>  
>  	if (IS_ERR(ptr))
>  		return PTR_ERR(ptr);
>  
> +	/* The buffer to store counters needs to be unprivileged */
> +	counters = msm_gem_kernel_new(gpu->dev,
> +		A5XX_PREEMPT_COUNTER_SIZE,
> +		MSM_BO_UNCACHED, gpu->aspace, &counters_bo, &counters_iova);
> +	if (IS_ERR(counters)) {
> +		msm_gem_kernel_put(bo, gpu->aspace, true);
> +		return PTR_ERR(counters);
> +	}
> +
>  	msm_gem_object_set_name(bo, "preempt");
> +	msm_gem_object_set_name(counters_bo, "preempt_counters");
>  
>  	a5xx_gpu->preempt_bo[ring->id] = bo;
> +	a5xx_gpu->preempt_counters_bo[ring->id] = counters_bo;
>  	a5xx_gpu->preempt_iova[ring->id] = iova;
>  	a5xx_gpu->preempt[ring->id] = ptr;
>  
> @@ -249,7 +261,7 @@ static int preempt_init_ring(struct a5xx
>  	ptr->data = 0;
>  	ptr->cntl = MSM_GPU_RB_CNTL_DEFAULT;
>  	ptr->rptr_addr = rbmemptr(ring, rptr);
> -	ptr->counter = iova + A5XX_PREEMPT_RECORD_SIZE;
> +	ptr->counter = counters_iova;
>  
>  	return 0;
>  }
> @@ -260,8 +272,11 @@ void a5xx_preempt_fini(struct msm_gpu *g
>  	struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu);
>  	int i;
>  
> -	for (i = 0; i < gpu->nr_rings; i++)
> +	for (i = 0; i < gpu->nr_rings; i++) {
>  		msm_gem_kernel_put(a5xx_gpu->preempt_bo[i], gpu->aspace, true);
> +		msm_gem_kernel_put(a5xx_gpu->preempt_counters_bo[i],
> +			gpu->aspace, true);
> +	}
>  }
>  
>  void a5xx_preempt_init(struct msm_gpu *gpu)
> 
>
Greg Kroah-Hartman Sept. 15, 2020, 4:39 p.m. UTC | #2 
On Tue, Sep 15, 2020 at 09:54:39AM -0600, Jordan Crouse wrote:
> On Tue, Sep 15, 2020 at 04:13:27PM +0200, Greg Kroah-Hartman wrote:
> > From: Jordan Crouse <jcrouse@codeaurora.org>
> > 
> > commit 34221545d2069dc947131f42392fd4cebabe1b39 upstream.
> > 
> > The main a5xx preemption record can be marked as privileged to
> > protect it from user access but the counters storage needs to be
> > remain unprivileged. Split the buffers and mark the critical memory
> > as privileged.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Hi. The MSM_BO_MAP_PRIV feature was added after 5.4. 
> 
> Since we are pulling in 7b3f3948c8b7 ("drm/msm: Disable preemption on all
> 5xx targets)" preemption will be disabled in the 5.4 stable tree which is enough
> to cover the security concern that this patch helped address.
> 
> This patch can be dropped.

Now dropped,t hanks!

greg k-h
diff mbox series

Patch

--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.h
@@ -31,6 +31,7 @@  struct a5xx_gpu {
 	struct msm_ringbuffer *next_ring;
 
 	struct drm_gem_object *preempt_bo[MSM_GPU_MAX_RINGS];
+	struct drm_gem_object *preempt_counters_bo[MSM_GPU_MAX_RINGS];
 	struct a5xx_preempt_record *preempt[MSM_GPU_MAX_RINGS];
 	uint64_t preempt_iova[MSM_GPU_MAX_RINGS];
 
--- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
@@ -226,19 +226,31 @@  static int preempt_init_ring(struct a5xx
 	struct adreno_gpu *adreno_gpu = &a5xx_gpu->base;
 	struct msm_gpu *gpu = &adreno_gpu->base;
 	struct a5xx_preempt_record *ptr;
-	struct drm_gem_object *bo = NULL;
-	u64 iova = 0;
+	void *counters;
+	struct drm_gem_object *bo = NULL, *counters_bo = NULL;
+	u64 iova = 0, counters_iova = 0;
 
 	ptr = msm_gem_kernel_new(gpu->dev,
 		A5XX_PREEMPT_RECORD_SIZE + A5XX_PREEMPT_COUNTER_SIZE,
-		MSM_BO_UNCACHED, gpu->aspace, &bo, &iova);
+		MSM_BO_UNCACHED | MSM_BO_MAP_PRIV, gpu->aspace, &bo, &iova);
 
 	if (IS_ERR(ptr))
 		return PTR_ERR(ptr);
 
+	/* The buffer to store counters needs to be unprivileged */
+	counters = msm_gem_kernel_new(gpu->dev,
+		A5XX_PREEMPT_COUNTER_SIZE,
+		MSM_BO_UNCACHED, gpu->aspace, &counters_bo, &counters_iova);
+	if (IS_ERR(counters)) {
+		msm_gem_kernel_put(bo, gpu->aspace, true);
+		return PTR_ERR(counters);
+	}
+
 	msm_gem_object_set_name(bo, "preempt");
+	msm_gem_object_set_name(counters_bo, "preempt_counters");
 
 	a5xx_gpu->preempt_bo[ring->id] = bo;
+	a5xx_gpu->preempt_counters_bo[ring->id] = counters_bo;
 	a5xx_gpu->preempt_iova[ring->id] = iova;
 	a5xx_gpu->preempt[ring->id] = ptr;
 
@@ -249,7 +261,7 @@  static int preempt_init_ring(struct a5xx
 	ptr->data = 0;
 	ptr->cntl = MSM_GPU_RB_CNTL_DEFAULT;
 	ptr->rptr_addr = rbmemptr(ring, rptr);
-	ptr->counter = iova + A5XX_PREEMPT_RECORD_SIZE;
+	ptr->counter = counters_iova;
 
 	return 0;
 }
@@ -260,8 +272,11 @@  void a5xx_preempt_fini(struct msm_gpu *g
 	struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu);
 	int i;
 
-	for (i = 0; i < gpu->nr_rings; i++)
+	for (i = 0; i < gpu->nr_rings; i++) {
 		msm_gem_kernel_put(a5xx_gpu->preempt_bo[i], gpu->aspace, true);
+		msm_gem_kernel_put(a5xx_gpu->preempt_counters_bo[i],
+			gpu->aspace, true);
+	}
 }
 
 void a5xx_preempt_init(struct msm_gpu *gpu)