diff mbox series

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

Message ID 20230421132020.7408-3-Jonathan.Cameron@huawei.com (mailing list archive)
State New, archived
Headers show
Series hw/cxl: CDAT file handling fixes. | expand

Commit Message

Jonathan Cameron April 21, 2023, 1:20 p.m. UTC 
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(-)

Comments

Fan Ni May 16, 2023, 4:15 p.m. UTC | #1 
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
> 
> 
>
diff mbox series

Patch

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;