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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <af5edb6c-3075-420e-b52f-05844c728180@amd.com>
Date: Fri, 1 Nov 2024 11:22:11 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Antonio Quartulli <antonio@...delbit.com>
Cc: Xinhui.Pan@....com, alexander.deucher@....com, christian.koenig@....com,
 amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] amdgpu: prevent NULL pointer dereference if ATIF is
 not supported

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@...delbit.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@....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);
>>
>