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 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?

Thanks.

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