[PATCH v2 0/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[PATCH v2 0/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[quic_ddhamara]

From: Devi prasad Dhamarasingi <quic_ddhamara@quicinc.com>

Fix a null pointer access when memory allocation fails in
a6xx_get_indexed_registers().

Akhil P Oommen (1):
  drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.37.0

[PATCH v2 0/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[quic_ddhamara]

From: Devi prasad Dhamarasingi <quic_ddhamara@quicinc.com>

Fix a null pointer access when memory allocation fails in
a6xx_get_indexed_registers().

Akhil P Oommen (1):
  drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.37.0

[PATCH v2 1/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[quic_ddhamara]

From: Akhil P Oommen <quic_akhilpo@quicinc.com>

Fix a null pointer access when memory allocation fails in
a6xx_get_indexed_registers().

Change-Id: I33e13745cd8e5841d2f377f48a199af98be2ed02
Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Signed-off-by: Devi prasad Dhamarasingi <quic_ddhamara@quicinc.com>
---

Changes in v2:
- Corrected the signoff name and email id.

 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 55f443328d8e..507074f6222c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -952,6 +952,12 @@ static void a6xx_get_indexed_registers(struct msm_gpu *gpu,
 	a6xx_get_indexed_regs(gpu, a6xx_state, &a6xx_cp_mempool_indexed,
 		&a6xx_state->indexed_regs[i]);
 
+	if (!a6xx_state->indexed_regs[i].data) {
+		gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, mempool_size);
+		a6xx_state->nr_indexed_regs = count - 1;
+		return;
+	}
+
 	/*
 	 * Offset 0x2000 in the mempool is the size - copy the saved size over
 	 * so the data is consistent
-- 
2.37.0

[PATCH v2 1/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[quic_ddhamara]

From: Akhil P Oommen <quic_akhilpo@quicinc.com>

Fix a null pointer access when memory allocation fails in
a6xx_get_indexed_registers().

Change-Id: I33e13745cd8e5841d2f377f48a199af98be2ed02
Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Signed-off-by: Devi prasad Dhamarasingi <quic_ddhamara@quicinc.com>
---

Changes in v2:
- Corrected the signoff name and email id.

 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 55f443328d8e..507074f6222c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -952,6 +952,12 @@ static void a6xx_get_indexed_registers(struct msm_gpu *gpu,
 	a6xx_get_indexed_regs(gpu, a6xx_state, &a6xx_cp_mempool_indexed,
 		&a6xx_state->indexed_regs[i]);
 
+	if (!a6xx_state->indexed_regs[i].data) {
+		gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, mempool_size);
+		a6xx_state->nr_indexed_regs = count - 1;
+		return;
+	}
+
 	/*
 	 * Offset 0x2000 in the mempool is the size - copy the saved size over
 	 * so the data is consistent
-- 
2.37.0

Re: [Freedreno] [PATCH v2 1/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[Rob Clark]

On Mon, Jul 25, 2022 at 9:46 PM <quic_ddhamara@quicinc.com> wrote:
>
> From: Akhil P Oommen <quic_akhilpo@quicinc.com>
>
> Fix a null pointer access when memory allocation fails in
> a6xx_get_indexed_registers().
>
> Change-Id: I33e13745cd8e5841d2f377f48a199af98be2ed02
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Signed-off-by: Devi prasad Dhamarasingi <quic_ddhamara@quicinc.com>
> ---
>
> Changes in v2:
> - Corrected the signoff name and email id.
>
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index 55f443328d8e..507074f6222c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -952,6 +952,12 @@ static void a6xx_get_indexed_registers(struct msm_gpu *gpu,
>         a6xx_get_indexed_regs(gpu, a6xx_state, &a6xx_cp_mempool_indexed,
>                 &a6xx_state->indexed_regs[i]);
>
> +       if (!a6xx_state->indexed_regs[i].data) {
> +               gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, mempool_size);
> +               a6xx_state->nr_indexed_regs = count - 1;
> +               return;
> +       }

Hmm, I don't see us adjusting nr_indexed_regs if any of the earlier
sections fails, so I don't think we need to do that here either.  So I
think you could just:

if (a6xx_state->indexed_regs[i].data)
   a6xx_state->indexed_regs[i].data[0x2000] = mempool_size;

And I kinda expect if there was an allocation failure we'd just end up
dereferencing a null ptr later in the show path.

But, I think in general you can assume small GFP_KERNEL allocations
will never fail.  If necessary they will block for reclaim/shrinker to
free up some memory or evict some pages to swap.  If you've gotten to
the point where even that isn't possible, then a null ptr deref is
really the least of your problems ;-)

BR,
-R

> +
>         /*
>          * Offset 0x2000 in the mempool is the size - copy the saved size over
>          * so the data is consistent
> --
> 2.37.0
>

Re: [Freedreno] [PATCH v2 1/1] drm/msm/a6xx: Fix null pointer access in a6xx_get_indexed_registers

[Rob Clark]

On Mon, Jul 25, 2022 at 9:46 PM <quic_ddhamara@quicinc.com> wrote:
>
> From: Akhil P Oommen <quic_akhilpo@quicinc.com>
>
> Fix a null pointer access when memory allocation fails in
> a6xx_get_indexed_registers().
>
> Change-Id: I33e13745cd8e5841d2f377f48a199af98be2ed02
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Signed-off-by: Devi prasad Dhamarasingi <quic_ddhamara@quicinc.com>
> ---
>
> Changes in v2:
> - Corrected the signoff name and email id.
>
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index 55f443328d8e..507074f6222c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -952,6 +952,12 @@ static void a6xx_get_indexed_registers(struct msm_gpu *gpu,
>         a6xx_get_indexed_regs(gpu, a6xx_state, &a6xx_cp_mempool_indexed,
>                 &a6xx_state->indexed_regs[i]);
>
> +       if (!a6xx_state->indexed_regs[i].data) {
> +               gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, mempool_size);
> +               a6xx_state->nr_indexed_regs = count - 1;
> +               return;
> +       }

Hmm, I don't see us adjusting nr_indexed_regs if any of the earlier
sections fails, so I don't think we need to do that here either.  So I
think you could just:

if (a6xx_state->indexed_regs[i].data)
   a6xx_state->indexed_regs[i].data[0x2000] = mempool_size;

And I kinda expect if there was an allocation failure we'd just end up
dereferencing a null ptr later in the show path.

But, I think in general you can assume small GFP_KERNEL allocations
will never fail.  If necessary they will block for reclaim/shrinker to
free up some memory or evict some pages to swap.  If you've gotten to
the point where even that isn't possible, then a null ptr deref is
really the least of your problems ;-)

BR,
-R

> +
>         /*
>          * Offset 0x2000 in the mempool is the size - copy the saved size over
>          * so the data is consistent
> --
> 2.37.0
>