Re: [PATCH for-4.9 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/06/17 10:50, Jan Beulich wrote:
>>>> On 22.06.17 at 20:15, <andrew.cooper3@citrix.com> wrote:
>> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
>> legitimate in the ELF standard, its existance in a livepatch is questionable
>> at best.  Until a plausible usecase presents itself, reject such a relocation
>> with -EOPNOTSUPP.
>>
>> Additionally, fix an off-by-one error while range checking symndx, and perform
>> a safety check on elf->sym[symndx].sym before derefencing it, to avoid
>> tripping over a NULL pointer when calculating val.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with two remarks:
>
>> --- a/xen/arch/x86/livepatch.c
>> +++ b/xen/arch/x86/livepatch.c
>> @@ -170,12 +170,24 @@ 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 )
>> +        {
>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
>> +                    elf->name);
>> +            return -EOPNOTSUPP;
>> +        }
>> +        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;
>>          }
>> +        else if ( !elf->sym[symndx].sym )
> Neither of the two "else" is really necessary, and elsewhere we've
> been telling people to avoid such.

I see two logically different scenarios.

Per the style, if I were to use fully separate if() statements, I'd need
a newline between each.  This expands the code, and separates a chain of
logically-related checks.

IMO, its better to keep logically related checks more obviously
together, while I would definitely agree that unrelated chains (which
could in principle be if/else like this) should be separated.

>
>> +        {
>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> Symbol tables can grow large, and for large numbers I generally
> find hex representation preferable of dec. Otoh the other
> (pre-existing) message uses dec too ...

I'll stay consistent with everything else.

~Andrew

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