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

[Jan Beulich <jbeulich@xxxxxxxx>]

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