1. Patchew
  2. linux
  3. View series

[PATCH v3] x86/bugs: Only harden syscalls when needed

Josh Poimboeuf posted 1 patch 2 weeks, 1 day ago 
arch/x86/entry/common.c            | 15 ++++++++++++---
arch/x86/entry/syscall_32.c        | 11 +----------
arch/x86/entry/syscall_64.c        |  6 ------
arch/x86/entry/syscall_x32.c       |  7 ++++++-
arch/x86/include/asm/cpufeatures.h |  1 +
arch/x86/include/asm/syscall.h     |  8 +++++++-
arch/x86/kernel/cpu/bugs.c         | 31 +++++++++++++++++++++++++++++-
7 files changed, 57 insertions(+), 22 deletions(-)
[PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Josh Poimboeuf 2 weeks, 1 day ago 
Syscall hardening (i.e., converting the syscall indirect branch to a
series of direct branches) may cause performance regressions in certain
scenarios.  Only use the syscall hardening when indirect branches are
considered unsafe.

Fixes: 1e3ad78334a6 ("x86/syscall: Don't force use of indirect calls for system calls")
Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
v3:
- fix X86_FEATURE_INDIRECT_SAFE value

 arch/x86/entry/common.c            | 15 ++++++++++++---
 arch/x86/entry/syscall_32.c        | 11 +----------
 arch/x86/entry/syscall_64.c        |  6 ------
 arch/x86/entry/syscall_x32.c       |  7 ++++++-
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/syscall.h     |  8 +++++++-
 arch/x86/kernel/cpu/bugs.c         | 31 +++++++++++++++++++++++++++++-
 7 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6de50b80702e..c0f8116291e4 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -49,7 +49,10 @@ static __always_inline bool do_syscall_x64(struct pt_regs *regs, int nr)
 
 	if (likely(unr < NR_syscalls)) {
 		unr = array_index_nospec(unr, NR_syscalls);
-		regs->ax = x64_sys_call(regs, unr);
+		if (likely(cpu_feature_enabled(X86_FEATURE_INDIRECT_SAFE)))
+			regs->ax = sys_call_table[unr](regs);
+		else
+			regs->ax = x64_sys_call(regs, unr);
 		return true;
 	}
 	return false;
@@ -66,7 +69,10 @@ static __always_inline bool do_syscall_x32(struct pt_regs *regs, int nr)
 
 	if (IS_ENABLED(CONFIG_X86_X32_ABI) && likely(xnr < X32_NR_syscalls)) {
 		xnr = array_index_nospec(xnr, X32_NR_syscalls);
-		regs->ax = x32_sys_call(regs, xnr);
+		if (likely(cpu_feature_enabled(X86_FEATURE_INDIRECT_SAFE)))
+			regs->ax = x32_sys_call_table[xnr](regs);
+		else
+			regs->ax = x32_sys_call(regs, xnr);
 		return true;
 	}
 	return false;
@@ -162,7 +168,10 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs, int nr)
 
 	if (likely(unr < IA32_NR_syscalls)) {
 		unr = array_index_nospec(unr, IA32_NR_syscalls);
-		regs->ax = ia32_sys_call(regs, unr);
+		if (likely(cpu_feature_enabled(X86_FEATURE_INDIRECT_SAFE)))
+			regs->ax = ia32_sys_call_table[unr](regs);
+		else
+			regs->ax = ia32_sys_call(regs, unr);
 	} else if (nr != -1) {
 		regs->ax = __ia32_sys_ni_syscall(regs);
 	}
diff --git a/arch/x86/entry/syscall_32.c b/arch/x86/entry/syscall_32.c
index c2235bae17ef..aab31760b4e3 100644
--- a/arch/x86/entry/syscall_32.c
+++ b/arch/x86/entry/syscall_32.c
@@ -14,25 +14,16 @@
 #endif
 
 #define __SYSCALL(nr, sym) extern long __ia32_##sym(const struct pt_regs *);
-
 #include <asm/syscalls_32.h>
 #undef __SYSCALL
 
-/*
- * The sys_call_table[] is no longer used for system calls, but
- * kernel/trace/trace_syscalls.c still wants to know the system
- * call address.
- */
-#ifdef CONFIG_X86_32
 #define __SYSCALL(nr, sym) __ia32_##sym,
-const sys_call_ptr_t sys_call_table[] = {
+const sys_call_ptr_t ia32_sys_call_table[] = {
 #include <asm/syscalls_32.h>
 };
 #undef __SYSCALL
-#endif
 
 #define __SYSCALL(nr, sym) case nr: return __ia32_##sym(regs);
-
 long ia32_sys_call(const struct pt_regs *regs, unsigned int nr)
 {
 	switch (nr) {
diff --git a/arch/x86/entry/syscall_64.c b/arch/x86/entry/syscall_64.c
index 33b3f09e6f15..96ea1f8a1d3f 100644
--- a/arch/x86/entry/syscall_64.c
+++ b/arch/x86/entry/syscall_64.c
@@ -11,11 +11,6 @@
 #include <asm/syscalls_64.h>
 #undef __SYSCALL
 
-/*
- * The sys_call_table[] is no longer used for system calls, but
- * kernel/trace/trace_syscalls.c still wants to know the system
- * call address.
- */
 #define __SYSCALL(nr, sym) __x64_##sym,
 const sys_call_ptr_t sys_call_table[] = {
 #include <asm/syscalls_64.h>
@@ -23,7 +18,6 @@ const sys_call_ptr_t sys_call_table[] = {
 #undef __SYSCALL
 
 #define __SYSCALL(nr, sym) case nr: return __x64_##sym(regs);
-
 long x64_sys_call(const struct pt_regs *regs, unsigned int nr)
 {
 	switch (nr) {
diff --git a/arch/x86/entry/syscall_x32.c b/arch/x86/entry/syscall_x32.c
index 03de4a932131..5aef4230faca 100644
--- a/arch/x86/entry/syscall_x32.c
+++ b/arch/x86/entry/syscall_x32.c
@@ -11,8 +11,13 @@
 #include <asm/syscalls_x32.h>
 #undef __SYSCALL
 
-#define __SYSCALL(nr, sym) case nr: return __x64_##sym(regs);
+#define __SYSCALL(nr, sym) __x64_##sym,
+const sys_call_ptr_t x32_sys_call_table[] = {
+#include <asm/syscalls_x32.h>
+};
+#undef __SYSCALL
 
+#define __SYSCALL(nr, sym) case nr: return __x64_##sym(regs);
 long x32_sys_call(const struct pt_regs *regs, unsigned int nr)
 {
 	switch (nr) {
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 3c7434329661..f0ea66cbd5f1 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -470,6 +470,7 @@
 #define X86_FEATURE_BHI_CTRL		(21*32+ 2) /* "" BHI_DIS_S HW control available */
 #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* "" BHI_DIS_S HW control enabled */
 #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* "" Clear branch history at vmexit using SW loop */
+#define X86_FEATURE_INDIRECT_SAFE	(21*32+ 5) /* "" Indirect branches aren't vulnerable to Spectre v2 */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 2fc7bc3863ff..dfb59521244c 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -16,14 +16,20 @@
 #include <asm/thread_info.h>	/* for TS_COMPAT */
 #include <asm/unistd.h>
 
-/* This is used purely for kernel/trace/trace_syscalls.c */
 typedef long (*sys_call_ptr_t)(const struct pt_regs *);
 extern const sys_call_ptr_t sys_call_table[];
 
+#if defined(CONFIG_X86_32)
+#define ia32_sys_call_table sys_call_table
+#else
 /*
  * These may not exist, but still put the prototypes in so we
  * can use IS_ENABLED().
  */
+extern const sys_call_ptr_t ia32_sys_call_table[];
+extern const sys_call_ptr_t x32_sys_call_table[];
+#endif
+
 extern long ia32_sys_call(const struct pt_regs *, unsigned int nr);
 extern long x32_sys_call(const struct pt_regs *, unsigned int nr);
 extern long x64_sys_call(const struct pt_regs *, unsigned int nr);
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index ca295b0c1eee..dcb97cc2758f 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1664,6 +1664,15 @@ static void __init bhi_select_mitigation(void)
 	if (!IS_ENABLED(CONFIG_X86_64))
 		return;
 
+	/*
+	 * There's no hardware mitigation in place, so mark indirect branches
+	 * as unsafe.
+	 *
+	 * One could argue the SW loop makes indirect branches safe again, but
+	 * Linus prefers it this way.
+	 */
+	setup_clear_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
+
 	/* Mitigate KVM by default */
 	setup_force_cpu_cap(X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT);
 	pr_info("Spectre BHI mitigation: SW BHB clearing on vm exit\n");
@@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
 	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
 	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
 
+	/*
+	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
+	 * considered safe.  That means either:
+	 *
+	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
+	 *
+	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
+	 *
+	 *   - the user turned off mitigations altogether.
+	 *
+	 * Assume innocence until proven guilty: set the cap bit now, then
+	 * clear it later if/when needed.
+	 */
+	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
+
 	/*
 	 * If the CPU is not affected and the command line mode is NONE or AUTO
 	 * then nothing to do.
@@ -1764,11 +1788,16 @@ static void __init spectre_v2_select_mitigation(void)
 		break;
 
 	case SPECTRE_V2_LFENCE:
+		setup_clear_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
+		fallthrough;
 	case SPECTRE_V2_EIBRS_LFENCE:
 		setup_force_cpu_cap(X86_FEATURE_RETPOLINE_LFENCE);
-		fallthrough;
+		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
+		break;
 
 	case SPECTRE_V2_RETPOLINE:
+		setup_clear_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
+		fallthrough;
 	case SPECTRE_V2_EIBRS_RETPOLINE:
 		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
 		break;
-- 
2.44.0
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Christoph Hellwig 2 weeks ago 
On Tue, Apr 16, 2024 at 04:02:21PM -0700, Josh Poimboeuf wrote:
> Syscall hardening (i.e., converting the syscall indirect branch to a
> series of direct branches) may cause performance regressions in certain
> scenarios.

Maybe spell out the scenarios, as this just sounds like hand waiving
right now..
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Josh Poimboeuf 1 week, 6 days ago 
On Wed, Apr 17, 2024 at 11:50:55PM -0700, Christoph Hellwig wrote:
> On Tue, Apr 16, 2024 at 04:02:21PM -0700, Josh Poimboeuf wrote:
> > Syscall hardening (i.e., converting the syscall indirect branch to a
> > series of direct branches) may cause performance regressions in certain
> > scenarios.
> 
> Maybe spell out the scenarios, as this just sounds like hand waiving
> right now..

Will do.

-- 
Josh
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Andrew Cooper 2 weeks, 1 day ago 
On 17/04/2024 12:02 am, Josh Poimboeuf wrote:
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index ca295b0c1eee..dcb97cc2758f 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
>  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
>  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
>  
> +	/*
> +	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
> +	 * considered safe.  That means either:
> +	 *
> +	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
> +	 *
> +	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
> +	 *
> +	 *   - the user turned off mitigations altogether.
> +	 *
> +	 * Assume innocence until proven guilty: set the cap bit now, then
> +	 * clear it later if/when needed.
> +	 */
> +	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);

Following on from the (re)discovery that X86_FEATURE_RETPOLINE is a poor
name given what it *actually* does, can I recommend s/SAFE/OK/ here?

This flag really is "do I want indirect branches or not", which - as
noted here - is more than just a judgement of whether indirect branches
are "safe".

~Andrew
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Pawan Gupta 2 weeks, 1 day ago 
On Wed, Apr 17, 2024 at 04:14:26PM +0100, Andrew Cooper wrote:
> On 17/04/2024 12:02 am, Josh Poimboeuf wrote:
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index ca295b0c1eee..dcb97cc2758f 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
> >  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
> >  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
> >  
> > +	/*
> > +	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
> > +	 * considered safe.  That means either:
> > +	 *
> > +	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
> > +	 *
> > +	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
> > +	 *
> > +	 *   - the user turned off mitigations altogether.
> > +	 *
> > +	 * Assume innocence until proven guilty: set the cap bit now, then
> > +	 * clear it later if/when needed.
> > +	 */
> > +	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
> 
> Following on from the (re)discovery that X86_FEATURE_RETPOLINE is a poor
> name given what it *actually* does, can I recommend s/SAFE/OK/ here?

Or simply X86_FEATURE_USE_INDIRECT_BRANCH.

> This flag really is "do I want indirect branches or not", which - as
> noted here - is more than just a judgement of whether indirect branches
> are "safe".
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Josh Poimboeuf 2 weeks, 1 day ago 
On Wed, Apr 17, 2024 at 09:45:14AM -0700, Pawan Gupta wrote:
> On Wed, Apr 17, 2024 at 04:14:26PM +0100, Andrew Cooper wrote:
> > On 17/04/2024 12:02 am, Josh Poimboeuf wrote:
> > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > > index ca295b0c1eee..dcb97cc2758f 100644
> > > --- a/arch/x86/kernel/cpu/bugs.c
> > > +++ b/arch/x86/kernel/cpu/bugs.c
> > > @@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
> > >  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
> > >  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
> > >  
> > > +	/*
> > > +	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
> > > +	 * considered safe.  That means either:
> > > +	 *
> > > +	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
> > > +	 *
> > > +	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
> > > +	 *
> > > +	 *   - the user turned off mitigations altogether.
> > > +	 *
> > > +	 * Assume innocence until proven guilty: set the cap bit now, then
> > > +	 * clear it later if/when needed.
> > > +	 */
> > > +	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
> > 
> > Following on from the (re)discovery that X86_FEATURE_RETPOLINE is a poor
> > name given what it *actually* does, can I recommend s/SAFE/OK/ here?
> 
> Or simply X86_FEATURE_USE_INDIRECT_BRANCH.
> 
> > This flag really is "do I want indirect branches or not", which - as
> > noted here - is more than just a judgement of whether indirect branches
> > are "safe".

X86_FEATURE_USE_INDIRECT_BRANCH sounds good.  It's a bit long but does
describe it better.

-- 
Josh
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Andrew Cooper 2 weeks, 1 day ago 
On 17/04/2024 6:57 pm, Josh Poimboeuf wrote:
> On Wed, Apr 17, 2024 at 09:45:14AM -0700, Pawan Gupta wrote:
>> On Wed, Apr 17, 2024 at 04:14:26PM +0100, Andrew Cooper wrote:
>>> On 17/04/2024 12:02 am, Josh Poimboeuf wrote:
>>>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>>>> index ca295b0c1eee..dcb97cc2758f 100644
>>>> --- a/arch/x86/kernel/cpu/bugs.c
>>>> +++ b/arch/x86/kernel/cpu/bugs.c
>>>> @@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
>>>>  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
>>>>  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
>>>>  
>>>> +	/*
>>>> +	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
>>>> +	 * considered safe.  That means either:
>>>> +	 *
>>>> +	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
>>>> +	 *
>>>> +	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
>>>> +	 *
>>>> +	 *   - the user turned off mitigations altogether.
>>>> +	 *
>>>> +	 * Assume innocence until proven guilty: set the cap bit now, then
>>>> +	 * clear it later if/when needed.
>>>> +	 */
>>>> +	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
>>> Following on from the (re)discovery that X86_FEATURE_RETPOLINE is a poor
>>> name given what it *actually* does, can I recommend s/SAFE/OK/ here?
>> Or simply X86_FEATURE_USE_INDIRECT_BRANCH.
>>
>>> This flag really is "do I want indirect branches or not", which - as
>>> noted here - is more than just a judgement of whether indirect branches
>>> are "safe".
> X86_FEATURE_USE_INDIRECT_BRANCH sounds good.  It's a bit long but does
> describe it better.

Works for me.  Definitely an improvement over SAFE.

~Andrew
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Josh Poimboeuf 1 week, 6 days ago 
On Wed, Apr 17, 2024 at 07:01:54PM +0100, Andrew Cooper wrote:
> On 17/04/2024 6:57 pm, Josh Poimboeuf wrote:
> > On Wed, Apr 17, 2024 at 09:45:14AM -0700, Pawan Gupta wrote:
> >> On Wed, Apr 17, 2024 at 04:14:26PM +0100, Andrew Cooper wrote:
> >>> On 17/04/2024 12:02 am, Josh Poimboeuf wrote:
> >>>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> >>>> index ca295b0c1eee..dcb97cc2758f 100644
> >>>> --- a/arch/x86/kernel/cpu/bugs.c
> >>>> +++ b/arch/x86/kernel/cpu/bugs.c
> >>>> @@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
> >>>>  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
> >>>>  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
> >>>>  
> >>>> +	/*
> >>>> +	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
> >>>> +	 * considered safe.  That means either:
> >>>> +	 *
> >>>> +	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
> >>>> +	 *
> >>>> +	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
> >>>> +	 *
> >>>> +	 *   - the user turned off mitigations altogether.
> >>>> +	 *
> >>>> +	 * Assume innocence until proven guilty: set the cap bit now, then
> >>>> +	 * clear it later if/when needed.
> >>>> +	 */
> >>>> +	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
> >>> Following on from the (re)discovery that X86_FEATURE_RETPOLINE is a poor
> >>> name given what it *actually* does, can I recommend s/SAFE/OK/ here?
> >> Or simply X86_FEATURE_USE_INDIRECT_BRANCH.
> >>
> >>> This flag really is "do I want indirect branches or not", which - as
> >>> noted here - is more than just a judgement of whether indirect branches
> >>> are "safe".
> > X86_FEATURE_USE_INDIRECT_BRANCH sounds good.  It's a bit long but does
> > describe it better.
> 
> Works for me.  Definitely an improvement over SAFE.

USE_INDIRECT_BRANCH is now irking me: "use indirect branch for what?
when? why?"

At the moment I'm leaning towards Andrew's suggestion of
X86_FEATURE_INDIRECT_OK as it at least says it's "ok" (safe or don't
care) to use indirect branches when desired (typically performance
raisins).  I'll probably go with that (or maybe
X86_FEATURE_INDIRECT_BRANCH_OK) unless anybody yells.

-- 
Josh
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Pawan Gupta 1 week, 6 days ago 
On Thu, Apr 18, 2024 at 05:48:45PM -0700, Josh Poimboeuf wrote:
> On Wed, Apr 17, 2024 at 07:01:54PM +0100, Andrew Cooper wrote:
> > On 17/04/2024 6:57 pm, Josh Poimboeuf wrote:
> > > On Wed, Apr 17, 2024 at 09:45:14AM -0700, Pawan Gupta wrote:
> > >> On Wed, Apr 17, 2024 at 04:14:26PM +0100, Andrew Cooper wrote:
> > >>> On 17/04/2024 12:02 am, Josh Poimboeuf wrote:
> > >>>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > >>>> index ca295b0c1eee..dcb97cc2758f 100644
> > >>>> --- a/arch/x86/kernel/cpu/bugs.c
> > >>>> +++ b/arch/x86/kernel/cpu/bugs.c
> > >>>> @@ -1678,6 +1687,21 @@ static void __init spectre_v2_select_mitigation(void)
> > >>>>  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
> > >>>>  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
> > >>>>  
> > >>>> +	/*
> > >>>> +	 * X86_FEATURE_INDIRECT_SAFE indicates whether indirect calls can be
> > >>>> +	 * considered safe.  That means either:
> > >>>> +	 *
> > >>>> +	 *   - the CPU isn't vulnerable to Spectre v2 or its variants;
> > >>>> +	 *
> > >>>> +	 *   - a hardware mitigation is in place (e.g., IBRS, BHI_DIS_S); or
> > >>>> +	 *
> > >>>> +	 *   - the user turned off mitigations altogether.
> > >>>> +	 *
> > >>>> +	 * Assume innocence until proven guilty: set the cap bit now, then
> > >>>> +	 * clear it later if/when needed.
> > >>>> +	 */
> > >>>> +	setup_force_cpu_cap(X86_FEATURE_INDIRECT_SAFE);
> > >>> Following on from the (re)discovery that X86_FEATURE_RETPOLINE is a poor
> > >>> name given what it *actually* does, can I recommend s/SAFE/OK/ here?
> > >> Or simply X86_FEATURE_USE_INDIRECT_BRANCH.
> > >>
> > >>> This flag really is "do I want indirect branches or not", which - as
> > >>> noted here - is more than just a judgement of whether indirect branches
> > >>> are "safe".
> > > X86_FEATURE_USE_INDIRECT_BRANCH sounds good.  It's a bit long but does
> > > describe it better.
> > 
> > Works for me.  Definitely an improvement over SAFE.
> 
> USE_INDIRECT_BRANCH is now irking me: "use indirect branch for what?
> when? why?"

I don't think feature bits in general tries to answer when & why. And it
shouldn't be the case, otherwise we will need multi-line names. IMO, it
should just tell what the feature means. But, I am not too hung up on
name, I am fine with X86_FEATURE_INDIRECT_OK or anything similar.
Re: [PATCH v3] x86/bugs: Only harden syscalls when needed
Posted by Borislav Petkov 2 weeks, 1 day ago 
On Tue, Apr 16, 2024 at 04:02:21PM -0700, Josh Poimboeuf wrote:
> Syscall hardening (i.e., converting the syscall indirect branch to a
> series of direct branches) may cause performance regressions in certain
> scenarios.  Only use the syscall hardening when indirect branches are
> considered unsafe.
> 
> Fixes: 1e3ad78334a6 ("x86/syscall: Don't force use of indirect calls for system calls")
> Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
> v3:
> - fix X86_FEATURE_INDIRECT_SAFE value
> 
>  arch/x86/entry/common.c            | 15 ++++++++++++---
>  arch/x86/entry/syscall_32.c        | 11 +----------
>  arch/x86/entry/syscall_64.c        |  6 ------
>  arch/x86/entry/syscall_x32.c       |  7 ++++++-
>  arch/x86/include/asm/cpufeatures.h |  1 +
>  arch/x86/include/asm/syscall.h     |  8 +++++++-
>  arch/x86/kernel/cpu/bugs.c         | 31 +++++++++++++++++++++++++++++-
>  7 files changed, 57 insertions(+), 22 deletions(-)

I poked at this a bit and can't find anything that I can complain about
so

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>

I'll pick it up into urgent if no one complains soon.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.