Re: [RFC PATCH v1] arch/x86: Livepatch: fix overflow check when computing ELF relocations

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: Tuesday, March 8, 2022 4:26 PM
> To: Bjoern Doebel <doebel@xxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Michael Kurth <mku@xxxxxxxxx>; Martin Pohlack <mpohlack@xxxxxxxxx>; 
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Ross Lagerwall 
> <ross.lagerwall@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx 
> <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [RFC PATCH v1] arch/x86: Livepatch: fix overflow check when 
> computing ELF relocations 
>  
> On Tue, Mar 08, 2022 at 05:15:33PM +0100, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 04:45:34PM +0100, Jan Beulich wrote:
> > > On 08.03.2022 16:36, Bjoern Doebel wrote:
> > > > --- a/xen/arch/x86/livepatch.c
> > > > +++ b/xen/arch/x86/livepatch.c
> > > > @@ -339,7 +339,7 @@ int arch_livepatch_perform_rela(struct 
> > > > livepatch_elf *elf,
> > > >  
> > > >              val -= (uint64_t)dest;
> > > >              *(int32_t *)dest = val;
> > > 
> > > Afaict after this assignment ...
> > > 
> > > > -            if ( (int64_t)val != *(int32_t *)dest )
> > > > +            if ( (int32_t)val != *(int32_t *)dest )
> > > 
> > > ... this condition can never be false. The cast really wants to be
> > > to int64_t, and the overflow you saw being reported is quite likely
> > > for a different reason. But from the sole message you did quote
> > > it's not really possible to figure what else is wrong.
> > 
> > It seems Linux has that check ifdef'ed [0], but there's no reference
> > as to why it's that way (I've tracked it back to the x86-64 import on
> > the historical tree, f3081f5b66a06175ff2dabfe885a53fb04e71076).
> > 
> > It's a 64bit relocation using a 32bit value, but it's unclear to me
> > that modifying the top 32bits is not allowed/intended.
> 
> Urg, I've worded this very badly. It's a 64bit relocation using a
> 32bit value, but it's unclear to me that modifying the top 32bits is
> not allowed/intended and fine to be dropped.
> 
> Thanks, Roger.

I'm not sure what you mean by that. The value is computed based on the
load address and the address of the target symbol - i.e. it is a
PC-relative relocation, and the code is checking that the computed
relative value hasn't overflowed the 32-bit destination in memory
e.g. in the unlikely case that the live patch is loaded far away in
memory from the hypervisor.

The code looks correct to me. It needs investigation to find out why this
particular patch is causing an issue since the code is unchanged since v7
of the original xSplice patch series.

Ross