Re: [PATCH] xen/x86: Livepatch: support patching CET-enhanced functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2022 11:17, Doebel, Bjoern wrote:
> On 07.03.22 10:37, Jan Beulich wrote:
>> On 07.03.2022 10:17, Bjoern Doebel wrote:
>>> --- a/xen/arch/x86/livepatch.c
>>> +++ b/xen/arch/x86/livepatch.c
>>> @@ -14,11 +14,28 @@
>>>   #include <xen/vm_event.h>
>>>   #include <xen/virtual_region.h>
>>>
>>> +#include <asm/endbr.h>
>>>   #include <asm/fixmap.h>
>>>   #include <asm/nmi.h>
>>>   #include <asm/livepatch.h>
>>>   #include <asm/setup.h>
>>>
>>> +/*
>>> + * CET hotpatching support: We may have functions starting with an ENDBR64 
>>> instruction
>>> + * that MUST remain the first instruction of the function, hence we need 
>>> to move any
>>> + * hotpatch trampoline further into the function. For that we need to keep 
>>> track of the
>>> + * patching offset used for any loaded hotpatch (to avoid racing against 
>>> other fixups
>>> + * adding/removing ENDBR64 or similar instructions).
>>> + *
>>> + * We do so by making use of the existing opaque metadata area. We use its 
>>> first 4 bytes
>>> + * to track the offset into the function used for patching and the 
>>> remainder of the data
>>> + * to store overwritten code bytes.
>>> + */
>>
>> Style: Line length (also elsewhere).
>>
>>> +struct __packed x86_livepatch_meta {
>>> +    int32_t patch_offset;
>>
>> I was first going to suggest to use plain int here to comply with
>> ./CODING_STYLE, but it looks like int8_t or uint8_t will do, leaving
>> more space for the insn. I'm also not convinced you really need the
>> __packed annotation. Furthermore, to avoid the need for casts,
>> simply using the ->opaque[] directly would look to be an option then
>> (with suitable #define-s to identify the two parts).
> 
> * Agree on the uint8_t.
> * The __packed was mostly to be really sure there's no added padding.
>    Likely will be needed once I change the type for the offset.
> * I think using an explicit struct is more readable+comprehensible than
>    magically defining macros to reinterpret the opaque[] array, so I'd
>    prefer that. No hard feelings, though.

While I agree on the readable/comprehensible aspect, casts - in particular
such as used here - come with a risk. But it'll be up to the maintainers
to judge anyway.

Jan