diff mbox series

[1/5] drm/amdgpu: Move a variable assignment behind a null pointer check in amdgpu_ras_interrupt_dispatch()

Message ID 0d4b92ab-f7c2-4f18-f3c3-c0f82ba47fc8@web.de (mailing list archive)
State New, archived
Headers show
Series drm/amd: Adjustments for three function implementations | expand

Commit Message

Markus Elfring April 11, 2023, 1:42 p.m. UTC 
Date: Tue, 11 Apr 2023 10:52:48 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “amdgpu_ras_interrupt_dispatch”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “data” behind the null pointer check.

This issue was detected by using the Coccinelle software.

Fixes: c030f2e4166c3f5597c7e7a70bcd9ab383695de4 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
2.40.0

Comments

Felix Kuehling April 11, 2023, 1:59 p.m. UTC | #1 
Am 2023-04-11 um 09:42 schrieb Markus Elfring:
> Date: Tue, 11 Apr 2023 10:52:48 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “amdgpu_ras_interrupt_dispatch”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “data” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: c030f2e4166c3f5597c7e7a70bcd9ab383695de4 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 4069bce9479f..a920c7888d07 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct amdgpu_device *adev,
>   		struct ras_dispatch_if *info)
>   {
>   	struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
> -	struct ras_ih_data *data = &obj->ih_data;
> +	struct ras_ih_data *data;
I'm curious, this only takes the address of obj->ih_data. It doesn't 
dereference the pointer until after the !obj check below. How is this 
undefined behaviour? Is this about the compiler being free to reorder 
stuff for optimization, unaware of the dependency? Is there a link to an 
explanation that could be added to the commit description?

Thanks,
   Felix


>
>   	if (!obj)
>   		return -EINVAL;
>
> +	data = &obj->ih_data;
>   	if (data->inuse == 0)
>   		return 0;
>
> --
> 2.40.0
>
Markus Elfring April 11, 2023, 2:45 p.m. UTC | #2 
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>> @@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct amdgpu_device *adev,
>>           struct ras_dispatch_if *info)
>>   {
>>       struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
>> -    struct ras_ih_data *data = &obj->ih_data;
>> +    struct ras_ih_data *data;
> I'm curious, this only takes the address of obj->ih_data.

Even if a null pointer would accidentally be returned by a call of
the function “amdgpu_ras_find_obj”?
https://elixir.bootlin.com/linux/v6.3-rc6/source/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c#L618


> It doesn't dereference the pointer until after the !obj check below.

Does the used arrow operator indicate a pointer dereference?


> How is this undefined behaviour?

I guess that another information source can be helpful for such an issue.
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504153#comment-405504153

Regards,
Markus
Markus Elfring Sept. 9, 2024, 9:42 a.m. UTC | #3 
> Date: Tue, 11 Apr 2023 10:52:48 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “amdgpu_ras_interrupt_dispatch”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “data” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: c030f2e4166c3f5597c7e7a70bcd9ab383695de4 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 4069bce9479f..a920c7888d07 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct amdgpu_device *adev,
>  		struct ras_dispatch_if *info)
>  {
>  	struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
> -	struct ras_ih_data *data = &obj->ih_data;
> +	struct ras_ih_data *data;
>
>  	if (!obj)
>  		return -EINVAL;
>
> +	data = &obj->ih_data;
>  	if (data->inuse == 0)
>  		return 0;
>

I would like to point out that another software adjustment got the desired
development attention (on 2024-05-11).

See also:
Commit 4c11d30c95576937c6c35e6f29884761f2dddb43 ("drm/amdgpu:
Fix the null pointer dereference to ras_manager")

Regards,
Markus
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 4069bce9479f..a920c7888d07 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -1730,11 +1730,12 @@  int amdgpu_ras_interrupt_dispatch(struct amdgpu_device *adev,
 		struct ras_dispatch_if *info)
 {
 	struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
-	struct ras_ih_data *data = &obj->ih_data;
+	struct ras_ih_data *data;

 	if (!obj)
 		return -EINVAL;

+	data = &obj->ih_data;
 	if (data->inuse == 0)
 		return 0;