[ARM] Avoid dereferencing null pointers

Message ID CAKdteOYqxMCpOrBnH_N8JmJZzLgMwxZx0ywkh_DDPUvub9OPKA@mail.gmail.com
State New
Headers show
Series
  • [ARM] Avoid dereferencing null pointers
Related show

Commit Message

Christophe Lyon Oct. 23, 2018, 2:21 p.m.
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?

Thanks,

Christophe

2018-10-23  Christophe Lyon  <christophe.lyon@linaro.org>

	* elf32-arm.c (cmse_scan): Avoid dereferencing NULL pointers.

Comments

Alan Modra Oct. 23, 2018, 10:16 p.m. | #1
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
Thomas Preudhomme Oct. 24, 2018, 9:37 a.m. | #2
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
Alan Modra Oct. 24, 2018, 11:36 a.m. | #3
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
Christophe Lyon Oct. 24, 2018, 12:49 p.m. | #4
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
Christophe Lyon Nov. 20, 2019, 3:11 p.m. | #5
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
Alan Modra Nov. 20, 2019, 9:49 p.m. | #6
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
Alan Modra Nov. 20, 2019, 10:01 p.m. | #7
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
Alan Modra Nov. 20, 2019, 10:24 p.m. | #8
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
Christophe Lyon Nov. 21, 2019, 7:44 a.m. | #9
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
Alan Modra Nov. 21, 2019, 8:16 a.m. | #10
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
Christophe Lyon Nov. 21, 2019, 10:12 a.m. | #11
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

Patch

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.  */