diff mbox series

[2/2] drm/msm/a6xx: Fix NULL dereference during crashstate capture

Message ID 1544443462-28736-2-git-send-email-smasetty@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/msm/adreno: Make adreno_gpu_state_get() return void | expand

Commit Message

Sharat Masetty Dec. 10, 2018, 12:04 p.m. UTC
The gpu crashstate's base objects registers pointer can be NULL if the
target implementation decides to capture the register dump on its own.
This patch simply checks for NULL before dereferencing.

Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Jordan Crouse Dec. 10, 2018, 3:39 p.m. UTC | #1
On Mon, Dec 10, 2018 at 05:34:22PM +0530, Sharat Masetty wrote:
> The gpu crashstate's base objects registers pointer can be NULL if the
> target implementation decides to capture the register dump on its own.
> This patch simply checks for NULL before dereferencing.
> 
> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 40bcf32..a39cebc 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -415,6 +415,9 @@ void adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state)
>  		}
>  	}
>  
> +	if (!adreno_gpu->registers)
> +		return;
> +

This looks good - we should get it in the 4.21 pull.

>  	/* Count the number of registers */
>  	for (i = 0; adreno_gpu->registers[i] != ~0; i += 2)
>  		count += adreno_gpu->registers[i + 1] -
> @@ -550,12 +553,14 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
>  		}
>  	}
>  
> -	drm_puts(p, "registers:\n");
> +	if (state->nr_registers > 0) {
> +		drm_puts(p, "registers:\n");
>  
> -	for (i = 0; i < state->nr_registers; i++) {
> -		drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
> -			state->registers[i * 2] << 2,
> -			state->registers[(i * 2) + 1]);
> +		for (i = 0; i < state->nr_registers; i++) {
> +			drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
> +					state->registers[i * 2] << 2,
> +					state->registers[(i * 2) + 1]);
> +		}

I don't think we need the extra indentation here - something like

for (i = 0; i < state->nr_registers; i++) {
+	if (i == 0)
+		drm_puts(p, "Registers:\n");
	drm_printf(p, " - { offset: 0x%04x, value: 0x%08x }\n",

would suffice since we won't go into the loop if state->nr_registers == 0.

Jordan
Jordan Crouse Dec. 11, 2018, 4:30 p.m. UTC | #2
On Mon, Dec 10, 2018 at 05:34:22PM +0530, Sharat Masetty wrote:
> The gpu crashstate's base objects registers pointer can be NULL if the
> target implementation decides to capture the register dump on its own.
> This patch simply checks for NULL before dereferencing.

Hi Sharat - this doesn't apply against msm-next - it looks like a similar fix
has already been done.

Jordan

> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 40bcf32..a39cebc 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -415,6 +415,9 @@ void adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state)
>  		}
>  	}
>  
> +	if (!adreno_gpu->registers)
> +		return;
> +
>  	/* Count the number of registers */
>  	for (i = 0; adreno_gpu->registers[i] != ~0; i += 2)
>  		count += adreno_gpu->registers[i + 1] -
> @@ -550,12 +553,14 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
>  		}
>  	}
>  
> -	drm_puts(p, "registers:\n");
> +	if (state->nr_registers > 0) {
> +		drm_puts(p, "registers:\n");
>  
> -	for (i = 0; i < state->nr_registers; i++) {
> -		drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
> -			state->registers[i * 2] << 2,
> -			state->registers[(i * 2) + 1]);
> +		for (i = 0; i < state->nr_registers; i++) {
> +			drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
> +					state->registers[i * 2] << 2,
> +					state->registers[(i * 2) + 1]);
> +		}
>  	}
>  }
>  #endif
> -- 
> 1.9.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 40bcf32..a39cebc 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -415,6 +415,9 @@  void adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state)
 		}
 	}
 
+	if (!adreno_gpu->registers)
+		return;
+
 	/* Count the number of registers */
 	for (i = 0; adreno_gpu->registers[i] != ~0; i += 2)
 		count += adreno_gpu->registers[i + 1] -
@@ -550,12 +553,14 @@  void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
 		}
 	}
 
-	drm_puts(p, "registers:\n");
+	if (state->nr_registers > 0) {
+		drm_puts(p, "registers:\n");
 
-	for (i = 0; i < state->nr_registers; i++) {
-		drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
-			state->registers[i * 2] << 2,
-			state->registers[(i * 2) + 1]);
+		for (i = 0; i < state->nr_registers; i++) {
+			drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
+					state->registers[i * 2] << 2,
+					state->registers[(i * 2) + 1]);
+		}
 	}
 }
 #endif