livepatch: do not use .livepatch.funcs section to store internal state

Message ID	20231116115841.71847-1-roger.pau@citrix.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <xen-devel-bounces@lists.xenproject.org> Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" <xen-devel-bounces@lists.xenproject.org> From: Roger Pau Monne <roger.pau@citrix.com> To: xen-devel@lists.xenproject.org Cc: Roger Pau Monne <roger.pau@citrix.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Ross Lagerwall <ross.lagerwall@citrix.com>, Stefano Stabellini <sstabellini@kernel.org>, Julien Grall <julien@xen.org>, Bertrand Marquis <bertrand.marquis@arm.com>, Michal Orzel <michal.orzel@amd.com>, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>, Andrew Cooper <andrew.cooper3@citrix.com>, George Dunlap <george.dunlap@citrix.com>, Jan Beulich <jbeulich@suse.com>, Wei Liu <wl@xen.org> Subject: [PATCH] livepatch: do not use .livepatch.funcs section to store internal state Date: Thu, 16 Nov 2023 12:58:41 +0100 Message-ID: <20231116115841.71847-1-roger.pau@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Series	livepatch: do not use .livepatch.funcs section to store internal state \| expand livepatch: do not use .livepatch.funcs section to store internal state

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c index 3c50283b2ab7..5dd877a5c625 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -11,23 +11,24 @@ #include <asm/page.h> #include <asm/livepatch.h> -void arch_livepatch_apply(struct livepatch_func *func) +void arch_livepatch_apply(const struct livepatch_func *func, + struct livepatch_fstate *state) { uint32_t insn; uint32_t *new_ptr; unsigned int i, len; - BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(state->opaque)); BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); ASSERT(vmap_of_xen_text); - len = livepatch_insn_len(func); + len = livepatch_insn_len(func, state); if ( !len ) return; /* Save old ones. */ - memcpy(func->opaque, func->old_addr, len); + memcpy(state->opaque, func->old_addr, len); if ( func->new_addr ) { diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index 62d2ef373a0e..9017e6b0c2bc 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -15,23 +15,24 @@ #include <asm/insn.h> #include <asm/livepatch.h> -void arch_livepatch_apply(struct livepatch_func *func) +void arch_livepatch_apply(const struct livepatch_func *func, + struct livepatch_fstate *state) { uint32_t insn; uint32_t *new_ptr; unsigned int i, len; - BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(state->opaque)); BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); ASSERT(vmap_of_xen_text); - len = livepatch_insn_len(func); + len = livepatch_insn_len(func, state); if ( !len ) return; /* Save old ones. */ - memcpy(func->opaque, func->old_addr, len); + memcpy(state->opaque, func->old_addr, len); if ( func->new_addr ) insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index d646379c8c5e..38f6b335a50e 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -69,7 +69,7 @@ void arch_livepatch_revive(void) int arch_livepatch_verify_func(const struct livepatch_func *func) { /* If NOPing only do up to maximum amount we can put in the ->opaque. */ - if ( !func->new_addr && (func->new_size > sizeof(func->opaque) || + if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE || func->new_size % ARCH_PATCH_INSN_SIZE) ) return -EOPNOTSUPP; @@ -79,15 +79,16 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) return 0; } -void arch_livepatch_revert(const struct livepatch_func *func) +void arch_livepatch_revert(const struct livepatch_func *func, + struct livepatch_fstate *state) { uint32_t *new_ptr; unsigned int len; new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; - len = livepatch_insn_len(func); - memcpy(new_ptr, func->opaque, len); + len = livepatch_insn_len(func, state); + memcpy(new_ptr, state->opaque, len); clean_and_invalidate_dcache_va_range(new_ptr, len); } diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index a54d991c5f0f..97e54b18a912 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -95,7 +95,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) if ( !func->new_addr ) { /* Only do up to maximum amount we can put in the ->opaque. */ - if ( func->new_size > sizeof(func->opaque) ) + if ( func->new_size > LIVEPATCH_OPAQUE_SIZE ) return -EOPNOTSUPP; if ( func->old_size < func->new_size ) @@ -123,13 +123,14 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) * "noinline" to cause control flow change and thus invalidate I$ and * cause refetch after modification. */ -void noinline arch_livepatch_apply(struct livepatch_func *func) +void noinline arch_livepatch_apply(const struct livepatch_func *func, + struct livepatch_fstate *state) { uint8_t *old_ptr; - uint8_t insn[sizeof(func->opaque)]; + uint8_t insn[sizeof(state->opaque)]; unsigned int len; - func->patch_offset = 0; + state->patch_offset = 0; old_ptr = func->old_addr; /* @@ -141,14 +142,14 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) * ENDBR64 or similar instructions). */ if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) ) - func->patch_offset += ENDBR64_LEN; + state->patch_offset += ENDBR64_LEN; /* This call must be done with ->patch_offset already set. */ - len = livepatch_insn_len(func); + len = livepatch_insn_len(func, state); if ( !len ) return; - memcpy(func->opaque, old_ptr + func->patch_offset, len); + memcpy(state->opaque, old_ptr + state->patch_offset, len); if ( func->new_addr ) { int32_t val; @@ -156,7 +157,7 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val))); insn[0] = 0xe9; /* Relative jump. */ - val = func->new_addr - (func->old_addr + func->patch_offset + + val = func->new_addr - (func->old_addr + state->patch_offset + ARCH_PATCH_INSN_SIZE); memcpy(&insn[1], &val, sizeof(val)); @@ -164,17 +165,18 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) else add_nops(insn, len); - memcpy(old_ptr + func->patch_offset, insn, len); + memcpy(old_ptr + state->patch_offset, insn, len); } /* * "noinline" to cause control flow change and thus invalidate I$ and * cause refetch after modification. */ -void noinline arch_livepatch_revert(const struct livepatch_func *func) +void noinline arch_livepatch_revert(const struct livepatch_func *func, + struct livepatch_fstate *state) { - memcpy(func->old_addr + func->patch_offset, func->opaque, - livepatch_insn_len(func)); + memcpy(func->old_addr + state->patch_offset, state->opaque, + livepatch_insn_len(func, state)); } /* diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index d89a904bd4e1..e635606c10fb 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -260,6 +260,9 @@ static void free_payload_data(struct payload *payload) vfree((void *)payload->text_addr); payload->pages = 0; + + /* fstate gets allocated strictly after move_payload. */ + XFREE(payload->fstate); } /* @@ -656,6 +659,7 @@ static int prepare_payload(struct payload *payload, { const struct livepatch_elf_sec *sec; unsigned int i; + struct livepatch_func *funcs; struct livepatch_func *f; struct virtual_region *region; const Elf_Note *n; @@ -666,14 +670,19 @@ static int prepare_payload(struct payload *payload, if ( !section_ok(elf, sec, sizeof(*payload->funcs)) ) return -EINVAL; - payload->funcs = sec->load_addr; + payload->funcs = funcs = sec->load_addr; payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs); + payload->fstate = xzalloc_array(typeof(*payload->fstate), + payload->nfuncs); + if ( !payload->fstate ) + return -ENOMEM; + for ( i = 0; i < payload->nfuncs; i++ ) { int rc; - f = &(payload->funcs[i]); + f = &(funcs[i]); if ( f->version != LIVEPATCH_PAYLOAD_VERSION ) { @@ -1361,7 +1370,7 @@ static int apply_payload(struct payload *data) ASSERT(!local_irq_is_enabled()); for ( i = 0; i < data->nfuncs; i++ ) - common_livepatch_apply(&data->funcs[i]); + common_livepatch_apply(&data->funcs[i], &data->fstate[i]); arch_livepatch_revive(); @@ -1397,7 +1406,7 @@ static int revert_payload(struct payload *data) } for ( i = 0; i < data->nfuncs; i++ ) - common_livepatch_revert(&data->funcs[i]); + common_livepatch_revert(&data->funcs[i], &data->fstate[i]); /* * Since we are running with IRQs disabled and the hooks may call common @@ -1438,9 +1447,10 @@ static inline bool was_action_consistent(const struct payload *data, livepatch_f for ( i = 0; i < data->nfuncs; i++ ) { - struct livepatch_func *f = &(data->funcs[i]); + const struct livepatch_func *f = &(data->funcs[i]); + const struct livepatch_fstate *s = &(data->fstate[i]); - if ( f->applied != expected_state ) + if ( s->applied != expected_state ) { printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n", data->name, f->name ?: "noname"); @@ -2157,7 +2167,8 @@ static void cf_check livepatch_printall(unsigned char key) for ( i = 0; i < data->nfuncs; i++ ) { - struct livepatch_func *f = &(data->funcs[i]); + const struct livepatch_func *f = &(data->funcs[i]); + printk(" %s patch %p(%u) with %p (%u)\n", f->name, f->old_addr, f->old_size, f->new_addr, f->new_size); diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index f1eba784059a..9b19679caeb1 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -991,10 +991,7 @@ struct livepatch_func { uint32_t new_size; uint32_t old_size; uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ - uint8_t opaque[LIVEPATCH_OPAQUE_SIZE]; - uint8_t applied; - uint8_t patch_offset; - uint8_t _pad[6]; + uint8_t _pad[39]; livepatch_expectation_t expect; }; typedef struct livepatch_func livepatch_func_t; diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 9fdb29c382b6..a44141a82ef2 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -13,6 +13,9 @@ struct xen_sysctl_livepatch_op; #include <xen/elfstructs.h> #include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */ + +#include <public/sysctl.h> /* For LIVEPATCH_OPAQUE_SIZE */ + #ifdef CONFIG_LIVEPATCH /* @@ -51,6 +54,12 @@ struct livepatch_symbol { bool_t new_symbol; }; +struct livepatch_fstate { + unsigned int patch_offset; + enum livepatch_func_state applied; + uint8_t opaque[LIVEPATCH_OPAQUE_SIZE]; +}; + int livepatch_op(struct xen_sysctl_livepatch_op *); void check_for_livepatch_work(void); unsigned long livepatch_symbols_lookup_by_name(const char *symname); @@ -87,10 +96,11 @@ void arch_livepatch_init(void); int arch_livepatch_verify_func(const struct livepatch_func *func); static inline -unsigned int livepatch_insn_len(const struct livepatch_func *func) +unsigned int livepatch_insn_len(const struct livepatch_func *func, + const struct livepatch_fstate *state) { if ( !func->new_addr ) - return func->new_size - func->patch_offset; + return func->new_size - state->patch_offset; return ARCH_PATCH_INSN_SIZE; } @@ -117,39 +127,43 @@ int arch_livepatch_safety_check(void); int arch_livepatch_quiesce(void); void arch_livepatch_revive(void); -void arch_livepatch_apply(struct livepatch_func *func); -void arch_livepatch_revert(const struct livepatch_func *func); +void arch_livepatch_apply(const struct livepatch_func *func, + struct livepatch_fstate *state); +void arch_livepatch_revert(const struct livepatch_func *func, + struct livepatch_fstate *state); void arch_livepatch_post_action(void); void arch_livepatch_mask(void); void arch_livepatch_unmask(void); -static inline void common_livepatch_apply(struct livepatch_func *func) +static inline void common_livepatch_apply(const struct livepatch_func *func, + struct livepatch_fstate *state) { /* If the action has been already executed on this function, do nothing. */ - if ( func->applied == LIVEPATCH_FUNC_APPLIED ) + if ( state->applied == LIVEPATCH_FUNC_APPLIED ) { printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n", __func__, func->name); return; } - arch_livepatch_apply(func); - func->applied = LIVEPATCH_FUNC_APPLIED; + arch_livepatch_apply(func, state); + state->applied = LIVEPATCH_FUNC_APPLIED; } -static inline void common_livepatch_revert(struct livepatch_func *func) +static inline void common_livepatch_revert(const struct livepatch_func *func, + struct livepatch_fstate *state) { /* If the apply action hasn't been executed on this function, do nothing. */ - if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED ) + if ( !func->old_addr || state->applied == LIVEPATCH_FUNC_NOT_APPLIED ) { printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n", __func__, func->name); return; } - arch_livepatch_revert(func); - func->applied = LIVEPATCH_FUNC_NOT_APPLIED; + arch_livepatch_revert(func, state); + state->applied = LIVEPATCH_FUNC_NOT_APPLIED; } #else diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h index 9f5f0642057c..b9cd4f209670 100644 --- a/xen/include/xen/livepatch_payload.h +++ b/xen/include/xen/livepatch_payload.h @@ -52,7 +52,8 @@ struct payload { size_t ro_size; /* .. and its size (if any). */ unsigned int pages; /* Total pages for [text,rw,ro]_addr */ struct list_head applied_list; /* Linked to 'applied_list'. */ - struct livepatch_func *funcs; /* The array of functions to patch. */ + const struct livepatch_func *funcs; /* The array of functions to patch. */ + struct livepatch_fstate *fstate; /* State of patched functions. */ unsigned int nfuncs; /* Nr of functions to patch. */ const struct livepatch_symbol *symtab; /* All symbols. */ const char *strtab; /* Pointer to .strtab. */

livepatch: do not use .livepatch.funcs section to store internal state

Commit Message

Comments

Patch