Re: [PATCH] x86/livepatch: enable livepatching assembly source files

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Apr 18, 2023 at 01:00:53PM +0200, Jan Beulich wrote:
> 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?

Oh, I see, livepatch uses a relative JMP, so that's not affected.

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

This is all fine, as long as they are not in the same translation
unit.  Livepatch creation operates against object files, so as long as
there are no section names clashes in a translation unit it should be
able to deal with it.

> > +  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've just put them together to the ENTRY() macros, but yes, I see no
reason to not move them to 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.)

I see, so that we could also use it for debug purposes.  I guess at
that point it might be better to use {START,END}_FUNC() to note that
the macros also have an effect beyond that of livepatching.

Maybe also introduce a START_ENTRY() that replaces ENTRY()?  Albeit I
find START_ENTRY a weird name.

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

So I take that overall you would agree to adding this extra
information using a pair of macros similar to the proposed ones.

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

That was done as to avoid patching the first part of the function that
does the speculative mitigations, but since the jmp introduced by
livepatch is a relative one we are safe and could indeed patch the
whole function.

Thanks, Roger.