[PATCH] drm/amdgpu: Handle potential NULL pointer dereference

[Russell, Kent]

[AMD Official Use Only - General]

It does indeed short-circuit on || (If the left side of an || statement is not 0, it doesn't evaluate the right side and returns true). So we can ignore this patch, since checking for each individual field on the 2nd term is probably overkill. We were still investigating what got passed in and why it wasn't valid, so I'll drop this for now. Thanks Lijo!

 Kent

-----Original Message-----
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Russell, Kent
Sent: Thursday, August 25, 2022 8:52 AM
To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Ghannam, Yazen <Yazen.Ghannam at amd.com>
Subject: RE: [PATCH] drm/amdgpu: Handle potential NULL pointer dereference

[AMD Official Use Only - General]

Good point, as if (1 || 0) should short-circuit at the if (1) part. Thus we should go down to NOTIFY_DONE if m is NULL. Yazen can confirm here, as he was the one who with me when we found the original issue. It's possible that one of the 3 message fields was NULL instead of the actual message in our repro situation, but I'll double-check the short-circuit side of C to confirm. I know it short circuits for &&, I don't know if it does for ||.

 Kent

-----Original Message-----
From: Lazar, Lijo <Lijo.Lazar at amd.com> 
Sent: Thursday, August 25, 2022 8:34 AM
To: Russell, Kent <Kent.Russell at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Ghannam, Yazen <Yazen.Ghannam at amd.com>
Subject: Re: [PATCH] drm/amdgpu: Handle potential NULL pointer dereference



On 8/25/2022 5:16 PM, Russell, Kent wrote:
> [AMD Official Use Only - General]
> 
> Friendly ping
> 

Wonder how it goes any further when m is NULL. It should do shortcut evaluation and return NOTIFY_DONE, right? Or is this for better readability?

Thanks,
Lijo

>   Kent
> 
> -----Original Message-----
> From: Russell, Kent <Kent.Russell at amd.com>
> Sent: Monday, August 15, 2022 11:31 AM
> To: amd-gfx at lists.freedesktop.org
> Cc: Ghannam, Yazen <Yazen.Ghannam at amd.com>; Russell, Kent 
> <Kent.Russell at amd.com>
> Subject: [PATCH] drm/amdgpu: Handle potential NULL pointer dereference
> 
> If m is NULL, we will end up referencing a NULL pointer in the subsequent m elements like extcpu, bank and status. Pull the NULL check out and do it first before referencing m's elements.
> 
> Signed-off-by: Kent Russell <kent.russell at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index ab9ba5a9c33d..028495fdfa62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -2838,12 +2838,15 @@ static int amdgpu_bad_page_notifier(struct notifier_block *nb,
>   	struct eeprom_table_record err_rec;
>   	uint64_t retired_page;
>   
> +	if (!m)
> +		return NOTIFY_DONE;
> +
>   	/*
>   	 * If the error was generated in UMC_V2, which belongs to GPU UMCs,
>   	 * and error occurred in DramECC (Extended error code = 0) then only
>   	 * process the error, else bail out.
>   	 */
> -	if (!m || !((smca_get_bank_type(m->extcpu, m->bank) == SMCA_UMC_V2) &&
> +	if (!((smca_get_bank_type(m->extcpu, m->bank) == SMCA_UMC_V2) &&
>   		    (XEC(m->status, 0x3f) == 0x0)))
>   		return NOTIFY_DONE;
>   
> --
> 2.25.1
>