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

["Doebel, Bjoern" <doebel@xxxxxxxxx>]

On 07.03.22 10:37, Jan Beulich wrote:
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open attachments unless you can confirm the sender and know the 
> content is safe.
>
>
>
> On 07.03.2022 10:17, Bjoern Doebel wrote:
> > Xen enabled CET for supporting architectures. The control flow aspect of
> > CET expects functions that can be called indirectly (i.e., via function
> > pointers) to start with an ENDBR64 instruction. Otherwise a control flow
> > exception is raised.
> >
> > This expectation breaks livepatching flows because we patch functions by
> > overwriting their first 5 bytes with a JMP + <offset>, thus breaking the
> > ENDBR64. We fix this by checking the start of a patched function for
> > being ENDBR64. In the positive case we move the livepatch JMP to start
> > behind the ENDBR64 instruction.
> >
> > To avoid having to guess the ENDBR64 offset again on patch reversal
> > (which might race with other mechanisms adding/removing ENDBR
> > dynamically), use the livepatch metadata to store the computed offset
> > along with the saved bytes of the overwritten function.
> >
> > Signed-off-by: Bjoern Doebel <doebel@xxxxxxxxx>
> > CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> >
> > Note that on top of livepatching functions, Xen supports an additional
> > mode where we can "remove" a function by overwriting it with NOPs. This
> > is only supported for functions up to 31 bytes in size and this patch
> > reduces this limit to 27 bytes.
> Is this intended to be part of the description (then it should move up)
> or a post-commit-message remark (then there should be a --- separator
> ahead of it)?
Missing separator. Will fix as well as your style comments below.

> > --- 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.

> > @@ -104,11 +121,14 @@ void noinline arch_livepatch_revive(void)
> >
> >   int arch_livepatch_verify_func(const struct livepatch_func *func)
> >   {
> > +    BUILD_BUG_ON(sizeof(struct x86_livepatch_meta) != LIVEPATCH_OPAQUE_SIZE);
> > +
> >       /* If NOPing.. */
> >       if ( !func->new_addr )
> >       {
> > +        struct x86_livepatch_meta *lp = (struct 
> > x86_livepatch_meta*)func->opaque;
> _If_ there is to remain a struct, please insert the missing blank before
> the star (applicable elsewhere as well), and please separate declaration(s)
> from statement(s) by a blank line.
>
> > -        if ( func->new_size > sizeof(func->opaque) )
> > +        if ( func->new_size > sizeof(lp->instruction) )
> This being the only use of the new variable, I expect compilers to warn
> about the variable being actually unused.
>
>
> > @@ -127,15 +147,20 @@ int arch_livepatch_verify_func(const struct 
> > livepatch_func *func)
> >   void noinline arch_livepatch_apply(struct livepatch_func *func)
> >   {
> >       uint8_t *old_ptr;
> > -    uint8_t insn[sizeof(func->opaque)];
> > +    struct x86_livepatch_meta *lp = (struct x86_livepatch_meta*)func->opaque;
> > +    uint8_t insn[sizeof(lp->instruction)];
> >       unsigned int len;
> >
> > +    lp->patch_offset = 0;
> >       old_ptr = func->old_addr;
> >       len = livepatch_insn_len(func);
> >       if ( !len )
> >           return;
> >
> > -    memcpy(func->opaque, old_ptr, len);
> > +    if ( is_endbr64( old_ptr ) )
> Style: No blanks inside the inner parentheses, please.
>
> > @@ -159,7 +184,9 @@ void noinline arch_livepatch_apply(struct livepatch_func 
> > *func)
> >    */
> >   void noinline arch_livepatch_revert(const struct livepatch_func *func)
> >   {
> > -    memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
> > +    struct x86_livepatch_meta *lp = (struct x86_livepatch_meta*)func->opaque;
> const (if the variable was to remain in the first place).
>
> Jan
>
> > +    memcpy(func->old_addr + lp->patch_offset, lp->instruction, 
> > livepatch_insn_len(func));
> >   }
> >
> >   /*

Bjoern



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