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.coop...@citrix.com> wrote: >>> On 14/06/17 11:11, Jan Beulich wrote: >>>>>>> On 13.06.17 at 22:51, <andrew.coop...@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). ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel