diff mbox series

[v2] linux-user: Fix GDB complaining about system-supplied DSO string table index

Message ID 20241023202850.55211-1-iii@linux.ibm.com (mailing list archive)
State New
Headers show
Series [v2] linux-user: Fix GDB complaining about system-supplied DSO string table index | expand

Commit Message

Ilya Leoshkevich Oct. 23, 2024, 8:27 p.m. UTC
When debugging qemu-user processes using gdbstub, the following warning
appears every time:

    warning: BFD: warning: system-supplied DSO at 0x7f8253cc3000 has a corrupt string table index

The reason is that QEMU does not map the VDSO's section headers. The
VDSO's ELF header's e_shoff points to zeros, which GDB fails to parse.

The difference with the kernel's VDSO is that the latter is mapped as a
blob, ignoring program headers - which also don't cover the section
table. QEMU, on the other hand, loads it as an ELF file.

There appears to be no way to place section headers inside a section,
and, therefore, no way to refer to them from a linker script. Also, ld
hardcodes section headers to be non-loadable, see
_bfd_elf_assign_file_positions_for_non_load(). In theory ld could be
enhanced by implementing an "SHDRS" keyword in addition to the existing
"FILEHDR" and "PHDRS".

There are multiple ways to resolve the issue:

- Copy VDSO as a blob in load_elf_vdso(). This would require creating
  specialized loader logic, that duplicates parts of load_elf_image().

- Fix up VDSO's PHDR size in load_elf_vdso(). This would require either
  duplicating the parsing logic, or adding an ugly parameter to
  load_elf_image().

- Fix up VDSO's PHDR size in gen-vdso. This is the simplest solution,
  so do it.

There are two tricky parts:

- Byte-swaps need to be done either on local copies, or in-place and
  then reverted in the end. To preserve the existing code structure, do
  the former for Sym and Dyn, and the latter for Ehdr, Phdr, and Shdr.

- There must be no .bss, which is already the case - but having an
  explicit check is helpful to ensure correctness.

To verify this change, I diffed the on-disk and the loaded VDSOs; the
result does not show anything unusual, except for what seems to be an
existing oversight (which should probably be fixed separately):

│  Symbol table '.dynsym' contains 8 entries:
│     Num:    Value          Size Type    Bind   Vis      Ndx Name
│ -     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
│ -     6: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.29
│ +     0: 00007f61075bf000     0 NOTYPE  LOCAL  DEFAULT  UND
│ +     6: 00007f61075bf000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.29

Fixes: 2fa536d10797 ("linux-user: Add gen-vdso tool")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---

v1: https://lore.kernel.org/qemu-devel/20241023144744.50440-1-iii@linux.ibm.com/
v1 -> v2: Check that there is no bss, mention linker scripts in the
          commit message (Richard).

 linux-user/gen-vdso-elfn.c.inc | 98 +++++++++++++++++++++++-----------
 linux-user/gen-vdso.c          | 40 +++++++-------
 2 files changed, 87 insertions(+), 51 deletions(-)

Comments

Richard Henderson Oct. 25, 2024, 11:41 a.m. UTC | #1
On 10/23/24 21:27, Ilya Leoshkevich wrote:
> When debugging qemu-user processes using gdbstub, the following warning
> appears every time:
> 
>      warning: BFD: warning: system-supplied DSO at 0x7f8253cc3000 has a corrupt string table index
> 
> The reason is that QEMU does not map the VDSO's section headers. The
> VDSO's ELF header's e_shoff points to zeros, which GDB fails to parse.
> 
> The difference with the kernel's VDSO is that the latter is mapped as a
> blob, ignoring program headers - which also don't cover the section
> table. QEMU, on the other hand, loads it as an ELF file.
> 
> There appears to be no way to place section headers inside a section,
> and, therefore, no way to refer to them from a linker script. Also, ld
> hardcodes section headers to be non-loadable, see
> _bfd_elf_assign_file_positions_for_non_load(). In theory ld could be
> enhanced by implementing an "SHDRS" keyword in addition to the existing
> "FILEHDR" and "PHDRS".
> 
> There are multiple ways to resolve the issue:
> 
> - Copy VDSO as a blob in load_elf_vdso(). This would require creating
>    specialized loader logic, that duplicates parts of load_elf_image().
> 
> - Fix up VDSO's PHDR size in load_elf_vdso(). This would require either
>    duplicating the parsing logic, or adding an ugly parameter to
>    load_elf_image().
> 
> - Fix up VDSO's PHDR size in gen-vdso. This is the simplest solution,
>    so do it.
> 
> There are two tricky parts:
> 
> - Byte-swaps need to be done either on local copies, or in-place and
>    then reverted in the end. To preserve the existing code structure, do
>    the former for Sym and Dyn, and the latter for Ehdr, Phdr, and Shdr.
> 
> - There must be no .bss, which is already the case - but having an
>    explicit check is helpful to ensure correctness.
> 
> To verify this change, I diffed the on-disk and the loaded VDSOs; the
> result does not show anything unusual, except for what seems to be an
> existing oversight (which should probably be fixed separately):
> 
> │  Symbol table '.dynsym' contains 8 entries:
> │     Num:    Value          Size Type    Bind   Vis      Ndx Name
> │ -     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
> │ -     6: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.29
> │ +     0: 00007f61075bf000     0 NOTYPE  LOCAL  DEFAULT  UND
> │ +     6: 00007f61075bf000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.29
> 
> Fixes: 2fa536d10797 ("linux-user: Add gen-vdso tool")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> 
> v1: https://lore.kernel.org/qemu-devel/20241023144744.50440-1-iii@linux.ibm.com/
> v1 -> v2: Check that there is no bss, mention linker scripts in the
>            commit message (Richard).

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Richard Henderson Nov. 4, 2024, 12:24 p.m. UTC | #2
On 10/25/24 12:41, Richard Henderson wrote:
> On 10/23/24 21:27, Ilya Leoshkevich wrote:
>> When debugging qemu-user processes using gdbstub, the following warning
>> appears every time:
>>
>>      warning: BFD: warning: system-supplied DSO at 0x7f8253cc3000 has a corrupt string 
>> table index
>>
>> The reason is that QEMU does not map the VDSO's section headers. The
>> VDSO's ELF header's e_shoff points to zeros, which GDB fails to parse.
>>
>> The difference with the kernel's VDSO is that the latter is mapped as a
>> blob, ignoring program headers - which also don't cover the section
>> table. QEMU, on the other hand, loads it as an ELF file.
>>
>> There appears to be no way to place section headers inside a section,
>> and, therefore, no way to refer to them from a linker script. Also, ld
>> hardcodes section headers to be non-loadable, see
>> _bfd_elf_assign_file_positions_for_non_load(). In theory ld could be
>> enhanced by implementing an "SHDRS" keyword in addition to the existing
>> "FILEHDR" and "PHDRS".
>>
>> There are multiple ways to resolve the issue:
>>
>> - Copy VDSO as a blob in load_elf_vdso(). This would require creating
>>    specialized loader logic, that duplicates parts of load_elf_image().
>>
>> - Fix up VDSO's PHDR size in load_elf_vdso(). This would require either
>>    duplicating the parsing logic, or adding an ugly parameter to
>>    load_elf_image().
>>
>> - Fix up VDSO's PHDR size in gen-vdso. This is the simplest solution,
>>    so do it.
>>
>> There are two tricky parts:
>>
>> - Byte-swaps need to be done either on local copies, or in-place and
>>    then reverted in the end. To preserve the existing code structure, do
>>    the former for Sym and Dyn, and the latter for Ehdr, Phdr, and Shdr.
>>
>> - There must be no .bss, which is already the case - but having an
>>    explicit check is helpful to ensure correctness.
>>
>> To verify this change, I diffed the on-disk and the loaded VDSOs; the
>> result does not show anything unusual, except for what seems to be an
>> existing oversight (which should probably be fixed separately):
>>
>> │  Symbol table '.dynsym' contains 8 entries:
>> │     Num:    Value          Size Type    Bind   Vis      Ndx Name
>> │ -     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>> │ -     6: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.29
>> │ +     0: 00007f61075bf000     0 NOTYPE  LOCAL  DEFAULT  UND
>> │ +     6: 00007f61075bf000     0 OBJECT  GLOBAL DEFAULT  ABS LINUX_2.6.29
>>
>> Fixes: 2fa536d10797 ("linux-user: Add gen-vdso tool")
>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> ---
>>
>> v1: https://lore.kernel.org/qemu-devel/20241023144744.50440-1-iii@linux.ibm.com/
>> v1 -> v2: Check that there is no bss, mention linker scripts in the
>>            commit message (Richard).
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> r~

Queued, thanks.

r~
diff mbox series

Patch

diff --git a/linux-user/gen-vdso-elfn.c.inc b/linux-user/gen-vdso-elfn.c.inc
index 95856eb839b..b47019e136c 100644
--- a/linux-user/gen-vdso-elfn.c.inc
+++ b/linux-user/gen-vdso-elfn.c.inc
@@ -68,28 +68,45 @@  static void elfN(search_symtab)(ElfN(Shdr) *shdr, unsigned sym_idx,
                                 void *buf, bool need_bswap)
 {
     unsigned str_idx = shdr[sym_idx].sh_link;
-    ElfN(Sym) *sym = buf + shdr[sym_idx].sh_offset;
-    unsigned sym_n = shdr[sym_idx].sh_size / sizeof(*sym);
+    ElfN(Sym) *target_sym = buf + shdr[sym_idx].sh_offset;
+    unsigned sym_n = shdr[sym_idx].sh_size / sizeof(*target_sym);
     const char *str = buf + shdr[str_idx].sh_offset;
 
     for (unsigned i = 0; i < sym_n; ++i) {
         const char *name;
+        ElfN(Sym) sym;
 
+        memcpy(&sym, &target_sym[i], sizeof(sym));
         if (need_bswap) {
-            elfN(bswap_sym)(sym + i);
+            elfN(bswap_sym)(&sym);
         }
-        name = str + sym[i].st_name;
+        name = str + sym.st_name;
 
         if (sigreturn_sym && strcmp(sigreturn_sym, name) == 0) {
-            sigreturn_addr = sym[i].st_value;
+            sigreturn_addr = sym.st_value;
         }
         if (rt_sigreturn_sym && strcmp(rt_sigreturn_sym, name) == 0) {
-            rt_sigreturn_addr = sym[i].st_value;
+            rt_sigreturn_addr = sym.st_value;
         }
     }
 }
 
-static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
+static void elfN(bswap_ps_hdrs)(ElfN(Ehdr) *ehdr)
+{
+    ElfN(Phdr) *phdr = (void *)ehdr + ehdr->e_phoff;
+    ElfN(Shdr) *shdr = (void *)ehdr + ehdr->e_shoff;
+    ElfN(Half) i;
+
+    for (i = 0; i < ehdr->e_phnum; ++i) {
+        elfN(bswap_phdr)(&phdr[i]);
+    }
+
+    for (i = 0; i < ehdr->e_shnum; ++i) {
+        elfN(bswap_shdr)(&shdr[i]);
+    }
+}
+
+static void elfN(process)(FILE *outf, void *buf, long len, bool need_bswap)
 {
     ElfN(Ehdr) *ehdr = buf;
     ElfN(Phdr) *phdr;
@@ -103,24 +120,14 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
     int errors = 0;
 
     if (need_bswap) {
-        elfN(bswap_ehdr)(ehdr);
+        elfN(bswap_ehdr)(buf);
+        elfN(bswap_ps_hdrs)(buf);
     }
 
     phnum = ehdr->e_phnum;
     phdr = buf + ehdr->e_phoff;
-    if (need_bswap) {
-        for (unsigned i = 0; i < phnum; ++i) {
-            elfN(bswap_phdr)(phdr + i);
-        }
-    }
-
     shnum = ehdr->e_shnum;
     shdr = buf + ehdr->e_shoff;
-    if (need_bswap) {
-        for (unsigned i = 0; i < shnum; ++i) {
-            elfN(bswap_shdr)(shdr + i);
-        }
-    }
     for (unsigned i = 0; i < shnum; ++i) {
         switch (shdr[i].sh_type) {
         case SHT_SYMTAB:
@@ -154,7 +161,24 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
             fprintf(stderr, "LOAD segment not loaded at address 0\n");
             errors++;
         }
-        first_segsz = phdr[i].p_filesz;
+        /*
+         * Extend the program header to cover the entire VDSO, so that
+         * load_elf_vdso() loads everything, including section headers.
+         *
+         * Require that there is no .bss, since it would break this
+         * approach.
+         */
+        if (phdr[i].p_filesz != phdr[i].p_memsz) {
+            fprintf(stderr, "LOAD segment's filesz and memsz differ\n");
+            errors++;
+        }
+        if (phdr[i].p_filesz > len) {
+            fprintf(stderr, "LOAD segment is larger than the whole VDSO\n");
+            errors++;
+        }
+        phdr[i].p_filesz = len;
+        phdr[i].p_memsz = len;
+        first_segsz = len;
         if (first_segsz < ehdr->e_phoff + phnum * sizeof(*phdr)) {
             fprintf(stderr, "LOAD segment does not cover PHDRs\n");
             errors++;
@@ -197,17 +221,24 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
         output_reloc(outf, buf, &phdr[i].p_paddr);
     }
 
+    /* Relocate the section headers. */
+    for (unsigned i = 0; i < shnum; ++i) {
+        output_reloc(outf, buf, &shdr[i].sh_addr);
+    }
+
     /* Relocate the DYNAMIC entries. */
     if (dynamic_addr) {
-        ElfN(Dyn) *dyn = buf + dynamic_ofs;
-        __typeof(dyn->d_tag) tag;
+        ElfN(Dyn) *target_dyn = buf + dynamic_ofs;
+        __typeof(((ElfN(Dyn) *)target_dyn)->d_tag) tag;
 
         do {
+            ElfN(Dyn) dyn;
 
+            memcpy(&dyn, target_dyn, sizeof(dyn));
             if (need_bswap) {
-                elfN(bswap_dyn)(dyn);
+                elfN(bswap_dyn)(&dyn);
             }
-            tag = dyn->d_tag;
+            tag = dyn.d_tag;
 
             switch (tag) {
             case DT_HASH:
@@ -218,7 +249,7 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
             case DT_PLTGOT:
             case DT_ADDRRNGLO ... DT_ADDRRNGHI:
                 /* These entries store an address in the entry. */
-                output_reloc(outf, buf, &dyn->d_un.d_val);
+                output_reloc(outf, buf, &target_dyn->d_un.d_val);
                 break;
 
             case DT_NULL:
@@ -235,7 +266,7 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
                 break;
 
             case DT_SYMENT:
-                if (dyn->d_un.d_val != sizeof(ElfN(Sym))) {
+                if (dyn.d_un.d_val != sizeof(ElfN(Sym))) {
                     fprintf(stderr, "VDSO has incorrect dynamic symbol size\n");
                     errors++;
                 }
@@ -251,7 +282,7 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
                  * ??? The RISC-V toolchain will emit these even when there
                  * are no relocations.  Validate zeros.
                  */
-                if (dyn->d_un.d_val != 0) {
+                if (dyn.d_un.d_val != 0) {
                     fprintf(stderr, "VDSO has dynamic relocations\n");
                     errors++;
                 }
@@ -287,7 +318,7 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
                 errors++;
                 break;
             }
-            dyn++;
+            target_dyn++;
         } while (tag != DT_NULL);
         if (errors) {
             exit(EXIT_FAILURE);
@@ -296,11 +327,11 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
 
     /* Relocate the dynamic symbol table. */
     if (dynsym_idx) {
-        ElfN(Sym) *sym = buf + shdr[dynsym_idx].sh_offset;
-        unsigned sym_n = shdr[dynsym_idx].sh_size / sizeof(*sym);
+        ElfN(Sym) *target_sym = buf + shdr[dynsym_idx].sh_offset;
+        unsigned sym_n = shdr[dynsym_idx].sh_size / sizeof(*target_sym);
 
         for (unsigned i = 0; i < sym_n; ++i) {
-            output_reloc(outf, buf, &sym[i].st_value);
+            output_reloc(outf, buf, &target_sym[i].st_value);
         }
     }
 
@@ -311,4 +342,9 @@  static void elfN(process)(FILE *outf, void *buf, bool need_bswap)
     if (symtab_idx) {
         elfN(search_symtab)(shdr, symtab_idx, buf, need_bswap);
     }
+
+    if (need_bswap) {
+        elfN(bswap_ps_hdrs)(buf);
+        elfN(bswap_ehdr)(buf);
+    }
 }
diff --git a/linux-user/gen-vdso.c b/linux-user/gen-vdso.c
index 31e333be802..721f38d5a3b 100644
--- a/linux-user/gen-vdso.c
+++ b/linux-user/gen-vdso.c
@@ -131,23 +131,6 @@  int main(int argc, char **argv)
     }
     fclose(inf);
 
-    /*
-     * Write out the vdso image now, before we make local changes.
-     */
-
-    fprintf(outf,
-            "/* Automatically generated from linux-user/gen-vdso.c. */\n"
-            "\n"
-            "static const uint8_t %s_image[] = {",
-            prefix);
-    for (long i = 0; i < total_len; ++i) {
-        if (i % 12 == 0) {
-            fputs("\n   ", outf);
-        }
-        fprintf(outf, " 0x%02x,", buf[i]);
-    }
-    fprintf(outf, "\n};\n\n");
-
     /*
      * Identify which elf flavor we're processing.
      * The first 16 bytes of the file are e_ident.
@@ -179,14 +162,17 @@  int main(int argc, char **argv)
      * Output relocation addresses as we go.
      */
 
-    fprintf(outf, "static const unsigned %s_relocs[] = {\n", prefix);
+    fprintf(outf,
+            "/* Automatically generated by linux-user/gen-vdso.c. */\n"
+            "\n"
+            "static const unsigned %s_relocs[] = {\n", prefix);
 
     switch (buf[EI_CLASS]) {
     case ELFCLASS32:
-        elf32_process(outf, buf, need_bswap);
+        elf32_process(outf, buf, total_len, need_bswap);
         break;
     case ELFCLASS64:
-        elf64_process(outf, buf, need_bswap);
+        elf64_process(outf, buf, total_len, need_bswap);
         break;
     default:
         fprintf(stderr, "%s: invalid elf EI_CLASS (%u)\n",
@@ -196,6 +182,20 @@  int main(int argc, char **argv)
 
     fprintf(outf, "};\n\n");   /* end vdso_relocs. */
 
+    /*
+     * Write out the vdso image now, after we made local changes.
+     */
+    fprintf(outf,
+            "static const uint8_t %s_image[] = {",
+            prefix);
+    for (long i = 0; i < total_len; ++i) {
+        if (i % 12 == 0) {
+            fputs("\n   ", outf);
+        }
+        fprintf(outf, " 0x%02x,", buf[i]);
+    }
+    fprintf(outf, "\n};\n\n");
+
     fprintf(outf, "static const VdsoImageInfo %s_image_info = {\n", prefix);
     fprintf(outf, "    .image = %s_image,\n", prefix);
     fprintf(outf, "    .relocs = %s_relocs,\n", prefix);