Comparing a signed 64bit integer to a signed 32 bit integer may lead to
unexpected overflows. Adjust the cast to use the same type.

Signed-off-by: Bjoern Doebel <doebel@amazon.de>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
---
I need some input here. When testing the CET-BIT livepatch updates I
noticed that my generated livepatch would not load due to

(XEN) livepatch: vmx: Overflow in relocation 1 in .rela.altinstructions for .altinstructions

A deeper look revealed that the ELF relocation adjustment seems to be
going wrong and that in fact the lower 32bit of the compared values in
my case were identical, but that the cast to int64_t for the value
pulled in extra 32 bits, which turned out to be different.

Applying this patch fixed the issue for my example and I got a fully
working livepatch. However, I do not understand what is actually going
on here, so I'm sending this RFC to get extra eyes on the code.
---
 xen/arch/x86/livepatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 59620b8a4f..5380e18bd9 100644
--- 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;
-            if ( (int64_t)val != *(int32_t *)dest )
+            if ( (int32_t)val != *(int32_t *)dest )
             {
                 printk(XENLOG_ERR LIVEPATCH "%s: Overflow in relocation %u in %s for %s\n",
                        elf->name, i, rela->name, base->name);
-- 
2.32.0




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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.

Jan

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.

Regards, Roger.

[0] https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/module.c#L190

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.

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Tuesday, March 8, 2022 4:26 PM
> To: Bjoern Doebel <doebel@amazon.de>; Jan Beulich <jbeulich@suse.com>
> Cc: Michael Kurth <mku@amazon.de>; Martin Pohlack <mpohlack@amazon.de>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> 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

On 08.03.2022 17:26, Roger Pau Monné wrote:
> 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.

I'm still confused: Afaics this is in the handling of R_X86_64_PC32,
which is a 32-bit relocation. Only a 32-bit field in memory is to be
modified, and the resulting value needs to fit such that when the
CPU fetches it and sign-extends it to 64 bits, the original value is
re-established. Hence the check, aiui.

Jan