| Message ID | 20190821081931.90887-10-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | livepatch: new features and fixes | expand |
On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote: > struct livepatch_func { > const char *name; /* Name of function to be patched. */ > void *new_addr; > @@ -834,6 +839,10 @@ struct livepatch_func { > uint32_t old_size; > uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ > uint8_t opaque[31]; > +#if defined CONFIG_X86 > + uint8_t applied; > + uint8_t _pad[7]; > +#endif Replying here as well. Could you use part of the 'opaque[31]' space instead for this field?
> On 21. Aug 2019, at 20:28, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote: >> struct livepatch_func { >> const char *name; /* Name of function to be patched. */ >> void *new_addr; >> @@ -834,6 +839,10 @@ struct livepatch_func { >> uint32_t old_size; >> uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ >> uint8_t opaque[31]; >> +#if defined CONFIG_X86 >> + uint8_t applied; >> + uint8_t _pad[7]; >> +#endif > > Replying here as well. Could you use part of the 'opaque[31]' space instead for this field? No, as explained earlier, I must not do that. The opaque[] is not a padding. Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Hi Pawel, On 8/21/19 9:19 AM, Pawel Wieczorkiewicz wrote: > Livepatch only tracks an entire payload applied/reverted state. But, > with an option to supply the apply_payload() and/or revert_payload() > functions as optional hooks, it becomes possible to intermix the > execution of the original apply_payload()/revert_payload() functions > with their dynamically supplied counterparts. > It is important then to track the current state of every function > being patched and prevent situations of unintentional double-apply > or unapplied revert. > > To support that, it is necessary to extend public interface of the > livepatch. The struct livepatch_func gets additional field holding > the applied/reverted state marker. > > To reflect the livepatch payload ABI change, bump the version flag > LIVEPATCH_PAYLOAD_VERSION up to 2. > > The above solution only applies to x86 architecture for now. Is this going to break livepatch on Arm? If so, do you have any plan to fix it? [...] > diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h > index 2aec532ee2..a93126f631 100644 > --- a/xen/include/xen/livepatch.h > +++ b/xen/include/xen/livepatch.h > @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); I doubt livepatch on Arm will compile after this change. > void arch_livepatch_post_action(void); > > void arch_livepatch_mask(void); Cheers,
On 21. Aug 2019, at 23:34, Julien Grall <julien.grall@arm.com<mailto:julien.grall@arm.com>> wrote: Hi Pawel, Hi Julien, On 8/21/19 9:19 AM, Pawel Wieczorkiewicz wrote: Livepatch only tracks an entire payload applied/reverted state. But, with an option to supply the apply_payload() and/or revert_payload() functions as optional hooks, it becomes possible to intermix the execution of the original apply_payload()/revert_payload() functions with their dynamically supplied counterparts. It is important then to track the current state of every function being patched and prevent situations of unintentional double-apply or unapplied revert. To support that, it is necessary to extend public interface of the livepatch. The struct livepatch_func gets additional field holding the applied/reverted state marker. To reflect the livepatch payload ABI change, bump the version flag LIVEPATCH_PAYLOAD_VERSION up to 2. The above solution only applies to x86 architecture for now. Is this going to break livepatch on Arm? If so, do you have any plan to fix it? No, I do not think it is. But, I am unable to test on Arm (No access to HW and SW), so I took the conservative approach here. [...] diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 2aec532ee2..a93126f631 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); I doubt livepatch on Arm will compile after this change. What would be you suggestion then? Shall I limit the change to X86 everywhere Or maybe drop the compilation flag completely? void arch_livepatch_post_action(void); void arch_livepatch_mask(void); Cheers, -- Julien Grall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Hi, On 22/08/2019 08:44, Wieczorkiewicz, Pawel wrote: > >> On 21. Aug 2019, at 23:34, Julien Grall <julien.grall@arm.com >> <mailto:julien.grall@arm.com>> wrote: >> On 8/21/19 9:19 AM, Pawel Wieczorkiewicz wrote: >>> Livepatch only tracks an entire payload applied/reverted state. But, >>> with an option to supply the apply_payload() and/or revert_payload() >>> functions as optional hooks, it becomes possible to intermix the >>> execution of the original apply_payload()/revert_payload() functions >>> with their dynamically supplied counterparts. >>> It is important then to track the current state of every function >>> being patched and prevent situations of unintentional double-apply >>> or unapplied revert. >>> To support that, it is necessary to extend public interface of the >>> livepatch. The struct livepatch_func gets additional field holding >>> the applied/reverted state marker. >>> To reflect the livepatch payload ABI change, bump the version flag >>> LIVEPATCH_PAYLOAD_VERSION up to 2. >>> The above solution only applies to x86 architecture for now. >> >> Is this going to break livepatch on Arm? If so, do you have any plan to fix it? >> > > No, I do not think it is. But, I am unable to test on Arm (No access to HW and SW), > so I took the conservative approach here. Arm provides decent free model (see FoundationModel) that you can use for basic testing. Alternatively, you QEMU also support virtualization extension. Let me have a look at the code (I will answer separately) to see if I can spot anything. > >> [...] >> >>> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h >>> index 2aec532ee2..a93126f631 100644 >>> --- a/xen/include/xen/livepatch.h >>> +++ b/xen/include/xen/livepatch.h >>> @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); >> >> I doubt livepatch on Arm will compile after this change. >> > > What would be you suggestion then? Cross-compiler are nowadays widely available. So build testing your changes in common code would be the minimum. In this case, as you dropped the const from the prototype, you will need to do the same in the declaration. > Shall I limit the change to X86 everywhere > Or maybe drop the compilation flag completely? I am a bit confused. Which compilation flag do you refer to? Cheers,
On 22. Aug 2019, at 12:07, Julien Grall <julien.grall@arm.com<mailto:julien.grall@arm.com>> wrote: Hi, On 22/08/2019 08:44, Wieczorkiewicz, Pawel wrote: …snip... Is this going to break livepatch on Arm? If so, do you have any plan to fix it? No, I do not think it is. But, I am unable to test on Arm (No access to HW and SW), so I took the conservative approach here. Arm provides decent free model (see FoundationModel) that you can use for basic testing. Alternatively, you QEMU also support virtualization extension. Let me have a look at the code (I will answer separately) to see if I can spot anything. [...] diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 2aec532ee2..a93126f631 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); I doubt livepatch on Arm will compile after this change. What would be you suggestion then? Cross-compiler are nowadays widely available. So build testing your changes in common code would be the minimum. I wish it was that simple. Nevertheless, I will try to prepare an environment to perform such builds. In this case, as you dropped the const from the prototype, you will need to do the same in the declaration. Yes, but I see 2 options here: - Enable the feature also for Arm (I prefer that, but will not be able to test that in nearest future) - Keep Arm excluded and sprinkle code with #ifdef CONFIG_X86 Shall I limit the change to X86 everywhere Or maybe drop the compilation flag completely? I am a bit confused. Which compilation flag do you refer to? CONFIG_X86 Cheers, -- Julien Grall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Hi Pawel, On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote: > Livepatch only tracks an entire payload applied/reverted state. But, > with an option to supply the apply_payload() and/or revert_payload() > functions as optional hooks, it becomes possible to intermix the > execution of the original apply_payload()/revert_payload() functions > with their dynamically supplied counterparts. > It is important then to track the current state of every function > being patched and prevent situations of unintentional double-apply > or unapplied revert. > > To support that, it is necessary to extend public interface of the > livepatch. The struct livepatch_func gets additional field holding > the applied/reverted state marker. > > To reflect the livepatch payload ABI change, bump the version flag > LIVEPATCH_PAYLOAD_VERSION up to 2. > > The above solution only applies to x86 architecture for now. > > Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> > Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com> > Reviewed-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Martin Pohlack <mpohlack@amazon.de> > --- > xen/arch/x86/livepatch.c | 20 +++++++++++++++++++- > xen/common/livepatch.c | 35 +++++++++++++++++++++++++++++++++++ > xen/include/public/sysctl.h | 11 ++++++++++- > xen/include/xen/livepatch.h | 2 +- > 4 files changed, 65 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > index 436ee40fe1..76fa91a082 100644 > --- a/xen/arch/x86/livepatch.c > +++ b/xen/arch/x86/livepatch.c > @@ -61,6 +61,14 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) > if ( !len ) > return; > > + /* If the apply action has been already executed on this function, do nothing... */ > + if ( func->applied == LIVEPATCH_FUNC_APPLIED ) > + { > + printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n", > + __func__, func->name); > + return; > + } Does this need to in arch specific code? > + > memcpy(func->opaque, old_ptr, len); > if ( func->new_addr ) > { > @@ -77,15 +85,25 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) > add_nops(insn, len); > > memcpy(old_ptr, insn, len); > + func->applied = LIVEPATCH_FUNC_APPLIED; > } > > /* > * "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(struct livepatch_func *func) > { > + /* If the apply action hasn't been executed on this function, do nothing... */ > + if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED ) > + { > + printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n", > + __func__, func->name); > + return; > + } > + > memcpy(func->old_addr, func->opaque, livepatch_insn_len(func)); > + func->applied = LIVEPATCH_FUNC_NOT_APPLIED; > } > > /* > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index 585ec9819a..090a48977b 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -1242,6 +1242,29 @@ static inline void revert_payload_tail(struct payload *data) > data->state = LIVEPATCH_STATE_CHECKED; > } > > +/* > + * Check if an action has applied the same state to all payload's functions consistently. > + */ > +static inline bool was_action_consistent(const struct payload *data, livepatch_func_state_t expected_state) > +{ > + int i; > + > + for ( i = 0; i < data->nfuncs; i++ ) > + { > + struct livepatch_func *f = &(data->funcs[i]); > + > + if ( f->applied != expected_state ) Per the definition of livepath_func, the field "applied" only exists for x86. So this will not compiled on Arm. > + { > + printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n", > + data->name, f->name ?: "noname"); > + > + return false; > + } > + } > + > + return true; > +} > + > /* > * This function is executed having all other CPUs with no deep stack (we may > * have cpu_idle on it) and IRQs disabled. > @@ -1268,6 +1291,9 @@ static void livepatch_do_action(void) > else > rc = apply_payload(data); > > + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) ) Regardless the compilation issue above, none of the common code will set the state. So for Arm, this would likely mean the function return false. > + panic("livepatch: partially applied payload '%s'!\n", data->name); I can see at least a case where the panic can be reached here. Per the changes in arch_livepatch_apply(), f->applied will only be set to LIVEPATCH_FUNC_APPLIED if livepatch_insn_len() is not 0. I don't know whether livepatch_insn_len() can ever return 0 as my livepatch knowledge is limited. But the code live little doubt this is a theoritical possibility that after this patch will turn into crashing Xen. More generally, I am not very comfortable to see panic() in the middle of the code. Could you explain why panic is the best solution over reverting the work? My question applies for all the other panic() below. > + > if ( rc == 0 ) > apply_payload_tail(data); > break; > @@ -1282,6 +1308,9 @@ static void livepatch_do_action(void) > else > rc = revert_payload(data); > > + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) > + panic("livepatch: partially reverted payload '%s'!\n", data->name); > + > if ( rc == 0 ) > revert_payload_tail(data); > break; > @@ -1304,6 +1333,9 @@ static void livepatch_do_action(void) > other->rc = revert_payload(other); > > > + if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) > + panic("livepatch: partially reverted payload '%s'!\n", other->name); > + > if ( other->rc == 0 ) > revert_payload_tail(other); > else > @@ -1324,6 +1356,9 @@ static void livepatch_do_action(void) > else > rc = apply_payload(data); > > + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) ) > + panic("livepatch: partially applied payload '%s'!\n", data->name); > + > if ( rc == 0 ) > apply_payload_tail(data); > } > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h > index 1b2b165a6d..b55ad6d050 100644 > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -818,7 +818,7 @@ struct xen_sysctl_cpu_featureset { > * If zero exit with success. > */ > > -#define LIVEPATCH_PAYLOAD_VERSION 1 > +#define LIVEPATCH_PAYLOAD_VERSION 2 > /* > * .livepatch.funcs structure layout defined in the `Payload format` > * section in the Live Patch design document. > @@ -826,6 +826,11 @@ struct xen_sysctl_cpu_featureset { > * We guard this with __XEN__ as toolstacks SHOULD not use it. > */ > #ifdef __XEN__ > +typedef enum livepatch_func_state { > + LIVEPATCH_FUNC_NOT_APPLIED = 0, > + LIVEPATCH_FUNC_APPLIED = 1 AFAIK, enum will always start counting from 0, so is it really necessary to specify the exact values? > +} livepatch_func_state_t; > + > struct livepatch_func { > const char *name; /* Name of function to be patched. */ > void *new_addr; > @@ -834,6 +839,10 @@ struct livepatch_func { > uint32_t old_size; > uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ > uint8_t opaque[31]; > +#if defined CONFIG_X86 > + uint8_t applied; > + uint8_t _pad[7]; > +#endif Above, you increase the version of the payload here even for Arm when there are no modification in Arm. This raises the question on whether we would need to increase the version when Arm is going to be supported? > }; > typedef struct livepatch_func livepatch_func_t; > #endif > diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h > index 2aec532ee2..a93126f631 100644 > --- a/xen/include/xen/livepatch.h > +++ b/xen/include/xen/livepatch.h > @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); > void arch_livepatch_post_action(void); > > void arch_livepatch_mask(void); > Cheers,
Hi, On 22/08/2019 11:20, Wieczorkiewicz, Pawel wrote: >> On 22. Aug 2019, at 12:07, Julien Grall <julien.grall@arm.com >> <mailto:julien.grall@arm.com>> wrote: >> On 22/08/2019 08:44, Wieczorkiewicz, Pawel wrote: >>>>> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h >>>>> index 2aec532ee2..a93126f631 100644 >>>>> --- a/xen/include/xen/livepatch.h >>>>> +++ b/xen/include/xen/livepatch.h >>>>> @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); >>>> >>>> I doubt livepatch on Arm will compile after this change. >>>> >>> What would be you suggestion then? >> >> Cross-compiler are nowadays widely available. So build testing your changes in >> common code would be the minimum. >> > > I wish it was that simple. Nevertheless, I will try to prepare an environment to > perform such builds. Cross-compiling the hypervisor is really easy ;). 1) Download the cross-compiler tarball (here one [1]) and uncompress it. You can also install the one provided by your distro. 2) Build Xen hypervisor. Here an example for arm64: 42sh> cd xen.git/xen 42sh> make XEN_TARGET_ARCH=arm64 CROSS_COMPILER=<triplet>- In my case, I am using the Arm toolchain AArch64 GNU/Linux target (aarch64-linux-gnu). So the <triplet> would be aarch64-linux-gnu. This is assuming you have the compilers binary in your PATH. If not, you can use give the full path: CROSS_COMPILER=/opt/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu- > >> In this case, as you dropped the const from the prototype, you will need to do >> the same in the declaration. >> > > Yes, but I see 2 options here: > - Enable the feature also for Arm (I prefer that, but will not be able to test > that in nearest future) I think some of the code can be made common. So we could possibly rely on x86 for that. Additionally, IIRC, Konrad has a setup on the cubietruck for testing livepatch. > - Keep Arm excluded and sprinkle code with #ifdef CONFIG_X86 Please no #ifdef CONFIG_X86 in the common code. If you don't plan to support Arm, then we should introduce a new Kconfig that will gate all those changes. Cheers, [1] https://developer.arm.com/tools-and-software/open-source-software/developer-tools/gnu-toolchain/gnu-a/downloads
On 22. Aug 2019, at 12:29, Julien Grall <julien.grall@arm.com<mailto:julien.grall@arm.com>> wrote: Hi Pawel, Hi Julien, Thank for looking at this! On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote: Livepatch only tracks an entire payload applied/reverted state. But, with an option to supply the apply_payload() and/or revert_payload() functions as optional hooks, it becomes possible to intermix the execution of the original apply_payload()/revert_payload() functions with their dynamically supplied counterparts. It is important then to track the current state of every function being patched and prevent situations of unintentional double-apply or unapplied revert. To support that, it is necessary to extend public interface of the livepatch. The struct livepatch_func gets additional field holding the applied/reverted state marker. To reflect the livepatch payload ABI change, bump the version flag LIVEPATCH_PAYLOAD_VERSION up to 2. The above solution only applies to x86 architecture for now. Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de<mailto:wipawel@amazon.de>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com<mailto:andraprs@amazon.com>> Reviewed-by: Bjoern Doebel <doebel@amazon.de<mailto:doebel@amazon.de>> Reviewed-by: Martin Pohlack <mpohlack@amazon.de<mailto:mpohlack@amazon.de>> --- xen/arch/x86/livepatch.c | 20 +++++++++++++++++++- xen/common/livepatch.c | 35 +++++++++++++++++++++++++++++++++++ xen/include/public/sysctl.h | 11 ++++++++++- xen/include/xen/livepatch.h | 2 +- 4 files changed, 65 insertions(+), 3 deletions(-) diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 436ee40fe1..76fa91a082 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -61,6 +61,14 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) if ( !len ) return; + /* If the apply action has been already executed on this function, do nothing... */ + if ( func->applied == LIVEPATCH_FUNC_APPLIED ) + { + printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n", + __func__, func->name); + return; + } Does this need to in arch specific code? As a matter of fact it does not. Let me enable this feature for Arm as well and make this code a common inline. + memcpy(func->opaque, old_ptr, len); if ( func->new_addr ) { @@ -77,15 +85,25 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) add_nops(insn, len); memcpy(old_ptr, insn, len); + func->applied = LIVEPATCH_FUNC_APPLIED; } /* * "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(struct livepatch_func *func) { + /* If the apply action hasn't been executed on this function, do nothing... */ + if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED ) + { + printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n", + __func__, func->name); + return; + } + memcpy(func->old_addr, func->opaque, livepatch_insn_len(func)); + func->applied = LIVEPATCH_FUNC_NOT_APPLIED; } /* diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 585ec9819a..090a48977b 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -1242,6 +1242,29 @@ static inline void revert_payload_tail(struct payload *data) data->state = LIVEPATCH_STATE_CHECKED; } +/* + * Check if an action has applied the same state to all payload's functions consistently. + */ +static inline bool was_action_consistent(const struct payload *data, livepatch_func_state_t expected_state) +{ + int i; + + for ( i = 0; i < data->nfuncs; i++ ) + { + struct livepatch_func *f = &(data->funcs[i]); + + if ( f->applied != expected_state ) Per the definition of livepath_func, the field "applied" only exists for x86. So this will not compiled on Arm. I will enable Arm too. + { + printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n", + data->name, f->name ?: "noname"); + + return false; + } + } + + return true; +} + /* * This function is executed having all other CPUs with no deep stack (we may * have cpu_idle on it) and IRQs disabled. @@ -1268,6 +1291,9 @@ static void livepatch_do_action(void) else rc = apply_payload(data); + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) ) Regardless the compilation issue above, none of the common code will set the state. So for Arm, this would likely mean the function return false. True, I should have disabled this code for Arm too. But now, let me just make it common for all archs. + panic("livepatch: partially applied payload '%s'!\n", data->name); I can see at least a case where the panic can be reached here. Per the changes in arch_livepatch_apply(), f->applied will only be set to LIVEPATCH_FUNC_APPLIED if livepatch_insn_len() is not 0. I don't know whether livepatch_insn_len() can ever return 0 as my livepatch knowledge is limited. But the code live little doubt this is a theoritical possibility that after this patch will turn into crashing Xen. I suppose the livepatch_insn_len() returning 0 (i.e. func->new_size being 0, which is valid AFAICS) is the to-be-implemented feature of noping out code: From the livepatch doc: """ ## TODO Goals * NOP out the code sequence if `new_size` is zero. """ So, in theory it can be 0 (it has to be introduced to the livepatch.funcs). Thus, you’re right. I will fix that, but marking such function (new_size==0) as applied. Thanks! More generally, I am not very comfortable to see panic() in the middle of the code. Could you explain why panic is the best solution over reverting the work? Yes. Production-ready hotpatches must not contain inconsistent hooks or functions-to-be-applied/reverted. The goal here is to detect such hotpatches and fail hard immediately highlighting the fact that such hotpatch is broken. The inconsistent application of a hotpatch (some function applied, some reverted while other left behined) leaves the system in a very bad state. I think the best what we could do here is panic() to enable post-mortem analysis of what went wrong and avoid leaking such system into production. Mis-detecting such conditions and leaving such inconsistent system running is a very bad idea. My question applies for all the other panic() below. + if ( rc == 0 ) apply_payload_tail(data); break; @@ -1282,6 +1308,9 @@ static void livepatch_do_action(void) else rc = revert_payload(data); + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) + panic("livepatch: partially reverted payload '%s'!\n", data->name); + if ( rc == 0 ) revert_payload_tail(data); break; @@ -1304,6 +1333,9 @@ static void livepatch_do_action(void) other->rc = revert_payload(other); + if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) + panic("livepatch: partially reverted payload '%s'!\n", other->name); + if ( other->rc == 0 ) revert_payload_tail(other); else @@ -1324,6 +1356,9 @@ static void livepatch_do_action(void) else rc = apply_payload(data); + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) ) + panic("livepatch: partially applied payload '%s'!\n", data->name); + if ( rc == 0 ) apply_payload_tail(data); } diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 1b2b165a6d..b55ad6d050 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -818,7 +818,7 @@ struct xen_sysctl_cpu_featureset { * If zero exit with success. */ -#define LIVEPATCH_PAYLOAD_VERSION 1 +#define LIVEPATCH_PAYLOAD_VERSION 2 /* * .livepatch.funcs structure layout defined in the `Payload format` * section in the Live Patch design document. @@ -826,6 +826,11 @@ struct xen_sysctl_cpu_featureset { * We guard this with __XEN__ as toolstacks SHOULD not use it. */ #ifdef __XEN__ +typedef enum livepatch_func_state { + LIVEPATCH_FUNC_NOT_APPLIED = 0, + LIVEPATCH_FUNC_APPLIED = 1 AFAIK, enum will always start counting from 0, so is it really necessary to specify the exact values? It’s probably my (or some earlier reviewer's) OCD. I am happy to remove these. +} livepatch_func_state_t; + struct livepatch_func { const char *name; /* Name of function to be patched. */ void *new_addr; @@ -834,6 +839,10 @@ struct livepatch_func { uint32_t old_size; uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ uint8_t opaque[31]; +#if defined CONFIG_X86 + uint8_t applied; + uint8_t _pad[7]; +#endif Above, you increase the version of the payload here even for Arm when there are no modification in Arm. This raises the question on whether we would need to increase the version when Arm is going to be supported? I think the best way forward is to add Arm support. This feature is simple enough and it does not make much sense to complicate it by adding support spaghetti. }; typedef struct livepatch_func livepatch_func_t; #endif diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 2aec532ee2..a93126f631 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); void arch_livepatch_post_action(void); void arch_livepatch_mask(void); Cheers, -- Julien Grall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 22. Aug 2019, at 12:43, Julien Grall <julien.grall@arm.com<mailto:julien.grall@arm.com>> wrote: Hi, On 22/08/2019 11:20, Wieczorkiewicz, Pawel wrote: ..snip.. Cross-compiler are nowadays widely available. So build testing your changes in common code would be the minimum. I wish it was that simple. Nevertheless, I will try to prepare an environment to perform such builds. Cross-compiling the hypervisor is really easy ;). 1) Download the cross-compiler tarball (here one [1]) and uncompress it. You can also install the one provided by your distro. 2) Build Xen hypervisor. Here an example for arm64: 42sh> cd xen.git/xen 42sh> make XEN_TARGET_ARCH=arm64 CROSS_COMPILER=<triplet>- In my case, I am using the Arm toolchain AArch64 GNU/Linux target (aarch64-linux-gnu). So the <triplet> would be aarch64-linux-gnu. This is assuming you have the compilers binary in your PATH. If not, you can use give the full path: CROSS_COMPILER=/opt/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu- Awesome! That really works (especially thanks for the [1] link… finally some toolstack that works on my system). One change was needed: s/CROSS_COMPILER/CROSS_COMPILE/g Thanks! Having this in a wiki would really help. Or have I missed it? In this case, as you dropped the const from the prototype, you will need to do the same in the declaration. Yes, but I see 2 options here: - Enable the feature also for Arm (I prefer that, but will not be able to test that in nearest future) I think some of the code can be made common. So we could possibly rely on x86 for that. Additionally, IIRC, Konrad has a setup on the cubietruck for testing livepatch. Yes, I will do that. - Keep Arm excluded and sprinkle code with #ifdef CONFIG_X86 Please no #ifdef CONFIG_X86 in the common code. If you don't plan to support Arm, then we should introduce a new Kconfig that will gate all those changes. Ugh, you’re right. Removing all that from common code. Cheers, [1] https://developer.arm.com/tools-and-software/open-source-software/developer-tools/gnu-toolchain/gnu-a/downloads -- Julien Grall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Hi, On 22/08/2019 12:15, Wieczorkiewicz, Pawel wrote: >> On 22. Aug 2019, at 12:43, Julien Grall <julien.grall@arm.com >> <mailto:julien.grall@arm.com>> wrote: >> On 22/08/2019 11:20, Wieczorkiewicz, Pawel wrote: >>>> Cross-compiler are nowadays widely available. So build testing your changes >>>> in common code would be the minimum. >>>> >>> I wish it was that simple. Nevertheless, I will try to prepare an environment >>> to perform such builds. >> >> Cross-compiling the hypervisor is really easy ;). >> >> 1) Download the cross-compiler tarball (here one [1]) and uncompress it. You >> can also install the one provided by your distro. >> >> 2) Build Xen hypervisor. Here an example for arm64: >> >> 42sh> cd xen.git/xen >> 42sh> make XEN_TARGET_ARCH=arm64 CROSS_COMPILER=<triplet>- >> >> In my case, I am using the Arm toolchain AArch64 GNU/Linux target >> (aarch64-linux-gnu). So the <triplet> would be aarch64-linux-gnu. >> >> This is assuming you have the compilers binary in your PATH. If not, you can >> use give the full path: >> >> CROSS_COMPILER=/opt/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu- > > Awesome! That really works (especially thanks for the [1] link… finally some > toolstack that works on my system). > > One change was needed: s/CROSS_COMPILER/CROSS_COMPILE/g Oh yes, sorry for that. I didn't copy/paste for once. > > Thanks! > > Having this in a wiki would really help. Or have I missed it? This is explained on the Xen on Arm wiki page [2]. But I have to admit the page is difficult to go through. I need to find some time to rework it. Cheers, >> [1] >> https://developer.arm.com/tools-and-software/open-source-software/developer-tools/gnu-toolchain/gnu-a/downloads [2] https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Cross_Compiling
Hi Pawel, On 22/08/2019 12:02, Wieczorkiewicz, Pawel wrote: >> On 22. Aug 2019, at 12:29, Julien Grall <julien.grall@arm.com >> <mailto:julien.grall@arm.com>> wrote: >> On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote: >> More generally, I am not very comfortable to see panic() in the middle of the >> code. Could you explain why panic is the best solution over reverting the work? >> > > Yes. Production-ready hotpatches must not contain inconsistent hooks or > functions-to-be-applied/reverted. > The goal here is to detect such hotpatches and fail hard immediately > highlighting the fact that such hotpatch > is broken. Aside the len = 0 that you are going to fix. How would this condition happen? Are you going to add code that will potentially trigger the panic? > > The inconsistent application of a hotpatch (some function applied, some reverted > while other left behined) leaves > the system in a very bad state. I think the best what we could do here is > panic() to enable post-mortem analysis > of what went wrong and avoid leaking such system into production. Thank you for the explanation here (and on IRC). May I ask some documentation regarding the panic in at least commit message? Ideally, this would explain why the panic the most sensible solution. Cheers,
On 22. Aug 2019, at 17:30, Julien Grall <julien.grall@arm.com<mailto:julien.grall@arm.com>> wrote: Hi Pawel, On 22/08/2019 12:02, Wieczorkiewicz, Pawel wrote: On 22. Aug 2019, at 12:29, Julien Grall <julien.grall@arm.com<mailto:julien.grall@arm.com> <mailto:julien.grall@arm.com>> wrote: On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote: More generally, I am not very comfortable to see panic() in the middle of the code. Could you explain why panic is the best solution over reverting the work? Yes. Production-ready hotpatches must not contain inconsistent hooks or functions-to-be-applied/reverted. The goal here is to detect such hotpatches and fail hard immediately highlighting the fact that such hotpatch is broken. Aside the len = 0 that you are going to fix. How would this condition happen? Are you going to add code that will potentially trigger the panic? The default livepatch code path is very conservative (to the extent it does not even need this check and panic). But, with the changes of this series, one can potentially construct a hotpatch that upon load would trigger the panic (or without the panic, would silently leave the Xen code in memory in an inconsistent state). This obviously must not happen in production, so the new livepatch feature should be used with care.The changes make livepatch more flexible and universal, yet when using new features, more care is needed. However, when someone is developing a hotpatch or is using the new feature for debugging or for whatever non-production-related reason, it is very helpful to detect immediately any sort of undefined state. I have been there myself when I was working on the changes presented here, and thought that would be a good idea to add this checks permanently. Also, when something changes in the future in the livepatch implementation, those checks could potentially highlight any inconsistencies. The inconsistent application of a hotpatch (some function applied, some reverted while other left behined) leaves the system in a very bad state. I think the best what we could do here is panic() to enable post-mortem analysis of what went wrong and avoid leaking such system into production. Thank you for the explanation here (and on IRC). May I ask some documentation regarding the panic in at least commit message? Ideally, this would explain why the panic the most sensible solution. Yes, certainly. I will add that. Cheers, -- Julien Grall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 436ee40fe1..76fa91a082 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -61,6 +61,14 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) if ( !len ) return; + /* If the apply action has been already executed on this function, do nothing... */ + if ( func->applied == LIVEPATCH_FUNC_APPLIED ) + { + printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied before\n", + __func__, func->name); + return; + } + memcpy(func->opaque, old_ptr, len); if ( func->new_addr ) { @@ -77,15 +85,25 @@ void noinline arch_livepatch_apply(struct livepatch_func *func) add_nops(insn, len); memcpy(old_ptr, insn, len); + func->applied = LIVEPATCH_FUNC_APPLIED; } /* * "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(struct livepatch_func *func) { + /* If the apply action hasn't been executed on this function, do nothing... */ + if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED ) + { + printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n", + __func__, func->name); + return; + } + memcpy(func->old_addr, func->opaque, livepatch_insn_len(func)); + func->applied = LIVEPATCH_FUNC_NOT_APPLIED; } /* diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 585ec9819a..090a48977b 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -1242,6 +1242,29 @@ static inline void revert_payload_tail(struct payload *data) data->state = LIVEPATCH_STATE_CHECKED; } +/* + * Check if an action has applied the same state to all payload's functions consistently. + */ +static inline bool was_action_consistent(const struct payload *data, livepatch_func_state_t expected_state) +{ + int i; + + for ( i = 0; i < data->nfuncs; i++ ) + { + struct livepatch_func *f = &(data->funcs[i]); + + if ( f->applied != expected_state ) + { + printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with inconsistent applied state.\n", + data->name, f->name ?: "noname"); + + return false; + } + } + + return true; +} + /* * This function is executed having all other CPUs with no deep stack (we may * have cpu_idle on it) and IRQs disabled. @@ -1268,6 +1291,9 @@ static void livepatch_do_action(void) else rc = apply_payload(data); + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) ) + panic("livepatch: partially applied payload '%s'!\n", data->name); + if ( rc == 0 ) apply_payload_tail(data); break; @@ -1282,6 +1308,9 @@ static void livepatch_do_action(void) else rc = revert_payload(data); + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) + panic("livepatch: partially reverted payload '%s'!\n", data->name); + if ( rc == 0 ) revert_payload_tail(data); break; @@ -1304,6 +1333,9 @@ static void livepatch_do_action(void) other->rc = revert_payload(other); + if ( !was_action_consistent(other, rc ? LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) + panic("livepatch: partially reverted payload '%s'!\n", other->name); + if ( other->rc == 0 ) revert_payload_tail(other); else @@ -1324,6 +1356,9 @@ static void livepatch_do_action(void) else rc = apply_payload(data); + if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : LIVEPATCH_FUNC_APPLIED) ) + panic("livepatch: partially applied payload '%s'!\n", data->name); + if ( rc == 0 ) apply_payload_tail(data); } diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 1b2b165a6d..b55ad6d050 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -818,7 +818,7 @@ struct xen_sysctl_cpu_featureset { * If zero exit with success. */ -#define LIVEPATCH_PAYLOAD_VERSION 1 +#define LIVEPATCH_PAYLOAD_VERSION 2 /* * .livepatch.funcs structure layout defined in the `Payload format` * section in the Live Patch design document. @@ -826,6 +826,11 @@ struct xen_sysctl_cpu_featureset { * We guard this with __XEN__ as toolstacks SHOULD not use it. */ #ifdef __XEN__ +typedef enum livepatch_func_state { + LIVEPATCH_FUNC_NOT_APPLIED = 0, + LIVEPATCH_FUNC_APPLIED = 1 +} livepatch_func_state_t; + struct livepatch_func { const char *name; /* Name of function to be patched. */ void *new_addr; @@ -834,6 +839,10 @@ struct livepatch_func { uint32_t old_size; uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ uint8_t opaque[31]; +#if defined CONFIG_X86 + uint8_t applied; + uint8_t _pad[7]; +#endif }; typedef struct livepatch_func livepatch_func_t; #endif diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 2aec532ee2..a93126f631 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -117,7 +117,7 @@ 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_revert(struct livepatch_func *func); void arch_livepatch_post_action(void); void arch_livepatch_mask(void);