[PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

With the introduction of KERNEL_IBRS, STIBP is no longer needed
to prevent cross thread training in the kernel space. When KERNEL_IBRS
was added, it also disabled the user-mode protections for spectre_v2.
KERNEL_IBRS does not mitigate cross thread training in the userspace.

In order to demonstrate the issue, one needs to avoid syscalls in the
victim as syscalls can shorten the window size due to
a user -> kernel -> user transition which sets the
IBRS bit when entering kernel space and clearing any training the
attacker may have done.

Allow users to select a spectre_v2_user mitigation (STIBP always on,
opt-in via prctl) when KERNEL_IBRS is enabled.

Reported-by: José Oliveira <joseloliveira11@gmail.com>
Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
Reviewed-by: Alexandra Sandulescu <aesa@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
Cc: stable@vger.kernel.org
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index bca0bd8f4846..b05ca1575d81 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1132,6 +1132,19 @@ static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
 	       mode == SPECTRE_V2_EIBRS_LFENCE;
 }
 
+static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode)
+{
+	/* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
+	 *
+	 * However, With KERNEL_IBRS, the IBRS bit is cleared on return
+	 * to user and the user-mode code needs to be able to enable protection
+	 * from cross-thread training, either by always enabling STIBP or
+	 * by enabling it via prctl.
+	 */
+	return (spectre_v2_in_ibrs_mode(mode) &&
+		!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));
+}
+
 static void __init
 spectre_v2_user_select_mitigation(void)
 {
@@ -1193,13 +1206,8 @@ spectre_v2_user_select_mitigation(void)
 			"always-on" : "conditional");
 	}
 
-	/*
-	 * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
-	 * STIBP is not required.
-	 */
-	if (!boot_cpu_has(X86_FEATURE_STIBP) ||
-	    !smt_possible ||
-	    spectre_v2_in_ibrs_mode(spectre_v2_enabled))
+	if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible ||
+	    spectre_v2_user_no_stibp(spectre_v2_enabled))
 		return;
 
 	/*
@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)
 		break;
 
 	case SPECTRE_V2_IBRS:
+		pr_err("enabling KERNEL_IBRS");
 		setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS);
 		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED))
 			pr_warn(SPECTRE_V2_IBRS_PERF_MSG);
@@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf)
 
 static char *stibp_state(void)
 {
-	if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
+	if (spectre_v2_user_no_stibp(spectre_v2_enabled))
 		return "";
 
 	switch (spectre_v2_user_stibp) {
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Greg KH]

On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> With the introduction of KERNEL_IBRS, STIBP is no longer needed
> to prevent cross thread training in the kernel space. When KERNEL_IBRS
> was added, it also disabled the user-mode protections for spectre_v2.
> KERNEL_IBRS does not mitigate cross thread training in the userspace.
> 
> In order to demonstrate the issue, one needs to avoid syscalls in the
> victim as syscalls can shorten the window size due to
> a user -> kernel -> user transition which sets the
> IBRS bit when entering kernel space and clearing any training the
> attacker may have done.
> 
> Allow users to select a spectre_v2_user mitigation (STIBP always on,
> opt-in via prctl) when KERNEL_IBRS is enabled.
> 
> Reported-by: José Oliveira <joseloliveira11@gmail.com>
> Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> Cc: stable@vger.kernel.org
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
>  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)

As this is posted publicly, there's no need to send it to
security@kernel.org, it doesn't need to be involved.

thanks,

greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

On Mon, Feb 20, 2023 at 2:52 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > was added, it also disabled the user-mode protections for spectre_v2.
> > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> >
> > In order to demonstrate the issue, one needs to avoid syscalls in the
> > victim as syscalls can shorten the window size due to
> > a user -> kernel -> user transition which sets the
> > IBRS bit when entering kernel space and clearing any training the
> > attacker may have done.
> >
> > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > opt-in via prctl) when KERNEL_IBRS is enabled.
> >
> > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
>
> As this is posted publicly, there's no need to send it to
> security@kernel.org, it doesn't need to be involved.

Sure, it's okay. Please do note in my first patch, I did follow
https://www.kernel.org/doc/Documentation/admin-guide/security-bugs.rst,
if you want folks to explicitly Cc maintainers with their fix or
report, I think it's worth mentioning in the guidelines there as the
current language seems to imply that the maintainers will be pulled in
by the security team:

"It is possible that the security team will bring in extra help from
area maintainers to understand and fix the security vulnerability."

- KP

>
> thanks,
>
> greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Greg KH]

On Mon, Feb 20, 2023 at 03:11:24AM -0800, KP Singh wrote:
> On Mon, Feb 20, 2023 at 2:52 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > > was added, it also disabled the user-mode protections for spectre_v2.
> > > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > >
> > > In order to demonstrate the issue, one needs to avoid syscalls in the
> > > victim as syscalls can shorten the window size due to
> > > a user -> kernel -> user transition which sets the
> > > IBRS bit when entering kernel space and clearing any training the
> > > attacker may have done.
> > >
> > > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > > opt-in via prctl) when KERNEL_IBRS is enabled.
> > >
> > > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > ---
> > >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> > >  1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > As this is posted publicly, there's no need to send it to
> > security@kernel.org, it doesn't need to be involved.
> 
> Sure, it's okay. Please do note in my first patch, I did follow
> https://www.kernel.org/doc/Documentation/admin-guide/security-bugs.rst,
> if you want folks to explicitly Cc maintainers with their fix or
> report, I think it's worth mentioning in the guidelines there as the
> current language seems to imply that the maintainers will be pulled in
> by the security team:
> 
> "It is possible that the security team will bring in extra help from
> area maintainers to understand and fix the security vulnerability."

Yes, but you already have a patch here, what "help" do you need?  You
didn't specify any help, you just sent us a patch with no context.  This
wasn't any sort of a "report" or "hey, I think we found a problem over
here, does this change look correct", right?

So please be specific as to what you are asking for, otherwise we have
to guess (i.e. you cc:ed a seemingly random set of people but not the
x86 maintainers).  And then you resent it to a public list, so there's
no need for security@k.o to get involved in at all as it's a public
issue now.

thanks,

greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

On Mon, Feb 20, 2023 at 3:20 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 20, 2023 at 03:11:24AM -0800, KP Singh wrote:
> > On Mon, Feb 20, 2023 at 2:52 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > > > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > > > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > > > was added, it also disabled the user-mode protections for spectre_v2.
> > > > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > > >
> > > > In order to demonstrate the issue, one needs to avoid syscalls in the
> > > > victim as syscalls can shorten the window size due to
> > > > a user -> kernel -> user transition which sets the
> > > > IBRS bit when entering kernel space and clearing any training the
> > > > attacker may have done.
> > > >
> > > > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > > > opt-in via prctl) when KERNEL_IBRS is enabled.
> > > >
> > > > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > > > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > > > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > > ---
> > > >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> > > >  1 file changed, 17 insertions(+), 8 deletions(-)
> > >
> > > As this is posted publicly, there's no need to send it to
> > > security@kernel.org, it doesn't need to be involved.
> >
> > Sure, it's okay. Please do note in my first patch, I did follow
> > https://www.kernel.org/doc/Documentation/admin-guide/security-bugs.rst,
> > if you want folks to explicitly Cc maintainers with their fix or
> > report, I think it's worth mentioning in the guidelines there as the
> > current language seems to imply that the maintainers will be pulled in
> > by the security team:
> >
> > "It is possible that the security team will bring in extra help from
> > area maintainers to understand and fix the security vulnerability."
>
> Yes, but you already have a patch here, what "help" do you need?  You
> didn't specify any help, you just sent us a patch with no context.  This
> wasn't any sort of a "report" or "hey, I think we found a problem over
> here, does this change look correct", right?
>
> So please be specific as to what you are asking for, otherwise we have
> to guess (i.e. you cc:ed a seemingly random set of people but not the

I don't see how it matters who I cc on the list. Anyways, I am still
not clear on what one is supposed to do in the case when one has a
patch for an issue already. Should this not be send it to security@?

> x86 maintainers).  And then you resent it to a public list, so there's
> no need for security@k.o to get involved in at all as it's a public
> issue now.

Agreed.
>
> thanks,
>
> greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

On Mon, Feb 20, 2023 at 3:25 AM KP Singh <kpsingh@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 3:20 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Feb 20, 2023 at 03:11:24AM -0800, KP Singh wrote:
> > > On Mon, Feb 20, 2023 at 2:52 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > > > > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > > > > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > > > > was added, it also disabled the user-mode protections for spectre_v2.
> > > > > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > > > >
> > > > > In order to demonstrate the issue, one needs to avoid syscalls in the
> > > > > victim as syscalls can shorten the window size due to
> > > > > a user -> kernel -> user transition which sets the
> > > > > IBRS bit when entering kernel space and clearing any training the
> > > > > attacker may have done.
> > > > >
> > > > > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > > > > opt-in via prctl) when KERNEL_IBRS is enabled.
> > > > >
> > > > > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > > > > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > > > > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > > > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > > > ---
> > > > >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> > > > >  1 file changed, 17 insertions(+), 8 deletions(-)
> > > >
> > > > As this is posted publicly, there's no need to send it to
> > > > security@kernel.org, it doesn't need to be involved.
> > >
> > > Sure, it's okay. Please do note in my first patch, I did follow
> > > https://www.kernel.org/doc/Documentation/admin-guide/security-bugs.rst,
> > > if you want folks to explicitly Cc maintainers with their fix or
> > > report, I think it's worth mentioning in the guidelines there as the
> > > current language seems to imply that the maintainers will be pulled in
> > > by the security team:
> > >
> > > "It is possible that the security team will bring in extra help from
> > > area maintainers to understand and fix the security vulnerability."
> >
> > Yes, but you already have a patch here, what "help" do you need?  You
> > didn't specify any help, you just sent us a patch with no context.  This
> > wasn't any sort of a "report" or "hey, I think we found a problem over
> > here, does this change look correct", right?
> >
> > So please be specific as to what you are asking for, otherwise we have
> > to guess (i.e. you cc:ed a seemingly random set of people but not the
>
> I don't see how it matters who I cc on the list. Anyways, I am still

Cc on my report, I mean.

> not clear on what one is supposed to do in the case when one has a
> patch for an issue already. Should this not be send it to security@?
>
> > x86 maintainers).  And then you resent it to a public list, so there's
> > no need for security@k.o to get involved in at all as it's a public
> > issue now.
>
> Agreed.
> >
> > thanks,
> >
> > greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Greg KH]

On Mon, Feb 20, 2023 at 03:25:08AM -0800, KP Singh wrote:
> On Mon, Feb 20, 2023 at 3:20 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Feb 20, 2023 at 03:11:24AM -0800, KP Singh wrote:
> > > On Mon, Feb 20, 2023 at 2:52 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > > > > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > > > > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > > > > was added, it also disabled the user-mode protections for spectre_v2.
> > > > > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > > > >
> > > > > In order to demonstrate the issue, one needs to avoid syscalls in the
> > > > > victim as syscalls can shorten the window size due to
> > > > > a user -> kernel -> user transition which sets the
> > > > > IBRS bit when entering kernel space and clearing any training the
> > > > > attacker may have done.
> > > > >
> > > > > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > > > > opt-in via prctl) when KERNEL_IBRS is enabled.
> > > > >
> > > > > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > > > > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > > > > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > > > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > > > ---
> > > > >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> > > > >  1 file changed, 17 insertions(+), 8 deletions(-)
> > > >
> > > > As this is posted publicly, there's no need to send it to
> > > > security@kernel.org, it doesn't need to be involved.
> > >
> > > Sure, it's okay. Please do note in my first patch, I did follow
> > > https://www.kernel.org/doc/Documentation/admin-guide/security-bugs.rst,
> > > if you want folks to explicitly Cc maintainers with their fix or
> > > report, I think it's worth mentioning in the guidelines there as the
> > > current language seems to imply that the maintainers will be pulled in
> > > by the security team:
> > >
> > > "It is possible that the security team will bring in extra help from
> > > area maintainers to understand and fix the security vulnerability."
> >
> > Yes, but you already have a patch here, what "help" do you need?  You
> > didn't specify any help, you just sent us a patch with no context.  This
> > wasn't any sort of a "report" or "hey, I think we found a problem over
> > here, does this change look correct", right?
> >
> > So please be specific as to what you are asking for, otherwise we have
> > to guess (i.e. you cc:ed a seemingly random set of people but not the
> 
> I don't see how it matters who I cc on the list.

It gives us a hint as to who you are leaving out, right?

> Anyways, I am still
> not clear on what one is supposed to do in the case when one has a
> patch for an issue already. Should this not be send it to security@?

security@ is to take reports of potential security problems, triage
them, and drag in the respective people to fix the problem as soon as
possible by creating a patch and getting it merged.

You already had a patch, so you did all of the work that security@ would
normally do, so what did you need us to do here?

You also did not ask or request anything, you just sent a patch with no
context other than the changelog text.

So if you have a fix for a potential problem already, you either just
send it to get it merged as soon as possible, in which case security@ is
not needed.  Or if you want to ask questions "is this really an issue
and is this the fix", then send the patch and ask that question.

Again, as it is, this looks like any other normal patch sent to
subsystems for review, and there was no request for help or context at
all.  Then you sent the patch to a public mailing list, so security@ is
not needed at al, the normal development process applies as you
determined it's not a secret by doing so.

If you have questions, ask them, we can't read minds.

thanks,

greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

On Mon, Feb 20, 2023 at 3:38 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 20, 2023 at 03:25:08AM -0800, KP Singh wrote:
> > On Mon, Feb 20, 2023 at 3:20 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Feb 20, 2023 at 03:11:24AM -0800, KP Singh wrote:
> > > > On Mon, Feb 20, 2023 at 2:52 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > > > > > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > > > > > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > > > > > was added, it also disabled the user-mode protections for spectre_v2.
> > > > > > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > > > > >
> > > > > > In order to demonstrate the issue, one needs to avoid syscalls in the
> > > > > > victim as syscalls can shorten the window size due to
> > > > > > a user -> kernel -> user transition which sets the
> > > > > > IBRS bit when entering kernel space and clearing any training the
> > > > > > attacker may have done.
> > > > > >
> > > > > > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > > > > > opt-in via prctl) when KERNEL_IBRS is enabled.
> > > > > >
> > > > > > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > > > > > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > > > > > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > > > > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > > > > ---
> > > > > >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> > > > > >  1 file changed, 17 insertions(+), 8 deletions(-)
> > > > >
> > > > > As this is posted publicly, there's no need to send it to
> > > > > security@kernel.org, it doesn't need to be involved.
> > > >
> > > > Sure, it's okay. Please do note in my first patch, I did follow
> > > > https://www.kernel.org/doc/Documentation/admin-guide/security-bugs.rst,
> > > > if you want folks to explicitly Cc maintainers with their fix or
> > > > report, I think it's worth mentioning in the guidelines there as the
> > > > current language seems to imply that the maintainers will be pulled in
> > > > by the security team:
> > > >
> > > > "It is possible that the security team will bring in extra help from
> > > > area maintainers to understand and fix the security vulnerability."
> > >
> > > Yes, but you already have a patch here, what "help" do you need?  You
> > > didn't specify any help, you just sent us a patch with no context.  This
> > > wasn't any sort of a "report" or "hey, I think we found a problem over
> > > here, does this change look correct", right?
> > >
> > > So please be specific as to what you are asking for, otherwise we have
> > > to guess (i.e. you cc:ed a seemingly random set of people but not the
> >
> > I don't see how it matters who I cc on the list.
>
> It gives us a hint as to who you are leaving out, right?
>
> > Anyways, I am still
> > not clear on what one is supposed to do in the case when one has a
> > patch for an issue already. Should this not be send it to security@?
>
> security@ is to take reports of potential security problems, triage
> them, and drag in the respective people to fix the problem as soon as
> possible by creating a patch and getting it merged.
>
> You already had a patch, so you did all of the work that security@ would
> normally do, so what did you need us to do here?
>
> You also did not ask or request anything, you just sent a patch with no
> context other than the changelog text.
>
> So if you have a fix for a potential problem already, you either just
> send it to get it merged as soon as possible, in which case security@ is
> not needed.  Or if you want to ask questions "is this really an issue
> and is this the fix", then send the patch and ask that question.
>
> Again, as it is, this looks like any other normal patch sent to
> subsystems for review, and there was no request for help or context at
> all.  Then you sent the patch to a public mailing list, so security@ is
> not needed at al, the normal development process applies as you
> determined it's not a secret by doing so.
>
> If you have questions, ask them, we can't read minds.

When one sends a report with the fix to security@kernel.org, the ask /
request is they are asking for a review and having it merged. Now, I
thought this is implied with the patch, but maybe something else is
expected and I should have added a cover letter specifically asking
what was expected.

In this case, I did not intend to make it public initially and would
have preferred to not make it public until it was merged, until I was
nudged to read https://people.kernel.org/tglx/notes-about-netiquette.
Anyways, what's done is done, let's focus on fixing the bug.

>
> thanks,
>
> greg k-h

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Borislav Petkov]

On Mon, Feb 20, 2023 at 03:48:33AM -0800, KP Singh wrote:
> until I was nudged to read
> https://people.kernel.org/tglx/notes-about-netiquette.

I think you misread my signature. It gets added by my mail client to
*every* mail I send. See below.

So it had only tangentially to do with your situation.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Josh Poimboeuf]

On Mon, Feb 20, 2023 at 11:52:27AM +0100, Greg KH wrote:
> On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > was added, it also disabled the user-mode protections for spectre_v2.
> > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > 
> > In order to demonstrate the issue, one needs to avoid syscalls in the
> > victim as syscalls can shorten the window size due to
> > a user -> kernel -> user transition which sets the
> > IBRS bit when entering kernel space and clearing any training the
> > attacker may have done.
> > 
> > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > opt-in via prctl) when KERNEL_IBRS is enabled.
> > 
> > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> As this is posted publicly, there's no need to send it to
> security@kernel.org, it doesn't need to be involved.

Also, since this seems intended to be public, please add lkml to Cc on
the next revision.

-- 
Josh

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

On Mon, Feb 20, 2023 at 3:39 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 11:52:27AM +0100, Greg KH wrote:
> > On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > > With the introduction of KERNEL_IBRS, STIBP is no longer needed
> > > to prevent cross thread training in the kernel space. When KERNEL_IBRS
> > > was added, it also disabled the user-mode protections for spectre_v2.
> > > KERNEL_IBRS does not mitigate cross thread training in the userspace.
> > >
> > > In order to demonstrate the issue, one needs to avoid syscalls in the
> > > victim as syscalls can shorten the window size due to
> > > a user -> kernel -> user transition which sets the
> > > IBRS bit when entering kernel space and clearing any training the
> > > attacker may have done.
> > >
> > > Allow users to select a spectre_v2_user mitigation (STIBP always on,
> > > opt-in via prctl) when KERNEL_IBRS is enabled.
> > >
> > > Reported-by: José Oliveira <joseloliveira11@gmail.com>
> > > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> > > Reviewed-by: Alexandra Sandulescu <aesa@google.com>
> > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > ---
> > >  arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
> > >  1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > As this is posted publicly, there's no need to send it to
> > security@kernel.org, it doesn't need to be involved.
>
> Also, since this seems intended to be public, please add lkml to Cc on
> the next revision.

Sure.

>
> --
> Josh

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Josh Poimboeuf]

On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1132,6 +1132,19 @@ static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
>  	       mode == SPECTRE_V2_EIBRS_LFENCE;
>  }
>  
> +static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode)
> +{
> +	/* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
> +	 *
> +	 * However, With KERNEL_IBRS, the IBRS bit is cleared on return
> +	 * to user and the user-mode code needs to be able to enable protection
> +	 * from cross-thread training, either by always enabling STIBP or
> +	 * by enabling it via prctl.
> +	 */
> +	return (spectre_v2_in_ibrs_mode(mode) &&
> +		!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));
> +}

The comments and code confused me, they both seem to imply some
distinction between IBRS and KERNEL_IBRS, but in the kernel those are
functionally the same thing.  e.g., the kernel doesn't have a user IBRS
mode.

And, unless I'm missing some subtlety here, it seems to be a convoluted
way of saying that eIBRS doesn't need STIBP in user space.

It would be simpler to just call it spectre_v2_in_eibrs_mode().

static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
{
	return mode == SPECTRE_V2_EIBRS ||
	       mode == SPECTRE_V2_EIBRS_RETPOLINE ||
	       mode == SPECTRE_V2_EIBRS_LFENCE;
}

And then spectre_v2_in_ibrs_mode() could be changed to call that:

static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
{
	return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS;
}

> @@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)
>  		break;
>  
>  	case SPECTRE_V2_IBRS:
> +		pr_err("enabling KERNEL_IBRS");

Why?

>  		setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS);
>  		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED))
>  			pr_warn(SPECTRE_V2_IBRS_PERF_MSG);
> @@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf)
>  
>  static char *stibp_state(void)
>  {
> -	if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> +	if (spectre_v2_user_no_stibp(spectre_v2_enabled))
>  		return "";

This seems like old cruft, can we just remove this check altogether?  In
the eIBRS case, spectre_v2_user_stibp will already have its default of
SPECTRE_V2_USER_NONE.

-- 
Josh

Re: [PATCH] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Josh Poimboeuf]

Getting confused with all the threads, will repost this review to the
lkml version...

On Mon, Feb 20, 2023 at 04:07:30AM -0800, Josh Poimboeuf wrote:
> On Mon, Feb 20, 2023 at 11:39:30AM +0100, KP Singh wrote:
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -1132,6 +1132,19 @@ static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
> >  	       mode == SPECTRE_V2_EIBRS_LFENCE;
> >  }
> >  
> > +static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode)
> > +{
> > +	/* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
> > +	 *
> > +	 * However, With KERNEL_IBRS, the IBRS bit is cleared on return
> > +	 * to user and the user-mode code needs to be able to enable protection
> > +	 * from cross-thread training, either by always enabling STIBP or
> > +	 * by enabling it via prctl.
> > +	 */
> > +	return (spectre_v2_in_ibrs_mode(mode) &&
> > +		!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));
> > +}
> 
> The comments and code confused me, they both seem to imply some
> distinction between IBRS and KERNEL_IBRS, but in the kernel those are
> functionally the same thing.  e.g., the kernel doesn't have a user IBRS
> mode.
> 
> And, unless I'm missing some subtlety here, it seems to be a convoluted
> way of saying that eIBRS doesn't need STIBP in user space.
> 
> It would be simpler to just call it spectre_v2_in_eibrs_mode().
> 
> static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
> {
> 	return mode == SPECTRE_V2_EIBRS ||
> 	       mode == SPECTRE_V2_EIBRS_RETPOLINE ||
> 	       mode == SPECTRE_V2_EIBRS_LFENCE;
> }
> 
> And then spectre_v2_in_ibrs_mode() could be changed to call that:
> 
> static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
> {
> 	return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS;
> }
> 
> > @@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)
> >  		break;
> >  
> >  	case SPECTRE_V2_IBRS:
> > +		pr_err("enabling KERNEL_IBRS");
> 
> Why?
> 
> >  		setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS);
> >  		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED))
> >  			pr_warn(SPECTRE_V2_IBRS_PERF_MSG);
> > @@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf)
> >  
> >  static char *stibp_state(void)
> >  {
> > -	if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> > +	if (spectre_v2_user_no_stibp(spectre_v2_enabled))
> >  		return "";
> 
> This seems like old cruft, can we just remove this check altogether?  In
> the eIBRS case, spectre_v2_user_stibp will already have its default of
> SPECTRE_V2_USER_NONE.
> 
> -- 
> Josh

-- 
Josh