| Message ID | CAKdteOYqxMCpOrBnH_N8JmJZzLgMwxZx0ywkh_DDPUvub9OPKA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ARM] Avoid dereferencing null pointers | expand |
On Tue, Oct 23, 2018 at 04:21:41PM +0200, Christophe Lyon wrote: > Hi, > > While building an ARM FDPIC toolchain with a compiler generating Thumb > code, I face a couple of null pointer dereferences in cmse_scan(). > > When browsing ld-uClibc.so.1, all the external symbols have no info in > sym_hashes (sym_hashes[X] == NULL), and when handling libgcc_s.so.1 > for the 2nd time in the same command, sym_hashes == NULL. > > I don't know why this doesn't happen with a compiler generating Arm > code (ie. why the symbol tables are handled differently), but the > attached small patch prevents the linker from crashing. > > OK? No, this is just papering over the real problem. You need to find out why the sym hashes are not being set up (or are being overwritten). -- Alan Modra Australia Development Lab, IBM
Hi Christophe, I'm a bit surprised cmse_scan is run at all in your case. Where you targeting an M profile core? Regarding sym_hashes[X] being null, under what conditions can a global symbol have a null hash? Best regards, Thomas On Tue, 23 Oct 2018 at 23:16, Alan Modra <amodra@gmail.com> wrote: > > On Tue, Oct 23, 2018 at 04:21:41PM +0200, Christophe Lyon wrote: > > Hi, > > > > While building an ARM FDPIC toolchain with a compiler generating Thumb > > code, I face a couple of null pointer dereferences in cmse_scan(). > > > > When browsing ld-uClibc.so.1, all the external symbols have no info in > > sym_hashes (sym_hashes[X] == NULL), and when handling libgcc_s.so.1 > > for the 2nd time in the same command, sym_hashes == NULL. > > > > I don't know why this doesn't happen with a compiler generating Arm > > code (ie. why the symbol tables are handled differently), but the > > attached small patch prevents the linker from crashing. > > > > OK? > > No, this is just papering over the real problem. You need to find out > why the sym hashes are not being set up (or are being overwritten). > > -- > Alan Modra > Australia Development Lab, IBM
On Wed, Oct 24, 2018 at 10:37:59AM +0100, Thomas Preudhomme wrote: > Hi Christophe, > > I'm a bit surprised cmse_scan is run at all in your case. Where you > targeting an M profile core? > > Regarding sym_hashes[X] being null, under what conditions can a global > symbol have a null hash? I sent a little more info to Christophe privately, after I thought a little more about the problem. Guess I should have sent it to the list. You can have sym_hashes[n] being 0 when you have an as-needed library that wasn't needed (it's loaded but then unloaded). Note this elflink.c code: if ((elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0) { unsigned int i; /* Restore the symbol table. */ old_ent = (char *) old_tab + tabsize; memset (elf_sym_hashes (abfd), 0, extsymcount * sizeof (struct elf_link_hash_entry *)); The memset zaps all the sym_hashes, because after restoring the symbol table to as it was before loading the as-needed library, the symbol pointers are no longer valid. The patch I suggest instead of the one Christophe posted is: diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c index 2c321bbcb6..5adec5e473 100644 --- a/bfd/elf32-arm.c +++ b/bfd/elf32-arm.c @@ -6449,7 +6449,8 @@ elf32_arm_size_stubs (bfd *output_bfd, asection *section; Elf_Internal_Sym *local_syms = NULL; - if (!is_arm_elf (input_bfd)) + if (!is_arm_elf (input_bfd) + || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0) continue; num_a8_relocs = 0; -- Alan Modra Australia Development Lab, IBM
On Wed, 24 Oct 2018 at 13:36, Alan Modra <amodra@gmail.com> wrote: > > On Wed, Oct 24, 2018 at 10:37:59AM +0100, Thomas Preudhomme wrote: > > Hi Christophe, > > > > I'm a bit surprised cmse_scan is run at all in your case. Where you > > targeting an M profile core? > > > > Regarding sym_hashes[X] being null, under what conditions can a global > > symbol have a null hash? > > I sent a little more info to Christophe privately, after I thought a > little more about the problem. Guess I should have sent it to the > list. > > You can have sym_hashes[n] being 0 when you have an as-needed library > that wasn't needed (it's loaded but then unloaded). > > Note this elflink.c code: > if ((elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0) > { > unsigned int i; > > /* Restore the symbol table. */ > old_ent = (char *) old_tab + tabsize; > memset (elf_sym_hashes (abfd), 0, > extsymcount * sizeof (struct elf_link_hash_entry *)); > > The memset zaps all the sym_hashes, because after restoring the symbol > table to as it was before loading the as-needed library, the symbol > pointers are no longer valid. > > The patch I suggest instead of the one Christophe posted is: > > diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c > index 2c321bbcb6..5adec5e473 100644 > --- a/bfd/elf32-arm.c > +++ b/bfd/elf32-arm.c > @@ -6449,7 +6449,8 @@ elf32_arm_size_stubs (bfd *output_bfd, > asection *section; > Elf_Internal_Sym *local_syms = NULL; > > - if (!is_arm_elf (input_bfd)) > + if (!is_arm_elf (input_bfd) > + || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0) > continue; > > num_a8_relocs = 0; > Thank you very much Alan for your time, explanations, and patch: I can confirm it works for me. Can you commit it? > -- > Alan Modra > Australia Development Lab, IBM
On Wed, 24 Oct 2018 at 13:36, Alan Modra <amodra@gmail.com> wrote: > > On Wed, Oct 24, 2018 at 10:37:59AM +0100, Thomas Preudhomme wrote: > > Hi Christophe, > > > > I'm a bit surprised cmse_scan is run at all in your case. Where you > > targeting an M profile core? > > > > Regarding sym_hashes[X] being null, under what conditions can a global > > symbol have a null hash? > > I sent a little more info to Christophe privately, after I thought a > little more about the problem. Guess I should have sent it to the > list. > > You can have sym_hashes[n] being 0 when you have an as-needed library > that wasn't needed (it's loaded but then unloaded). > > Note this elflink.c code: > if ((elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0) > { > unsigned int i; > > /* Restore the symbol table. */ > old_ent = (char *) old_tab + tabsize; > memset (elf_sym_hashes (abfd), 0, > extsymcount * sizeof (struct elf_link_hash_entry *)); > > The memset zaps all the sym_hashes, because after restoring the symbol > table to as it was before loading the as-needed library, the symbol > pointers are no longer valid. > > The patch I suggest instead of the one Christophe posted is: > Hi Alan, Despite your fix below, I am again facing the same crash, in a case which might be similar to the one you fixed. My link command has: -lgcc_s -lgcc -lc -lgcc_s and cmse_scan crashes again because sym_hashes is null when scanning the second occurrence of -lgcc_s. If I remove -lgcc_s, the link succeeds, which suggests that even though I'm not uses --as-needed in this case, the behaviour is similar: the second -lgcc_s is useless (does not help resolve any reference), so its sym_hashes is null. Does that sound right? What's the proper way of skipping it, since DYN_AS_NEEDED is not set? Thanks, Christophe > diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c > index 2c321bbcb6..5adec5e473 100644 > --- a/bfd/elf32-arm.c > +++ b/bfd/elf32-arm.c > @@ -6449,7 +6449,8 @@ elf32_arm_size_stubs (bfd *output_bfd, > asection *section; > Elf_Internal_Sym *local_syms = NULL; > > - if (!is_arm_elf (input_bfd)) > + if (!is_arm_elf (input_bfd) > + || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0) > continue; > > num_a8_relocs = 0; > > -- > Alan Modra > Australia Development Lab, IBM
On Wed, Nov 20, 2019 at 04:11:51PM +0100, Christophe Lyon wrote: > Despite your fix below, I am again facing the same crash, in a case > which might be similar to the one you fixed. > My link command has: > -lgcc_s -lgcc -lc -lgcc_s > and cmse_scan crashes again because sym_hashes is null when scanning > the second occurrence of -lgcc_s. > If I remove -lgcc_s, the link succeeds, which suggests that even > though I'm not uses --as-needed in this case, the behaviour is > similar: the second -lgcc_s is useless (does not help resolve any > reference), so its sym_hashes is null. > > Does that sound right? What's the proper way of skipping it, since > DYN_AS_NEEDED is not set? I guess you're hitting this code in elf_link_add_object_symbols: ret = elf_add_dt_needed_tag (abfd, info, soname, add_needed); if (ret < 0) goto error_return; /* If we have already included this dynamic object in the link, just ignore it. There is no reason to include a particular dynamic object more than once. */ if (ret > 0) return TRUE; and returning because the lib has indeed already been loaded. That's before sym_hashes are allocated, so sym_hashes will be NULL. It's a wonder I didn't think of this case last year, even though you were reporting sym_hashes[i] being NULL rather than sym_hashes NULL. Using this should work: if (!is_arm_elf (input_bfd) || elf_sym_hashes (input_bfd) == 0 || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0) continue; You may also want to cover the case of sym_hashes[i] being zero in cmse_scan, which is possible when badly formed shared libraries hit the following elf_link_add_object_symbols code /* If we aren't prepared to handle locals within the globals then we'll likely segfault on a NULL symbol hash if the symbol is ever referenced in relocations. */ shindex = elf_elfheader (abfd)->e_shstrndx; name = bfd_elf_string_from_elf_section (abfd, shindex, hdr->sh_name); _bfd_error_handler (_("%pB: %s local symbol at index %lu" " (>= sh_info of %lu)"), abfd, name, (long) (isym - isymbuf + extsymoff), (long) extsymoff); /* Dynamic object relocations are not processed by ld, so ld won't run into the problem mentioned above. */ if (dynamic) continue; -- Alan Modra Australia Development Lab, IBM
On Thu, Nov 21, 2019 at 08:19:03AM +1030, Alan Modra wrote: > Using this should work: > > if (!is_arm_elf (input_bfd) > || elf_sym_hashes (input_bfd) == 0 > || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0) > continue; Actually, that might not be such a good idea. An object without any global symbols will have sym_hashes NULL too. -- Alan Modra Australia Development Lab, IBM
On Thu, Nov 21, 2019 at 08:31:57AM +1030, Alan Modra wrote: > On Thu, Nov 21, 2019 at 08:19:03AM +1030, Alan Modra wrote: > > Using this should work: > > > > if (!is_arm_elf (input_bfd) > > || elf_sym_hashes (input_bfd) == 0 > > || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0) > > continue; > > Actually, that might not be such a good idea. An object without any > global symbols will have sym_hashes NULL too. So, getting back to this after a small interruption, if (!is_arm_elf (input_bfd)) continue; if ((input_bfd->flags & DYNAMIC) != 0 && (elf_sym_hashes (input_bfd) == NULL || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0)) continue; -- Alan Modra Australia Development Lab, IBM
On Wed, 20 Nov 2019 at 23:25, Alan Modra <amodra@gmail.com> wrote: > > On Thu, Nov 21, 2019 at 08:31:57AM +1030, Alan Modra wrote: > > On Thu, Nov 21, 2019 at 08:19:03AM +1030, Alan Modra wrote: > > > Using this should work: > > > > > > if (!is_arm_elf (input_bfd) > > > || elf_sym_hashes (input_bfd) == 0 > > > || (elf_dyn_lib_class (inputif ((input_bfd->flags & DYNAMIC) != 0 && (elf_sym_hashes (input_bfd) == NULL || (e_bfd) & DYN_AS_NEEDED) != 0) > > > continue; > > > > Actually, that might not be such a good idea. An object without any > > global symbols will have sym_hashes NULL too. > > So, getting back to this after a small interruption, Thanks, this works for me. I don't know what I didn't see this problem until now :-( > > if (!is_arm_elf (input_bfd)) > continue; > if ((input_bfd->flags & DYNAMIC) != 0 > && (elf_sym_hashes (input_bfd) == NULL I'm not sure I understand your comment about an object without any global symbols: whatever the reason, if sym_hashes is NULL, cmse_scan will crash, won't it? Also, I see cmse_scan has an early exit when local_syms is NULL, wouldn't it be cleaner to check if sym_hashes is NULL is cmse_scan too? > || (elf_dyn_lib_class (input_bfd) & DYN_AS_NEEDED) != 0)) > continue; > > -- > Alan Modra > Australia Development Lab, IBM
On Thu, Nov 21, 2019 at 08:44:26AM +0100, Christophe Lyon wrote: > I'm not sure I understand your comment about an object without any > global symbols: whatever the reason, if sym_hashes is NULL, cmse_scan > will crash, won't it? No, because cmse_scan only uses sym_hashes for global syms. If there are no globals it should be fine with sym_hashes NULL. > Also, I see cmse_scan has an early exit when local_syms is NULL, That's an error exit on not being able to read local syms. > wouldn't it be cleaner to check if sym_hashes is NULL is cmse_scan > too? Nope, because it looks to me like cmse_scan needs to work on a relocatable object file with only local syms. (At least, it should report errors about any __acle_se_* local syms.) As I said in another email, you could add a check that cmse_hash is non-NULL before dereferencing. -- Alan Modra Australia Development Lab, IBM
On Thu, 21 Nov 2019 at 09:16, Alan Modra <amodra@gmail.com> wrote: > > On Thu, Nov 21, 2019 at 08:44:26AM +0100, Christophe Lyon wrote: > > I'm not sure I understand your comment about an object without any > > global symbols: whatever the reason, if sym_hashes is NULL, cmse_scan > > will crash, won't it? > > No, because cmse_scan only uses sym_hashes for global syms. If there > are no globals it should be fine with sym_hashes NULL. > I see, thanks for the explanation. Can you push your fix? Thanks, Christophe
diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c index 9c61181..6ea348b 100644 --- a/bfd/elf32-arm.c +++ b/bfd/elf32-arm.c @@ -5939,7 +5939,16 @@ cmse_scan (bfd *input_bfd, struct elf32_arm_link_hash_table *htab, } else { + /* No hash table, stop iterating. */ + if (sym_hashes == NULL) + break; + cmse_hash = elf32_arm_hash_entry (sym_hashes[i - ext_start]); + + /* Avoid dereferencing if info is not present. */ + if (cmse_hash == NULL) + continue; + sym_name = (char *) cmse_hash->root.root.root.string; /* Not a special symbol. */