diff mbox series

[v2,1/5] elf2dmp: replace PE export name check with PDB name check

Message ID 20230915170153.10959-2-viktor@daynix.com
State New
Headers show
Series elf2dmp: improve Win2022, Win11 and large dumps | expand

Commit Message

Viktor Prutyanov Sept. 15, 2023, 5:01 p.m. UTC 
PE export name check introduced in d399d6b179 isn't reliable enough,
because a page with the export directory may be not present for some
reason. On the other hand, elf2dmp retrieves the PDB name in any case.
It can be also used to check that a PE image is the kernel image. So,
check PDB name when searching for Windows kernel image.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2165917

Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
---
 contrib/elf2dmp/main.c | 93 +++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 60 deletions(-)

Comments

Peter Maydell Sept. 26, 2023, 1:43 p.m. UTC | #1 
On Fri, 15 Sept 2023 at 18:02, Viktor Prutyanov <viktor@daynix.com> wrote:
>
> PE export name check introduced in d399d6b179 isn't reliable enough,
> because a page with the export directory may be not present for some
> reason. On the other hand, elf2dmp retrieves the PDB name in any case.
> It can be also used to check that a PE image is the kernel image. So,
> check PDB name when searching for Windows kernel image.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2165917
>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> ---
>  contrib/elf2dmp/main.c | 93 +++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 60 deletions(-)

Hi; Coverity points out a bug in this code (CID 1521598):


> -static int pe_get_pdb_symstore_hash(uint64_t base, void *start_addr,
> -        char *hash, struct va_space *vs)
> +static bool pe_check_pdb_name(uint64_t base, void *start_addr,
> +        struct va_space *vs, OMFSignatureRSDS *rsds)
>  {
>      const char sign_rsds[4] = "RSDS";

sign_rsds[] is not NUL-terminated...

>      IMAGE_DEBUG_DIRECTORY debug_dir;
> -    OMFSignatureRSDS rsds;
> -    char *pdb_name;
> -    size_t pdb_name_sz;
> -    size_t i;
> +    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)) {
>          eprintf("Failed to get Debug Directory\n");
> -        return 1;
> +        return false;
>      }
>
>      if (debug_dir.Type != IMAGE_DEBUG_TYPE_CODEVIEW) {
> -        return 1;
> +        eprintf("Debug Directory type is not CodeView\n");
> +        return false;
>      }
>
>      if (va_space_rw(vs,
>                  base + debug_dir.AddressOfRawData,
> -                &rsds, sizeof(rsds), 0)) {
> -        return 1;
> +                rsds, sizeof(*rsds), 0)) {
> +        eprintf("Failed to resolve OMFSignatureRSDS\n");
> +        return false;
>      }
>
> -    printf("CodeView signature is \'%.4s\'\n", rsds.Signature);
> -
> -    if (memcmp(&rsds.Signature, sign_rsds, sizeof(sign_rsds))) {
> -        return 1;
> +    if (memcmp(&rsds->Signature, sign_rsds, sizeof(sign_rsds))) {
> +        eprintf("CodeView signature is \'%.4s\', \'%s\' expected\n",
> +                rsds->Signature, sign_rsds);

...but in this printf() we pass sign_rsds to a "%s" format
specifier, which requires NUL termination.

If you want to print a non-NUL-terminated string you need
to do the same thing we do for rsds->Signature, which is
give it a precision which is the correct length so printf
doesn't read off the end (i.e. "%.4s").

thanks
-- PMM
diff mbox series

Patch

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 6d4d18501a..bb6744c0cd 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -411,89 +411,64 @@  static int write_dump(struct pa_space *ps,
     return fclose(dmp_file);
 }
 
-static bool pe_check_export_name(uint64_t base, void *start_addr,
-        struct va_space *vs)
-{
-    IMAGE_EXPORT_DIRECTORY export_dir;
-    const char *pe_name;
-
-    if (pe_get_data_dir_entry(base, start_addr, IMAGE_FILE_EXPORT_DIRECTORY,
-                &export_dir, sizeof(export_dir), vs)) {
-        return false;
-    }
-
-    pe_name = va_space_resolve(vs, base + export_dir.Name);
-    if (!pe_name) {
-        return false;
-    }
-
-    return !strcmp(pe_name, PE_NAME);
-}
-
-static int pe_get_pdb_symstore_hash(uint64_t base, void *start_addr,
-        char *hash, struct va_space *vs)
+static bool pe_check_pdb_name(uint64_t base, void *start_addr,
+        struct va_space *vs, OMFSignatureRSDS *rsds)
 {
     const char sign_rsds[4] = "RSDS";
     IMAGE_DEBUG_DIRECTORY debug_dir;
-    OMFSignatureRSDS rsds;
-    char *pdb_name;
-    size_t pdb_name_sz;
-    size_t i;
+    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)) {
         eprintf("Failed to get Debug Directory\n");
-        return 1;
+        return false;
     }
 
     if (debug_dir.Type != IMAGE_DEBUG_TYPE_CODEVIEW) {
-        return 1;
+        eprintf("Debug Directory type is not CodeView\n");
+        return false;
     }
 
     if (va_space_rw(vs,
                 base + debug_dir.AddressOfRawData,
-                &rsds, sizeof(rsds), 0)) {
-        return 1;
+                rsds, sizeof(*rsds), 0)) {
+        eprintf("Failed to resolve OMFSignatureRSDS\n");
+        return false;
     }
 
-    printf("CodeView signature is \'%.4s\'\n", rsds.Signature);
-
-    if (memcmp(&rsds.Signature, sign_rsds, sizeof(sign_rsds))) {
-        return 1;
+    if (memcmp(&rsds->Signature, sign_rsds, sizeof(sign_rsds))) {
+        eprintf("CodeView signature is \'%.4s\', \'%s\' expected\n",
+                rsds->Signature, sign_rsds);
+        return false;
     }
 
-    pdb_name_sz = debug_dir.SizeOfData - sizeof(rsds);
-    pdb_name = malloc(pdb_name_sz);
-    if (!pdb_name) {
-        return 1;
+    if (debug_dir.SizeOfData - sizeof(*rsds) != sizeof(PDB_NAME)) {
+        eprintf("PDB name size doesn't match\n");
+        return false;
     }
 
     if (va_space_rw(vs, base + debug_dir.AddressOfRawData +
-                offsetof(OMFSignatureRSDS, name), pdb_name, pdb_name_sz, 0)) {
-        free(pdb_name);
-        return 1;
+                offsetof(OMFSignatureRSDS, name), pdb_name, sizeof(PDB_NAME),
+                0)) {
+        eprintf("Failed to resolve PDB name\n");
+        return false;
     }
 
     printf("PDB name is \'%s\', \'%s\' expected\n", pdb_name, PDB_NAME);
 
-    if (strcmp(pdb_name, PDB_NAME)) {
-        eprintf("Unexpected PDB name, it seems the kernel isn't found\n");
-        free(pdb_name);
-        return 1;
-    }
-
-    free(pdb_name);
+    return !strcmp(pdb_name, PDB_NAME);
+}
 
-    sprintf(hash, "%.08x%.04x%.04x%.02x%.02x", rsds.guid.a, rsds.guid.b,
-            rsds.guid.c, rsds.guid.d[0], rsds.guid.d[1]);
+static void pe_get_pdb_symstore_hash(OMFSignatureRSDS *rsds, char *hash)
+{
+    sprintf(hash, "%.08x%.04x%.04x%.02x%.02x", rsds->guid.a, rsds->guid.b,
+            rsds->guid.c, rsds->guid.d[0], rsds->guid.d[1]);
     hash += 20;
-    for (i = 0; i < 6; i++, hash += 2) {
-        sprintf(hash, "%.02x", rsds.guid.e[i]);
+    for (unsigned int i = 0; i < 6; i++, hash += 2) {
+        sprintf(hash, "%.02x", rsds->guid.e[i]);
     }
 
-    sprintf(hash, "%.01x", rsds.age);
-
-    return 0;
+    sprintf(hash, "%.01x", rsds->age);
 }
 
 int main(int argc, char *argv[])
@@ -515,6 +490,7 @@  int main(int argc, char *argv[])
     KDDEBUGGER_DATA64 *kdbg;
     uint64_t KdVersionBlock;
     bool kernel_found = false;
+    OMFSignatureRSDS rsds;
 
     if (argc != 3) {
         eprintf("usage:\n\t%s elf_file dmp_file\n", argv[0]);
@@ -562,7 +538,8 @@  int main(int argc, char *argv[])
         }
 
         if (*(uint16_t *)nt_start_addr == 0x5a4d) { /* MZ */
-            if (pe_check_export_name(KernBase, nt_start_addr, &vs)) {
+            printf("Checking candidate KernBase = 0x%016"PRIx64"\n", KernBase);
+            if (pe_check_pdb_name(KernBase, nt_start_addr, &vs, &rsds)) {
                 kernel_found = true;
                 break;
             }
@@ -578,11 +555,7 @@  int main(int argc, char *argv[])
     printf("KernBase = 0x%016"PRIx64", signature is \'%.2s\'\n", KernBase,
             (char *)nt_start_addr);
 
-    if (pe_get_pdb_symstore_hash(KernBase, nt_start_addr, pdb_hash, &vs)) {
-        eprintf("Failed to get PDB symbol store hash\n");
-        err = 1;
-        goto out_ps;
-    }
+    pe_get_pdb_symstore_hash(&rsds, pdb_hash);
 
     sprintf(pdb_url, "%s%s/%s/%s", SYM_URL_BASE, PDB_NAME, pdb_hash, PDB_NAME);
     printf("PDB URL is %s\n", pdb_url);