Re: Protect mips_hi16_list from fuzzed debug info

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

On Thu, 9 Feb 2023, Alan Modra wrote:

> >  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?
> 
> The normal linker processing of sections occurs as usual.  Parsing of
> the debug info is separate to this, relocations being applied to
> .debug_info by bfd_simple_get_relocated_section_contents for the
> error/warning message.  That relocated copy of .debug_info is not used
> by the linker to produce the output file .debug_info.

 Ack, makes sense to me.

> >  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).
> 
> If it is really true that hi16/lo16 pairs are always in the same
> section, then we wouldn't need freeze_mips_hi16_list.

 It is, by definition[1]:

"The AHL addend is a composite computed from the addends of two 
consecutive relocation entries.  Each relocation type of R_MIPS_HI16 must 
have an associated R_MIPS_LO16 entry immediately following it in the list 
of relocations.

"These relocation entries are always processed as a pair and both addend 
fields contribute to the AHL addend.  If AHI and ALO are the addends from 
the paired R_MIPS_HI16 and R_MIPS_LO16 entries, then the addend AHL is com 
puted as (AHI << 16) + (short)ALO.  R_MIPS_LO16 entries without an 
R_MIPS_HI16 entry immediately preceding are orphaned and the previously 
defined R_MIPS_HI16 is used for computing the addend."

Two consecutive entries must come from a single .rel.* section or they 
wouldn't be consecutive (there's no relationship between different .rel.* 
sections).

 While not explicitly stated I don't think anyone considered reusing an 
R_MIPS_HI16 reloc defined in another relocation section for an orphaned 
R_MIPS_LO16 reloc.  That would IMO make no semantic sense (for instance 
you can shuffle sections via a linker script in a relocatable link) and I 
think we ought not strive for doing so (i.e. there is not supposed to be 
any "previously defined R_MIPS_HI16" at the beginning of any given reloc 
section).

 Of course this consideration applies to the REL format only (i.e. o32); 
there's no need to track HI16/LO16 pairing with RELA objects as the addend 
is readily available.  AFAICT we don't get this right either: for 
`!partial_inplace' howtos we need not use `_bfd_mips_elf_hi16_reloc' or 
`_bfd_mips_elf_lo16_reloc' and just `_bfd_mips_elf_generic_reloc' will do.

>  Instead it
> would be much better to attach the list to mips_elf_section_data.  I
> wasn't sure enough to do that, given things like gcc's hot/cold
> section partitioning, when I moved the mip_hi16_list to be per-bfd.

 That sounds like a plan to me, but it's unrealistic for me to commit to 
it in the next two days (and then I head out to California for a week).

References:

[1] "SYSTEM V APPLICATION BINARY INTERFACE, MIPS RISC Processor
    Supplement, 3rd Edition", Section "Relocation", pp. 4-17, 4-18

  Maciej