[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/x86: Livepatch: support patching CET-enhanced functions
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)? > --- 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). > @@ -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)); > } > > /*
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |