| Message ID | b74a68b038c31df4bb94a5b5e87453f5a249cfe2.1646753657.git.doebel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] arch/x86: Livepatch: fix overflow check when computing ELF relocations | expand |
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.
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
> 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
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);
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(-)