diff mbox series

eif: cope with huge section sizes

Message ID 20241106174443.557557-1-pbonzini@redhat.com (mailing list archive)
State New
Headers show
Series eif: cope with huge section sizes | expand

Commit Message

Paolo Bonzini Nov. 6, 2024, 5:44 p.m. UTC 
Check for overflow as well as allocation failure.  Resolves Coverity CID 1564859.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/eif.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

Comments

Pierrick Bouvier Nov. 6, 2024, 5:49 p.m. UTC | #1 
On 11/6/24 09:44, Paolo Bonzini wrote:
> Check for overflow as well as allocation failure.  Resolves Coverity CID 1564859.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/core/eif.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/eif.c b/hw/core/eif.c
> index cbcd80de58b..25f2aedf3fa 100644
> --- a/hw/core/eif.c
> +++ b/hw/core/eif.c
> @@ -123,6 +123,10 @@ static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
>   
>       for (int i = 0; i < MAX_SECTIONS; ++i) {
>           header->section_sizes[i] = be64_to_cpu(header->section_sizes[i]);
> +        if (header->section_sizes[i] > SSIZE_MAX) {
> +            error_setg(errp, "Invalid EIF image. Section size out of bounds");
> +            return false;
> +        }
>       }
>   
>       header->unused = be32_to_cpu(header->unused);
> @@ -282,7 +286,12 @@ static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
>       struct cbor_load_result result;
>       bool ret = false;
>   
> -    sig = g_malloc(size);
> +    sig = g_try_malloc(size);
> +    if (!sig) {
> +        error_setg(errp, "Out of memory reading signature section");
> +        goto cleanup;
> +    }
> +
>       got = fread(sig, 1, size, eif);
>       if ((uint64_t) got != size) {
>           error_setg(errp, "Failed to read EIF signature section data");
> @@ -324,7 +333,12 @@ static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
>           error_setg(errp, "Invalid signature CBOR");
>           goto cleanup;
>       }
> -    cert = g_malloc(len);
> +    cert = g_try_malloc(len);
> +    if (!cert) {
> +        error_setg(errp, "Out of memory reading signature section");
> +        goto cleanup;
> +    }
> +
>       for (int i = 0; i < len; ++i) {
>           cbor_item_t *tmp = cbor_array_get(pair->value, i);
>           if (!tmp) {
> @@ -503,7 +517,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>                   goto cleanup;
>               }
>   
> -            ptr = g_malloc(hdr.section_size);
> +            ptr = g_try_malloc(hdr.section_size);
> +            if (!ptr) {
> +                error_setg(errp, "Out of memory reading kernel section");
> +                goto cleanup;
> +            }
>   
>               iov_ptr = g_malloc(sizeof(struct iovec));
>               iov_ptr->iov_base = ptr;
> @@ -528,7 +546,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>                   goto cleanup;
>               }
>               size = hdr.section_size;
> -            *cmdline = g_malloc(size + 1);
> +            *cmdline = g_try_malloc(size + 1);
> +            if (!cmdline) {
> +                error_setg(errp, "Out of memory reading command line section");
> +                goto cleanup;
> +            }
>               if (!read_eif_cmdline(f, size, *cmdline, &crc, errp)) {
>                   goto cleanup;
>               }
> @@ -567,7 +589,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>                   }
>               }
>   
> -            ptr = g_malloc(hdr.section_size);
> +            ptr = g_try_malloc(hdr.section_size);
> +            if (!ptr) {
> +                error_setg(errp, "Out of memory reading initrd section");
> +                goto cleanup;
> +            }
>   
>               iov_ptr = g_malloc(sizeof(struct iovec));
>               iov_ptr->iov_base = ptr;
> @@ -606,7 +632,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>               uint8_t *buf;
>               size_t got;
>               uint64_t size = hdr.section_size;
> -            buf = g_malloc(size);
> +            buf = g_try_malloc(size);
> +            if (!buf) {
> +                error_setg(errp, "Out of memory reading unknown section");
> +                goto cleanup;
> +            }
>               got = fread(buf, 1, size, f);
>               if ((uint64_t) got != size) {
>                   g_free(buf);
> @@ -662,7 +692,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>               goto cleanup;
>           }
>   
> -        ptr = g_malloc(machine_initrd_size);
> +        ptr = g_try_malloc(machine_initrd_size);
> +        if (!ptr) {
> +            error_setg(errp, "Out of memory reading initrd file");
> +            goto cleanup;
> +        }
>   
>           iov_ptr = g_malloc(sizeof(struct iovec));
>           iov_ptr->iov_base = ptr;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Dorjoy Chowdhury Nov. 6, 2024, 5:56 p.m. UTC | #2 
On Wed, Nov 6, 2024 at 11:44 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Check for overflow as well as allocation failure.  Resolves Coverity CID 1564859.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/eif.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 7 deletions(-)
>

Reviewed-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>

Thanks for fixing!

Regards,
Dorjoy
Dorjoy Chowdhury Nov. 8, 2024, 5:24 p.m. UTC | #3 
On Wed, Nov 6, 2024 at 11:44 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Check for overflow as well as allocation failure.  Resolves Coverity CID 1564859.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/eif.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/eif.c b/hw/core/eif.c
> index cbcd80de58b..25f2aedf3fa 100644
> --- a/hw/core/eif.c
> +++ b/hw/core/eif.c
> @@ -123,6 +123,10 @@ static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
>
>      for (int i = 0; i < MAX_SECTIONS; ++i) {
>          header->section_sizes[i] = be64_to_cpu(header->section_sizes[i]);
> +        if (header->section_sizes[i] > SSIZE_MAX) {
> +            error_setg(errp, "Invalid EIF image. Section size out of bounds");
> +            return false;
> +        }
>      }
>
>      header->unused = be32_to_cpu(header->unused);
> @@ -282,7 +286,12 @@ static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
>      struct cbor_load_result result;
>      bool ret = false;
>
> -    sig = g_malloc(size);
> +    sig = g_try_malloc(size);
> +    if (!sig) {
> +        error_setg(errp, "Out of memory reading signature section");
> +        goto cleanup;
> +    }
> +
>      got = fread(sig, 1, size, eif);
>      if ((uint64_t) got != size) {
>          error_setg(errp, "Failed to read EIF signature section data");
> @@ -324,7 +333,12 @@ static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
>          error_setg(errp, "Invalid signature CBOR");
>          goto cleanup;
>      }
> -    cert = g_malloc(len);
> +    cert = g_try_malloc(len);
> +    if (!cert) {
> +        error_setg(errp, "Out of memory reading signature section");
> +        goto cleanup;
> +    }
> +
>      for (int i = 0; i < len; ++i) {
>          cbor_item_t *tmp = cbor_array_get(pair->value, i);
>          if (!tmp) {
> @@ -503,7 +517,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>                  goto cleanup;
>              }
>
> -            ptr = g_malloc(hdr.section_size);
> +            ptr = g_try_malloc(hdr.section_size);
> +            if (!ptr) {
> +                error_setg(errp, "Out of memory reading kernel section");
> +                goto cleanup;
> +            }
>
>              iov_ptr = g_malloc(sizeof(struct iovec));
>              iov_ptr->iov_base = ptr;
> @@ -528,7 +546,11 @@ bool read_eif_file(const char *eif_path, const char *machine_initrd,
>                  goto cleanup;
>              }
>              size = hdr.section_size;
> -            *cmdline = g_malloc(size + 1);
> +            *cmdline = g_try_malloc(size + 1);
> +            if (!cmdline) {
> +                error_setg(errp, "Out of memory reading command line section");
> +                goto cleanup;
> +            }

I was looking into doing some changes on top of this patch and this
check above should be if (!(*cmdline)), right?

Regards,
Dorjoy
diff mbox series

Patch

diff --git a/hw/core/eif.c b/hw/core/eif.c
index cbcd80de58b..25f2aedf3fa 100644
--- a/hw/core/eif.c
+++ b/hw/core/eif.c
@@ -123,6 +123,10 @@  static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
 
     for (int i = 0; i < MAX_SECTIONS; ++i) {
         header->section_sizes[i] = be64_to_cpu(header->section_sizes[i]);
+        if (header->section_sizes[i] > SSIZE_MAX) {
+            error_setg(errp, "Invalid EIF image. Section size out of bounds");
+            return false;
+        }
     }
 
     header->unused = be32_to_cpu(header->unused);
@@ -282,7 +286,12 @@  static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
     struct cbor_load_result result;
     bool ret = false;
 
-    sig = g_malloc(size);
+    sig = g_try_malloc(size);
+    if (!sig) {
+        error_setg(errp, "Out of memory reading signature section");
+        goto cleanup;
+    }
+
     got = fread(sig, 1, size, eif);
     if ((uint64_t) got != size) {
         error_setg(errp, "Failed to read EIF signature section data");
@@ -324,7 +333,12 @@  static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
         error_setg(errp, "Invalid signature CBOR");
         goto cleanup;
     }
-    cert = g_malloc(len);
+    cert = g_try_malloc(len);
+    if (!cert) {
+        error_setg(errp, "Out of memory reading signature section");
+        goto cleanup;
+    }
+
     for (int i = 0; i < len; ++i) {
         cbor_item_t *tmp = cbor_array_get(pair->value, i);
         if (!tmp) {
@@ -503,7 +517,11 @@  bool read_eif_file(const char *eif_path, const char *machine_initrd,
                 goto cleanup;
             }
 
-            ptr = g_malloc(hdr.section_size);
+            ptr = g_try_malloc(hdr.section_size);
+            if (!ptr) {
+                error_setg(errp, "Out of memory reading kernel section");
+                goto cleanup;
+            }
 
             iov_ptr = g_malloc(sizeof(struct iovec));
             iov_ptr->iov_base = ptr;
@@ -528,7 +546,11 @@  bool read_eif_file(const char *eif_path, const char *machine_initrd,
                 goto cleanup;
             }
             size = hdr.section_size;
-            *cmdline = g_malloc(size + 1);
+            *cmdline = g_try_malloc(size + 1);
+            if (!cmdline) {
+                error_setg(errp, "Out of memory reading command line section");
+                goto cleanup;
+            }
             if (!read_eif_cmdline(f, size, *cmdline, &crc, errp)) {
                 goto cleanup;
             }
@@ -567,7 +589,11 @@  bool read_eif_file(const char *eif_path, const char *machine_initrd,
                 }
             }
 
-            ptr = g_malloc(hdr.section_size);
+            ptr = g_try_malloc(hdr.section_size);
+            if (!ptr) {
+                error_setg(errp, "Out of memory reading initrd section");
+                goto cleanup;
+            }
 
             iov_ptr = g_malloc(sizeof(struct iovec));
             iov_ptr->iov_base = ptr;
@@ -606,7 +632,11 @@  bool read_eif_file(const char *eif_path, const char *machine_initrd,
             uint8_t *buf;
             size_t got;
             uint64_t size = hdr.section_size;
-            buf = g_malloc(size);
+            buf = g_try_malloc(size);
+            if (!buf) {
+                error_setg(errp, "Out of memory reading unknown section");
+                goto cleanup;
+            }
             got = fread(buf, 1, size, f);
             if ((uint64_t) got != size) {
                 g_free(buf);
@@ -662,7 +692,11 @@  bool read_eif_file(const char *eif_path, const char *machine_initrd,
             goto cleanup;
         }
 
-        ptr = g_malloc(machine_initrd_size);
+        ptr = g_try_malloc(machine_initrd_size);
+        if (!ptr) {
+            error_setg(errp, "Out of memory reading initrd file");
+            goto cleanup;
+        }
 
         iov_ptr = g_malloc(sizeof(struct iovec));
         iov_ptr->iov_base = ptr;