drm/nouveau/acpi: Fix memory leak in nouveau_acpi_edid()

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@huawei.com>
Signed-off-by: Yu Liao <liaoyu15@huawei.com>
---
 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(-)

On Thu, Nov 24, 2022 at 12:33 PM Yu Liao <liaoyu15@huawei.com> 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@huawei.com>
> Signed-off-by: Yu Liao <liaoyu15@huawei.com>
> ---
>  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
>