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

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

On Tue, Jun 20, 2017 at 01:56:28AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 09:39, <andrew.cooper3@citrix.com> wrote:
> > On 20/06/2017 08:36, Jan Beulich wrote:
> >>>>> On 19.06.17 at 20:18, <konrad.wilk@oracle.com> wrote:
> >>> On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
> >>>> 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 )             
> >> Where's the uint32_t coming from here?
> > 
> > ARM32.  It's a range check that (void *)&disp is within r_offset, as it
> > (void *)&disp + sizeof(disp) -1
> 
> But not all ARM32 relocations fiddle with a 32-bit word. Granted all

Correct. However 'dest' (which is what get_addend operates on) at this point
is the combination of base->load_addr + r->r_offset.

It is more of making sure that what 'get_addend' operates on is
within the livepatch and not somewhere outside of it.

Then get_addend can get more fine grain approach to figuring out the
next part - that is based on the instruction whether the offset
there is OK or not (and boy the semantics for ARM32 ELF REL are
a complex beast).

> that livepatch code currently supports are, but baking something like
> this in makes future modifications more error prone.
> 
> Jan
> 

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