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

[Mario Limonciello <mario.limonciello@amd.com>]

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);
>>
>