Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jun 14, 2017 at 07:33:57PM +0100, Andrew Cooper wrote:
> On 14/06/17 15:18, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jun 14, 2017 at 04:24:00AM -0600, Jan Beulich wrote:
> >>>>> On 14.06.17 at 12:13, <andrew.cooper3@citrix.com> wrote:
> >>> On 14/06/17 11:11, Jan Beulich wrote:
> >>>>>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
> >>>>> --- a/xen/arch/x86/livepatch.c
> >>>>> +++ b/xen/arch/x86/livepatch.c
> >>>>> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> >>> *elf,
> >>>>>          uint8_t *dest = base->load_addr + r->r_offset;
> >>>>>          uint64_t val;
> >>>>>  
> >>>>> -        if ( symndx > elf->nsym )
> >>>>> +        if ( symndx == STN_UNDEF )
> >>>>> +            val = 0;
> >>>>> +        else if ( symndx > elf->nsym )
> >>>>>          {
> >>>>>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> >>> symbol@%u which is past end!\n",
> >>>>>                      elf->name, symndx);
> >>>>>              return -EINVAL;
> >>>>>          }
> >>>>> -
> >>>>> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
> >>>>> +        else if ( !elf->sym[symndx].sym )
> >>>>> +        {
> >>>>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> >>>>> +                    elf->name, symndx);
> >>>>> +            return -EINVAL;
> >>>>> +        }
> >>>>> +        else
> >>>>> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
> >>>> I don't understand this: st_value for STN_UNDEF is going to be zero
> >>>> (so far there's also no extension defined for the first entry, afaict),
> >>>> so there should be no difference between hard-coding the zero and
> >>>> reading the symbol table entry. Furthermore r_addend would still
> >>>> need applying. And finally "val" is never being cast to a pointer, and
> >>>> hence I miss the connection to whatever crash you've been
> >>>> observing.
> >>> elf->sym[0].sym is the NULL pointer.
> >>>
> >>> ->st_value dereferences it.
> >> Ah, but that is then what you want to change (unless we decide
> >> to outright refuse STN_UNDEF, which still depends on why it's
> >> there in the first place).
> > That the !elf->sym[0].sym is very valid case.
> > And in that context the 'val=r->r_addend' makes sense.
> >
> > And from an EFI spec, the relocations can point to the SHN_UNDEF area (why
> > would it I have no clue) - but naturally we can't mess with that.
> >
> > But I am curious as Jan about this - and whether this is something that
> > could be constructed with a test-case?
> 
> Well - I've got a livepatch with such a relocation.  It is probably a
> livepatch build tools issue, but the question is whether Xen should ever
> accept such a livepatch or not (irrespective of whether this exact
> relocation is permitted within the ELF spec).

CC-ing Jamie

I would say no, as I can't find a good use-case for a relocation 
to point to the SHN_UNDEF symbol [0]. It feels to me as if somebody
would be mucking with a NULL pointer.

But perhaps if the addendum had a value it would make sense?

As in NULL + <offset>?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel