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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
> On 14/06/17 15:02, Jan Beulich wrote:
> >>>> On 14.06.17 at 15:44, <konrad.wilk@oracle.com> 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.

/me nods.
> 
> 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.

True.

And the tricky part (it was to me at least) was that ARM32 is all
REL and not RELA so the opcode gets modified after the operation.

Which means it gets a bit complex to add a boundary check in
'get_addend' .

Hm, it would seem the best way is to add a

if ( r->r_offset >= base->sec->sh_size ||                               
    (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             

that is common for use_rela and not.

Anyhow I can do that as I would want to check this
on real hardware :-)

For your patch (as is)
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Are you OK committing it in?
> 
> ~Andrew

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