Re: CVE-2023-52605: ACPI: extlog: fix NULL pointer dereference check

["Wysocki, Rafael J" <rafael.j.wysocki@intel.com>]

On 3/14/2024 12:01 PM, Lee Jones wrote:
> On Mon, 11 Mar 2024, Prarit Bhargava wrote:
>
>> On 3/10/24 04:10, Vegard Nossum wrote:
>>> (Added author/maintainer to Cc)
>>>
>>> On 06/03/2024 07:46, Greg Kroah-Hartman wrote:
>>>> Description
>>>> ===========
>>>>
>>>> In the Linux kernel, the following vulnerability has been resolved:
>>>>
>>>> ACPI: extlog: fix NULL pointer dereference check
>>>>
>>>> The gcc plugin -fanalyzer [1] tries to detect various
>>>> patterns of incorrect behaviour.  The tool reports:
>>>>
>>>> drivers/acpi/acpi_extlog.c: In function ‘extlog_exit’:
>>>> drivers/acpi/acpi_extlog.c:307:12: warning: check of
>>>> ‘extlog_l1_addr’ for NULL after already dereferencing it
>>>> [-Wanalyzer-deref-before-check]
>>>>       |
>>>>       |  306 |         ((struct extlog_l1_head
>>>> *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
>>>>       |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
>>>>       |      |                                                  |
>>>>       |      |                                                  (1)
>>>> pointer ‘extlog_l1_addr’ is dereferenced here
>>>>       |  307 |         if (extlog_l1_addr)
>>>>       |      |            ~
>>>>       |      |            |
>>>>       |      |            (2) pointer ‘extlog_l1_addr’ is checked for
>>>> NULL here but it was already dereferenced at (1)
>>>>       |
>>>>
>>>> Fix the NULL pointer dereference check in extlog_exit().
>>>>
>>>> The Linux kernel CVE team has assigned CVE-2023-52605 to this issue.
>>> This code is in an __exit function:
>>>
>>> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
>>> index e120a96e1eaee..193147769146e 100644
>>> --- a/drivers/acpi/acpi_extlog.c
>>> +++ b/drivers/acpi/acpi_extlog.c
>>> @@ -303,9 +303,10 @@ err:
>>>    static void __exit extlog_exit(void)
>>>    {
>>>        mce_unregister_decode_chain(&extlog_mce_dec);
>>> -    ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
>>> -    if (extlog_l1_addr)
>>> +    if (extlog_l1_addr) {
>>> +        ((struct extlog_l1_head *)extlog_l1_addr)->flags &=
>>> ~FLAG_OS_OPTIN;
>>>            acpi_os_unmap_iomem(extlog_l1_addr, l1_size);
>>> +    }
>>>        if (elog_addr)
>>>            acpi_os_unmap_iomem(elog_addr, elog_size);
>>>        release_mem_region(elog_base, elog_size);
>>>
>>> This can only run when you unload a module, which is a privileged
>>> operation (restricted to CAP_SYS_MODULE).
>>>
>>> Moreover, extlog_l1_addr is only ever assigned in the corresponding
>>> module init function, and it looks like it will never be NULL if the
>>> module was loaded successfully, at least on a recent mainline kernel.
>>>
>>> Since the module exit won't be called unless module init succeeded, I
>>> don't see a way to trigger this bug. Is this a vulnerability?
>>>
>> This is certainly not a CVE.
>>
>>> It might be better to just delete the NULL check altogether.
>>>
>>> As usual, I could be wrong...
>>>
>> When I made this code change I thought the same thing: Perhaps it's better
>> to remove the NULL check given the status of the code.  I assumed that the
>> check was there as a failsafe on unload.
> If Rafael agrees with you both, I'd be happy to revoke its CVE status.

I do agree with the analysis above, sorry for the delay.

Thanks,

Rafael