[PATCH 0/2] hw/cxl: CDAT file handling fixes.

[PATCH 0/2] hw/cxl: CDAT file handling fixes.

[Jonathan Cameron via]

I've picked up Hao Zeng's v5 and added a patch to deal with the
issues that Peter pointed out in this area.

On error, the CDAT file handling left resource dangling and didn't
cleanly exit from calling functions.  Hao Zeng dealt with closing
the file. The second patch deals with the buffer allocation and
ensuring a clean exit at the two callers.

Hao Zeng (1):
  hw/cxl: cdat: Fix open file not closed in ct3_load_cdat()

Jonathan Cameron (1):
  hw/cxl: cdat: Fix failure to free buffer in erorr paths

 hw/cxl/cxl-cdat.c            | 60 +++++++++++++++---------------------
 hw/mem/cxl_type3.c           |  4 +++
 hw/pci-bridge/cxl_upstream.c |  3 ++
 3 files changed, 32 insertions(+), 35 deletions(-)

-- 
2.37.2

[PATCH 1/2] hw/cxl: cdat: Fix open file not closed in ct3_load_cdat()

[Jonathan Cameron via]

From: Hao Zeng <zenghao@kylinos.cn>

Open file descriptor not closed in error paths. Fix by replace
open coded handling of read of whole file into a buffer with
g_file_get_contents()

Fixes: aba578bdac ("hw/cxl: CDAT Data Object Exchange implementation")
Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Jonathan Cameron via <qemu-devel@nongnu.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

--
Changes since v5:
- Drop if guard on g_free() as per checkpatch warning.
---
 hw/cxl/cxl-cdat.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 137abd0992..056711d63d 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -110,29 +110,18 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
     g_autofree CDATEntry *cdat_st = NULL;
     uint8_t sum = 0;
     int num_ent;
-    int i = 0, ent = 1, file_size = 0;
+    int i = 0, ent = 1;
+    gsize file_size = 0;
     CDATSubHeader *hdr;
-    FILE *fp = NULL;
+    GError *error = NULL;
 
     /* Read CDAT file and create its cache */
-    fp = fopen(cdat->filename, "r");
-    if (!fp) {
-        error_setg(errp, "CDAT: Unable to open file");
+    if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
+                             &file_size, &error)) {
+        error_setg(errp, "CDAT: File read failed: %s", error->message);
+        g_error_free(error);
         return;
     }
-
-    fseek(fp, 0, SEEK_END);
-    file_size = ftell(fp);
-    fseek(fp, 0, SEEK_SET);
-    cdat->buf = g_malloc0(file_size);
-
-    if (fread(cdat->buf, file_size, 1, fp) == 0) {
-        error_setg(errp, "CDAT: File read failed");
-        return;
-    }
-
-    fclose(fp);
-
     if (file_size < sizeof(CDATTableHeader)) {
         error_setg(errp, "CDAT: File too short");
         return;
@@ -218,7 +207,5 @@ void cxl_doe_cdat_release(CXLComponentState *cxl_cstate)
         cdat->free_cdat_table(cdat->built_buf, cdat->built_buf_len,
                               cdat->private);
     }
-    if (cdat->buf) {
-        free(cdat->buf);
-    }
+    g_free(cdat->buf);
 }
-- 
2.37.2

Re: [PATCH 1/2] hw/cxl: cdat: Fix open file not closed in ct3_load_cdat()

[Fan Ni]

On Fri, Apr 21, 2023 at 02:20:19PM +0100, Jonathan Cameron via wrote:
> From: Hao Zeng <zenghao@kylinos.cn>
> 
> Open file descriptor not closed in error paths. Fix by replace
> open coded handling of read of whole file into a buffer with
> g_file_get_contents()
> 
> Fixes: aba578bdac ("hw/cxl: CDAT Data Object Exchange implementation")
> Signed-off-by: Zeng Hao <zenghao@kylinos.cn>
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Jonathan Cameron via <qemu-devel@nongnu.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> --
> Changes since v5:
> - Drop if guard on g_free() as per checkpatch warning.
> ---
>  hw/cxl/cxl-cdat.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 137abd0992..056711d63d 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -110,29 +110,18 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>      g_autofree CDATEntry *cdat_st = NULL;
>      uint8_t sum = 0;
>      int num_ent;
> -    int i = 0, ent = 1, file_size = 0;
> +    int i = 0, ent = 1;
> +    gsize file_size = 0;
>      CDATSubHeader *hdr;
> -    FILE *fp = NULL;
> +    GError *error = NULL;
>  
>      /* Read CDAT file and create its cache */
> -    fp = fopen(cdat->filename, "r");
> -    if (!fp) {
> -        error_setg(errp, "CDAT: Unable to open file");
> +    if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
> +                             &file_size, &error)) {
> +        error_setg(errp, "CDAT: File read failed: %s", error->message);
> +        g_error_free(error);
>          return;
>      }
> -
> -    fseek(fp, 0, SEEK_END);
> -    file_size = ftell(fp);
> -    fseek(fp, 0, SEEK_SET);
> -    cdat->buf = g_malloc0(file_size);
> -
> -    if (fread(cdat->buf, file_size, 1, fp) == 0) {
> -        error_setg(errp, "CDAT: File read failed");
> -        return;
> -    }
> -
> -    fclose(fp);
> -
>      if (file_size < sizeof(CDATTableHeader)) {
>          error_setg(errp, "CDAT: File too short");
>          return;
> @@ -218,7 +207,5 @@ void cxl_doe_cdat_release(CXLComponentState *cxl_cstate)
>          cdat->free_cdat_table(cdat->built_buf, cdat->built_buf_len,
>                                cdat->private);
>      }
> -    if (cdat->buf) {
> -        free(cdat->buf);
> -    }
> +    g_free(cdat->buf);
>  }
> -- 
> 2.37.2
> 
> 
> 

[PATCH 2/2] hw/cxl: cdat: Fix failure to free buffer in erorr paths

[Jonathan Cameron via]

The failure paths in CDAT file loading did not clear up properly.
Change to using g_auto_free and a local pointer for the buffer to
ensure this function has no side effects on error.
Also drop some unnecessary checks that can not fail.

Cleanup properly after a failure to load a CDAT file.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-cdat.c            | 33 ++++++++++++++++++---------------
 hw/mem/cxl_type3.c           |  4 ++++
 hw/pci-bridge/cxl_upstream.c |  3 +++
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 056711d63d..d246d6885b 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -108,6 +108,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
 static void ct3_load_cdat(CDATObject *cdat, Error **errp)
 {
     g_autofree CDATEntry *cdat_st = NULL;
+    g_autofree char *buf = NULL;
     uint8_t sum = 0;
     int num_ent;
     int i = 0, ent = 1;
@@ -116,7 +117,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
     GError *error = NULL;
 
     /* Read CDAT file and create its cache */
-    if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
+    if (!g_file_get_contents(cdat->filename, (gchar **)&buf,
                              &file_size, &error)) {
         error_setg(errp, "CDAT: File read failed: %s", error->message);
         g_error_free(error);
@@ -129,9 +130,17 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
     i = sizeof(CDATTableHeader);
     num_ent = 1;
     while (i < file_size) {
-        hdr = (CDATSubHeader *)(cdat->buf + i);
+        hdr = (CDATSubHeader *)(buf + i);
+        if (i + sizeof(CDATSubHeader) > file_size) {
+            error_setg(errp, "CDAT: Truncated table");
+            return;
+        }
         cdat_len_check(hdr, errp);
         i += hdr->length;
+        if (i > file_size) {
+            error_setg(errp, "CDAT: Truncated table");
+            return;
+        }
         num_ent++;
     }
     if (i != file_size) {
@@ -139,33 +148,26 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
         return;
     }
 
-    cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent);
-    if (!cdat_st) {
-        error_setg(errp, "CDAT: Failed to allocate entry array");
-        return;
-    }
+    cdat_st = g_new0(CDATEntry, num_ent);
 
     /* Set CDAT header, Entry = 0 */
-    cdat_st[0].base = cdat->buf;
+    cdat_st[0].base = buf;
     cdat_st[0].length = sizeof(CDATTableHeader);
     i = 0;
 
     while (i < cdat_st[0].length) {
-        sum += cdat->buf[i++];
+        sum += buf[i++];
     }
 
     /* Read CDAT structures */
     while (i < file_size) {
-        hdr = (CDATSubHeader *)(cdat->buf + i);
-        cdat_len_check(hdr, errp);
-
+        hdr = (CDATSubHeader *)(buf + i);
         cdat_st[ent].base = hdr;
         cdat_st[ent].length = hdr->length;
 
-        while (cdat->buf + i <
-               (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
+        while (buf + i < (char *)cdat_st[ent].base + cdat_st[ent].length) {
             assert(i < file_size);
-            sum += cdat->buf[i++];
+            sum += buf[i++];
         }
 
         ent++;
@@ -176,6 +178,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
     }
     cdat->entry_len = num_ent;
     cdat->entry = g_steal_pointer(&cdat_st);
+    cdat->buf = g_steal_pointer(&buf);
 }
 
 void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index abe60b362c..7647122cc6 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -593,6 +593,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
     cxl_cstate->cdat.private = ct3d;
     cxl_doe_cdat_init(cxl_cstate, errp);
+    if (*errp) {
+        goto err_free_special_ops;
+    }
 
     pcie_cap_deverr_init(pci_dev);
     /* Leave a bit of room for expansion */
@@ -605,6 +608,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
 
 err_release_cdat:
     cxl_doe_cdat_release(cxl_cstate);
+err_free_special_ops:
     g_free(regs->special_ops);
 err_address_space_free:
     address_space_destroy(&ct3d->hostmem_as);
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index 9df436cb73..ef47e5d625 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -346,6 +346,9 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
     cxl_cstate->cdat.free_cdat_table = free_default_cdat_table;
     cxl_cstate->cdat.private = d;
     cxl_doe_cdat_init(cxl_cstate, errp);
+    if (*errp) {
+        goto err_cap;
+    }
 
     return;
 
-- 
2.37.2

Re: [PATCH 2/2] hw/cxl: cdat: Fix failure to free buffer in erorr paths

[Fan Ni]

On Fri, Apr 21, 2023 at 02:20:20PM +0100, Jonathan Cameron via wrote:
> The failure paths in CDAT file loading did not clear up properly.
> Change to using g_auto_free and a local pointer for the buffer to
> ensure this function has no side effects on error.
> Also drop some unnecessary checks that can not fail.
> 
> Cleanup properly after a failure to load a CDAT file.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  hw/cxl/cxl-cdat.c            | 33 ++++++++++++++++++---------------
>  hw/mem/cxl_type3.c           |  4 ++++
>  hw/pci-bridge/cxl_upstream.c |  3 +++
>  3 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 056711d63d..d246d6885b 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -108,6 +108,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>  static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>  {
>      g_autofree CDATEntry *cdat_st = NULL;
> +    g_autofree char *buf = NULL;
>      uint8_t sum = 0;
>      int num_ent;
>      int i = 0, ent = 1;
> @@ -116,7 +117,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>      GError *error = NULL;
>  
>      /* Read CDAT file and create its cache */
> -    if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
> +    if (!g_file_get_contents(cdat->filename, (gchar **)&buf,
>                               &file_size, &error)) {
>          error_setg(errp, "CDAT: File read failed: %s", error->message);
>          g_error_free(error);
> @@ -129,9 +130,17 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>      i = sizeof(CDATTableHeader);
>      num_ent = 1;
>      while (i < file_size) {
> -        hdr = (CDATSubHeader *)(cdat->buf + i);
> +        hdr = (CDATSubHeader *)(buf + i);
> +        if (i + sizeof(CDATSubHeader) > file_size) {
> +            error_setg(errp, "CDAT: Truncated table");
> +            return;
> +        }
>          cdat_len_check(hdr, errp);
>          i += hdr->length;
> +        if (i > file_size) {
> +            error_setg(errp, "CDAT: Truncated table");
> +            return;
> +        }
>          num_ent++;
>      }
>      if (i != file_size) {
> @@ -139,33 +148,26 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>          return;
>      }
>  
> -    cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent);
> -    if (!cdat_st) {
> -        error_setg(errp, "CDAT: Failed to allocate entry array");
> -        return;
> -    }
> +    cdat_st = g_new0(CDATEntry, num_ent);
>  
>      /* Set CDAT header, Entry = 0 */
> -    cdat_st[0].base = cdat->buf;
> +    cdat_st[0].base = buf;
>      cdat_st[0].length = sizeof(CDATTableHeader);
>      i = 0;
>  
>      while (i < cdat_st[0].length) {
> -        sum += cdat->buf[i++];
> +        sum += buf[i++];
>      }
>  
>      /* Read CDAT structures */
>      while (i < file_size) {
> -        hdr = (CDATSubHeader *)(cdat->buf + i);
> -        cdat_len_check(hdr, errp);
> -
> +        hdr = (CDATSubHeader *)(buf + i);
>          cdat_st[ent].base = hdr;
>          cdat_st[ent].length = hdr->length;
>  
> -        while (cdat->buf + i <
> -               (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
> +        while (buf + i < (char *)cdat_st[ent].base + cdat_st[ent].length) {
>              assert(i < file_size);
> -            sum += cdat->buf[i++];
> +            sum += buf[i++];
>          }
>  
>          ent++;
> @@ -176,6 +178,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
>      }
>      cdat->entry_len = num_ent;
>      cdat->entry = g_steal_pointer(&cdat_st);
> +    cdat->buf = g_steal_pointer(&buf);
>  }
>  
>  void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index abe60b362c..7647122cc6 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -593,6 +593,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
>      cxl_cstate->cdat.private = ct3d;
>      cxl_doe_cdat_init(cxl_cstate, errp);
> +    if (*errp) {
> +        goto err_free_special_ops;
> +    }
>  
>      pcie_cap_deverr_init(pci_dev);
>      /* Leave a bit of room for expansion */
> @@ -605,6 +608,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>  
>  err_release_cdat:
>      cxl_doe_cdat_release(cxl_cstate);
> +err_free_special_ops:
>      g_free(regs->special_ops);
>  err_address_space_free:
>      address_space_destroy(&ct3d->hostmem_as);
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> index 9df436cb73..ef47e5d625 100644
> --- a/hw/pci-bridge/cxl_upstream.c
> +++ b/hw/pci-bridge/cxl_upstream.c
> @@ -346,6 +346,9 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
>      cxl_cstate->cdat.free_cdat_table = free_default_cdat_table;
>      cxl_cstate->cdat.private = d;
>      cxl_doe_cdat_init(cxl_cstate, errp);
> +    if (*errp) {
> +        goto err_cap;
> +    }
>  
>      return;
>  
> -- 
> 2.37.2
> 
> 
> 

Re: [PATCH 0/2] hw/cxl: CDAT file handling fixes.

[Michael Tokarev]

21.04.2023 16:20, Jonathan Cameron via wrote:
> I've picked up Hao Zeng's v5 and added a patch to deal with the
> issues that Peter pointed out in this area.
> 
> On error, the CDAT file handling left resource dangling and didn't
> cleanly exit from calling functions.  Hao Zeng dealt with closing
> the file. The second patch deals with the buffer allocation and
> ensuring a clean exit at the two callers.
> 
> Hao Zeng (1):
>    hw/cxl: cdat: Fix open file not closed in ct3_load_cdat()
> 
> Jonathan Cameron (1):
>    hw/cxl: cdat: Fix failure to free buffer in erorr paths
> 
>   hw/cxl/cxl-cdat.c            | 60 +++++++++++++++---------------------
>   hw/mem/cxl_type3.c           |  4 +++
>   hw/pci-bridge/cxl_upstream.c |  3 ++
>   3 files changed, 32 insertions(+), 35 deletions(-)

Hi!

Is it a stable (8.0 & 7.2) material?
The issues does not seem to be very important, still smells like
a good thing to have in -stable.

Thanks,

/mjt

Re: [PATCH 0/2] hw/cxl: CDAT file handling fixes.

[Jonathan Cameron via]

On Sat, 20 May 2023 07:46:16 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 21.04.2023 16:20, Jonathan Cameron via wrote:
> > I've picked up Hao Zeng's v5 and added a patch to deal with the
> > issues that Peter pointed out in this area.
> > 
> > On error, the CDAT file handling left resource dangling and didn't
> > cleanly exit from calling functions.  Hao Zeng dealt with closing
> > the file. The second patch deals with the buffer allocation and
> > ensuring a clean exit at the two callers.
> > 
> > Hao Zeng (1):
> >    hw/cxl: cdat: Fix open file not closed in ct3_load_cdat()
> > 
> > Jonathan Cameron (1):
> >    hw/cxl: cdat: Fix failure to free buffer in erorr paths
> > 
> >   hw/cxl/cxl-cdat.c            | 60 +++++++++++++++---------------------
> >   hw/mem/cxl_type3.c           |  4 +++
> >   hw/pci-bridge/cxl_upstream.c |  3 ++
> >   3 files changed, 32 insertions(+), 35 deletions(-)  
> 
> Hi!
> 
> Is it a stable (8.0 & 7.2) material?
> The issues does not seem to be very important, still smells like
> a good thing to have in -stable.

Issues are leaks only on an error path, so I'd say no for stable.
If others disagree these should be harmless.

Jonathan

> 
> Thanks,
> 
> /mjt
>