On Thu, Nov 24, 2022 at 12:33 PM Yu Liao <liaoyu15@xxxxxxxxxx> wrote:
>
> The ACPI buffer memory 'edid' should be freed as the buffer is not used
> after kmemdup(). But we can't free 'edid' directly because it doesn't
> point to acpi_object which should be passed to kfree(). Make
> acpi_video_get_edid() get the address of acpi_object instead, so we can
> free it to prevent memory leak.
>
> Fixes: 24b102d3488c ("drm/nouveau: we can't free ACPI EDID, so make a copy that we can")
> Reported-by: Hanjun Guo <guohanjun@xxxxxxxxxx>
> Signed-off-by: Yu Liao <liaoyu15@xxxxxxxxxx>
> ---
> drivers/acpi/acpi_video.c | 4 ++--
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 8 ++++++--
> include/acpi/video.h | 5 +++--
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 32953646caeb..f050a755efef 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -1441,7 +1441,7 @@ acpi_video_switch_brightness(struct work_struct *work)
> }
>
> int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
> - void **edid)
> + union acpi_object **edid)
I don't think you need to make this change.
> {
> struct acpi_video_bus *video;
> struct acpi_video_device *video_device;
> @@ -1500,7 +1500,7 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
> }
> }
>
> - *edid = buffer->buffer.pointer;
> + *edid = buffer;
Because this would still be valid without the previous one.
However, why can't you just free the buffer after storing the
buffer.pointer value under the address pointed to by edid?
> return length;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> index 8cf096f841a9..075ecad31572 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> @@ -365,7 +365,8 @@ nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector)
> {
> struct acpi_device *acpidev;
> int type, ret;
> - void *edid;
> + union acpi_object *edid;
> + void *ptr;
>
> switch (connector->connector_type) {
> case DRM_MODE_CONNECTOR_LVDS:
> @@ -384,7 +385,10 @@ nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector)
> if (ret < 0)
> return NULL;
>
> - return kmemdup(edid, EDID_LENGTH, GFP_KERNEL);
> + ptr = kmemdup(edid->buffer.pointer, EDID_LENGTH, GFP_KERNEL);
Here you ccould do
ptr = kmemdup(((union acpi_object *)edid)->buffer.pointer,
EDID_LENGTH, GFP_KERNEL);
> + kfree(edid);
But anyway I don't think you need to defer freeing the buffer
allocated by acpi_video_get_edid() till this point.
> +
> + return ptr;
> }
>
> bool nouveau_acpi_video_backlight_use_native(void)
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index a275c35e5249..868749f95a34 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -19,6 +19,7 @@ struct acpi_video_device_brightness {
> };
>
> struct acpi_device;
> +union acpi_object;
>
> #define ACPI_VIDEO_CLASS "video"
>
> @@ -57,7 +58,7 @@ extern int acpi_video_register(void);
> extern void acpi_video_unregister(void);
> extern void acpi_video_register_backlight(void);
> extern int acpi_video_get_edid(struct acpi_device *device, int type,
> - int device_id, void **edid);
> + int device_id, union acpi_object **edid);
> extern enum acpi_backlight_type acpi_video_get_backlight_type(void);
> extern bool acpi_video_backlight_use_native(void);
> /*
> @@ -73,7 +74,7 @@ static inline int acpi_video_register(void) { return -ENODEV; }
> static inline void acpi_video_unregister(void) { return; }
> static inline void acpi_video_register_backlight(void) { return; }
> static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> - int device_id, void **edid)
> + int device_id, union acpi_object **edid)
> {
> return -ENODEV;
> }
> --
> 2.25.1
>
