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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx> wrote:
>>> On 14/06/17 11:11, Jan Beulich wrote:
>>>>>>> On 13.06.17 at 22:51, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel