Re: Protect mips_hi16_list from fuzzed debug info

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

On Wed, 8 Feb 2023, Alan Modra wrote:

> >  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.
> 
> I could do that, but my time is limited for mips problems.

 Completely understood (and mine BTW too).

>  I'll
> understand if you say the patch is not worth committing just to cover
> a potential fuzzed object file segfault.

 No, not at all, I agree we do need to maintain at least basic quality,
and preventing crashes from happening even for garbage input is about the 
minimum required.

> >  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...
> 
> OK, done.  I'm presuming I don't need to repost the patch.

 Sure, no need to repost.

> > > +  /* 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.  */
> 
> s/an error/a warning/

 Thanks, that changes things a bit.

> >  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.
> 
> A number of the error/warning handlers in ldmain.c use %C.  This can
> cause debug info to be parsed for the first time in order to print
> file/function/line.

 Hmm, I find it an interesting general phenomenon.  What it means the 
order sections are processed in can change depending on whether a warning 
has been issued in the course or not.  Is it not a problem in the first 
place?  Shouldn't we give priority to debugs sections and parse them first 
then before moving on to the other sections?

>  If one of those warnings is triggered after some
> hi16 relocs have been processed but before the matching lo16 reloc is
> handled, *and* the debug info is corrupted with a lo16 reloc, then the
> mips_hi16_list will be flushed with the result that printing a warning
> changes linker output.

 OK, but wouldn't these relocs be resolved in the same problematic way 
anyway when the turn came to processing debug sections?

>  It is also possible that corrupted debug info
> adds to the hi16 list, with the result that when the linker handles a
> later lo16 reloc in a text section, ld will segfault accessing
> mips_hi16.data after the debug buffers have be freed.

 This smells a HI16/LO16 pair processing bug to me by itself.  Such pairs 
must come from the same relocation section, so any HI16/LO16 relocations 
in a relocation section associated with a debug section are not supposed 
to influence any such relocations referring to the text section.  I think 
I need to look into it (though see above as to my availability).

>  Is this likely
> to happen in the real world?  No, of course not, but fuzzers keep
> finding this sort of thing, and the occasional real problem found by
> the fuzzers is enough that I haven't yet decided to ignore all fuzzing
> reports.

 Well, we must not crash, period!

 I hope there's no hurry with this change, so please let me chew it over 
yet for a couple days.

 Long-term I think the MIPS target would benefit from proper day-to-day 
maintenance (and I can't just clone myself no matter how much I might 
desire).

  Maciej