[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/livepatch: enable livepatching assembly source files
On 18.04.2023 11:24, Roger Pau Monne wrote: > Some of the assembly entry points cannot be safely patched until it's > safe to use jmp, as livepatch can replace a whole block with a jmp to > a new address, and that won't be safe until speculative mitigations > have been applied. Isn't the issue only with indirect JMP, whereas livepatch uses only direct ones? > --- a/xen/arch/x86/include/asm/config.h > +++ b/xen/arch/x86/include/asm/config.h > @@ -44,6 +44,20 @@ > /* Linkage for x86 */ > #ifdef __ASSEMBLY__ > #define ALIGN .align 16,0x90 > +#ifdef CONFIG_LIVEPATCH > +#define START_LP(name) \ > + jmp name; \ > + .pushsection .text.name, "ax", @progbits; \ In how far is livepatch susceptible to two .text.* sections of the same name? This can result here and perhaps also for static C functions. > + name: > +#define END_LP(name) \ > + .size name, . - name; \ > + .type name, @function; \ > + .popsection > +#else > +#define START_LP(name) \ > + name: > +#define END_LP(name) > +#endif > #define ENTRY(name) \ > .globl name; \ > ALIGN; \ Do these really need to go into config.h, instead of e.g. asm_defns.h? I'd prefer if stuff like this was moved out of here, rather than more things accumulating. (Perhaps these also would better be assembler macros, in which case asm-defns.h might be the place to put them, but I guess that's fairly much a matter of taste.) Couldn't END_LP() set type and size unconditionally? (But see also below.) > --- a/xen/arch/x86/x86_64/entry.S > +++ b/xen/arch/x86/x86_64/entry.S > @@ -660,7 +660,7 @@ ENTRY(early_page_fault) > > ALIGN > /* No special register assumptions. */ > -restore_all_xen: > +START_LP(restore_all_xen) > /* > * Check whether we need to switch to the per-CPU page tables, in > * case we return to late PV exit code (from an NMI or #MC). > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) > > RESTORE_ALL adj=8 > iretq > +END_LP(restore_all_xen) While I'm fine with this conversion, ... > ENTRY(common_interrupt) > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > @@ -687,6 +688,7 @@ ENTRY(common_interrupt) > SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: > acd */ > /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ > > +START_LP(common_interrupt_lp) > mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx > mov STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl > mov %rcx, %r15 > @@ -707,6 +709,7 @@ ENTRY(common_interrupt) > mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14) > mov %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14) > jmp ret_from_intr > +END_LP(common_interrupt_lp) ... this one's odd, as it doesn't cover the entire "function". How would you envision we sensibly add ELF metadata also for common_interrupt? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |