[v2] amdgpu: prevent NULL pointer dereference if ATIF is not supported

[PATCH v2] amdgpu: prevent NULL pointer dereference if ATIF is not supported

Posted by Antonio Quartulli 3 weeks, 3 days ago

acpi_evaluate_object() may return AE_NOT_FOUND (failure), which
would result in dereferencing buffer.pointer (obj) while being NULL.

Although this case may be unrealistic for the current code, it is
still better to protect against possible bugs.

Bail out also when status is AE_NOT_FOUND.

This fixes 1 FORWARD_NULL issue reported by Coverity
Report: CID 1600951:  Null pointer dereferences  (FORWARD_NULL)

Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index cce85389427f..b8d4e07d2043 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -172,8 +172,8 @@ static union acpi_object *amdgpu_atif_call(struct amdgpu_atif *atif,
 				      &buffer);
 	obj = (union acpi_object *)buffer.pointer;
 
-	/* Fail if calling the method fails and ATIF is supported */
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+	/* Fail if calling the method fails */
+	if (ACPI_FAILURE(status)) {
 		DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
 				 acpi_format_exception(status));
 		kfree(obj);
-- 
2.45.2

Re: [PATCH v2] amdgpu: prevent NULL pointer dereference if ATIF is not supported

Posted by Mario Limonciello 3 weeks, 3 days ago

On 10/31/2024 10:28, Antonio Quartulli wrote:
> acpi_evaluate_object() may return AE_NOT_FOUND (failure), which
> would result in dereferencing buffer.pointer (obj) while being NULL.
> 
> Although this case may be unrealistic for the current code, it is
> still better to protect against possible bugs.
> 
> Bail out also when status is AE_NOT_FOUND.
> 
> This fixes 1 FORWARD_NULL issue reported by Coverity
> Report: CID 1600951:  Null pointer dereferences  (FORWARD_NULL)
> 
> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>

Can you please dig up the right Fixes: tag?

Besides that, LGTM.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index cce85389427f..b8d4e07d2043 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -172,8 +172,8 @@ static union acpi_object *amdgpu_atif_call(struct amdgpu_atif *atif,
>   				      &buffer);
>   	obj = (union acpi_object *)buffer.pointer;
>   
> -	/* Fail if calling the method fails and ATIF is supported */
> -	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> +	/* Fail if calling the method fails */
> +	if (ACPI_FAILURE(status)) {
>   		DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
>   				 acpi_format_exception(status));
>   		kfree(obj);

Re: [PATCH v2] amdgpu: prevent NULL pointer dereference if ATIF is not supported

Posted by Antonio Quartulli 3 weeks, 3 days ago

On 31/10/2024 20:37, Mario Limonciello wrote:
> On 10/31/2024 10:28, Antonio Quartulli wrote:
>> acpi_evaluate_object() may return AE_NOT_FOUND (failure), which
>> would result in dereferencing buffer.pointer (obj) while being NULL.
>>
>> Although this case may be unrealistic for the current code, it is
>> still better to protect against possible bugs.
>>
>> Bail out also when status is AE_NOT_FOUND.
>>
>> This fixes 1 FORWARD_NULL issue reported by Coverity
>> Report: CID 1600951:  Null pointer dereferences  (FORWARD_NULL)
>>
>> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
> 
> Can you please dig up the right Fixes: tag?

Fixes: c9b7c809b89f ("drm/amd: Guard against bad data for ATIF ACPI method")

Your commit :)

Should I send v3 with the Fixes tag in it?

Interestingly, this pattern of checking for AE_NOT_FOUND is shared by 
other functions, however, they don't try to dereference the pointer to 
the buffer before the return statement (which caused the Coverity report).
It's the caller that checks if the return value is NULL or not.

For this function it was the same, until you added this extra check on 
obj->type, without checking if obj was NULL or not.

If we want to keep the original pattern and continue checking for 
AE_NOT_FOUND, we could rather do:

-       if (obj->type != ACPI_TYPE_BUFFER) {
+       if (obj && obj->type != ACPI_TYPE_BUFFER) {

But this feel more like "bike shed color picking" than anything else :)
Anyway, up to you Mario, I am open to change the patch again if the 
latter pattern is more preferable.

Regards,

> 
> Besides that, LGTM.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/ 
>> drm/amd/amdgpu/amdgpu_acpi.c
>> index cce85389427f..b8d4e07d2043 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> @@ -172,8 +172,8 @@ static union acpi_object *amdgpu_atif_call(struct 
>> amdgpu_atif *atif,
>>                         &buffer);
>>       obj = (union acpi_object *)buffer.pointer;
>> -    /* Fail if calling the method fails and ATIF is supported */
>> -    if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
>> +    /* Fail if calling the method fails */
>> +    if (ACPI_FAILURE(status)) {
>>           DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
>>                    acpi_format_exception(status));
>>           kfree(obj);
> 

-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com

Re: [PATCH v2] amdgpu: prevent NULL pointer dereference if ATIF is not supported

Posted by Mario Limonciello 3 weeks, 2 days ago

On 10/31/2024 15:50, Antonio Quartulli wrote:
> On 31/10/2024 20:37, Mario Limonciello wrote:
>> On 10/31/2024 10:28, Antonio Quartulli wrote:
>>> acpi_evaluate_object() may return AE_NOT_FOUND (failure), which
>>> would result in dereferencing buffer.pointer (obj) while being NULL.
>>>
>>> Although this case may be unrealistic for the current code, it is
>>> still better to protect against possible bugs.
>>>
>>> Bail out also when status is AE_NOT_FOUND.
>>>
>>> This fixes 1 FORWARD_NULL issue reported by Coverity
>>> Report: CID 1600951:  Null pointer dereferences  (FORWARD_NULL)
>>>
>>> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
>>
>> Can you please dig up the right Fixes: tag?
> 
> Fixes: c9b7c809b89f ("drm/amd: Guard against bad data for ATIF ACPI 
> method")
> 
> Your commit :)
> 
> Should I send v3 with the Fixes tag in it?

Don't worry about it, I'll pick it up while we commit it.

Thanks!

> 
> Interestingly, this pattern of checking for AE_NOT_FOUND is shared by 
> other functions, however, they don't try to dereference the pointer to 
> the buffer before the return statement (which caused the Coverity report).
> It's the caller that checks if the return value is NULL or not.
> 
> For this function it was the same, until you added this extra check on 
> obj->type, without checking if obj was NULL or not.
> 
> If we want to keep the original pattern and continue checking for 
> AE_NOT_FOUND, we could rather do:
> 
> -       if (obj->type != ACPI_TYPE_BUFFER) {
> +       if (obj && obj->type != ACPI_TYPE_BUFFER) {
> 
> But this feel more like "bike shed color picking" than anything else :)
> Anyway, up to you Mario, I am open to change the patch again if the 
> latter pattern is more preferable.
> 
> Regards,
> 
>>
>> Besides that, LGTM.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/ 
>>> drm/amd/amdgpu/amdgpu_acpi.c
>>> index cce85389427f..b8d4e07d2043 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> @@ -172,8 +172,8 @@ static union acpi_object *amdgpu_atif_call(struct 
>>> amdgpu_atif *atif,
>>>                         &buffer);
>>>       obj = (union acpi_object *)buffer.pointer;
>>> -    /* Fail if calling the method fails and ATIF is supported */
>>> -    if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
>>> +    /* Fail if calling the method fails */
>>> +    if (ACPI_FAILURE(status)) {
>>>           DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
>>>                    acpi_format_exception(status));
>>>           kfree(obj);
>>
>

Re: [PATCH v2] amdgpu: prevent NULL pointer dereference if ATIF is not supported

Posted by Markus Elfring 3 weeks, 3 days ago

…
> Although this case may be unrealistic for the current code, it is
> still better to protect against possible bugs.
>
> Bail out also when status is AE_NOT_FOUND.
…

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc5#n145


Regards,
Markus