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

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

[Pawan Gupta]

On Fri, Oct 27, 2023 at 05:32:03PM +0200, Borislav Petkov wrote:
> On Fri, Oct 27, 2023 at 07:38:40AM -0700, Pawan Gupta wrote:
> >   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
> 
> Some leftover '('

Ok.

> > 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_CLEAR_CPU_BUF.
> 
> The macros don't depend on the feature flag - VERW patching is done
> based on it.

Will fix.

> > @@ -20,3 +23,17 @@ SYM_FUNC_END(entry_ibpb)
> >  EXPORT_SYMBOL_GPL(entry_ibpb);
> >  
> >  .popsection
> > +
> > +.pushsection .entry.text, "ax"
> > +
> > +.align L1_CACHE_BYTES, 0xcc
> > +SYM_CODE_START_NOALIGN(mds_verw_sel)
> 
> That weird thing needs a comment explaining what it is for.

Right.

> > +#define X86_FEATURE_CLEAR_CPU_BUF	(11*32+27) /* "" Clear CPU buffers */
> 
> 									   ... using VERW

Ok.

> > +/*
> > + * Macros to execute VERW instruction that mitigate transient data sampling
> > + * attacks such as MDS. On affected systems a microcode update overloaded VERW
> > + * instruction to also clear the CPU buffers. VERW clobbers CFLAGS.ZF.
> > + *
> > + * Note: Only the memory operand variant of VERW clears the CPU buffers.
> > + */
> > +.macro EXEC_VERW
> > +	verw _ASM_RIP(mds_verw_sel)
> > +.endm
> > +
> > +.macro CLEAR_CPU_BUFFERS
> > +	ALTERNATIVE "", __stringify(EXEC_VERW), X86_FEATURE_CLEAR_CPU_BUF
> > +.endm
> 
> Why can't this simply be:
> 
> .macro CLEAR_CPU_BUFFERS
>         ALTERNATIVE "", "verw mds_verw_sel(%rip)", X86_FEATURE_CLEAR_CPU_BUF

This will not work in 32-bit mode that uses the same macro.

Thanks for the review.

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

[Josh Poimboeuf]

On Fri, Oct 27, 2023 at 07:38:40AM -0700, Pawan Gupta wrote:
> +.pushsection .entry.text, "ax"
> +
> +.align L1_CACHE_BYTES, 0xcc
> +SYM_CODE_START_NOALIGN(mds_verw_sel)
> +	UNWIND_HINT_UNDEFINED
> +	ANNOTATE_NOENDBR
> +	.word __KERNEL_DS
> +.align L1_CACHE_BYTES, 0xcc
> +SYM_CODE_END(mds_verw_sel);
> +/* For KVM */
> +EXPORT_SYMBOL_GPL(mds_verw_sel);
> +
> +.popsection

This is data, so why is it "CODE" in .entry.text?

-- 
Josh

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

[Andrew Cooper]

On 01/12/2023 7:36 pm, Josh Poimboeuf wrote:
> On Fri, Oct 27, 2023 at 07:38:40AM -0700, Pawan Gupta wrote:
>> +.pushsection .entry.text, "ax"
>> +
>> +.align L1_CACHE_BYTES, 0xcc
>> +SYM_CODE_START_NOALIGN(mds_verw_sel)
>> +	UNWIND_HINT_UNDEFINED
>> +	ANNOTATE_NOENDBR
>> +	.word __KERNEL_DS
>> +.align L1_CACHE_BYTES, 0xcc
>> +SYM_CODE_END(mds_verw_sel);
>> +/* For KVM */
>> +EXPORT_SYMBOL_GPL(mds_verw_sel);
>> +
>> +.popsection
> This is data, so why is it "CODE" in .entry.text?

Because KPTI.

~Andrew

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

[Josh Poimboeuf]

On Fri, Dec 01, 2023 at 07:39:05PM +0000, Andrew Cooper wrote:
> On 01/12/2023 7:36 pm, Josh Poimboeuf wrote:
> > On Fri, Oct 27, 2023 at 07:38:40AM -0700, Pawan Gupta wrote:
> >> +.pushsection .entry.text, "ax"
> >> +
> >> +.align L1_CACHE_BYTES, 0xcc
> >> +SYM_CODE_START_NOALIGN(mds_verw_sel)
> >> +	UNWIND_HINT_UNDEFINED
> >> +	ANNOTATE_NOENDBR
> >> +	.word __KERNEL_DS
> >> +.align L1_CACHE_BYTES, 0xcc
> >> +SYM_CODE_END(mds_verw_sel);
> >> +/* For KVM */
> >> +EXPORT_SYMBOL_GPL(mds_verw_sel);
> >> +
> >> +.popsection
> > This is data, so why is it "CODE" in .entry.text?
> 
> Because KPTI.

Urgh... Pawan please add a comment.

-- 
Josh

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

[Pawan Gupta]

On Fri, Dec 01, 2023 at 12:04:42PM -0800, Josh Poimboeuf wrote:
> On Fri, Dec 01, 2023 at 07:39:05PM +0000, Andrew Cooper wrote:
> > On 01/12/2023 7:36 pm, Josh Poimboeuf wrote:
> > > On Fri, Oct 27, 2023 at 07:38:40AM -0700, Pawan Gupta wrote:
> > >> +.pushsection .entry.text, "ax"
> > >> +
> > >> +.align L1_CACHE_BYTES, 0xcc
> > >> +SYM_CODE_START_NOALIGN(mds_verw_sel)
> > >> +	UNWIND_HINT_UNDEFINED
> > >> +	ANNOTATE_NOENDBR
> > >> +	.word __KERNEL_DS
> > >> +.align L1_CACHE_BYTES, 0xcc
> > >> +SYM_CODE_END(mds_verw_sel);
> > >> +/* For KVM */
> > >> +EXPORT_SYMBOL_GPL(mds_verw_sel);
> > >> +
> > >> +.popsection
> > > This is data, so why is it "CODE" in .entry.text?
> > 
> > Because KPTI.
> 
> Urgh... Pawan please add a comment.

Yes, this place needs a comment, will add.

Re: [PATCH v4 4/6] x86/bugs: Use ALTERNATIVE() instead of mds_user_clear static key

[Josh Poimboeuf]

On Fri, Oct 27, 2023 at 07:38:59AM -0700, Pawan Gupta wrote:
> The VERW mitigation at exit-to-user is enabled via a static branch
> mds_user_clear. This static branch is never toggled after boot, and can
> be safely replaced with an ALTERNATIVE() which is convenient to use in
> asm.
> 
> Switch to ALTERNATIVE() to use the VERW mitigation late in exit-to-user
> path. Also remove the now redundant VERW in exc_nmi() and
> arch_exit_to_user_mode().
> 
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  Documentation/arch/x86/mds.rst       | 38 +++++++++++++++++++++++++-----------
>  arch/x86/include/asm/entry-common.h  |  1 -
>  arch/x86/include/asm/nospec-branch.h | 12 ------------
>  arch/x86/kernel/cpu/bugs.c           | 15 ++++++--------
>  arch/x86/kernel/nmi.c                |  2 --
>  arch/x86/kvm/vmx/vmx.c               |  2 +-
>  6 files changed, 34 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/arch/x86/mds.rst b/Documentation/arch/x86/mds.rst
> index e73fdff62c0a..a5c5091b9ccd 100644
> --- a/Documentation/arch/x86/mds.rst
> +++ b/Documentation/arch/x86/mds.rst
> @@ -95,6 +95,9 @@ The kernel provides a function to invoke the buffer clearing:
>  
>      mds_clear_cpu_buffers()
>  
> +Also macro CLEAR_CPU_BUFFERS is meant to be used in ASM late in exit-to-user
> +path. This macro works for cases where GPRs can't be clobbered.

What does this last sentence mean?  Is it trying to say that the macro
doesn't clobber registers (other than ZF)?

-- 
Josh

Re: [PATCH v4 4/6] x86/bugs: Use ALTERNATIVE() instead of mds_user_clear static key

[Pawan Gupta]

On Fri, Dec 01, 2023 at 11:59:54AM -0800, Josh Poimboeuf wrote:
> On Fri, Oct 27, 2023 at 07:38:59AM -0700, Pawan Gupta wrote:
> > The VERW mitigation at exit-to-user is enabled via a static branch
> > mds_user_clear. This static branch is never toggled after boot, and can
> > be safely replaced with an ALTERNATIVE() which is convenient to use in
> > asm.
> > 
> > Switch to ALTERNATIVE() to use the VERW mitigation late in exit-to-user
> > path. Also remove the now redundant VERW in exc_nmi() and
> > arch_exit_to_user_mode().
> > 
> > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > ---
> >  Documentation/arch/x86/mds.rst       | 38 +++++++++++++++++++++++++-----------
> >  arch/x86/include/asm/entry-common.h  |  1 -
> >  arch/x86/include/asm/nospec-branch.h | 12 ------------
> >  arch/x86/kernel/cpu/bugs.c           | 15 ++++++--------
> >  arch/x86/kernel/nmi.c                |  2 --
> >  arch/x86/kvm/vmx/vmx.c               |  2 +-
> >  6 files changed, 34 insertions(+), 36 deletions(-)
> > 
> > diff --git a/Documentation/arch/x86/mds.rst b/Documentation/arch/x86/mds.rst
> > index e73fdff62c0a..a5c5091b9ccd 100644
> > --- a/Documentation/arch/x86/mds.rst
> > +++ b/Documentation/arch/x86/mds.rst
> > @@ -95,6 +95,9 @@ The kernel provides a function to invoke the buffer clearing:
> >  
> >      mds_clear_cpu_buffers()
> >  
> > +Also macro CLEAR_CPU_BUFFERS is meant to be used in ASM late in exit-to-user
> > +path. This macro works for cases where GPRs can't be clobbered.
> 
> What does this last sentence mean?  Is it trying to say that the macro
> doesn't clobber registers (other than ZF)?

Yes. I will rephrase it to say that macro doesn't clobber registers
other than ZF.

Re: [PATCH v4 6/6] KVM: VMX: Move VERW closer to VMentry for MDS mitigation

[Josh Poimboeuf]

On Fri, Oct 27, 2023 at 07:39:12AM -0700, Pawan Gupta wrote:
> -	vmx_disable_fb_clear(vmx);
> +	/*
> +	 * Optimize the latency of VERW in guests for MMIO mitigation. Skip
> +	 * the optimization when MDS mitigation(later in asm) is enabled.
> +	 */
> +	if (!cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF))
> +		vmx_disable_fb_clear(vmx);
>  
>  	if (vcpu->arch.cr2 != native_read_cr2())
>  		native_write_cr2(vcpu->arch.cr2);
> @@ -7248,7 +7256,8 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
>  
>  	vmx->idt_vectoring_info = 0;
>  
> -	vmx_enable_fb_clear(vmx);
> +	if (!cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF))
> +		vmx_enable_fb_clear(vmx);
>  

It may be cleaner to instead check X86_FEATURE_CLEAR_CPU_BUF when
setting vmx->disable_fb_clear in the first place, in
vmx_update_fb_clear_dis().

-- 
Josh

Re: [PATCH v4 6/6] KVM: VMX: Move VERW closer to VMentry for MDS mitigation

[Pawan Gupta]

On Fri, Dec 01, 2023 at 12:02:47PM -0800, Josh Poimboeuf wrote:
> On Fri, Oct 27, 2023 at 07:39:12AM -0700, Pawan Gupta wrote:
> > -	vmx_disable_fb_clear(vmx);
> > +	/*
> > +	 * Optimize the latency of VERW in guests for MMIO mitigation. Skip
> > +	 * the optimization when MDS mitigation(later in asm) is enabled.
> > +	 */
> > +	if (!cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF))
> > +		vmx_disable_fb_clear(vmx);
> >  
> >  	if (vcpu->arch.cr2 != native_read_cr2())
> >  		native_write_cr2(vcpu->arch.cr2);
> > @@ -7248,7 +7256,8 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
> >  
> >  	vmx->idt_vectoring_info = 0;
> >  
> > -	vmx_enable_fb_clear(vmx);
> > +	if (!cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF))
> > +		vmx_enable_fb_clear(vmx);
> >  
> 
> It may be cleaner to instead check X86_FEATURE_CLEAR_CPU_BUF when
> setting vmx->disable_fb_clear in the first place, in
> vmx_update_fb_clear_dis().

Right. Thanks for the review.