* [PATCH v2 00/13] contrib/elf2dmp: Improve robustness
@ 2024-03-05 7:36 Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags Akihiko Odaki
` (12 more replies)
0 siblings, 13 replies; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, 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>
---
Changes in v2:
- Added patch "contrib/elf2dmp: Remove unnecessary err flags".
- Added patch "contrib/elf2dmp: Assume error by default".
- Added patch "contrib/elf2dmp: Conform to the error reporting pattern".
- Added patch "contrib/elf2dmp: Build only for little endian host".
- Added patch "contrib/elf2dmp: Use GPtrArray".
- Added patch "contrib/elf2dmp: Clamp QEMU note to file size".
- Changed error handling in patch "contrib/elf2dmp: Ensure segment fits
in file" (Peter Maydell)
- Added a comment to fill_context() that it continues on failure.
(Peter Maydell)
- Link to v1: https://lore.kernel.org/r/20240303-elf2dmp-v1-0-bea6649fe3e6@daynix.com
---
Akihiko Odaki (13):
contrib/elf2dmp: Remove unnecessary err flags
contrib/elf2dmp: Assume error by default
contrib/elf2dmp: Continue even contexts are lacking
contrib/elf2dmp: Conform to the error reporting pattern
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
MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer
contrib/elf2dmp: Build only for little endian host
contrib/elf2dmp: Use GPtrArray
contrib/elf2dmp: Clamp QEMU note to file size
MAINTAINERS | 1 +
contrib/elf2dmp/addrspace.h | 6 +-
contrib/elf2dmp/download.h | 2 +-
contrib/elf2dmp/pdb.h | 2 +-
contrib/elf2dmp/qemu_elf.h | 2 +-
contrib/elf2dmp/addrspace.c | 63 ++++++++++--------
contrib/elf2dmp/download.c | 12 ++--
contrib/elf2dmp/main.c | 159 ++++++++++++++++++++------------------------
contrib/elf2dmp/pdb.c | 61 ++++++++---------
contrib/elf2dmp/qemu_elf.c | 142 +++++++++++++++++++++------------------
contrib/elf2dmp/meson.build | 2 +-
11 files changed, 226 insertions(+), 226 deletions(-)
---
base-commit: bfe8020c814a30479a4241aaa78b63960655962b
change-id: 20240301-elf2dmp-1a6a551f8663
Best regards,
--
Akihiko Odaki <akihiko.odaki@daynix.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:24 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 02/13] contrib/elf2dmp: Assume error by default Akihiko Odaki
` (11 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
They are always evaluated to 1.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
contrib/elf2dmp/pdb.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index 40991f5f4c34..abf17c2e7c12 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -177,7 +177,6 @@ static int pdb_init_segments(struct pdb_reader *r)
static int pdb_init_symbols(struct pdb_reader *r)
{
- int err = 0;
PDB_SYMBOLS *symbols;
symbols = pdb_ds_read_file(r, 3);
@@ -196,7 +195,6 @@ static int pdb_init_symbols(struct pdb_reader *r)
/* Read global symbol table */
r->modimage = pdb_ds_read_file(r, symbols->gsym_file);
if (!r->modimage) {
- err = 1;
goto out_symbols;
}
@@ -205,7 +203,7 @@ static int pdb_init_symbols(struct pdb_reader *r)
out_symbols:
g_free(symbols);
- return err;
+ return 1;
}
static int pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER *hdr)
@@ -228,7 +226,6 @@ static int pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER *hdr)
static int pdb_reader_init(struct pdb_reader *r, void *data)
{
- int err = 0;
const char pdb7[] = "Microsoft C/C++ MSF 7.00";
if (memcmp(data, pdb7, sizeof(pdb7) - 1)) {
@@ -241,17 +238,14 @@ static int pdb_reader_init(struct pdb_reader *r, void *data)
r->ds.root = pdb_ds_read_file(r, 1);
if (!r->ds.root) {
- err = 1;
goto out_ds;
}
if (pdb_init_symbols(r)) {
- err = 1;
goto out_root;
}
if (pdb_init_segments(r)) {
- err = 1;
goto out_sym;
}
@@ -264,7 +258,7 @@ out_root:
out_ds:
pdb_reader_ds_exit(r);
- return err;
+ return 1;
}
static void pdb_reader_exit(struct pdb_reader *r)
@@ -278,7 +272,6 @@ static void pdb_reader_exit(struct pdb_reader *r)
int pdb_init_from_file(const char *name, struct pdb_reader *reader)
{
GError *gerr = NULL;
- int err = 0;
void *map;
reader->gmf = g_mapped_file_new(name, TRUE, &gerr);
@@ -291,7 +284,6 @@ int pdb_init_from_file(const char *name, struct pdb_reader *reader)
reader->file_size = g_mapped_file_get_length(reader->gmf);
map = g_mapped_file_get_contents(reader->gmf);
if (pdb_reader_init(reader, map)) {
- err = 1;
goto out_unmap;
}
@@ -300,7 +292,7 @@ int pdb_init_from_file(const char *name, struct pdb_reader *reader)
out_unmap:
g_mapped_file_unref(reader->gmf);
- return err;
+ return 1;
}
void pdb_exit(struct pdb_reader *reader)
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 02/13] contrib/elf2dmp: Assume error by default
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:24 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 03/13] contrib/elf2dmp: Continue even contexts are lacking Akihiko Odaki
` (10 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
A common construct in contrib/elf2dmp is to set "err" flag and goto
in error paths. In such a construct, there is only one successful path
while there are several error paths, so it will be more simpler to
initialize "err" flag set, and clear it in the successful path.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
contrib/elf2dmp/download.c | 4 +---
contrib/elf2dmp/main.c | 15 +++------------
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
index bd7650a7a27f..902dc04ffa5c 100644
--- a/contrib/elf2dmp/download.c
+++ b/contrib/elf2dmp/download.c
@@ -11,7 +11,7 @@
int download_url(const char *name, const char *url)
{
- int err = 0;
+ int err = 1;
FILE *file;
CURL *curl = curl_easy_init();
@@ -21,7 +21,6 @@ int download_url(const char *name, const char *url)
file = fopen(name, "wb");
if (!file) {
- err = 1;
goto out_curl;
}
@@ -33,7 +32,6 @@ int download_url(const char *name, const char *url)
|| curl_easy_perform(curl) != CURLE_OK) {
unlink(name);
fclose(file);
- err = 1;
} else {
err = fclose(file);
}
diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index cbc38a7c103a..9b278f392e39 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -511,7 +511,7 @@ static void pe_get_pdb_symstore_hash(OMFSignatureRSDS *rsds, char *hash)
int main(int argc, char *argv[])
{
- int err = 0;
+ int err = 1;
QEMU_Elf qemu_elf;
struct pa_space ps;
struct va_space vs;
@@ -542,7 +542,6 @@ int main(int argc, char *argv[])
if (pa_space_create(&ps, &qemu_elf)) {
eprintf("Failed to initialize physical address space\n");
- err = 1;
goto out_elf;
}
@@ -552,7 +551,6 @@ int main(int argc, char *argv[])
va_space_create(&vs, &ps, state->cr[3]);
if (fix_dtb(&vs, &qemu_elf)) {
eprintf("Failed to find paging base\n");
- err = 1;
goto out_elf;
}
@@ -561,7 +559,6 @@ int main(int argc, char *argv[])
if (va_space_rw(&vs, state->idt.base,
&first_idt_desc, sizeof(first_idt_desc), 0)) {
eprintf("Failed to get CPU #0 IDT[0]\n");
- err = 1;
goto out_ps;
}
printf("CPU #0 IDT[0] -> 0x%016"PRIx64"\n", idt_desc_addr(first_idt_desc));
@@ -586,7 +583,6 @@ int main(int argc, char *argv[])
if (!kernel_found) {
eprintf("Failed to find NT kernel image\n");
- err = 1;
goto out_ps;
}
@@ -600,45 +596,40 @@ int main(int argc, char *argv[])
if (download_url(PDB_NAME, pdb_url)) {
eprintf("Failed to download PDB file\n");
- err = 1;
goto out_ps;
}
if (pdb_init_from_file(PDB_NAME, &pdb)) {
eprintf("Failed to initialize PDB reader\n");
- err = 1;
goto out_pdb_file;
}
if (!SYM_RESOLVE(KernBase, &pdb, KdDebuggerDataBlock) ||
!SYM_RESOLVE(KernBase, &pdb, KdVersionBlock)) {
- err = 1;
goto out_pdb;
}
kdbg = get_kdbg(KernBase, &pdb, &vs, KdDebuggerDataBlock);
if (!kdbg) {
- err = 1;
goto out_pdb;
}
if (fill_header(&header, &ps, &vs, KdDebuggerDataBlock, kdbg,
KdVersionBlock, qemu_elf.state_nr)) {
- err = 1;
goto out_kdbg;
}
if (fill_context(kdbg, &vs, &qemu_elf)) {
- err = 1;
goto out_kdbg;
}
if (write_dump(&ps, &header, argv[2])) {
eprintf("Failed to save dump\n");
- err = 1;
goto out_kdbg;
}
+ err = 0;
+
out_kdbg:
g_free(kdbg);
out_pdb:
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 03/13] contrib/elf2dmp: Continue even contexts are lacking
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 02/13] contrib/elf2dmp: Assume error by default Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern Akihiko Odaki
` (9 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
Let fill_context() continue even if it fails to fill contexts of some
CPUs. A dump may still contain valuable information even if it lacks
contexts of some CPUs due to dump corruption or a failure before
starting CPUs.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
contrib/elf2dmp/main.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 9b278f392e39..89bf4e23566b 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -336,7 +336,12 @@ static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
return 0;
}
-static int fill_context(KDDEBUGGER_DATA64 *kdbg,
+/*
+ * fill_context() continues even if it fails to fill contexts of some CPUs.
+ * A dump may still contain valuable information even if it lacks contexts of
+ * some CPUs due to dump corruption or a failure before starting CPUs.
+ */
+static void fill_context(KDDEBUGGER_DATA64 *kdbg,
struct va_space *vs, QEMU_Elf *qe)
{
int i;
@@ -350,7 +355,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) {
@@ -361,7 +366,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);
@@ -369,11 +374,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,
@@ -619,9 +622,7 @@ int main(int argc, char *argv[])
goto out_kdbg;
}
- if (fill_context(kdbg, &vs, &qemu_elf)) {
- 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (2 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 03/13] contrib/elf2dmp: Continue even contexts are lacking Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:28 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 05/13] contrib/elf2dmp: Always check for PA resolution failure Akihiko Odaki
` (8 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
include/qapi/error.h says:
> We recommend
> * bool-valued functions return true on success / false on failure,
> ...
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
contrib/elf2dmp/addrspace.h | 6 +--
contrib/elf2dmp/download.h | 2 +-
contrib/elf2dmp/pdb.h | 2 +-
contrib/elf2dmp/qemu_elf.h | 2 +-
contrib/elf2dmp/addrspace.c | 12 ++---
contrib/elf2dmp/download.c | 10 ++--
contrib/elf2dmp/main.c | 114 +++++++++++++++++++++-----------------------
contrib/elf2dmp/pdb.c | 50 +++++++++----------
contrib/elf2dmp/qemu_elf.c | 32 ++++++-------
9 files changed, 112 insertions(+), 118 deletions(-)
diff --git a/contrib/elf2dmp/addrspace.h b/contrib/elf2dmp/addrspace.h
index 039c70c5b079..2ad30a9da48a 100644
--- a/contrib/elf2dmp/addrspace.h
+++ b/contrib/elf2dmp/addrspace.h
@@ -33,13 +33,13 @@ struct va_space {
struct pa_space *ps;
};
-int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf);
+void pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf);
void pa_space_destroy(struct pa_space *ps);
void va_space_create(struct va_space *vs, struct pa_space *ps, uint64_t dtb);
void va_space_set_dtb(struct va_space *vs, uint64_t dtb);
void *va_space_resolve(struct va_space *vs, uint64_t va);
-int va_space_rw(struct va_space *vs, uint64_t addr,
- void *buf, size_t size, int is_write);
+bool va_space_rw(struct va_space *vs, uint64_t addr,
+ void *buf, size_t size, int is_write);
#endif /* ADDRSPACE_H */
diff --git a/contrib/elf2dmp/download.h b/contrib/elf2dmp/download.h
index 5c274925f7aa..f65adb5d0894 100644
--- a/contrib/elf2dmp/download.h
+++ b/contrib/elf2dmp/download.h
@@ -8,6 +8,6 @@
#ifndef DOWNLOAD_H
#define DOWNLOAD_H
-int download_url(const char *name, const char *url);
+bool download_url(const char *name, const char *url);
#endif /* DOWNLOAD_H */
diff --git a/contrib/elf2dmp/pdb.h b/contrib/elf2dmp/pdb.h
index 2a50da56ac96..feddf1862f08 100644
--- a/contrib/elf2dmp/pdb.h
+++ b/contrib/elf2dmp/pdb.h
@@ -233,7 +233,7 @@ struct pdb_reader {
size_t segs_size;
};
-int pdb_init_from_file(const char *name, struct pdb_reader *reader);
+bool pdb_init_from_file(const char *name, struct pdb_reader *reader);
void pdb_exit(struct pdb_reader *reader);
uint64_t pdb_resolve(uint64_t img_base, struct pdb_reader *r, const char *name);
uint64_t pdb_find_public_v3_symbol(struct pdb_reader *reader, const char *name);
diff --git a/contrib/elf2dmp/qemu_elf.h b/contrib/elf2dmp/qemu_elf.h
index afa75f10b2d2..adc50238b46b 100644
--- a/contrib/elf2dmp/qemu_elf.h
+++ b/contrib/elf2dmp/qemu_elf.h
@@ -42,7 +42,7 @@ typedef struct QEMU_Elf {
int has_kernel_gs_base;
} QEMU_Elf;
-int QEMU_Elf_init(QEMU_Elf *qe, const char *filename);
+bool QEMU_Elf_init(QEMU_Elf *qe, const char *filename);
void QEMU_Elf_exit(QEMU_Elf *qe);
Elf64_Phdr *elf64_getphdr(void *map);
diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
index 6f608a517b1e..c995c723ae80 100644
--- a/contrib/elf2dmp/addrspace.c
+++ b/contrib/elf2dmp/addrspace.c
@@ -57,7 +57,7 @@ static void pa_block_align(struct pa_block *b)
b->paddr += low_align;
}
-int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf)
+void pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf)
{
Elf64_Half phdr_nr = elf_getphdrnum(qemu_elf->map);
Elf64_Phdr *phdr = elf64_getphdr(qemu_elf->map);
@@ -87,8 +87,6 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf)
}
ps->block_nr = block_i;
-
- return 0;
}
void pa_space_destroy(struct pa_space *ps)
@@ -228,8 +226,8 @@ void *va_space_resolve(struct va_space *vs, uint64_t va)
return pa_space_resolve(vs->ps, pa);
}
-int va_space_rw(struct va_space *vs, uint64_t addr,
- void *buf, size_t size, int is_write)
+bool va_space_rw(struct va_space *vs, uint64_t addr,
+ void *buf, size_t size, int is_write)
{
while (size) {
uint64_t page = addr & ELF2DMP_PFN_MASK;
@@ -240,7 +238,7 @@ int va_space_rw(struct va_space *vs, uint64_t addr,
ptr = va_space_resolve(vs, addr);
if (!ptr) {
- return 1;
+ return false;
}
if (is_write) {
@@ -254,5 +252,5 @@ int va_space_rw(struct va_space *vs, uint64_t addr,
addr += s;
}
- return 0;
+ return true;
}
diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
index 902dc04ffa5c..ec8d33ba1e4b 100644
--- a/contrib/elf2dmp/download.c
+++ b/contrib/elf2dmp/download.c
@@ -9,14 +9,14 @@
#include <curl/curl.h>
#include "download.h"
-int download_url(const char *name, const char *url)
+bool download_url(const char *name, const char *url)
{
- int err = 1;
+ bool success = false;
FILE *file;
CURL *curl = curl_easy_init();
if (!curl) {
- return 1;
+ return success;
}
file = fopen(name, "wb");
@@ -33,11 +33,11 @@ int download_url(const char *name, const char *url)
unlink(name);
fclose(file);
} else {
- err = fclose(file);
+ success = !fclose(file);
}
out_curl:
curl_easy_cleanup(curl);
- return err;
+ return success;
}
diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 89bf4e23566b..140ac6e00cfe 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -79,7 +79,7 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb,
bool decode = false;
uint64_t kwn, kwa, KdpDataBlockEncoded;
- if (va_space_rw(vs,
+ if (!va_space_rw(vs,
KdDebuggerDataBlock + offsetof(KDDEBUGGER_DATA64, Header),
&kdbg_hdr, sizeof(kdbg_hdr), 0)) {
eprintf("Failed to extract KDBG header\n");
@@ -97,8 +97,8 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb,
return NULL;
}
- if (va_space_rw(vs, KiWaitNever, &kwn, sizeof(kwn), 0) ||
- va_space_rw(vs, KiWaitAlways, &kwa, sizeof(kwa), 0)) {
+ if (!va_space_rw(vs, KiWaitNever, &kwn, sizeof(kwn), 0) ||
+ !va_space_rw(vs, KiWaitAlways, &kwa, sizeof(kwa), 0)) {
return NULL;
}
@@ -122,7 +122,7 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb,
kdbg = g_malloc(kdbg_hdr.Size);
- if (va_space_rw(vs, KdDebuggerDataBlock, kdbg, kdbg_hdr.Size, 0)) {
+ if (!va_space_rw(vs, KdDebuggerDataBlock, kdbg, kdbg_hdr.Size, 0)) {
eprintf("Failed to extract entire KDBG\n");
g_free(kdbg);
return NULL;
@@ -186,13 +186,13 @@ static void win_context_init_from_qemu_cpu_state(WinContext64 *ctx,
* Finds paging-structure hierarchy base,
* if previously set doesn't give access to kernel structures
*/
-static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
+static bool fix_dtb(struct va_space *vs, QEMU_Elf *qe)
{
/*
* Firstly, test previously set DTB.
*/
if (va_space_resolve(vs, SharedUserData)) {
- return 0;
+ return true;
}
/*
@@ -206,7 +206,7 @@ static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
va_space_set_dtb(vs, s->cr[3]);
printf("DTB 0x%016"PRIx64" has been found from CPU #%zu"
" as system task CR3\n", vs->dtb, i);
- return !(va_space_resolve(vs, SharedUserData));
+ return !!(va_space_resolve(vs, SharedUserData));
}
}
@@ -226,10 +226,10 @@ static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
va_space_set_dtb(vs, *cr3);
printf("DirectoryTableBase = 0x%016"PRIx64" has been found from CPU #0"
" as interrupt handling CR3\n", vs->dtb);
- return !(va_space_resolve(vs, SharedUserData));
+ return !!(va_space_resolve(vs, SharedUserData));
}
- return 1;
+ return false;
}
static void try_merge_runs(struct pa_space *ps,
@@ -268,9 +268,10 @@ static void try_merge_runs(struct pa_space *ps,
}
}
-static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
- struct va_space *vs, uint64_t KdDebuggerDataBlock,
- KDDEBUGGER_DATA64 *kdbg, uint64_t KdVersionBlock, int nr_cpus)
+static bool fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
+ struct va_space *vs, uint64_t KdDebuggerDataBlock,
+ KDDEBUGGER_DATA64 *kdbg, uint64_t KdVersionBlock,
+ int nr_cpus)
{
uint32_t *suite_mask = va_space_resolve(vs, SharedUserData +
KUSD_OFFSET_SUITE_MASK);
@@ -283,12 +284,12 @@ static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
QEMU_BUILD_BUG_ON(KUSD_OFFSET_PRODUCT_TYPE >= ELF2DMP_PAGE_SIZE);
if (!suite_mask || !product_type) {
- return 1;
+ return false;
}
- if (va_space_rw(vs, KdVersionBlock, &kvb, sizeof(kvb), 0)) {
+ if (!va_space_rw(vs, KdVersionBlock, &kvb, sizeof(kvb), 0)) {
eprintf("Failed to extract KdVersionBlock\n");
- return 1;
+ return false;
}
h = (WinDumpHeader64) {
@@ -333,7 +334,7 @@ static int fill_header(WinDumpHeader64 *hdr, struct pa_space *ps,
*hdr = h;
- return 0;
+ return true;
}
/*
@@ -352,8 +353,8 @@ static void fill_context(KDDEBUGGER_DATA64 *kdbg,
WinContext64 ctx;
QEMUCPUState *s = qe->state[i];
- if (va_space_rw(vs, kdbg->KiProcessorBlock + sizeof(Prcb) * i,
- &Prcb, sizeof(Prcb), 0)) {
+ if (!va_space_rw(vs, kdbg->KiProcessorBlock + sizeof(Prcb) * i,
+ &Prcb, sizeof(Prcb), 0)) {
eprintf("Failed to read CPU #%d PRCB location\n", i);
continue;
}
@@ -363,8 +364,8 @@ static void fill_context(KDDEBUGGER_DATA64 *kdbg,
continue;
}
- if (va_space_rw(vs, Prcb + kdbg->OffsetPrcbContext,
- &Context, sizeof(Context), 0)) {
+ if (!va_space_rw(vs, Prcb + kdbg->OffsetPrcbContext,
+ &Context, sizeof(Context), 0)) {
eprintf("Failed to read CPU #%d ContextFrame location\n", i);
continue;
}
@@ -372,15 +373,15 @@ static void fill_context(KDDEBUGGER_DATA64 *kdbg,
printf("Filling context for CPU #%d...\n", i);
win_context_init_from_qemu_cpu_state(&ctx, s);
- if (va_space_rw(vs, Context, &ctx, sizeof(ctx), 1)) {
+ if (!va_space_rw(vs, Context, &ctx, sizeof(ctx), 1)) {
eprintf("Failed to fill CPU #%d context\n", i);
continue;
}
}
}
-static int pe_get_data_dir_entry(uint64_t base, void *start_addr, int idx,
- void *entry, size_t size, struct va_space *vs)
+static bool pe_get_data_dir_entry(uint64_t base, void *start_addr, int idx,
+ void *entry, size_t size, struct va_space *vs)
{
const char e_magic[2] = "MZ";
const char Signature[4] = "PE\0\0";
@@ -393,40 +394,39 @@ static int pe_get_data_dir_entry(uint64_t base, void *start_addr, int idx,
QEMU_BUILD_BUG_ON(sizeof(*dos_hdr) >= ELF2DMP_PAGE_SIZE);
if (memcmp(&dos_hdr->e_magic, e_magic, sizeof(e_magic))) {
- return 1;
+ return false;
}
- if (va_space_rw(vs, base + dos_hdr->e_lfanew,
- &nt_hdrs, sizeof(nt_hdrs), 0)) {
- return 1;
+ if (!va_space_rw(vs, base + dos_hdr->e_lfanew,
+ &nt_hdrs, sizeof(nt_hdrs), 0)) {
+ return false;
}
if (memcmp(&nt_hdrs.Signature, Signature, sizeof(Signature)) ||
file_hdr->Machine != 0x8664 || opt_hdr->Magic != 0x020b) {
- return 1;
+ return false;
}
- if (va_space_rw(vs,
- base + data_dir[idx].VirtualAddress,
- entry, size, 0)) {
- return 1;
+ if (!va_space_rw(vs, base + data_dir[idx].VirtualAddress,
+ entry, size, 0)) {
+ return false;
}
printf("Data directory entry #%d: RVA = 0x%08"PRIx32"\n", idx,
(uint32_t)data_dir[idx].VirtualAddress);
- return 0;
+ return true;
}
-static int write_dump(struct pa_space *ps,
- WinDumpHeader64 *hdr, const char *name)
+static bool write_dump(struct pa_space *ps,
+ WinDumpHeader64 *hdr, const char *name)
{
FILE *dmp_file = fopen(name, "wb");
size_t i;
if (!dmp_file) {
eprintf("Failed to open output file \'%s\'\n", name);
- return 1;
+ return false;
}
printf("Writing header to file...\n");
@@ -434,7 +434,7 @@ static int write_dump(struct pa_space *ps,
if (fwrite(hdr, sizeof(*hdr), 1, dmp_file) != 1) {
eprintf("Failed to write dump header\n");
fclose(dmp_file);
- return 1;
+ return false;
}
for (i = 0; i < ps->block_nr; i++) {
@@ -445,11 +445,11 @@ static int write_dump(struct pa_space *ps,
if (fwrite(b->addr, b->size, 1, dmp_file) != 1) {
eprintf("Failed to write block\n");
fclose(dmp_file);
- return 1;
+ return false;
}
}
- return fclose(dmp_file);
+ return !fclose(dmp_file);
}
static bool pe_check_pdb_name(uint64_t base, void *start_addr,
@@ -459,8 +459,8 @@ static bool pe_check_pdb_name(uint64_t base, void *start_addr,
IMAGE_DEBUG_DIRECTORY debug_dir;
char pdb_name[sizeof(PDB_NAME)];
- if (pe_get_data_dir_entry(base, start_addr, IMAGE_FILE_DEBUG_DIRECTORY,
- &debug_dir, sizeof(debug_dir), vs)) {
+ if (!pe_get_data_dir_entry(base, start_addr, IMAGE_FILE_DEBUG_DIRECTORY,
+ &debug_dir, sizeof(debug_dir), vs)) {
eprintf("Failed to get Debug Directory\n");
return false;
}
@@ -470,9 +470,8 @@ static bool pe_check_pdb_name(uint64_t base, void *start_addr,
return false;
}
- if (va_space_rw(vs,
- base + debug_dir.AddressOfRawData,
- rsds, sizeof(*rsds), 0)) {
+ if (!va_space_rw(vs, base + debug_dir.AddressOfRawData,
+ rsds, sizeof(*rsds), 0)) {
eprintf("Failed to resolve OMFSignatureRSDS\n");
return false;
}
@@ -488,9 +487,9 @@ static bool pe_check_pdb_name(uint64_t base, void *start_addr,
return false;
}
- if (va_space_rw(vs, base + debug_dir.AddressOfRawData +
- offsetof(OMFSignatureRSDS, name), pdb_name, sizeof(PDB_NAME),
- 0)) {
+ if (!va_space_rw(vs, base + debug_dir.AddressOfRawData +
+ offsetof(OMFSignatureRSDS, name),
+ pdb_name, sizeof(PDB_NAME), 0)) {
eprintf("Failed to resolve PDB name\n");
return false;
}
@@ -538,28 +537,25 @@ int main(int argc, char *argv[])
return 1;
}
- if (QEMU_Elf_init(&qemu_elf, argv[1])) {
+ if (!QEMU_Elf_init(&qemu_elf, argv[1])) {
eprintf("Failed to initialize QEMU ELF dump\n");
return 1;
}
- if (pa_space_create(&ps, &qemu_elf)) {
- eprintf("Failed to initialize physical address space\n");
- goto out_elf;
- }
+ pa_space_create(&ps, &qemu_elf);
state = qemu_elf.state[0];
printf("CPU #0 CR3 is 0x%016"PRIx64"\n", state->cr[3]);
va_space_create(&vs, &ps, state->cr[3]);
- if (fix_dtb(&vs, &qemu_elf)) {
+ if (!fix_dtb(&vs, &qemu_elf)) {
eprintf("Failed to find paging base\n");
goto out_elf;
}
printf("CPU #0 IDT is at 0x%016"PRIx64"\n", state->idt.base);
- if (va_space_rw(&vs, state->idt.base,
+ if (!va_space_rw(&vs, state->idt.base,
&first_idt_desc, sizeof(first_idt_desc), 0)) {
eprintf("Failed to get CPU #0 IDT[0]\n");
goto out_ps;
@@ -597,12 +593,12 @@ int main(int argc, char *argv[])
sprintf(pdb_url, "%s%s/%s/%s", SYM_URL_BASE, PDB_NAME, pdb_hash, PDB_NAME);
printf("PDB URL is %s\n", pdb_url);
- if (download_url(PDB_NAME, pdb_url)) {
+ if (!download_url(PDB_NAME, pdb_url)) {
eprintf("Failed to download PDB file\n");
goto out_ps;
}
- if (pdb_init_from_file(PDB_NAME, &pdb)) {
+ if (!pdb_init_from_file(PDB_NAME, &pdb)) {
eprintf("Failed to initialize PDB reader\n");
goto out_pdb_file;
}
@@ -617,14 +613,14 @@ int main(int argc, char *argv[])
goto out_pdb;
}
- if (fill_header(&header, &ps, &vs, KdDebuggerDataBlock, kdbg,
- KdVersionBlock, qemu_elf.state_nr)) {
+ if (!fill_header(&header, &ps, &vs, KdDebuggerDataBlock, kdbg,
+ KdVersionBlock, qemu_elf.state_nr)) {
goto out_kdbg;
}
fill_context(kdbg, &vs, &qemu_elf);
- if (write_dump(&ps, &header, argv[2])) {
+ if (!write_dump(&ps, &header, argv[2])) {
eprintf("Failed to save dump\n");
goto out_kdbg;
}
diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index abf17c2e7c12..1c5051425185 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -158,30 +158,30 @@ static void *pdb_ds_read_file(struct pdb_reader* r, uint32_t file_number)
return pdb_ds_read(r->ds.header, block_list, file_size[file_number]);
}
-static int pdb_init_segments(struct pdb_reader *r)
+static bool pdb_init_segments(struct pdb_reader *r)
{
unsigned stream_idx = r->segments;
r->segs = pdb_ds_read_file(r, stream_idx);
if (!r->segs) {
- return 1;
+ return false;
}
r->segs_size = pdb_get_file_size(r, stream_idx);
if (!r->segs_size) {
- return 1;
+ return false;
}
- return 0;
+ return true;
}
-static int pdb_init_symbols(struct pdb_reader *r)
+static bool pdb_init_symbols(struct pdb_reader *r)
{
PDB_SYMBOLS *symbols;
symbols = pdb_ds_read_file(r, 3);
if (!symbols) {
- return 1;
+ return false;
}
r->symbols = symbols;
@@ -198,18 +198,18 @@ static int pdb_init_symbols(struct pdb_reader *r)
goto out_symbols;
}
- return 0;
+ return true;
out_symbols:
g_free(symbols);
- return 1;
+ return false;
}
-static int pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER *hdr)
+static bool pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER *hdr)
{
if (hdr->block_size == 0) {
- return 1;
+ return false;
}
memset(r->file_used, 0, sizeof(r->file_used));
@@ -218,22 +218,22 @@ static int pdb_reader_ds_init(struct pdb_reader *r, PDB_DS_HEADER *hdr)
hdr->toc_page * hdr->block_size), hdr->toc_size);
if (!r->ds.toc) {
- return 1;
+ return false;
}
- return 0;
+ return true;
}
-static int pdb_reader_init(struct pdb_reader *r, void *data)
+static bool pdb_reader_init(struct pdb_reader *r, void *data)
{
const char pdb7[] = "Microsoft C/C++ MSF 7.00";
if (memcmp(data, pdb7, sizeof(pdb7) - 1)) {
- return 1;
+ return false;
}
- if (pdb_reader_ds_init(r, data)) {
- return 1;
+ if (!pdb_reader_ds_init(r, data)) {
+ return false;
}
r->ds.root = pdb_ds_read_file(r, 1);
@@ -241,15 +241,15 @@ static int pdb_reader_init(struct pdb_reader *r, void *data)
goto out_ds;
}
- if (pdb_init_symbols(r)) {
+ if (!pdb_init_symbols(r)) {
goto out_root;
}
- if (pdb_init_segments(r)) {
+ if (!pdb_init_segments(r)) {
goto out_sym;
}
- return 0;
+ return true;
out_sym:
pdb_exit_symbols(r);
@@ -258,7 +258,7 @@ out_root:
out_ds:
pdb_reader_ds_exit(r);
- return 1;
+ return false;
}
static void pdb_reader_exit(struct pdb_reader *r)
@@ -269,7 +269,7 @@ static void pdb_reader_exit(struct pdb_reader *r)
pdb_reader_ds_exit(r);
}
-int pdb_init_from_file(const char *name, struct pdb_reader *reader)
+bool pdb_init_from_file(const char *name, struct pdb_reader *reader)
{
GError *gerr = NULL;
void *map;
@@ -278,21 +278,21 @@ int pdb_init_from_file(const char *name, struct pdb_reader *reader)
if (gerr) {
eprintf("Failed to map PDB file \'%s\'\n", name);
g_error_free(gerr);
- return 1;
+ return false;
}
reader->file_size = g_mapped_file_get_length(reader->gmf);
map = g_mapped_file_get_contents(reader->gmf);
- if (pdb_reader_init(reader, map)) {
+ if (!pdb_reader_init(reader, map)) {
goto out_unmap;
}
- return 0;
+ return true;
out_unmap:
g_mapped_file_unref(reader->gmf);
- return 1;
+ return false;
}
void pdb_exit(struct pdb_reader *reader)
diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
index 055e6f8792e9..a22c057d3ec3 100644
--- a/contrib/elf2dmp/qemu_elf.c
+++ b/contrib/elf2dmp/qemu_elf.c
@@ -60,7 +60,7 @@ Elf64_Half elf_getphdrnum(void *map)
return ehdr->e_phnum;
}
-static int init_states(QEMU_Elf *qe)
+static bool init_states(QEMU_Elf *qe)
{
Elf64_Phdr *phdr = elf64_getphdr(qe->map);
Elf64_Nhdr *start = (void *)((uint8_t *)qe->map + phdr[0].p_offset);
@@ -70,7 +70,7 @@ static int init_states(QEMU_Elf *qe)
if (phdr[0].p_type != PT_NOTE) {
eprintf("Failed to find PT_NOTE\n");
- return 1;
+ return false;
}
qe->has_kernel_gs_base = 1;
@@ -107,7 +107,7 @@ static int init_states(QEMU_Elf *qe)
qe->state_nr = cpu_nr;
- return 0;
+ return true;
}
static void exit_states(QEMU_Elf *qe)
@@ -162,7 +162,7 @@ static bool check_ehdr(QEMU_Elf *qe)
return true;
}
-static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
+static bool QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
{
#ifdef CONFIG_LINUX
struct stat st;
@@ -173,13 +173,13 @@ static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
fd = open(filename, O_RDONLY, 0);
if (fd == -1) {
eprintf("Failed to open ELF dump file \'%s\'\n", filename);
- return 1;
+ return false;
}
if (fstat(fd, &st)) {
eprintf("Failed to get size of ELF dump file\n");
close(fd);
- return 1;
+ return false;
}
qe->size = st.st_size;
@@ -188,7 +188,7 @@ static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
if (qe->map == MAP_FAILED) {
eprintf("Failed to map ELF file\n");
close(fd);
- return 1;
+ return false;
}
close(fd);
@@ -201,14 +201,14 @@ static int QEMU_Elf_map(QEMU_Elf *qe, const char *filename)
if (gerr) {
eprintf("Failed to map ELF dump file \'%s\'\n", filename);
g_error_free(gerr);
- return 1;
+ return false;
}
qe->map = g_mapped_file_get_contents(qe->gmf);
qe->size = g_mapped_file_get_length(qe->gmf);
#endif
- return 0;
+ return true;
}
static void QEMU_Elf_unmap(QEMU_Elf *qe)
@@ -220,25 +220,25 @@ static void QEMU_Elf_unmap(QEMU_Elf *qe)
#endif
}
-int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
+bool QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
{
- if (QEMU_Elf_map(qe, filename)) {
- return 1;
+ if (!QEMU_Elf_map(qe, filename)) {
+ return false;
}
if (!check_ehdr(qe)) {
eprintf("Input file has the wrong format\n");
QEMU_Elf_unmap(qe);
- return 1;
+ return false;
}
- if (init_states(qe)) {
+ if (!init_states(qe)) {
eprintf("Failed to extract QEMU CPU states\n");
QEMU_Elf_unmap(qe);
- return 1;
+ return false;
}
- return 0;
+ return true;
}
void QEMU_Elf_exit(QEMU_Elf *qe)
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 05/13] contrib/elf2dmp: Always check for PA resolution failure
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (3 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:29 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 06/13] contrib/elf2dmp: Always destroy PA space Akihiko Odaki
` (7 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, 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 c995c723ae80..e01860d15b07 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 bool pa_space_read64(struct pa_space *ps, uint64_t pa, uint64_t *value)
+{
+ uint64_t *resolved = pa_space_resolve(ps, pa);
+
+ if (!resolved) {
+ return false;
+ }
+
+ *value = *resolved;
+
+ return true;
+}
+
static void pa_block_align(struct pa_block *b)
{
uint64_t low_align = ((b->paddr - 1) | ELF2DMP_PAGE_MASK) + 1 - b->paddr;
@@ -106,19 +119,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 bool 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 bool 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)
@@ -131,11 +145,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 bool 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)
@@ -148,11 +163,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 bool 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)
@@ -184,13 +200,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;
}
@@ -198,8 +212,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;
}
@@ -207,8 +220,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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 06/13] contrib/elf2dmp: Always destroy PA space
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (4 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 05/13] contrib/elf2dmp: Always check for PA resolution failure Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 07/13] contrib/elf2dmp: Ensure segment fits in file Akihiko Odaki
` (6 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
contrib/elf2dmp/main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 140ac6e00cfe..25cf0fdff724 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -550,7 +550,7 @@ int main(int argc, char *argv[])
va_space_create(&vs, &ps, state->cr[3]);
if (!fix_dtb(&vs, &qemu_elf)) {
eprintf("Failed to find paging base\n");
- goto out_elf;
+ goto out_ps;
}
printf("CPU #0 IDT is at 0x%016"PRIx64"\n", state->idt.base);
@@ -635,7 +635,6 @@ out_pdb_file:
unlink(PDB_NAME);
out_ps:
pa_space_destroy(&ps);
-out_elf:
QEMU_Elf_exit(&qemu_elf);
return err;
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 07/13] contrib/elf2dmp: Ensure segment fits in file
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (5 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 06/13] contrib/elf2dmp: Always destroy PA space Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:31 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 08/13] contrib/elf2dmp: Use lduw_le_p() to read PDB Akihiko Odaki
` (5 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, 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 e01860d15b07..81295a11534a 100644
--- a/contrib/elf2dmp/addrspace.c
+++ b/contrib/elf2dmp/addrspace.c
@@ -88,11 +88,12 @@ void 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 08/13] contrib/elf2dmp: Use lduw_le_p() to read PDB
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (6 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 07/13] contrib/elf2dmp: Ensure segment fits in file Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:32 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 09/13] contrib/elf2dmp: Use rol64() to decode Akihiko Odaki
` (4 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, 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 1c5051425185..492aca4434c8 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"
@@ -186,7 +187,7 @@ static bool 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 09/13] contrib/elf2dmp: Use rol64() to decode
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (7 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 08/13] contrib/elf2dmp: Use lduw_le_p() to read PDB Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 10/13] MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer Akihiko Odaki
` (3 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
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>
---
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 25cf0fdff724..20547fd8f819 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 10/13] MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (8 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 09/13] contrib/elf2dmp: Use rol64() to decode Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host Akihiko Odaki
` (2 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (9 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 10/13] MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:33 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 12/13] contrib/elf2dmp: Use GPtrArray Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 13/13] contrib/elf2dmp: Clamp QEMU note to file size Akihiko Odaki
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
elf2dmp assumes little endian host in many places.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
contrib/elf2dmp/meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/elf2dmp/meson.build b/contrib/elf2dmp/meson.build
index 6707d43c4fa5..046569861f7a 100644
--- a/contrib/elf2dmp/meson.build
+++ b/contrib/elf2dmp/meson.build
@@ -1,4 +1,4 @@
-if curl.found()
+if curl.found() and host_machine.endian() == 'little'
executable('elf2dmp', files('main.c', 'addrspace.c', 'download.c', 'pdb.c', 'qemu_elf.c'), genh,
dependencies: [glib, curl],
install: true)
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 12/13] contrib/elf2dmp: Use GPtrArray
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (10 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:35 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 13/13] contrib/elf2dmp: Clamp QEMU note to file size Akihiko Odaki
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
This removes the need to enumarate QEMUCPUState twice and saves code.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
contrib/elf2dmp/qemu_elf.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
index a22c057d3ec3..7d896cac5b15 100644
--- a/contrib/elf2dmp/qemu_elf.c
+++ b/contrib/elf2dmp/qemu_elf.c
@@ -66,7 +66,7 @@ static bool init_states(QEMU_Elf *qe)
Elf64_Nhdr *start = (void *)((uint8_t *)qe->map + phdr[0].p_offset);
Elf64_Nhdr *end = (void *)((uint8_t *)start + phdr[0].p_memsz);
Elf64_Nhdr *nhdr;
- size_t cpu_nr = 0;
+ GPtrArray *states;
if (phdr[0].p_type != PT_NOTE) {
eprintf("Failed to find PT_NOTE\n");
@@ -74,38 +74,29 @@ static bool init_states(QEMU_Elf *qe)
}
qe->has_kernel_gs_base = 1;
+ states = g_ptr_array_new();
for (nhdr = start; nhdr < end; nhdr = nhdr_get_next(nhdr)) {
if (!strcmp(nhdr_get_name(nhdr), QEMU_NOTE_NAME)) {
QEMUCPUState *state = nhdr_get_desc(nhdr);
if (state->size < sizeof(*state)) {
- eprintf("CPU #%zu: QEMU CPU state size %u doesn't match\n",
- cpu_nr, state->size);
+ eprintf("CPU #%u: QEMU CPU state size %u doesn't match\n",
+ states->len, state->size);
/*
* We assume either every QEMU CPU state has KERNEL_GS_BASE or
* no one has.
*/
qe->has_kernel_gs_base = 0;
}
- cpu_nr++;
+ g_ptr_array_add(states, state);
}
}
- printf("%zu CPU states has been found\n", cpu_nr);
+ printf("%u CPU states has been found\n", states->len);
- qe->state = g_new(QEMUCPUState*, cpu_nr);
-
- cpu_nr = 0;
-
- for (nhdr = start; nhdr < end; nhdr = nhdr_get_next(nhdr)) {
- if (!strcmp(nhdr_get_name(nhdr), QEMU_NOTE_NAME)) {
- qe->state[cpu_nr] = nhdr_get_desc(nhdr);
- cpu_nr++;
- }
- }
-
- qe->state_nr = cpu_nr;
+ qe->state_nr = states->len;
+ qe->state = (void *)g_ptr_array_free(states, FALSE);
return true;
}
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 13/13] contrib/elf2dmp: Clamp QEMU note to file size
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
` (11 preceding siblings ...)
2024-03-05 7:36 ` [PATCH v2 12/13] contrib/elf2dmp: Use GPtrArray Akihiko Odaki
@ 2024-03-05 7:36 ` Akihiko Odaki
2024-03-05 13:38 ` Peter Maydell
12 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-05 7:36 UTC (permalink / raw)
To: Viktor Prutyanov, Peter Maydell; +Cc: qemu-devel, Akihiko Odaki
This fixes crashes with truncated dumps.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2202
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
contrib/elf2dmp/qemu_elf.c | 87 +++++++++++++++++++++++++++++-----------------
1 file changed, 55 insertions(+), 32 deletions(-)
diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
index 7d896cac5b15..8d750adf904a 100644
--- a/contrib/elf2dmp/qemu_elf.c
+++ b/contrib/elf2dmp/qemu_elf.c
@@ -6,6 +6,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/host-utils.h"
#include "err.h"
#include "qemu_elf.h"
@@ -15,36 +16,11 @@
#define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d)))
#endif
-#ifndef DIV_ROUND_UP
-#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
-#endif
-
-#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size) \
- ((DIV_ROUND_UP((hdr_size), 4) + \
- DIV_ROUND_UP((name_size), 4) + \
- DIV_ROUND_UP((desc_size), 4)) * 4)
-
int is_system(QEMUCPUState *s)
{
return s->gs.base >> 63;
}
-static char *nhdr_get_name(Elf64_Nhdr *nhdr)
-{
- return (char *)nhdr + ROUND_UP(sizeof(*nhdr), 4);
-}
-
-static void *nhdr_get_desc(Elf64_Nhdr *nhdr)
-{
- return nhdr_get_name(nhdr) + ROUND_UP(nhdr->n_namesz, 4);
-}
-
-static Elf64_Nhdr *nhdr_get_next(Elf64_Nhdr *nhdr)
-{
- return (void *)((uint8_t *)nhdr + ELF_NOTE_SIZE(sizeof(*nhdr),
- nhdr->n_namesz, nhdr->n_descsz));
-}
-
Elf64_Phdr *elf64_getphdr(void *map)
{
Elf64_Ehdr *ehdr = map;
@@ -60,13 +36,35 @@ Elf64_Half elf_getphdrnum(void *map)
return ehdr->e_phnum;
}
+static bool advance_note_offset(uint64_t *offsetp, uint64_t size, uint64_t end)
+{
+ uint64_t offset = *offsetp;
+
+ if (uadd64_overflow(offset, size, &offset) || offset > UINT64_MAX - 3) {
+ return false;
+ }
+
+ offset = ROUND_UP(offset, 4);
+
+ if (offset > end) {
+ return false;
+ }
+
+ *offsetp = offset;
+
+ return true;
+}
+
static bool init_states(QEMU_Elf *qe)
{
Elf64_Phdr *phdr = elf64_getphdr(qe->map);
- Elf64_Nhdr *start = (void *)((uint8_t *)qe->map + phdr[0].p_offset);
- Elf64_Nhdr *end = (void *)((uint8_t *)start + phdr[0].p_memsz);
Elf64_Nhdr *nhdr;
GPtrArray *states;
+ QEMUCPUState *state;
+ uint32_t state_size;
+ uint64_t offset;
+ uint64_t end_offset;
+ char *name;
if (phdr[0].p_type != PT_NOTE) {
eprintf("Failed to find PT_NOTE\n");
@@ -74,15 +72,40 @@ static bool init_states(QEMU_Elf *qe)
}
qe->has_kernel_gs_base = 1;
+ offset = phdr[0].p_offset;
states = g_ptr_array_new();
- for (nhdr = start; nhdr < end; nhdr = nhdr_get_next(nhdr)) {
- if (!strcmp(nhdr_get_name(nhdr), QEMU_NOTE_NAME)) {
- QEMUCPUState *state = nhdr_get_desc(nhdr);
+ if (uadd64_overflow(offset, phdr[0].p_memsz, &end_offset) ||
+ end_offset > qe->size) {
+ end_offset = qe->size;
+ }
+
+ while (offset < end_offset) {
+ nhdr = (void *)((uint8_t *)qe->map + offset);
+
+ if (!advance_note_offset(&offset, sizeof(*nhdr), end_offset)) {
+ break;
+ }
+
+ name = (char *)qe->map + offset;
+
+ if (!advance_note_offset(&offset, nhdr->n_namesz, end_offset)) {
+ break;
+ }
+
+ state = (void *)((uint8_t *)qe->map + offset);
+
+ if (!advance_note_offset(&offset, nhdr->n_descsz, end_offset)) {
+ break;
+ }
+
+ if (!strcmp(name, QEMU_NOTE_NAME) &&
+ nhdr->n_descsz >= offsetof(QEMUCPUState, kernel_gs_base)) {
+ state_size = MIN(state->size, nhdr->n_descsz);
- if (state->size < sizeof(*state)) {
+ if (state_size < sizeof(*state)) {
eprintf("CPU #%u: QEMU CPU state size %u doesn't match\n",
- states->len, state->size);
+ states->len, state_size);
/*
* We assume either every QEMU CPU state has KERNEL_GS_BASE or
* no one has.
--
2.44.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags
2024-03-05 7:36 ` [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags Akihiko Odaki
@ 2024-03-05 13:24 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:24 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> They are always evaluated to 1.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> contrib/elf2dmp/pdb.c | 14 +++-----------
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 02/13] contrib/elf2dmp: Assume error by default
2024-03-05 7:36 ` [PATCH v2 02/13] contrib/elf2dmp: Assume error by default Akihiko Odaki
@ 2024-03-05 13:24 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:24 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> A common construct in contrib/elf2dmp is to set "err" flag and goto
> in error paths. In such a construct, there is only one successful path
> while there are several error paths, so it will be more simpler to
> initialize "err" flag set, and clear it in the successful path.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern
2024-03-05 7:36 ` [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern Akihiko Odaki
@ 2024-03-05 13:28 ` Peter Maydell
2024-03-06 5:00 ` Akihiko Odaki
0 siblings, 1 reply; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:28 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> include/qapi/error.h says:
> > We recommend
> > * bool-valued functions return true on success / false on failure,
> > ...
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> contrib/elf2dmp/addrspace.h | 6 +--
> contrib/elf2dmp/download.h | 2 +-
> contrib/elf2dmp/pdb.h | 2 +-
> contrib/elf2dmp/qemu_elf.h | 2 +-
> contrib/elf2dmp/addrspace.c | 12 ++---
> contrib/elf2dmp/download.c | 10 ++--
> contrib/elf2dmp/main.c | 114 +++++++++++++++++++++-----------------------
> contrib/elf2dmp/pdb.c | 50 +++++++++----------
> contrib/elf2dmp/qemu_elf.c | 32 ++++++-------
> 9 files changed, 112 insertions(+), 118 deletions(-)
This is a bit big to review easily. Converting one function
(or a small set of closely related functions) at once would
make for an easier to review split.
> diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
> index 902dc04ffa5c..ec8d33ba1e4b 100644
> --- a/contrib/elf2dmp/download.c
> +++ b/contrib/elf2dmp/download.c
> @@ -9,14 +9,14 @@
> #include <curl/curl.h>
> #include "download.h"
>
> -int download_url(const char *name, const char *url)
> +bool download_url(const char *name, const char *url)
> {
> - int err = 1;
> + bool success = false;
> FILE *file;
> CURL *curl = curl_easy_init();
>
> if (!curl) {
> - return 1;
> + return success;
Why not just "return false" ? "return success" makes it look
like a success-path, not a failure-path.
> }
>
> file = fopen(name, "wb");
> @@ -33,11 +33,11 @@ int download_url(const char *name, const char *url)
> unlink(name);
> fclose(file);
> } else {
> - err = fclose(file);
> + success = !fclose(file);
> }
>
> out_curl:
> curl_easy_cleanup(curl);
>
> - return err;
> + return success;
> }
> @@ -186,13 +186,13 @@ static void win_context_init_from_qemu_cpu_state(WinContext64 *ctx,
> * Finds paging-structure hierarchy base,
> * if previously set doesn't give access to kernel structures
> */
> -static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
> +static bool fix_dtb(struct va_space *vs, QEMU_Elf *qe)
> {
> /*
> * Firstly, test previously set DTB.
> */
> if (va_space_resolve(vs, SharedUserData)) {
> - return 0;
> + return true;
> }
>
> /*
> @@ -206,7 +206,7 @@ static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
> va_space_set_dtb(vs, s->cr[3]);
> printf("DTB 0x%016"PRIx64" has been found from CPU #%zu"
> " as system task CR3\n", vs->dtb, i);
> - return !(va_space_resolve(vs, SharedUserData));
> + return !!(va_space_resolve(vs, SharedUserData));
If the function returns bool type, we don't need the !! idiom
to coerce the value to bool.
> }
> }
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 05/13] contrib/elf2dmp: Always check for PA resolution failure
2024-03-05 7:36 ` [PATCH v2 05/13] contrib/elf2dmp: Always check for PA resolution failure Akihiko Odaki
@ 2024-03-05 13:29 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:29 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:36, 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 07/13] contrib/elf2dmp: Ensure segment fits in file
2024-03-05 7:36 ` [PATCH v2 07/13] contrib/elf2dmp: Ensure segment fits in file Akihiko Odaki
@ 2024-03-05 13:31 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:31 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:36, 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(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Though I'm not sure where we'd get a truncated dump from
that we still cared about.
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 08/13] contrib/elf2dmp: Use lduw_le_p() to read PDB
2024-03-05 7:36 ` [PATCH v2 08/13] contrib/elf2dmp: Use lduw_le_p() to read PDB Akihiko Odaki
@ 2024-03-05 13:32 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:32 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:36, 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 1c5051425185..492aca4434c8 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"
> @@ -186,7 +187,7 @@ static bool 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 +
Same comment still applies as on v1 version of this patch.
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host
2024-03-05 7:36 ` [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host Akihiko Odaki
@ 2024-03-05 13:33 ` Peter Maydell
2024-03-06 5:03 ` Akihiko Odaki
0 siblings, 1 reply; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:33 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:37, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> elf2dmp assumes little endian host in many places.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> contrib/elf2dmp/meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/elf2dmp/meson.build b/contrib/elf2dmp/meson.build
> index 6707d43c4fa5..046569861f7a 100644
> --- a/contrib/elf2dmp/meson.build
> +++ b/contrib/elf2dmp/meson.build
> @@ -1,4 +1,4 @@
> -if curl.found()
> +if curl.found() and host_machine.endian() == 'little'
> executable('elf2dmp', files('main.c', 'addrspace.c', 'download.c', 'pdb.c', 'qemu_elf.c'), genh,
> dependencies: [glib, curl],
> install: true)
If it assumes a little-endian host that is a bug and we
should fix it, not just disable building the tool on
big-endian systems.
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 12/13] contrib/elf2dmp: Use GPtrArray
2024-03-05 7:36 ` [PATCH v2 12/13] contrib/elf2dmp: Use GPtrArray Akihiko Odaki
@ 2024-03-05 13:35 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:35 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:37, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This removes the need to enumarate QEMUCPUState twice and saves code.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 13/13] contrib/elf2dmp: Clamp QEMU note to file size
2024-03-05 7:36 ` [PATCH v2 13/13] contrib/elf2dmp: Clamp QEMU note to file size Akihiko Odaki
@ 2024-03-05 13:38 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-05 13:38 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Tue, 5 Mar 2024 at 07:37, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This fixes crashes with truncated dumps.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2202
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern
2024-03-05 13:28 ` Peter Maydell
@ 2024-03-06 5:00 ` Akihiko Odaki
2024-03-06 12:53 ` Peter Maydell
0 siblings, 1 reply; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-06 5:00 UTC (permalink / raw)
To: Peter Maydell; +Cc: Viktor Prutyanov, qemu-devel
On 2024/03/05 22:28, Peter Maydell wrote:
> On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> include/qapi/error.h says:
>>> We recommend
>>> * bool-valued functions return true on success / false on failure,
>>> ...
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> contrib/elf2dmp/addrspace.h | 6 +--
>> contrib/elf2dmp/download.h | 2 +-
>> contrib/elf2dmp/pdb.h | 2 +-
>> contrib/elf2dmp/qemu_elf.h | 2 +-
>> contrib/elf2dmp/addrspace.c | 12 ++---
>> contrib/elf2dmp/download.c | 10 ++--
>> contrib/elf2dmp/main.c | 114 +++++++++++++++++++++-----------------------
>> contrib/elf2dmp/pdb.c | 50 +++++++++----------
>> contrib/elf2dmp/qemu_elf.c | 32 ++++++-------
>> 9 files changed, 112 insertions(+), 118 deletions(-)
>
> This is a bit big to review easily. Converting one function
> (or a small set of closely related functions) at once would
> make for an easier to review split.
I'll split patches for each source files.
>
>
>> diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
>> index 902dc04ffa5c..ec8d33ba1e4b 100644
>> --- a/contrib/elf2dmp/download.c
>> +++ b/contrib/elf2dmp/download.c
>> @@ -9,14 +9,14 @@
>> #include <curl/curl.h>
>> #include "download.h"
>>
>> -int download_url(const char *name, const char *url)
>> +bool download_url(const char *name, const char *url)
>> {
>> - int err = 1;
>> + bool success = false;
>> FILE *file;
>> CURL *curl = curl_easy_init();
>>
>> if (!curl) {
>> - return 1;
>> + return success;
>
> Why not just "return false" ? "return success" makes it look
> like a success-path, not a failure-path.
It is a mistake. I'll fix in the next version.
>
>> }
>>
>> file = fopen(name, "wb");
>> @@ -33,11 +33,11 @@ int download_url(const char *name, const char *url)
>> unlink(name);
>> fclose(file);
>> } else {
>> - err = fclose(file);
>> + success = !fclose(file);
>> }
>>
>> out_curl:
>> curl_easy_cleanup(curl);
>>
>> - return err;
>> + return success;
>> }
>
>> @@ -186,13 +186,13 @@ static void win_context_init_from_qemu_cpu_state(WinContext64 *ctx,
>> * Finds paging-structure hierarchy base,
>> * if previously set doesn't give access to kernel structures
>> */
>> -static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
>> +static bool fix_dtb(struct va_space *vs, QEMU_Elf *qe)
>> {
>> /*
>> * Firstly, test previously set DTB.
>> */
>> if (va_space_resolve(vs, SharedUserData)) {
>> - return 0;
>> + return true;
>> }
>>
>> /*
>> @@ -206,7 +206,7 @@ static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
>> va_space_set_dtb(vs, s->cr[3]);
>> printf("DTB 0x%016"PRIx64" has been found from CPU #%zu"
>> " as system task CR3\n", vs->dtb, i);
>> - return !(va_space_resolve(vs, SharedUserData));
>> + return !!(va_space_resolve(vs, SharedUserData));
>
> If the function returns bool type, we don't need the !! idiom
> to coerce the value to bool.
va_space_resolve() returns void *.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host
2024-03-05 13:33 ` Peter Maydell
@ 2024-03-06 5:03 ` Akihiko Odaki
0 siblings, 0 replies; 26+ messages in thread
From: Akihiko Odaki @ 2024-03-06 5:03 UTC (permalink / raw)
To: Peter Maydell; +Cc: Viktor Prutyanov, qemu-devel
On 2024/03/05 22:33, Peter Maydell wrote:
> On Tue, 5 Mar 2024 at 07:37, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> elf2dmp assumes little endian host in many places.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> contrib/elf2dmp/meson.build | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/elf2dmp/meson.build b/contrib/elf2dmp/meson.build
>> index 6707d43c4fa5..046569861f7a 100644
>> --- a/contrib/elf2dmp/meson.build
>> +++ b/contrib/elf2dmp/meson.build
>> @@ -1,4 +1,4 @@
>> -if curl.found()
>> +if curl.found() and host_machine.endian() == 'little'
>> executable('elf2dmp', files('main.c', 'addrspace.c', 'download.c', 'pdb.c', 'qemu_elf.c'), genh,
>> dependencies: [glib, curl],
>> install: true)
>
> If it assumes a little-endian host that is a bug and we
> should fix it, not just disable building the tool on
> big-endian systems.
I will accept if someone submits a proper fix in the future, but I'm not
going to put effort on that. This is the best thing I can offer for now.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern
2024-03-06 5:00 ` Akihiko Odaki
@ 2024-03-06 12:53 ` Peter Maydell
0 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2024-03-06 12:53 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: Viktor Prutyanov, qemu-devel
On Wed, 6 Mar 2024 at 05:01, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/03/05 22:28, Peter Maydell wrote:
> > On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >> @@ -206,7 +206,7 @@ static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
> >> va_space_set_dtb(vs, s->cr[3]);
> >> printf("DTB 0x%016"PRIx64" has been found from CPU #%zu"
> >> " as system task CR3\n", vs->dtb, i);
> >> - return !(va_space_resolve(vs, SharedUserData));
> >> + return !!(va_space_resolve(vs, SharedUserData));
> >
> > If the function returns bool type, we don't need the !! idiom
> > to coerce the value to bool.
>
> va_space_resolve() returns void *.
Yes, and so when we return that value, because the function
return type is 'bool' it gets correctly turned into a
true/false value. You only need !! if you want to get
a 0-or-1 value in an int return type. Or does the compiler
otherwise issue a warning here?
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2024-03-06 12:54 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-05 7:36 [PATCH v2 00/13] contrib/elf2dmp: Improve robustness Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 01/13] contrib/elf2dmp: Remove unnecessary err flags Akihiko Odaki
2024-03-05 13:24 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 02/13] contrib/elf2dmp: Assume error by default Akihiko Odaki
2024-03-05 13:24 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 03/13] contrib/elf2dmp: Continue even contexts are lacking Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern Akihiko Odaki
2024-03-05 13:28 ` Peter Maydell
2024-03-06 5:00 ` Akihiko Odaki
2024-03-06 12:53 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 05/13] contrib/elf2dmp: Always check for PA resolution failure Akihiko Odaki
2024-03-05 13:29 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 06/13] contrib/elf2dmp: Always destroy PA space Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 07/13] contrib/elf2dmp: Ensure segment fits in file Akihiko Odaki
2024-03-05 13:31 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 08/13] contrib/elf2dmp: Use lduw_le_p() to read PDB Akihiko Odaki
2024-03-05 13:32 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 09/13] contrib/elf2dmp: Use rol64() to decode Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 10/13] MAINTAINERS: Add Akihiko Odaki as a elf2dmp reviewer Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 11/13] contrib/elf2dmp: Build only for little endian host Akihiko Odaki
2024-03-05 13:33 ` Peter Maydell
2024-03-06 5:03 ` Akihiko Odaki
2024-03-05 7:36 ` [PATCH v2 12/13] contrib/elf2dmp: Use GPtrArray Akihiko Odaki
2024-03-05 13:35 ` Peter Maydell
2024-03-05 7:36 ` [PATCH v2 13/13] contrib/elf2dmp: Clamp QEMU note to file size Akihiko Odaki
2024-03-05 13:38 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).