[1/6] x86/bugs: Add asm helpers for executing VERW

  MDS mitigation requires clearing the CPU buffers before returning to
user. This needs to be done late in the exit-to-user path. Current
location of VERW leaves a possibility of kernel data ending up in CPU
buffers for memory accesses done after VERW such as:

  1. Kernel data accessed by an NMI between VERW and return-to-user can
     remain in CPU buffers ( since NMI returning to kernel does not
     execute VERW to clear CPU buffers.
  2. Alyssa reported that after VERW is executed,
     CONFIG_GCC_PLUGIN_STACKLEAK=y scrubs the stack used by a system
     call. Memory accesses during stack scrubbing can move kernel stack
     contents into CPU buffers.
  3. When caller saved registers are restored after a return from
     function executing VERW, the kernel stack accesses can remain in
     CPU buffers(since they occur after VERW).

To fix this VERW needs to be moved very late in exit-to-user path.

In preparation for moving VERW to entry/exit asm code, create macros
that can be used in asm. Also make them depend on a new feature flag
X86_FEATURE_USER_CLEAR_CPU_BUF.

Reported-by: Alyssa Milburn <alyssa.milburn@intel.com>
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h   |  2 +-
 arch/x86/include/asm/nospec-branch.h | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)
  

On Fri, Oct 20, 2023, Pawan Gupta wrote:
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index c55cc243592e..e1b623a27e1b 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -111,6 +111,24 @@
>  #define RESET_CALL_DEPTH_FROM_CALL
>  #endif
>  
> +/*
> + * Macro to execute VERW instruction to mitigate transient data sampling
> + * attacks such as MDS. On affected systems a microcode update overloaded VERW
> + * instruction to also clear the CPU buffers.
> + *
> + * Note: Only the memory operand variant of VERW clears the CPU buffers. To
> + * handle the case when VERW is executed after user registers are restored, use
> + * RIP to point the memory operand to a part NOPL instruction that contains
> + * __KERNEL_DS.
> + */
> +#define __EXEC_VERW(m)	verw _ASM_RIP(m)
> +
> +#define EXEC_VERW				\
> +	__EXEC_VERW(551f);			\
> +	/* nopl __KERNEL_DS(%rax) */		\
> +	.byte 0x0f, 0x1f, 0x80, 0x00, 0x00;	\
> +551:	.word __KERNEL_DS;			\

Why are there so many macro layers?  Nothing jumps out to justfying two layers,
let alone three.

> +
>  /*
>   * Fill the CPU return stack buffer.
>   *
> @@ -329,6 +347,13 @@
>  #endif
>  .endm
>  
> +/* Clear CPU buffers before returning to user */
> +.macro USER_CLEAR_CPU_BUFFERS
> +	ALTERNATIVE "jmp .Lskip_verw_\@;", "", X86_FEATURE_USER_CLEAR_CPU_BUF
> +	EXEC_VERW

Rather than a NOP after VERW, why not something like this?

/* Clear CPU buffers before returning to user */
.macro USER_CLEAR_CPU_BUFFERS
                ALTERNATIVE "jmp .Lskip_verw_\@;", "jmp .Ldo_verw_\@;", X86_FEATURE_USER_CLEAR_CPU_BUF
551:            .word __KERNEL_DS
.Ldo_verw_\@:   verw _ASM_RIP(551b)
.Lskip_verw_\@:
.endm

> +.Lskip_verw_\@:
> +.endm
  

On 20/10/2023 9:44 pm, Pawan Gupta wrote:
> +#define EXEC_VERW				\
> +	__EXEC_VERW(551f);			\
> +	/* nopl __KERNEL_DS(%rax) */		\
> +	.byte 0x0f, 0x1f, 0x80, 0x00, 0x00;	\
> +551:	.word __KERNEL_DS;			\

Is this actually wise from a perf point of view?

You're causing a data access to the instruction stream, and not only
that, the immediate next instruction.  Some parts don't take kindly to
snoops hitting L1I.

A better option would be to simply have

.section .text.entry
.align CACHELINE
mds_verw_sel:
    .word __KERNEL_DS
    int3
.align CACHELINE


And then just have EXEC_VERW be

    verw mds_verw_sel(%rip)

in the fastpaths.  That keeps the memory operand in .text.entry it works
on Meltdown-vulnerable CPUs, but creates effectively a data cacheline
that isn't mixed into anywhere in the frontend, which also gets far
better locality of reference rather than having it duplicated in 9
different places.

Also it avoids playing games with hiding data inside an instruction.
It's a neat trick, but the neater trick is avoid it whenever possible.

~Andrew
  

On Fri, Oct 20, 2023 at 04:13:13PM -0700, Sean Christopherson wrote:
> On Fri, Oct 20, 2023, Pawan Gupta wrote:
> > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> > index c55cc243592e..e1b623a27e1b 100644
> > --- a/arch/x86/include/asm/nospec-branch.h
> > +++ b/arch/x86/include/asm/nospec-branch.h
> > @@ -111,6 +111,24 @@
> >  #define RESET_CALL_DEPTH_FROM_CALL
> >  #endif
> >  
> > +/*
> > + * Macro to execute VERW instruction to mitigate transient data sampling
> > + * attacks such as MDS. On affected systems a microcode update overloaded VERW
> > + * instruction to also clear the CPU buffers.
> > + *
> > + * Note: Only the memory operand variant of VERW clears the CPU buffers. To
> > + * handle the case when VERW is executed after user registers are restored, use
> > + * RIP to point the memory operand to a part NOPL instruction that contains
> > + * __KERNEL_DS.
> > + */
> > +#define __EXEC_VERW(m)	verw _ASM_RIP(m)
> > +
> > +#define EXEC_VERW				\
> > +	__EXEC_VERW(551f);			\
> > +	/* nopl __KERNEL_DS(%rax) */		\
> > +	.byte 0x0f, 0x1f, 0x80, 0x00, 0x00;	\
> > +551:	.word __KERNEL_DS;			\
> 
> Why are there so many macro layers?  Nothing jumps out to justfying two layers,
> let alone three.

I can't remember the exact reason, but I think the reason I added
__EXEC_VERW() was because using EXEC_VERW() in a C function was leading
to build error in the internal draft version. This version is not
calling it from C, so yes I can get rid of the extra layer.

> >  /*
> >   * Fill the CPU return stack buffer.
> >   *
> > @@ -329,6 +347,13 @@
> >  #endif
> >  .endm
> >  
> > +/* Clear CPU buffers before returning to user */
> > +.macro USER_CLEAR_CPU_BUFFERS
> > +	ALTERNATIVE "jmp .Lskip_verw_\@;", "", X86_FEATURE_USER_CLEAR_CPU_BUF
> > +	EXEC_VERW
> 
> Rather than a NOP after VERW, why not something like this?
> 
> /* Clear CPU buffers before returning to user */
> .macro USER_CLEAR_CPU_BUFFERS
>                 ALTERNATIVE "jmp .Lskip_verw_\@;", "jmp .Ldo_verw_\@;", X86_FEATURE_USER_CLEAR_CPU_BUF
> 551:            .word __KERNEL_DS
> .Ldo_verw_\@:   verw _ASM_RIP(551b)
> .Lskip_verw_\@:
> .endm

I wasn't comfortable adding a variable directly in the instruction
stream because the CPU may interpret it wrongly. With NOP it is bound to
ignore the data part.
  

On Sat, Oct 21, 2023 at 12:55:45AM +0100, Andrew Cooper wrote:
> On 20/10/2023 9:44 pm, Pawan Gupta wrote:
> > +#define EXEC_VERW				\
> > +	__EXEC_VERW(551f);			\
> > +	/* nopl __KERNEL_DS(%rax) */		\
> > +	.byte 0x0f, 0x1f, 0x80, 0x00, 0x00;	\
> > +551:	.word __KERNEL_DS;			\
> 
> Is this actually wise from a perf point of view?
> 
> You're causing a data access to the instruction stream, and not only
> that, the immediate next instruction.  Some parts don't take kindly to
> snoops hitting L1I.

I suspected the same and asked CPU architects, they did not anticipate
reads being interpreted as part of self modifying code. The perf numbers
do not indicate a problem, but they dont speak for all the parts. It
could be an issue with some parts.

> A better option would be to simply have
> 
> .section .text.entry
> .align CACHELINE
> mds_verw_sel:
>     .word __KERNEL_DS
>     int3
> .align CACHELINE
> 
> 
> And then just have EXEC_VERW be
> 
>     verw mds_verw_sel(%rip)
> 
> in the fastpaths.  That keeps the memory operand in .text.entry it works
> on Meltdown-vulnerable CPUs, but creates effectively a data cacheline
> that isn't mixed into anywhere in the frontend, which also gets far
> better locality of reference rather than having it duplicated in 9
> different places.

> Also it avoids playing games with hiding data inside an instruction.
> It's a neat trick, but the neater trick is avoid it whenever possible.

Thanks for the pointers. I think verw in 32-bit mode won't be able to
address the operand outside of 4GB range. Maybe this is fine or could it
be a problem addressing from e.g. KVM module?
  

On 21/10/2023 2:18 am, Pawan Gupta wrote:
> On Sat, Oct 21, 2023 at 12:55:45AM +0100, Andrew Cooper wrote:
>> Also it avoids playing games with hiding data inside an instruction.
>> It's a neat trick, but the neater trick is avoid it whenever possible.
> Thanks for the pointers. I think verw in 32-bit mode won't be able to
> address the operand outside of 4GB range.

And?  In a 32bit kernel, what lives outside of a 4G range?

> Maybe this is fine or could it
> be a problem addressing from e.g. KVM module?

RIP-relative addressing is disp32.  Which is the same as it is for
direct calls.

So if your module is far enough away for VERW to have issues, you've got
far more basic problems to solve first.

~Andrew
  

On Sat, Oct 21, 2023 at 02:33:47AM +0100, Andrew Cooper wrote:
> On 21/10/2023 2:18 am, Pawan Gupta wrote:
> > On Sat, Oct 21, 2023 at 12:55:45AM +0100, Andrew Cooper wrote:
> >> Also it avoids playing games with hiding data inside an instruction.
> >> It's a neat trick, but the neater trick is avoid it whenever possible.
> > Thanks for the pointers. I think verw in 32-bit mode won't be able to
> > address the operand outside of 4GB range.
> 
> And?  In a 32bit kernel, what lives outside of a 4G range?
> 
> > Maybe this is fine or could it
> > be a problem addressing from e.g. KVM module?
> 
> RIP-relative addressing is disp32.  Which is the same as it is for
> direct calls.
> 
> So if your module is far enough away for VERW to have issues, you've got
> far more basic problems to solve first.

Sorry, I raised the wrong problem. In 64-bit mode, verww only has 32-bit
of relative addressing, so memory operand has to be within 4GB of
callsite. That could be a constraint.
  

On Sat, Oct 21, 2023 at 12:50:37AM +0100, Andrew Cooper wrote:
> On 20/10/2023 9:44 pm, Pawan Gupta wrote:
> > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> > index c55cc243592e..e1b623a27e1b 100644
> > --- a/arch/x86/include/asm/nospec-branch.h
> > +++ b/arch/x86/include/asm/nospec-branch.h
> > @@ -111,6 +111,24 @@
> >  #define RESET_CALL_DEPTH_FROM_CALL
> >  #endif
> >  
> > +/*
> > + * Macro to execute VERW instruction to mitigate transient data sampling
> > + * attacks such as MDS. On affected systems a microcode update overloaded VERW
> > + * instruction to also clear the CPU buffers.
> > + *
> > + * Note: Only the memory operand variant of VERW clears the CPU buffers. To
> > + * handle the case when VERW is executed after user registers are restored, use
> > + * RIP to point the memory operand to a part NOPL instruction that contains
> > + * __KERNEL_DS.
> > + */
> > +#define __EXEC_VERW(m)	verw _ASM_RIP(m)
> > +
> > +#define EXEC_VERW				\
> > +	__EXEC_VERW(551f);			\
> > +	/* nopl __KERNEL_DS(%rax) */		\
> > +	.byte 0x0f, 0x1f, 0x80, 0x00, 0x00;	\
> > +551:	.word __KERNEL_DS;			\
> > +
> 
> Is this actually wise from a perf point of view?
> 
> You're causing a data access to the instruction stream, and not only
> that, the immediate next instruction.  Some parts don't take kindly to
> snoops hitting L1I.
> 
> A better option would be to simply have
> 
> .section .text.entry
> .align CACHELINE
> mds_verw_sel:
>     .word __KERNEL_DS
>     int3
> .align CACHELINE
> 
> 
> And then just have EXEC_VERW be
> 
>     verw mds_verw_sel(%rip)

	ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_USER_CLEAR_CPU_BUF

But yeah, his seems like the sanest form.
  

On Fri, Oct 20, 2023 at 07:21:34PM -0700, Pawan Gupta wrote:
> On Sat, Oct 21, 2023 at 02:33:47AM +0100, Andrew Cooper wrote:
> > On 21/10/2023 2:18 am, Pawan Gupta wrote:
> > > On Sat, Oct 21, 2023 at 12:55:45AM +0100, Andrew Cooper wrote:
> > >> Also it avoids playing games with hiding data inside an instruction.
> > >> It's a neat trick, but the neater trick is avoid it whenever possible.
> > > Thanks for the pointers. I think verw in 32-bit mode won't be able to
> > > address the operand outside of 4GB range.
> > 
> > And?  In a 32bit kernel, what lives outside of a 4G range?
> > 
> > > Maybe this is fine or could it
> > > be a problem addressing from e.g. KVM module?
> > 
> > RIP-relative addressing is disp32.  Which is the same as it is for
> > direct calls.
> > 
> > So if your module is far enough away for VERW to have issues, you've got
> > far more basic problems to solve first.
> 
> Sorry, I raised the wrong problem. In 64-bit mode, verww only has 32-bit
> of relative addressing, so memory operand has to be within 4GB of
> callsite. That could be a constraint.

Even on x86-64, modules are mapped within 4GB of the kernel, so I don't
think that's a concern.
  

On Mon, Oct 23, 2023 at 11:08:06AM -0700, Josh Poimboeuf wrote:
> On Fri, Oct 20, 2023 at 07:21:34PM -0700, Pawan Gupta wrote:
> > On Sat, Oct 21, 2023 at 02:33:47AM +0100, Andrew Cooper wrote:
> > > On 21/10/2023 2:18 am, Pawan Gupta wrote:
> > > > On Sat, Oct 21, 2023 at 12:55:45AM +0100, Andrew Cooper wrote:
> > > >> Also it avoids playing games with hiding data inside an instruction.
> > > >> It's a neat trick, but the neater trick is avoid it whenever possible.
> > > > Thanks for the pointers. I think verw in 32-bit mode won't be able to
> > > > address the operand outside of 4GB range.
> > > 
> > > And?  In a 32bit kernel, what lives outside of a 4G range?
> > > 
> > > > Maybe this is fine or could it
> > > > be a problem addressing from e.g. KVM module?
> > > 
> > > RIP-relative addressing is disp32.  Which is the same as it is for
> > > direct calls.
> > > 
> > > So if your module is far enough away for VERW to have issues, you've got
> > > far more basic problems to solve first.
> > 
> > Sorry, I raised the wrong problem. In 64-bit mode, verww only has 32-bit
> > of relative addressing, so memory operand has to be within 4GB of
> > callsite. That could be a constraint.
> 
> Even on x86-64, modules are mapped within 4GB of the kernel, so I don't
> think that's a concern.

You are correct, modules are indeed mapped within 4GB of the kernel. So
what Andrew suggested is feasible. Is that your preference?
  

On Sat, Oct 21, 2023 at 12:55:45AM +0100, Andrew Cooper wrote:
> On 20/10/2023 9:44 pm, Pawan Gupta wrote:
> > +#define EXEC_VERW				\
> > +	__EXEC_VERW(551f);			\
> > +	/* nopl __KERNEL_DS(%rax) */		\
> > +	.byte 0x0f, 0x1f, 0x80, 0x00, 0x00;	\
> > +551:	.word __KERNEL_DS;			\
> 
> Is this actually wise from a perf point of view?
> 
> You're causing a data access to the instruction stream, and not only
> that, the immediate next instruction.  Some parts don't take kindly to
> snoops hitting L1I.
> 
> A better option would be to simply have
> 
> .section .text.entry
> .align CACHELINE
> mds_verw_sel:
>     .word __KERNEL_DS
>     int3
> .align CACHELINE
> 
> 
> And then just have EXEC_VERW be
> 
>     verw mds_verw_sel(%rip)
> 
> in the fastpaths.  That keeps the memory operand in .text.entry it works
> on Meltdown-vulnerable CPUs, but creates effectively a data cacheline
> that isn't mixed into anywhere in the frontend, which also gets far
> better locality of reference.

With .text.entry section I am getting getting below warnings and an
error:

-----------------------------------------------------------------
    LD      vmlinux.o
  vmlinux.o: warning: objtool: .text.entry+0x0: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x40: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x80: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0xc0: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x100: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x140: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x180: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x1c0: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x200: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x240: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x280: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x2c0: unreachable instruction
  vmlinux.o: warning: objtool: .text.entry+0x300: unreachable instruction
  vmlinux.o: warning: objtool: .altinstr_replacement+0x2c: relocation to !ENDBR: .text.entry+0x0
  vmlinux.o: warning: objtool: .altinstr_replacement+0x1c4: relocation to !ENDBR: .text.entry+0x0
  vmlinux.o: warning: objtool: .altinstr_replacement+0x1d0: relocation to !ENDBR: .text.entry+0x0
  vmlinux.o: warning: objtool: .altinstr_replacement+0x2d2: relocation to !ENDBR: .text.entry+0x80
  vmlinux.o: warning: objtool: .altinstr_replacement+0x5d5: relocation to !ENDBR: .text.entry+0xc0
    OBJCOPY modules.builtin.modinfo
    GEN     modules.builtin
    MODPOST vmlinux.symvers
    UPD     include/generated/utsversion.h
    CC      init/version-timestamp.o
    LD      .tmp_vmlinux.kallsyms1
  ld: error: unplaced orphan section `.text.entry' from `vmlinux.o'
  make[2]: *** [scripts/Makefile.vmlinux:36: vmlinux] Error 1
-----------------------------------------------------------------

... because my config has CONFIG_LD_ORPHAN_WARN_LEVEL="error" and
objtool needs to be told about this entry.

Do you think its worth fighting these warnings and error, or simply use
.rodata section for verw memory operand?
  

On Tue, Oct 24, 2023 at 11:28:18PM -0700, Pawan Gupta wrote:

> With .text.entry section I am getting getting below warnings and an
> error:
> 
> -----------------------------------------------------------------
>     LD      vmlinux.o
>   vmlinux.o: warning: objtool: .text.entry+0x0: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x40: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x80: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0xc0: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x100: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x140: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x180: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x1c0: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x200: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x240: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x280: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x2c0: unreachable instruction
>   vmlinux.o: warning: objtool: .text.entry+0x300: unreachable instruction
>   vmlinux.o: warning: objtool: .altinstr_replacement+0x2c: relocation to !ENDBR: .text.entry+0x0
>   vmlinux.o: warning: objtool: .altinstr_replacement+0x1c4: relocation to !ENDBR: .text.entry+0x0
>   vmlinux.o: warning: objtool: .altinstr_replacement+0x1d0: relocation to !ENDBR: .text.entry+0x0
>   vmlinux.o: warning: objtool: .altinstr_replacement+0x2d2: relocation to !ENDBR: .text.entry+0x80
>   vmlinux.o: warning: objtool: .altinstr_replacement+0x5d5: relocation to !ENDBR: .text.entry+0xc0
>     OBJCOPY modules.builtin.modinfo
>     GEN     modules.builtin
>     MODPOST vmlinux.symvers
>     UPD     include/generated/utsversion.h
>     CC      init/version-timestamp.o
>     LD      .tmp_vmlinux.kallsyms1
>   ld: error: unplaced orphan section `.text.entry' from `vmlinux.o'
>   make[2]: *** [scripts/Makefile.vmlinux:36: vmlinux] Error 1
> -----------------------------------------------------------------
> 
> ... because my config has CONFIG_LD_ORPHAN_WARN_LEVEL="error" and
> objtool needs to be told about this entry.
> 
> Do you think its worth fighting these warnings and error, or simply use
> .rodata section for verw memory operand?

I'm thinking you need to at the very least stay in a section that's
actually still mapped with PTI :-)

.entry.text is the only thing that is reliably mapped with PTI (IIRC),
some configs also get a chunk of the kernel image, but not all.

Something like the below should do I suppose.

---
 arch/x86/entry/entry.S | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S
index bfb7bcb362bc..9eb2b532c92a 100644
--- a/arch/x86/entry/entry.S
+++ b/arch/x86/entry/entry.S
@@ -6,6 +6,8 @@
 #include <linux/linkage.h>
 #include <asm/export.h>
 #include <asm/msr-index.h>
+#include <asm/unwind_hints.h>
+#include <asm/segment.h>
 
 .pushsection .noinstr.text, "ax"
 
@@ -20,3 +22,16 @@ SYM_FUNC_END(entry_ibpb)
 EXPORT_SYMBOL_GPL(entry_ibpb);
 
 .popsection
+
+.pushsection .entry.text, "ax"
+
+.align 64
+SYM_CODE_START_NOALIGN(mds_verw_sel)
+	UNWIND_HINT_UNDEFINED
+	ANNOTATE_NOENDBR
+1:
+	.word __KERNEL_DS
+	.skip 64 - (. - 1b), 0xcc
+SYM_CODE_END(mds_verw_sel);
+
+.popsection
  

On 25/10/2023 8:22 am, Peter Zijlstra wrote:
> On Tue, Oct 24, 2023 at 11:28:18PM -0700, Pawan Gupta wrote:
>
>> With .text.entry section I am getting getting below warnings and an
>> error:
>>
>> -----------------------------------------------------------------
>>     LD      vmlinux.o
>>   vmlinux.o: warning: objtool: .text.entry+0x0: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x40: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x80: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0xc0: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x100: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x140: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x180: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x1c0: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x200: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x240: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x280: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x2c0: unreachable instruction
>>   vmlinux.o: warning: objtool: .text.entry+0x300: unreachable instruction
>>   vmlinux.o: warning: objtool: .altinstr_replacement+0x2c: relocation to !ENDBR: .text.entry+0x0
>>   vmlinux.o: warning: objtool: .altinstr_replacement+0x1c4: relocation to !ENDBR: .text.entry+0x0
>>   vmlinux.o: warning: objtool: .altinstr_replacement+0x1d0: relocation to !ENDBR: .text.entry+0x0
>>   vmlinux.o: warning: objtool: .altinstr_replacement+0x2d2: relocation to !ENDBR: .text.entry+0x80
>>   vmlinux.o: warning: objtool: .altinstr_replacement+0x5d5: relocation to !ENDBR: .text.entry+0xc0
>>     OBJCOPY modules.builtin.modinfo
>>     GEN     modules.builtin
>>     MODPOST vmlinux.symvers
>>     UPD     include/generated/utsversion.h
>>     CC      init/version-timestamp.o
>>     LD      .tmp_vmlinux.kallsyms1
>>   ld: error: unplaced orphan section `.text.entry' from `vmlinux.o'
>>   make[2]: *** [scripts/Makefile.vmlinux:36: vmlinux] Error 1
>> -----------------------------------------------------------------
>>
>> ... because my config has CONFIG_LD_ORPHAN_WARN_LEVEL="error" and
>> objtool needs to be told about this entry.
>>
>> Do you think its worth fighting these warnings and error, or simply use
>> .rodata section for verw memory operand?
> I'm thinking you need to at the very least stay in a section that's
> actually still mapped with PTI :-)

Sorry.  Xen and Linux have this section named opposite ways around.

> diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S
> index bfb7bcb362bc..9eb2b532c92a 100644
> --- a/arch/x86/entry/entry.S
> +++ b/arch/x86/entry/entry.S
> @@ -20,3 +22,16 @@ SYM_FUNC_END(entry_ibpb)
>  EXPORT_SYMBOL_GPL(entry_ibpb);
>  
>  .popsection
> +
> +.pushsection .entry.text, "ax"
> +
> +.align 64
> +SYM_CODE_START_NOALIGN(mds_verw_sel)
> +	UNWIND_HINT_UNDEFINED
> +	ANNOTATE_NOENDBR
> +1:
> +	.word __KERNEL_DS
> +	.skip 64 - (. - 1b), 0xcc

The 1 label aliases mds_verw_sel and this must remain like this for the
construct to work.

So instead of .skip, why not simply .align 64, 0xcc and get rid of the
1: label?

Do we have a suitably named constant cacheline size, rather than
opencoding 64?

> +SYM_CODE_END(mds_verw_sel);

Given that KVM needs it, this probably needs an EXPORT_SYMBOL_GPL() on it.

~Andrew
  

On Wed, Oct 25, 2023 at 08:52:50AM +0100, Andrew Cooper wrote:

> > +.pushsection .entry.text, "ax"
> > +
> > +.align 64
> > +SYM_CODE_START_NOALIGN(mds_verw_sel)
> > +	UNWIND_HINT_UNDEFINED
> > +	ANNOTATE_NOENDBR
> > +1:
> > +	.word __KERNEL_DS
> > +	.skip 64 - (. - 1b), 0xcc
> 
> The 1 label aliases mds_verw_sel and this must remain like this for the
> construct to work.
> 
> So instead of .skip, why not simply .align 64, 0xcc and get rid of the
> 1: label?

Because I forgot you can add a filler byte to .align :/ Yes, that's much
saner.

> Do we have a suitably named constant cacheline size, rather than
> opencoding 64?

L1_CACHE_BYTES probably.

> 
> > +SYM_CODE_END(mds_verw_sel);
> 
> Given that KVM needs it, this probably needs an EXPORT_SYMBOL_GPL() on it.

localyesconfig ftw ;-)

/me runs
  

On Wed, Oct 25, 2023 at 09:22:55AM +0200, Peter Zijlstra wrote:
 
> I'm thinking you need to at the very least stay in a section that's
> actually still mapped with PTI :-)
> 
> .entry.text is the only thing that is reliably mapped with PTI (IIRC),
> some configs also get a chunk of the kernel image, but not all.
> 
> Something like the below should do I suppose.

Thanks, will do this with Andrew's improvements.

> ---
>  arch/x86/entry/entry.S | 15 +++++++++++++++