Re: Protect mips_hi16_list from fuzzed debug info

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Mon, 6 Feb 2023, Alan Modra wrote:

> This is another fix for the testcase mentioned in
> https://sourceware.org/pipermail/binutils/2023-February/125915.html
> either of which will stop the addr2line segfault.  This one also fixes
> a potential problem when linking corrupted debug info.

 Hmm, from the other message I gather DWARF info is not going to be 
processed twice anymore, so why is this change to the MIPS backend also 
required?

 Also would it be possible to have a MIPS test case for your change?  
Orchestrating HI16/LO16 relocations in a debug section should be pretty 
straightforward with the use of the `.reloc' pseudo-op.  This might help 
me understand what is really going on here.

 Also one concern about code proposed itself, see below.

> @@ -2596,38 +2600,41 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
[...]
> +  if (!tdata->freeze_mips_hi16_list)

 This conditional ought to wrap all the preceding code in the function as 
well (including the declaration block), because it's sole purpose is to 
retrieve `vallo', which is only used within the `while' loop now placed 
under the conditional...

> +    while (tdata->mips_hi16_list != NULL)
> +      {
> +	bfd_reloc_status_type ret;
> +	struct mips_hi16 *hi;
> +
> +	hi = tdata->mips_hi16_list;
> +
> +	/* R_MIPS*_GOT16 relocations are something of a special case.
> +	   We want to install the addend in the same way as for a
> +	   R_MIPS*_HI16 relocation (with a rightshift of 16).
> +	   However, since GOT16 relocations can also be used with
> +	   global symbols, their howto has a rightshift of 0.  */
> +	if (hi->rel.howto->type == R_MIPS_GOT16)
> +	  hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MIPS_HI16, false);
> +	else if (hi->rel.howto->type == R_MIPS16_GOT16)
> +	  hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MIPS16_HI16, false);
> +	else if (hi->rel.howto->type == R_MICROMIPS_GOT16)
> +	  hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MICROMIPS_HI16,
> +						   false);
> +
> +	/* VALLO is a signed 16-bit number.  Bias it by 0x8000 so that
> +	   any carry or borrow will induce a change of +1 or -1 in the
> +	   high part.  */
> +	hi->rel.addend += (vallo + 0x8000) & 0xffff;

 ... here.

> +  /* Debug info should not contain hi16 or lo16 relocs.  If it does
> +     then someone is playing fuzzing games.  Altering the hi16 list
> +     during linking when printing an error message is bad.  */

 And I really cannot extract the meaning of the second sentence here.  I 
mean I know what it literally means, but that does not really tell me 
anything.  Why would altering the list be a problem given that we're 
bailing out anyway?  I'm confused.

  Maciej