Re: [Xen-devel] [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/06/17 15:02, Jan Beulich wrote:
>>>> On 14.06.17 at 15:44, <konrad.wilk@xxxxxxxxxx> wrote:
>> On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
>>> --- a/xen/arch/arm/arm32/livepatch.c
>>> +++ b/xen/arch/arm/arm32/livepatch.c
>>> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>>                             const struct livepatch_elf_sec *rela,
>>>                             bool use_rela)
>>>  {
>>> -    const Elf_RelA *r_a;
>>> -    const Elf_Rel *r;
>>> -    unsigned int symndx, i;
>>> -    uint32_t val;
>>> -    void *dest;
>>> +    unsigned int i;
>>>      int rc = 0;
>>>  
>>>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>>>      {
>>> +        unsigned int symndx;
>>> +        uint32_t val;
>>> +        void *dest;
>>>          unsigned char type;
>>> -        s32 addend = 0;
>>> +        s32 addend;
>>>  
>>>          if ( use_rela )
>>>          {
>>> -            r_a = rela->data + i * rela->sec->sh_entsize;
>>> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
>>> +
>>>              symndx = ELF32_R_SYM(r_a->r_info);
>>>              type = ELF32_R_TYPE(r_a->r_info);
>>>              dest = base->load_addr + r_a->r_offset; /* P */
>>> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>>          }
>>>          else
>>>          {
>>> -            r = rela->data + i * rela->sec->sh_entsize;
>>> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
>>> +
>>>              symndx = ELF32_R_SYM(r->r_info);
>>>              type = ELF32_R_TYPE(r->r_info);
>>>              dest = base->load_addr + r->r_offset; /* P */
>>> +            addend = get_addend(type, dest);
>>>          }
>>>  
>>>          if ( symndx > elf->nsym )
>>> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>>              return -EINVAL;
>>>          }
>>>  
>>> -        if ( !use_rela )
>>> -            addend = get_addend(type, dest);
>> This was added right after the symndx > elf->nsym check as
>> way to make sure we won't dereference the dest (b/c the symbol
>> may be outside the bounds).
> But symndx isn't being used here.

Indeed.  r->r_offset (and therefore dest) has no direct bearing on symndx.

Having said that, there is no sanity check that r->r_offset is within
base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
appear to do this check.

~Andrew

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