Re: [PATCH v2] ACPI: video: Fix random crashes due to bad kfree

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

On 1/13/2025 08:19, Mario Limonciello wrote:
> On 1/11/2025 12:59, Chris Bainbridge wrote:
>> Commit c6a837088bed ("drm/amd/display: Fetch the EDID from _DDC if
>> available for eDP") added function dm_helpers_probe_acpi_edid, which
>> fetches the EDID from the BIOS by calling acpi_video_get_edid.
>> acpi_video_get_edid returns a pointer to the EDID, but this pointer does
>> not originate from kmalloc - it is actually the internal "pointer" field
>> from an acpi_buffer struct (which did come from kmalloc).
>> dm_helpers_probe_acpi_edid then attempts to kfree the EDID pointer,
>> resulting in memory corruption which leads to random, intermittent
>> crashes (e.g. 4% of boots will fail with some Oops).
>>
>> Fix this by allocating a new array (which can be safely freed) for the
>> EDID data, and correctly freeing the acpi_buffer pointer.
>>
>> The only other caller of acpi_video_get_edid is nouveau_acpi_edid:
>> remove the extraneous kmemdup here as the EDID data is now copied in
>> acpi_video_device_EDID.
>>
>> Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
>> Fixes: c6a837088bed ("drm/amd/display: Fetch the EDID from _DDC if 
>> available for eDP")
> 
> Two minor documentation related comments to consider, otherwise I think 
> the code change looks good.  Feel free to include:
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

A few more tags to collate from another thread:

Reported-by: Borislav Petkov (AMD) <bp@alien8.de>
Closes: 
https://lore.kernel.org/amd-gfx/20250110175252.GBZ4FedNKqmBRaY4T3@fat_crate.local/T/#m324a23eb4c4c32fa7e89e31f8ba96c781e496fb1
Tested-by: Borislav Petkov (AMD) <bp@alien8.de>

> 
>> ---
>> Changes in v2:
>>     - check kmemdup() return value
>>     - move buffer management into acpi_video_device_EDID()
>>     - return actual length value of buffer
>> ---
>>   drivers/acpi/acpi_video.c              | 50 ++++++++++++++------------
>>   drivers/gpu/drm/nouveau/nouveau_acpi.c |  2 +-
>>   2 files changed, 29 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 8274a17872ed..3c627bdf2d1b 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -610,16 +610,29 @@ acpi_video_device_lcd_get_level_current(struct 
>> acpi_video_device *device,
>>       return 0;
>>   }
>> +/*
>> + *  Arg:
> 
> As you've pretty much written kernel doc, us it better to just make this 
> proper kerneldoc (IE use /**)?
> 
>> + *    device    : video output device (LCD, CRT, ..)
>> + *    edid    : address for returned EDID pointer
>> + *    length  : _DDC length to request (must be a multiple of 128)
>> + *
>> + *  Return Value:
>> + *    Length of EDID (positive value) or error (negative value)
>> + *
>> + *  Get EDID from ACPI _DDC. On success, a pointer to the EDID data 
>> is written
>> + *  to the edid address, and the length of the EDID is returned. The 
>> caller is
> 
> Since 'EDID' and 'edid' mean different things in the context of this 
> description for the purpose of clarity I think it would be better to say 
> "the edid pointer address".
> 
>> + *  responsible for freeing the edid pointer.
>> + */
>> +
>>   static int
>> -acpi_video_device_EDID(struct acpi_video_device *device,
>> -               union acpi_object **edid, int length)
>> +acpi_video_device_EDID(struct acpi_video_device *device, void **edid, 
>> int length)
>>   {
>> -    int status;
>> +    acpi_status status;
>>       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>>       union acpi_object *obj;
>>       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>>       struct acpi_object_list args = { 1, &arg0 };
>> -
>> +    int ret;
>>       *edid = NULL;
>> @@ -636,16 +649,17 @@ acpi_video_device_EDID(struct acpi_video_device 
>> *device,
>>       obj = buffer.pointer;
>> -    if (obj && obj->type == ACPI_TYPE_BUFFER)
>> -        *edid = obj;
>> -    else {
>> +    if (obj && obj->type == ACPI_TYPE_BUFFER) {
>> +        *edid = kmemdup(obj->buffer.pointer, obj->buffer.length, 
>> GFP_KERNEL);
>> +        ret = *edid ? obj->buffer.length : -ENOMEM;
>> +    } else {
>>           acpi_handle_debug(device->dev->handle,
>>                    "Invalid _DDC data for length %d\n", length);
>> -        status = -EFAULT;
>> -        kfree(obj);
>> +        ret = -EFAULT;
>>       }
>> -    return status;
>> +    kfree(obj);
>> +    return ret;
>>   }
>>   /* bus */
>> @@ -1435,9 +1449,7 @@ int acpi_video_get_edid(struct acpi_device 
>> *device, int type, int device_id,
>>   {
>>       struct acpi_video_bus *video;
>>       struct acpi_video_device *video_device;
>> -    union acpi_object *buffer = NULL;
>> -    acpi_status status;
>> -    int i, length;
>> +    int i, length, ret;
>>       if (!device || !acpi_driver_data(device))
>>           return -EINVAL;
>> @@ -1477,16 +1489,10 @@ int acpi_video_get_edid(struct acpi_device 
>> *device, int type, int device_id,
>>           }
>>           for (length = 512; length > 0; length -= 128) {
>> -            status = acpi_video_device_EDID(video_device, &buffer,
>> -                            length);
>> -            if (ACPI_SUCCESS(status))
>> -                break;
>> +            ret = acpi_video_device_EDID(video_device, edid, length);
>> +            if (ret > 0)
>> +                return ret;
>>           }
>> -        if (!length)
>> -            continue;
>> -
>> -        *edid = buffer->buffer.pointer;
>> -        return length;
>>       }
>>       return -ENODEV;
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/ 
>> nouveau/nouveau_acpi.c
>> index 8f0c69aad248..21b56cc7605c 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
>> @@ -384,7 +384,7 @@ nouveau_acpi_edid(struct drm_device *dev, struct 
>> drm_connector *connector)
>>       if (ret < 0)
>>           return NULL;
>> -    return kmemdup(edid, EDID_LENGTH, GFP_KERNEL);
>> +    return edid;
>>   }
>>   bool nouveau_acpi_video_backlight_use_native(void)
>