[PATCH 0/7] contrib/elf2dmp: Improve robustness

[PATCH 0/7] contrib/elf2dmp: Improve robustness

[Akihiko Odaki]

elf2dmp sometimes fails to work with partially corrupted dumps, and also
emits warnings when sanitizers are in use. This series are collections
of changes to improve the situation.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (7):
      contrib/elf2dmp: Always check for PA resolution failure
      contrib/elf2dmp: Always destroy PA space
      contrib/elf2dmp: Ensure segment fits in file
      contrib/elf2dmp: Use lduw_le_p() to read PDB
      contrib/elf2dmp: Use rol64() to decode
      contrib/elf2dmp: Continue even contexts are lacking
      MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer

 MAINTAINERS                 |  1 +
 contrib/elf2dmp/addrspace.c | 51 ++++++++++++++++++++++++++++-----------------
 contrib/elf2dmp/main.c      | 25 +++++++---------------
 contrib/elf2dmp/pdb.c       |  3 ++-
 4 files changed, 43 insertions(+), 37 deletions(-)
---
base-commit: bfe8020c814a30479a4241aaa78b63960655962b
change-id: 20240301-elf2dmp-1a6a551f8663

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 1/7] contrib/elf2dmp: Always check for PA resolution failure

[Akihiko Odaki]

Not checking PA resolution failure can result in NULL deference.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 contrib/elf2dmp/addrspace.c | 46 ++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
index 6f608a517b1e..980a7aa5f8fb 100644
--- a/contrib/elf2dmp/addrspace.c
+++ b/contrib/elf2dmp/addrspace.c
@@ -22,7 +22,7 @@ static struct pa_block *pa_space_find_block(struct pa_space *ps, uint64_t pa)
     return NULL;
 }
 
-static uint8_t *pa_space_resolve(struct pa_space *ps, uint64_t pa)
+static void *pa_space_resolve(struct pa_space *ps, uint64_t pa)
 {
     struct pa_block *block = pa_space_find_block(ps, pa);
 
@@ -33,6 +33,19 @@ static uint8_t *pa_space_resolve(struct pa_space *ps, uint64_t pa)
     return block->addr + (pa - block->paddr);
 }
 
+static int pa_space_read64(struct pa_space *ps, uint64_t pa, uint64_t *value)
+{
+    uint64_t *resolved = pa_space_resolve(ps, pa);
+
+    if (!resolved) {
+        return 1;
+    }
+
+    *value = *resolved;
+
+    return 0;
+}
+
 static void pa_block_align(struct pa_block *b)
 {
     uint64_t low_align = ((b->paddr - 1) | ELF2DMP_PAGE_MASK) + 1 - b->paddr;
@@ -108,19 +121,20 @@ void va_space_create(struct va_space *vs, struct pa_space *ps, uint64_t dtb)
     va_space_set_dtb(vs, dtb);
 }
 
-static uint64_t get_pml4e(struct va_space *vs, uint64_t va)
+static int get_pml4e(struct va_space *vs, uint64_t va, uint64_t *value)
 {
     uint64_t pa = (vs->dtb & 0xffffffffff000) | ((va & 0xff8000000000) >> 36);
 
-    return *(uint64_t *)pa_space_resolve(vs->ps, pa);
+    return pa_space_read64(vs->ps, pa, value);
 }
 
-static uint64_t get_pdpi(struct va_space *vs, uint64_t va, uint64_t pml4e)
+static int get_pdpi(struct va_space *vs, uint64_t va, uint64_t pml4e,
+                    uint64_t *value)
 {
     uint64_t pdpte_paddr = (pml4e & 0xffffffffff000) |
         ((va & 0x7FC0000000) >> 27);
 
-    return *(uint64_t *)pa_space_resolve(vs->ps, pdpte_paddr);
+    return pa_space_read64(vs->ps, pdpte_paddr, value);
 }
 
 static uint64_t pde_index(uint64_t va)
@@ -133,11 +147,12 @@ static uint64_t pdba_base(uint64_t pdpe)
     return pdpe & 0xFFFFFFFFFF000;
 }
 
-static uint64_t get_pgd(struct va_space *vs, uint64_t va, uint64_t pdpe)
+static int get_pgd(struct va_space *vs, uint64_t va, uint64_t pdpe,
+                   uint64_t *value)
 {
     uint64_t pgd_entry = pdba_base(pdpe) + pde_index(va) * 8;
 
-    return *(uint64_t *)pa_space_resolve(vs->ps, pgd_entry);
+    return pa_space_read64(vs->ps, pgd_entry, value);
 }
 
 static uint64_t pte_index(uint64_t va)
@@ -150,11 +165,12 @@ static uint64_t ptba_base(uint64_t pde)
     return pde & 0xFFFFFFFFFF000;
 }
 
-static uint64_t get_pte(struct va_space *vs, uint64_t va, uint64_t pgd)
+static int get_pte(struct va_space *vs, uint64_t va, uint64_t pgd,
+                   uint64_t *value)
 {
     uint64_t pgd_val = ptba_base(pgd) + pte_index(va) * 8;
 
-    return *(uint64_t *)pa_space_resolve(vs->ps, pgd_val);
+    return pa_space_read64(vs->ps, pgd_val, value);
 }
 
 static uint64_t get_paddr(uint64_t va, uint64_t pte)
@@ -186,13 +202,11 @@ static uint64_t va_space_va2pa(struct va_space *vs, uint64_t va)
 {
     uint64_t pml4e, pdpe, pgd, pte;
 
-    pml4e = get_pml4e(vs, va);
-    if (!is_present(pml4e)) {
+    if (get_pml4e(vs, va, &pml4e) || !is_present(pml4e)) {
         return INVALID_PA;
     }
 
-    pdpe = get_pdpi(vs, va, pml4e);
-    if (!is_present(pdpe)) {
+    if (get_pdpi(vs, va, pml4e, &pdpe) || !is_present(pdpe)) {
         return INVALID_PA;
     }
 
@@ -200,8 +214,7 @@ static uint64_t va_space_va2pa(struct va_space *vs, uint64_t va)
         return get_1GB_paddr(va, pdpe);
     }
 
-    pgd = get_pgd(vs, va, pdpe);
-    if (!is_present(pgd)) {
+    if (get_pgd(vs, va, pdpe, &pgd) || !is_present(pgd)) {
         return INVALID_PA;
     }
 
@@ -209,8 +222,7 @@ static uint64_t va_space_va2pa(struct va_space *vs, uint64_t va)
         return get_2MB_paddr(va, pgd);
     }
 
-    pte = get_pte(vs, va, pgd);
-    if (!is_present(pte)) {
+    if (get_pte(vs, va, pgd, &pte) || !is_present(pte)) {
         return INVALID_PA;
     }
 

-- 
2.44.0

[PATCH 2/7] contrib/elf2dmp: Always destroy PA space

[Akihiko Odaki]

Destroy PA space even if paging base couldn't be found, fixing memory
leak.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 contrib/elf2dmp/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index cbc38a7c103a..dd686280f981 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -553,7 +553,7 @@ int main(int argc, char *argv[])
     if (fix_dtb(&vs, &qemu_elf)) {
         eprintf("Failed to find paging base\n");
         err = 1;
-        goto out_elf;
+        goto out_ps;
     }
 
     printf("CPU #0 IDT is at 0x%016"PRIx64"\n", state->idt.base);

-- 
2.44.0

[PATCH 3/7] contrib/elf2dmp: Ensure segment fits in file

[Akihiko Odaki]

This makes elf2dmp more robust against corrupted inputs.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 contrib/elf2dmp/addrspace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
index 980a7aa5f8fb..d546a400dfda 100644
--- a/contrib/elf2dmp/addrspace.c
+++ b/contrib/elf2dmp/addrspace.c
@@ -88,11 +88,12 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf)
     ps->block = g_new(struct pa_block, ps->block_nr);
 
     for (i = 0; i < phdr_nr; i++) {
-        if (phdr[i].p_type == PT_LOAD) {
+        if (phdr[i].p_type == PT_LOAD && phdr[i].p_offset < qemu_elf->size) {
             ps->block[block_i] = (struct pa_block) {
                 .addr = (uint8_t *)qemu_elf->map + phdr[i].p_offset,
                 .paddr = phdr[i].p_paddr,
-                .size = phdr[i].p_filesz,
+                .size = MIN(phdr[i].p_filesz,
+                            qemu_elf->size - phdr[i].p_offset),
             };
             pa_block_align(&ps->block[block_i]);
             block_i = ps->block[block_i].size ? (block_i + 1) : block_i;

-- 
2.44.0

[PATCH 4/7] contrib/elf2dmp: Use lduw_le_p() to read PDB

[Akihiko Odaki]

This resolved UBSan warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 contrib/elf2dmp/pdb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index 40991f5f4c34..2541234205c3 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
 
 #include "pdb.h"
 #include "err.h"
@@ -187,7 +188,7 @@ static int pdb_init_symbols(struct pdb_reader *r)
 
     r->symbols = symbols;
 
-    r->segments = *(uint16_t *)((const char *)symbols + sizeof(PDB_SYMBOLS) +
+    r->segments = lduw_le_p((const char *)symbols + sizeof(PDB_SYMBOLS) +
             symbols->module_size + symbols->offset_size +
             symbols->hash_size + symbols->srcmodule_size +
             symbols->pdbimport_size + symbols->unknown2_size +

-- 
2.44.0

[PATCH 5/7] contrib/elf2dmp: Use rol64() to decode

[Akihiko Odaki]

rol64() is roubust against too large shift values and fixes UBSan
warnings.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 contrib/elf2dmp/main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index dd686280f981..432f8629f321 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -6,6 +6,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bitops.h"
 
 #include "err.h"
 #include "addrspace.h"
@@ -47,11 +48,6 @@ static const uint64_t SharedUserData = 0xfffff78000000000;
     s ? printf(#s" = 0x%016"PRIx64"\n", s) :\
     eprintf("Failed to resolve "#s"\n"), s)
 
-static uint64_t rol(uint64_t x, uint64_t y)
-{
-    return (x << y) | (x >> (64 - y));
-}
-
 /*
  * Decoding algorithm can be found in Volatility project
  */
@@ -64,7 +60,7 @@ static void kdbg_decode(uint64_t *dst, uint64_t *src, size_t size,
         uint64_t block;
 
         block = src[i];
-        block = rol(block ^ kwn, (uint8_t)kwn);
+        block = rol64(block ^ kwn, kwn);
         block = __builtin_bswap64(block ^ kdbe) ^ kwa;
         dst[i] = block;
     }

-- 
2.44.0

[PATCH 6/7] contrib/elf2dmp: Continue even contexts are lacking

[Akihiko Odaki]

Contexts of some CPUs may be lacking or corrupted due to premature boot,
but the output may still contain valuable information of other CPUs and
memory.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 contrib/elf2dmp/main.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 432f8629f321..33066310b9fa 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -332,7 +332,7 @@ static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
     return 0;
 }
 
-static int fill_context(KDDEBUGGER_DATA64 *kdbg,
+static void fill_context(KDDEBUGGER_DATA64 *kdbg,
         struct va_space *vs, QEMU_Elf *qe)
 {
     int i;
@@ -346,7 +346,7 @@ static int fill_context(KDDEBUGGER_DATA64 *kdbg,
         if (va_space_rw(vs, kdbg->KiProcessorBlock + sizeof(Prcb) * i,
                     &Prcb, sizeof(Prcb), 0)) {
             eprintf("Failed to read CPU #%d PRCB location\n", i);
-            return 1;
+            continue;
         }
 
         if (!Prcb) {
@@ -357,7 +357,7 @@ static int fill_context(KDDEBUGGER_DATA64 *kdbg,
         if (va_space_rw(vs, Prcb + kdbg->OffsetPrcbContext,
                     &Context, sizeof(Context), 0)) {
             eprintf("Failed to read CPU #%d ContextFrame location\n", i);
-            return 1;
+            continue;
         }
 
         printf("Filling context for CPU #%d...\n", i);
@@ -365,11 +365,9 @@ static int fill_context(KDDEBUGGER_DATA64 *kdbg,
 
         if (va_space_rw(vs, Context, &ctx, sizeof(ctx), 1)) {
             eprintf("Failed to fill CPU #%d context\n", i);
-            return 1;
+            continue;
         }
     }
-
-    return 0;
 }
 
 static int pe_get_data_dir_entry(uint64_t base, void *start_addr, int idx,
@@ -624,10 +622,7 @@ int main(int argc, char *argv[])
         goto out_kdbg;
     }
 
-    if (fill_context(kdbg, &vs, &qemu_elf)) {
-        err = 1;
-        goto out_kdbg;
-    }
+    fill_context(kdbg, &vs, &qemu_elf);
 
     if (write_dump(&ps, &header, argv[2])) {
         eprintf("Failed to save dump\n");

-- 
2.44.0

[PATCH 7/7] MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer

[Akihiko Odaki]

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 65dfdc9677e4..d25403f3709b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3583,6 +3583,7 @@ F: util/iova-tree.c
 
 elf2dmp
 M: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
+R: Akihiko Odaki <akihiko.odaki@daynix.com>
 S: Maintained
 F: contrib/elf2dmp/
 

-- 
2.44.0

Re: [PATCH 1/7] contrib/elf2dmp: Always check for PA resolution failure

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Not checking PA resolution failure can result in NULL deference.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  contrib/elf2dmp/addrspace.c | 46 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
> index 6f608a517b1e..980a7aa5f8fb 100644
> --- a/contrib/elf2dmp/addrspace.c
> +++ b/contrib/elf2dmp/addrspace.c
> @@ -22,7 +22,7 @@ static struct pa_block *pa_space_find_block(struct pa_space *ps, uint64_t pa)
>      return NULL;
>  }
>
> -static uint8_t *pa_space_resolve(struct pa_space *ps, uint64_t pa)
> +static void *pa_space_resolve(struct pa_space *ps, uint64_t pa)
>  {
>      struct pa_block *block = pa_space_find_block(ps, pa);
>
> @@ -33,6 +33,19 @@ static uint8_t *pa_space_resolve(struct pa_space *ps, uint64_t pa)
>      return block->addr + (pa - block->paddr);
>  }
>
> +static int pa_space_read64(struct pa_space *ps, uint64_t pa, uint64_t *value)
> +{
> +    uint64_t *resolved = pa_space_resolve(ps, pa);
> +
> +    if (!resolved) {
> +        return 1;
> +    }
> +
> +    *value = *resolved;
> +
> +    return 0;
> +}

This is effectively returning a bool, so we could use a 'bool'
for the return type. I also think it would be preferable to
have the return type be 'true for success, false for failure':
in the callsites, having

   if (!get_pml4e(vs, va, &pml4e) || !is_present(pml4e)) {
          return INVALID_PA;
      }

seems to me to read more clearly ("if we couldn't get the PML,
or the PML isn't present, then...").

thanks
-- PMM

Re: [PATCH 2/7] contrib/elf2dmp: Always destroy PA space

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:52, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Destroy PA space even if paging base couldn't be found, fixing memory
> leak.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  contrib/elf2dmp/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/7] contrib/elf2dmp: Ensure segment fits in file

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This makes elf2dmp more robust against corrupted inputs.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  contrib/elf2dmp/addrspace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
> index 980a7aa5f8fb..d546a400dfda 100644
> --- a/contrib/elf2dmp/addrspace.c
> +++ b/contrib/elf2dmp/addrspace.c
> @@ -88,11 +88,12 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf)
>      ps->block = g_new(struct pa_block, ps->block_nr);
>
>      for (i = 0; i < phdr_nr; i++) {
> -        if (phdr[i].p_type == PT_LOAD) {
> +        if (phdr[i].p_type == PT_LOAD && phdr[i].p_offset < qemu_elf->size) {
>              ps->block[block_i] = (struct pa_block) {
>                  .addr = (uint8_t *)qemu_elf->map + phdr[i].p_offset,
>                  .paddr = phdr[i].p_paddr,
> -                .size = phdr[i].p_filesz,
> +                .size = MIN(phdr[i].p_filesz,
> +                            qemu_elf->size - phdr[i].p_offset),

Shouldn't "p_filesz is smaller than the actual amount of data in the
file" be a failure condition? In include/hw/elf_ops.h we treat it
that way:

            mem_size = ph->p_memsz; /* Size of the ROM */
            file_size = ph->p_filesz; /* Size of the allocated data */
            data_offset = ph->p_offset; /* Offset where the data is located */

            if (file_size > 0) {
                if (g_mapped_file_get_length(mapped_file) <
                    file_size + data_offset) {
                    goto fail;
                }
                [etc]

Like that code, we could then only check if p_offset + p_filesz is off
the end of the file, rather than checking p_offset separately.

>              };
>              pa_block_align(&ps->block[block_i]);
>              block_i = ps->block[block_i].size ? (block_i + 1) : block_i;

thanks
-- PMM

Re: [PATCH 7/7] MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 65dfdc9677e4..d25403f3709b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3583,6 +3583,7 @@ F: util/iova-tree.c
>
>  elf2dmp
>  M: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
> +R: Akihiko Odaki <akihiko.odaki@daynix.com>
>  S: Maintained
>  F: contrib/elf2dmp/

Thanks for taking on reviewing of this part of the codebase.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 4/7] contrib/elf2dmp: Use lduw_le_p() to read PDB

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:52, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This resolved UBSan warnings.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  contrib/elf2dmp/pdb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
> index 40991f5f4c34..2541234205c3 100644
> --- a/contrib/elf2dmp/pdb.c
> +++ b/contrib/elf2dmp/pdb.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>
>  #include "pdb.h"
>  #include "err.h"
> @@ -187,7 +188,7 @@ static int pdb_init_symbols(struct pdb_reader *r)
>
>      r->symbols = symbols;
>
> -    r->segments = *(uint16_t *)((const char *)symbols + sizeof(PDB_SYMBOLS) +
> +    r->segments = lduw_le_p((const char *)symbols + sizeof(PDB_SYMBOLS) +
>              symbols->module_size + symbols->offset_size +
>              symbols->hash_size + symbols->srcmodule_size +
>              symbols->pdbimport_size + symbols->unknown2_size +

This is a behaviour change -- previously we did a load in
host-endian order, but now we do it in little-endian order.
Which is correct?

If we need host-endian, then we have lduw_he_p() for that.
If we need little-endian, then maybe other parts of the code
also are loading data in the wrong endianness?

thanks
-- PMM

Re: [PATCH 5/7] contrib/elf2dmp: Use rol64() to decode

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:52, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> rol64() is roubust against too large shift values and fixes UBSan
> warnings.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 6/7] contrib/elf2dmp: Continue even contexts are lacking

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:52, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Contexts of some CPUs may be lacking or corrupted due to premature boot,
> but the output may still contain valuable information of other CPUs and
> memory.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  contrib/elf2dmp/main.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

I think it would be worth having a brief comment explaining why
we don't make these memory read failures fatal errors, to
avoid somebody in future putting the error path back in again.

Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 0/7] contrib/elf2dmp: Improve robustness

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> elf2dmp sometimes fails to work with partially corrupted dumps, and also
> emits warnings when sanitizers are in use. This series are collections
> of changes to improve the situation.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Akihiko Odaki (7):
>       contrib/elf2dmp: Always check for PA resolution failure
>       contrib/elf2dmp: Always destroy PA space
>       contrib/elf2dmp: Ensure segment fits in file
>       contrib/elf2dmp: Use lduw_le_p() to read PDB
>       contrib/elf2dmp: Use rol64() to decode
>       contrib/elf2dmp: Continue even contexts are lacking
>       MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer
>

I noticed somebody filed a bug report about elf2dmp
crashing recently:

https://gitlab.com/qemu-project/qemu/-/issues/2202

Does this patchset happen to fix that crash?

thanks
-- PMM

Re: [PATCH 3/7] contrib/elf2dmp: Ensure segment fits in file

[Akihiko Odaki]

On 2024/03/05 2:52, Peter Maydell wrote:
> On Sun, 3 Mar 2024 at 10:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> This makes elf2dmp more robust against corrupted inputs.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   contrib/elf2dmp/addrspace.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
>> index 980a7aa5f8fb..d546a400dfda 100644
>> --- a/contrib/elf2dmp/addrspace.c
>> +++ b/contrib/elf2dmp/addrspace.c
>> @@ -88,11 +88,12 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf)
>>       ps->block = g_new(struct pa_block, ps->block_nr);
>>
>>       for (i = 0; i < phdr_nr; i++) {
>> -        if (phdr[i].p_type == PT_LOAD) {
>> +        if (phdr[i].p_type == PT_LOAD && phdr[i].p_offset < qemu_elf->size) {
>>               ps->block[block_i] = (struct pa_block) {
>>                   .addr = (uint8_t *)qemu_elf->map + phdr[i].p_offset,
>>                   .paddr = phdr[i].p_paddr,
>> -                .size = phdr[i].p_filesz,
>> +                .size = MIN(phdr[i].p_filesz,
>> +                            qemu_elf->size - phdr[i].p_offset),
> 
> Shouldn't "p_filesz is smaller than the actual amount of data in the
> file" be a failure condition? In include/hw/elf_ops.h we treat it
> that way:
> 
>              mem_size = ph->p_memsz; /* Size of the ROM */
>              file_size = ph->p_filesz; /* Size of the allocated data */
>              data_offset = ph->p_offset; /* Offset where the data is located */
> 
>              if (file_size > 0) {
>                  if (g_mapped_file_get_length(mapped_file) <
>                      file_size + data_offset) {
>                      goto fail;
>                  }
>                  [etc]
> 
> Like that code, we could then only check if p_offset + p_filesz is off
> the end of the file, rather than checking p_offset separately.
> 
>>               };
>>               pa_block_align(&ps->block[block_i]);
>>               block_i = ps->block[block_i].size ? (block_i + 1) : block_i;
> 
> thanks
> -- PMM

I'm making this permissive for corrupted dumps since they may still 
include valuable information.

It is different from include/hw/elf_ops.h, which is presumably used to 
load executables rather than dumps. Loading a corrupted executable does 
nothing good.

Regards,
Akihiko Odaki