| Message ID | Z2yQvTyg_MWwrlj3@debian.local (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/amd: Fix random crashes due to bad kfree | expand |
Hi Chris! On 12/26/24 00:09, 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 in acpi_video_get_edid, and correctly freeing the acpi_buffer. Hmm, maybe I'm missing something here. But shouldn't it suffice to just remove the kfree call in dm_helpers_probe_acpi_edid()? With best wishes, Tobias > > 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_get_edid. > > Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com> > Fixes: c6a837088bed ("drm/amd/display: Fetch the EDID from _DDC if available for eDP") > --- > drivers/acpi/acpi_video.c | 3 ++- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 8274a17872ed..151d1d534264 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -1485,7 +1485,8 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, > if (!length) > continue; > > - *edid = buffer->buffer.pointer; > + *edid = kmemdup(buffer->buffer.pointer, length, GFP_KERNEL); > + kfree(buffer); > return length; > } > > 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)
On Thu, Dec 26, 2024 at 12:19:02AM +0100, Tobias Jakobi wrote: > Hi Chris! > > On 12/26/24 00:09, 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 in acpi_video_get_edid, and correctly freeing the acpi_buffer. > > Hmm, maybe I'm missing something here. But shouldn't it suffice to just > remove the kfree call in dm_helpers_probe_acpi_edid()? Yes, that would work to fix the bad kfree, but there would be a small memory leak of the acpi_buffer struct. It's not a huge problem since this code is rarely run, and the Nouveau code has never tried to free the edid buffer and apparently nobody noticed, but it would be better to do the correct thing. One other curiosity is this comment in the code that allocates the memory: case ACPI_ALLOCATE_BUFFER: /* * Allocate a new buffer. We directectly call acpi_os_allocate here to * purposefully bypass the (optionally enabled) internal allocation * tracking mechanism since we only want to track internal * allocations. Note: The caller should use acpi_os_free to free this * buffer created via ACPI_ALLOCATE_BUFFER. */ Which makes me wonder if all the calls to kfree on acpi_buffer structs with ACPI_ALLOCATE_BUFFER in acpi_video.c should actually be calls to acpi_os_free instead? I used kfree just for consistency with the existing code.
On 12/26/24 02:27, Chris Bainbridge wrote: > On Thu, Dec 26, 2024 at 12:19:02AM +0100, Tobias Jakobi wrote: >> Hi Chris! >> >> On 12/26/24 00:09, 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 in acpi_video_get_edid, and correctly freeing the acpi_buffer. >> Hmm, maybe I'm missing something here. But shouldn't it suffice to just >> remove the kfree call in dm_helpers_probe_acpi_edid()? > Yes, that would work to fix the bad kfree, but there would be a small > memory leak of the acpi_buffer struct. It's not a huge problem since > this code is rarely run, and the Nouveau code has never tried to free > the edid buffer and apparently nobody noticed, but it would be better to > do the correct thing. OK, thanks for explaining. I didn't immediately understand that something was leaking memory. Only that we were freeing something that we are not supposed to free. > One other curiosity is this comment in the code that allocates the > memory: > > case ACPI_ALLOCATE_BUFFER: > /* > * Allocate a new buffer. We directectly call acpi_os_allocate here to > * purposefully bypass the (optionally enabled) internal allocation > * tracking mechanism since we only want to track internal > * allocations. Note: The caller should use acpi_os_free to free this > * buffer created via ACPI_ALLOCATE_BUFFER. > */ > > Which makes me wonder if all the calls to kfree on acpi_buffer structs > with ACPI_ALLOCATE_BUFFER in acpi_video.c should actually be calls to > acpi_os_free instead? I used kfree just for consistency with the > existing code. Wouldn't it make more sense to do the memdup handling in acpi_video_device_EDID()? This way you have both alloc and free in the same function. But I'm no expert when it comes to the ACPI kernel code. Just my two cents :-D With best wishes, Tobias
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 8274a17872ed..151d1d534264 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -1485,7 +1485,8 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, if (!length) continue; - *edid = buffer->buffer.pointer; + *edid = kmemdup(buffer->buffer.pointer, length, GFP_KERNEL); + kfree(buffer); return length; } 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)
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 in acpi_video_get_edid, and correctly freeing the acpi_buffer. 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_get_edid. Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com> Fixes: c6a837088bed ("drm/amd/display: Fetch the EDID from _DDC if available for eDP") --- drivers/acpi/acpi_video.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)