[PATCH 0/2] Misc fixes on vfio display

[PATCH 0/2] Misc fixes on vfio display

[Zhenzhong Duan]

Hi,

This is trying to address an issue Cédric found.
See https://www.mail-archive.com/qemu-devel@nongnu.org/msg1043142.html
While looking into it, also found a potential memory leak.

I'm sorry that I didn't find how to test this fix, because it looks
a GFX card is needed. Any idea on how to test or help test are quite
appreciated.

Thanks
Zhenzhong

Zhenzhong Duan (2):
  vfio/display: Fix potential memleak of edid info
  vfio/display: Fix vfio_display_edid_init() error path

 hw/vfio/display.c | 15 +++++++++------
 hw/vfio/helpers.c |  1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] vfio/display: Fix potential memleak of edid info

[Zhenzhong Duan]

EDID related device region info is leaked in three paths:
1. In vfio_get_dev_region_info(), when edid info isn't find, the last
device region info is leaked.
2. In vfio_display_edid_init() error path, edid info is leaked.
3. In VFIODisplay destroying path, edid info is leaked.

Fixes: 08479114b0de ("vfio/display: add edid support.")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/display.c | 2 ++
 hw/vfio/helpers.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 661e921616..5926bd6628 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -171,6 +171,7 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
 
 err:
     trace_vfio_display_edid_write_error();
+    g_free(dpy->edid_info);
     g_free(dpy->edid_regs);
     dpy->edid_regs = NULL;
     return;
@@ -182,6 +183,7 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
         return;
     }
 
+    g_free(dpy->edid_info);
     g_free(dpy->edid_regs);
     g_free(dpy->edid_blob);
     timer_free(dpy->edid_link_timer);
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index b14edd46ed..3dd32b26a4 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -586,6 +586,7 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
         g_free(*info);
     }
 
+    g_free(*info);
     *info = NULL;
     return -ENODEV;
 }
-- 
2.34.1

[PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path

[Zhenzhong Duan]

vfio_display_edid_init() can fail for many reasons and return silently.
It would be good to report the error.

Old mdev driver may not support vfio edid region and we allow to go
through in this case.

vfio_display_edid_update() isn't changed because it can be called at
runtime when UI changes (i.e. window resize).

Fixes: 08479114b0de ("vfio/display: add edid support.")
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/display.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 5926bd6628..462845ce69 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -124,7 +124,7 @@ static void vfio_display_edid_ui_info(void *opaque, uint32_t idx,
     }
 }
 
-static void vfio_display_edid_init(VFIOPCIDevice *vdev)
+static bool vfio_display_edid_init(VFIOPCIDevice *vdev, Error **errp)
 {
     VFIODisplay *dpy = vdev->dpy;
     int fd = vdev->vbasedev.fd;
@@ -135,7 +135,8 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
                                    VFIO_REGION_SUBTYPE_GFX_EDID,
                                    &dpy->edid_info);
     if (ret) {
-        return;
+        /* Failed to get GFX edid info, allow to go through without edid. */
+        return true;
     }
 
     trace_vfio_display_edid_available();
@@ -167,14 +168,15 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
                                         vfio_display_edid_link_up, vdev);
 
     vfio_display_edid_update(vdev, true, 0, 0);
-    return;
+    return true;
 
 err:
+    error_setg(errp, "vfio: failed to read GFX edid field");
     trace_vfio_display_edid_write_error();
     g_free(dpy->edid_info);
     g_free(dpy->edid_regs);
     dpy->edid_regs = NULL;
-    return;
+    return false;
 }
 
 static void vfio_display_edid_exit(VFIODisplay *dpy)
@@ -367,8 +369,7 @@ static bool vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
             return false;
         }
     }
-    vfio_display_edid_init(vdev);
-    return true;
+    return vfio_display_edid_init(vdev, errp);
 }
 
 static void vfio_display_dmabuf_exit(VFIODisplay *dpy)
-- 
2.34.1

Re: [PATCH 1/2] vfio/display: Fix potential memleak of edid info

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

Hi

On Fri, Jun 28, 2024 at 1:32 PM Zhenzhong Duan <zhenzhong.duan@intel.com>
wrote:

> EDID related device region info is leaked in three paths:
> 1. In vfio_get_dev_region_info(), when edid info isn't find, the last
> device region info is leaked.
> 2. In vfio_display_edid_init() error path, edid info is leaked.
> 3. In VFIODisplay destroying path, edid info is leaked.
>
> Fixes: 08479114b0de ("vfio/display: add edid support.")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/display.c | 2 ++
>  hw/vfio/helpers.c | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 661e921616..5926bd6628 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -171,6 +171,7 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>
>  err:
>      trace_vfio_display_edid_write_error();
> +    g_free(dpy->edid_info);
>

It would be better to set it to NULL.


>      g_free(dpy->edid_regs);
>      dpy->edid_regs = NULL;
>      return;
> @@ -182,6 +183,7 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
>          return;
>      }
>
> +    g_free(dpy->edid_info);
>      g_free(dpy->edid_regs);
>      g_free(dpy->edid_blob);
>      timer_free(dpy->edid_link_timer);
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index b14edd46ed..3dd32b26a4 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -586,6 +586,7 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev,
> uint32_t type,
>          g_free(*info);
>      }
>
> +    g_free(*info);
>

This seems incorrect, it is freed at the end of the loop above if it didn't
retun.


>      *info = NULL;
>      return -ENODEV;
>  }
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2810 bytes --]

Re: [PATCH 2/2] vfio/display: Fix vfio_display_edid_init() error path

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]

On Fri, Jun 28, 2024 at 1:31 PM Zhenzhong Duan <zhenzhong.duan@intel.com>
wrote:

> vfio_display_edid_init() can fail for many reasons and return silently.
> It would be good to report the error.
>
> Old mdev driver may not support vfio edid region and we allow to go
> through in this case.
>
> vfio_display_edid_update() isn't changed because it can be called at
> runtime when UI changes (i.e. window resize).
>
> Fixes: 08479114b0de ("vfio/display: add edid support.")
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  hw/vfio/display.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 5926bd6628..462845ce69 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -124,7 +124,7 @@ static void vfio_display_edid_ui_info(void *opaque,
> uint32_t idx,
>      }
>  }
>
> -static void vfio_display_edid_init(VFIOPCIDevice *vdev)
> +static bool vfio_display_edid_init(VFIOPCIDevice *vdev, Error **errp)
>  {
>      VFIODisplay *dpy = vdev->dpy;
>      int fd = vdev->vbasedev.fd;
> @@ -135,7 +135,8 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>                                     VFIO_REGION_SUBTYPE_GFX_EDID,
>                                     &dpy->edid_info);
>      if (ret) {
> -        return;
> +        /* Failed to get GFX edid info, allow to go through without edid.
> */
> +        return true;
>      }
>
>      trace_vfio_display_edid_available();
> @@ -167,14 +168,15 @@ static void vfio_display_edid_init(VFIOPCIDevice
> *vdev)
>                                          vfio_display_edid_link_up, vdev);
>
>      vfio_display_edid_update(vdev, true, 0, 0);
> -    return;
> +    return true;
>
>  err:
> +    error_setg(errp, "vfio: failed to read GFX edid field");
>      trace_vfio_display_edid_write_error();
>      g_free(dpy->edid_info);
>      g_free(dpy->edid_regs);
>      dpy->edid_regs = NULL;
> -    return;
> +    return false;
>  }
>
>  static void vfio_display_edid_exit(VFIODisplay *dpy)
> @@ -367,8 +369,7 @@ static bool vfio_display_dmabuf_init(VFIOPCIDevice
> *vdev, Error **errp)
>              return false;
>          }
>      }
> -    vfio_display_edid_init(vdev);
> -    return true;
> +    return vfio_display_edid_init(vdev, errp);
>  }
>
>  static void vfio_display_dmabuf_exit(VFIODisplay *dpy)
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3714 bytes --]

Re: [PATCH 1/2] vfio/display: Fix potential memleak of edid info

[Duan, Zhenzhong]

Hi,

On 6/29/2024 8:15 PM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Jun 28, 2024 at 1:32 PM Zhenzhong Duan 
> <zhenzhong.duan@intel.com> wrote:
>
>     EDID related device region info is leaked in three paths:
>     1. In vfio_get_dev_region_info(), when edid info isn't find, the last
>     device region info is leaked.
>     2. In vfio_display_edid_init() error path, edid info is leaked.
>     3. In VFIODisplay destroying path, edid info is leaked.
>
>     Fixes: 08479114b0de ("vfio/display: add edid support.")
>     Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>     ---
>      hw/vfio/display.c | 2 ++
>      hw/vfio/helpers.c | 1 +
>      2 files changed, 3 insertions(+)
>
>     diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>     index 661e921616..5926bd6628 100644
>     --- a/hw/vfio/display.c
>     +++ b/hw/vfio/display.c
>     @@ -171,6 +171,7 @@ static void
>     vfio_display_edid_init(VFIOPCIDevice *vdev)
>
>      err:
>          trace_vfio_display_edid_write_error();
>     +    g_free(dpy->edid_info);
>
>
> It would be better to set it to NULL.
Will do.
>
>          g_free(dpy->edid_regs);
>          dpy->edid_regs = NULL;
>          return;
>     @@ -182,6 +183,7 @@ static void vfio_display_edid_exit(VFIODisplay
>     *dpy)
>              return;
>          }
>
>     +    g_free(dpy->edid_info);
>          g_free(dpy->edid_regs);
>          g_free(dpy->edid_blob);
>          timer_free(dpy->edid_link_timer);
>     diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
>     index b14edd46ed..3dd32b26a4 100644
>     --- a/hw/vfio/helpers.c
>     +++ b/hw/vfio/helpers.c
>     @@ -586,6 +586,7 @@ int vfio_get_dev_region_info(VFIODevice
>     *vbasedev, uint32_t type,
>              g_free(*info);
>          }
>
>     +    g_free(*info);
>
>
> This seems incorrect, it is freed at the end of the loop above if it 
> didn't retun.

Good catch! Will remove it.

Thanks

Zhenzhong

>
>          *info = NULL;
>          return -ENODEV;
>      }
>     -- 
>     2.34.1
>
>
>
>
> -- 
> Marc-André Lureau