diff mbox series

[RFC,v2,1/3] x86: cpu/bugs: update SpectreRSB comments for AMD

Message ID 20241111163913.36139-2-amit@kernel.org (mailing list archive)
State New
Headers show
Series Add support for the ERAPS feature | expand

Commit Message

Amit Shah Nov. 11, 2024, 4:39 p.m. UTC
From: Amit Shah <amit.shah@amd.com>

AMD CPUs do not fall back to the BTB when the RSB underflows for RET
address speculation.  AMD CPUs have not needed to stuff the RSB for
underflow conditions.

The RSB poisoning case is addressed by RSB filling - clean up the FIXME
comment about it.

Signed-off-by: Amit Shah <amit.shah@amd.com>
---
 arch/x86/kernel/cpu/bugs.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Dave Hansen Nov. 11, 2024, 5:01 p.m. UTC | #1
On 11/11/24 08:39, Amit Shah wrote:
> From: Amit Shah <amit.shah@amd.com>
> 
> AMD CPUs do not fall back to the BTB when the RSB underflows for RET
> address speculation.  AMD CPUs have not needed to stuff the RSB for
> underflow conditions.
> 
> The RSB poisoning case is addressed by RSB filling - clean up the FIXME
> comment about it.

This amounts to "Josh was wrong" in commit 9756bba284. Before moving
forward with this, it would be great to get his ack on this to make sure
you two are on the same page.
Josh Poimboeuf Nov. 11, 2024, 7:33 p.m. UTC | #2
On Mon, Nov 11, 2024 at 05:39:11PM +0100, Amit Shah wrote:
> From: Amit Shah <amit.shah@amd.com>
> 
> AMD CPUs do not fall back to the BTB when the RSB underflows for RET
> address speculation.  AMD CPUs have not needed to stuff the RSB for
> underflow conditions.
> 
> The RSB poisoning case is addressed by RSB filling - clean up the FIXME
> comment about it.

I'm thinking the comments need more clarification in light of BTC and
SRSO.

This:

> -	 *    AMD has it even worse: *all* returns are speculated from the BTB,
> -	 *    regardless of the state of the RSB.

is still true (mostly: "all" should be "some"), though it doesn't belong
in the "RSB underflow" section.

Also the RSB stuffing not only mitigates RET, it mitigates any other
instruction which happen to be predicted as a RET.  Which is presumably
why it's still needed even when SRSO is enabled.

Something like below?

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 47a01d4028f6..e95d3aa14259 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1828,9 +1828,6 @@ static void __init spectre_v2_select_mitigation(void)
 	 *    speculated return targets may come from the branch predictor,
 	 *    which could have a user-poisoned BTB or BHB entry.
 	 *
-	 *    AMD has it even worse: *all* returns are speculated from the BTB,
-	 *    regardless of the state of the RSB.
-	 *
 	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
 	 *    scenario is mitigated by the IBRS branch prediction isolation
 	 *    properties, so the RSB buffer filling wouldn't be necessary to
@@ -1850,10 +1847,22 @@ static void __init spectre_v2_select_mitigation(void)
 	 *    The "user -> user" scenario, also known as SpectreBHB, requires
 	 *    RSB clearing.
 	 *
+	 *    AMD Branch Type Confusion (aka "AMD retbleed") adds some
+	 *    additional wrinkles:
+	 *
+	 *      - A RET can be mispredicted as a direct or indirect branch,
+	 *        causing the CPU to speculatively branch to a BTB target, in
+	 *        which case the RSB filling obviously doesn't help.  That case
+	 *        is mitigated by removing all the RETs (SRSO mitigation).
+	 *
+	 *      - The RSB is not only used for architectural RET instructions,
+	 *        it may also be used for other instructions which happen to
+	 *        get mispredicted as RETs.  Therefore RSB filling is still
+	 *        needed even when the RETs have all been removed by the SRSO
+	 *        mitigation.
+	 *
 	 * So to mitigate all cases, unconditionally fill RSB on context
 	 * switches.
-	 *
-	 * FIXME: Is this pointless for retbleed-affected AMD?
 	 */
 	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
Kaplan, David Nov. 11, 2024, 7:58 p.m. UTC | #3
[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@kernel.org>
> Sent: Monday, November 11, 2024 1:33 PM
> To: Amit Shah <amit@kernel.org>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; x86@kernel.org; linux-
> doc@vger.kernel.org; Shah, Amit <Amit.Shah@amd.com>; Lendacky, Thomas
> <Thomas.Lendacky@amd.com>; bp@alien8.de; tglx@linutronix.de;
> peterz@infradead.org; pawan.kumar.gupta@linux.intel.com; corbet@lwn.net;
> mingo@redhat.com; dave.hansen@linux.intel.com; hpa@zytor.com;
> seanjc@google.com; pbonzini@redhat.com; daniel.sneddon@linux.intel.com;
> kai.huang@intel.com; Das1, Sandipan <Sandipan.Das@amd.com>;
> boris.ostrovsky@oracle.com; Moger, Babu <Babu.Moger@amd.com>; Kaplan,
> David <David.Kaplan@amd.com>; dwmw@amazon.co.uk;
> andrew.cooper3@citrix.com
> Subject: Re: [RFC PATCH v2 1/3] x86: cpu/bugs: update SpectreRSB comments
> for AMD
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Mon, Nov 11, 2024 at 05:39:11PM +0100, Amit Shah wrote:
> > From: Amit Shah <amit.shah@amd.com>
> >
> > AMD CPUs do not fall back to the BTB when the RSB underflows for RET
> > address speculation.  AMD CPUs have not needed to stuff the RSB for
> > underflow conditions.
> >
> > The RSB poisoning case is addressed by RSB filling - clean up the
> > FIXME comment about it.
>
> I'm thinking the comments need more clarification in light of BTC and SRSO.
>
> This:
>
> > -      *    AMD has it even worse: *all* returns are speculated from the BTB,
> > -      *    regardless of the state of the RSB.
>
> is still true (mostly: "all" should be "some"), though it doesn't belong in the "RSB
> underflow" section.
>
> Also the RSB stuffing not only mitigates RET, it mitigates any other instruction
> which happen to be predicted as a RET.  Which is presumably why it's still needed
> even when SRSO is enabled.
>

While that's partly true, I'm not sure I'm understanding which scenario you're concerned with.  As noted in the AMD BTC whitepaper, there are various types of potential mis-speculation depending on what the actual branch is. The 'late redirect' ones are the most concerning since those have enough of a speculation window to be able to do a load-op-load gadget.  The only 'late redirect' case involving an instruction being incorrectly predicted as a RET is when the actual instruction is an indirect JMP/CALL.  But those instructions are either removed entirely (due to retpoline) or being protected with IBRS.  The cases of other instructions (like Jcc) being predicted as a RET are subject to the 'early redirect' behavior which is much more limited (and note that these can also be predicted as indirect branches for which there is no special protection).  So I'm not sure why BTC specifically would necessitate needing to stuff the RSB here.

Also, BTC only affects some AMD parts (not Family 19h and later for instance).

--David Kaplan
Josh Poimboeuf Nov. 11, 2024, 8:39 p.m. UTC | #4
On Mon, Nov 11, 2024 at 07:58:03PM +0000, Kaplan, David wrote:
> > I'm thinking the comments need more clarification in light of BTC and SRSO.
> >
> > This:
> >
> > > -      *    AMD has it even worse: *all* returns are speculated from the BTB,
> > > -      *    regardless of the state of the RSB.
> >
> > is still true (mostly: "all" should be "some"), though it doesn't belong in the "RSB
> > underflow" section.
> >
> > Also the RSB stuffing not only mitigates RET, it mitigates any other instruction
> > which happen to be predicted as a RET.  Which is presumably why it's still needed
> > even when SRSO is enabled.
> >
> 
> While that's partly true, I'm not sure I'm understanding which
> scenario you're concerned with.  As noted in the AMD BTC whitepaper,
> there are various types of potential mis-speculation depending on what
> the actual branch is. The 'late redirect' ones are the most concerning
> since those have enough of a speculation window to be able to do a
> load-op-load gadget.  The only 'late redirect' case involving an
> instruction being incorrectly predicted as a RET is when the actual
> instruction is an indirect JMP/CALL.  But those instructions are
> either removed entirely (due to retpoline) or being protected with
> IBRS.  The cases of other instructions (like Jcc) being predicted as a
> RET are subject to the 'early redirect' behavior which is much more
> limited (and note that these can also be predicted as indirect
> branches for which there is no special protection).  So I'm not sure
> why BTC specifically would necessitate needing to stuff the RSB here.
> 
> Also, BTC only affects some AMD parts (not Family 19h and later for
> instance).

This is why it's important to spell out all the different cases in the
comments.  I was attempting to document the justifications for the
existing behavior.

You make some good points, though backing up a bit, I realize my comment
was flawed for another reason: the return thunks only protect the
kernel, but RSB filling on context switch is meant to protect user
space.

So, never mind...
Josh Poimboeuf Nov. 11, 2024, 8:40 p.m. UTC | #5
On Mon, Nov 11, 2024 at 12:39:09PM -0800, Josh Poimboeuf wrote:
> This is why it's important to spell out all the different cases in the
> comments.  I was attempting to document the justifications for the
> existing behavior.
> 
> You make some good points, though backing up a bit, I realize my comment
> was flawed for another reason: the return thunks only protect the
> kernel, but RSB filling on context switch is meant to protect user
> space.
> 
> So, never mind...

That said, I still think the comments need an update.  I'll try to come
up with something later.
Andrew Cooper Nov. 12, 2024, 12:29 a.m. UTC | #6
On 11/11/2024 7:33 pm, Josh Poimboeuf wrote:
> On Mon, Nov 11, 2024 at 05:39:11PM +0100, Amit Shah wrote:
>> From: Amit Shah <amit.shah@amd.com>
>>
>> AMD CPUs do not fall back to the BTB when the RSB underflows for RET
>> address speculation.  AMD CPUs have not needed to stuff the RSB for
>> underflow conditions.
>>
>> The RSB poisoning case is addressed by RSB filling - clean up the FIXME
>> comment about it.
> I'm thinking the comments need more clarification in light of BTC and
> SRSO.
>
> This:
>
>> -	 *    AMD has it even worse: *all* returns are speculated from the BTB,
>> -	 *    regardless of the state of the RSB.
> is still true (mostly: "all" should be "some"), though it doesn't belong
> in the "RSB underflow" section.
>
> Also the RSB stuffing not only mitigates RET, it mitigates any other
> instruction which happen to be predicted as a RET.  Which is presumably
> why it's still needed even when SRSO is enabled.
>
> Something like below?
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 47a01d4028f6..e95d3aa14259 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1828,9 +1828,6 @@ static void __init spectre_v2_select_mitigation(void)
>  	 *    speculated return targets may come from the branch predictor,
>  	 *    which could have a user-poisoned BTB or BHB entry.
>  	 *
> -	 *    AMD has it even worse: *all* returns are speculated from the BTB,
> -	 *    regardless of the state of the RSB.
> -	 *
>  	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
>  	 *    scenario is mitigated by the IBRS branch prediction isolation
>  	 *    properties, so the RSB buffer filling wouldn't be necessary to
> @@ -1850,10 +1847,22 @@ static void __init spectre_v2_select_mitigation(void)
>  	 *    The "user -> user" scenario, also known as SpectreBHB, requires
>  	 *    RSB clearing.
>  	 *
> +	 *    AMD Branch Type Confusion (aka "AMD retbleed") adds some
> +	 *    additional wrinkles:
> +	 *
> +	 *      - A RET can be mispredicted as a direct or indirect branch,
> +	 *        causing the CPU to speculatively branch to a BTB target, in
> +	 *        which case the RSB filling obviously doesn't help.  That case
> +	 *        is mitigated by removing all the RETs (SRSO mitigation).
> +	 *
> +	 *      - The RSB is not only used for architectural RET instructions,
> +	 *        it may also be used for other instructions which happen to
> +	 *        get mispredicted as RETs.  Therefore RSB filling is still
> +	 *        needed even when the RETs have all been removed by the SRSO
> +	 *        mitigation.

This is my take.  On AMD CPUs, there are two unrelated issues to take
into account:

1) SRSO

Affects anything which doesn't enumerate SRSO_NO, which is all parts to
date including Zen5.

SRSO ends up overflowing the RAS with arbitrary BTB targets, such that a
subsequent genuine RET follows a prediction which never came from a real
CALL instruction.

Mitigations for SRSO are either safe-ret, or IBPB-on-entry.  Parts
without IBPB_RET using IBPB-on-entry need to manually flush the RAS.

Importantly, SMEP does not protection you against SRSO across the
user->kernel boundary, because the bad RAS entries are arbitrary.  New
in Zen5 is the SRSO_U/S_NO bit which says this case can't occur any
more.  So on Zen5, you can in principle get away without a RAS flush on
entry.


2) BTC

Affects anything which doesn't enumerate BTC_NO, which is Zen2 and older
(Fam17h for AMD, Fam18h for Hygon).

Attacker can forge any branch type prediction, and the most dangerous
one is RET-mispredicted-as-INDIRECT.  This causes a genuine RET
instruction to follow a prediction that was believed to be an indirect
branch.

All CPUs which suffer BTC also suffer SRSO, so while jmp2ret is a
mitigation for BTC, it's utility became 0 when SRSO was discovered. 
(Which as shame, because it's equal parts beautiful and terrifying.) 
Mitigations for BTC are therefore safe-ret or IBPB-on-entry.

Flushing the RAS has no effect on BTC, because the whole problem with
BTC is that the prediction comes from the "wrong" predictor, but you
need to do it for other reasons.

~Andrew
Josh Poimboeuf Nov. 12, 2024, 12:30 a.m. UTC | #7
On Mon, Nov 11, 2024 at 12:40:41PM -0800, Josh Poimboeuf wrote:
> On Mon, Nov 11, 2024 at 12:39:09PM -0800, Josh Poimboeuf wrote:
> > This is why it's important to spell out all the different cases in the
> > comments.  I was attempting to document the justifications for the
> > existing behavior.
> > 
> > You make some good points, though backing up a bit, I realize my comment
> > was flawed for another reason: the return thunks only protect the
> > kernel, but RSB filling on context switch is meant to protect user
> > space.
> > 
> > So, never mind...
> 
> That said, I still think the comments need an update.  I'll try to come
> up with something later.

Here are some clarifications to the comments.  Amit, feel free to
include this in your next revision.

----8<----

From: Josh Poimboeuf <jpoimboe@kernel.org>
Subject: [PATCH] x86/bugs: Update insanely long comment about RSB attacks

The long comment above the setting of X86_FEATURE_RSB_CTXSW is a bit
confusing.  It starts out being about context switching specifically,
but then goes on to describe "user -> kernel" mitigations, which aren't
necessarily limited to context switches.

Clarify that it's about *all* RSB attacks and their mitigations.

For consistency, add the "guest -> host" mitigations as well.  Then the
comment above spectre_v2_determine_rsb_fill_type_at_vmexit() can be
removed and the overall line count is reduced.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 59 ++++++++++++--------------------------
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 47a01d4028f6..fbdfa151b7a9 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1581,26 +1581,6 @@ static void __init spec_ctrl_disable_kernel_rrsba(void)
 
 static void __init spectre_v2_determine_rsb_fill_type_at_vmexit(enum spectre_v2_mitigation mode)
 {
-	/*
-	 * Similar to context switches, there are two types of RSB attacks
-	 * after VM exit:
-	 *
-	 * 1) RSB underflow
-	 *
-	 * 2) Poisoned RSB entry
-	 *
-	 * When retpoline is enabled, both are mitigated by filling/clearing
-	 * the RSB.
-	 *
-	 * When IBRS is enabled, while #1 would be mitigated by the IBRS branch
-	 * prediction isolation protections, RSB still needs to be cleared
-	 * because of #2.  Note that SMEP provides no protection here, unlike
-	 * user-space-poisoned RSB entries.
-	 *
-	 * eIBRS should protect against RSB poisoning, but if the EIBRS_PBRSB
-	 * bug is present then a LITE version of RSB protection is required,
-	 * just a single call needs to retire before a RET is executed.
-	 */
 	switch (mode) {
 	case SPECTRE_V2_NONE:
 		return;
@@ -1818,43 +1798,42 @@ static void __init spectre_v2_select_mitigation(void)
 	pr_info("%s\n", spectre_v2_strings[mode]);
 
 	/*
-	 * If Spectre v2 protection has been enabled, fill the RSB during a
-	 * context switch.  In general there are two types of RSB attacks
-	 * across context switches, for which the CALLs/RETs may be unbalanced.
+	 * In general there are two types of RSB attacks:
 	 *
-	 * 1) RSB underflow
+	 * 1) RSB underflow ("Intel Retbleed")
 	 *
 	 *    Some Intel parts have "bottomless RSB".  When the RSB is empty,
 	 *    speculated return targets may come from the branch predictor,
 	 *    which could have a user-poisoned BTB or BHB entry.
 	 *
-	 *    AMD has it even worse: *all* returns are speculated from the BTB,
-	 *    regardless of the state of the RSB.
+	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack is
+	 *    mitigated by the IBRS branch prediction isolation properties, so
+	 *    the RSB buffer filling wouldn't be necessary to protect against
+	 *    this type of attack.
 	 *
-	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
-	 *    scenario is mitigated by the IBRS branch prediction isolation
-	 *    properties, so the RSB buffer filling wouldn't be necessary to
-	 *    protect against this type of attack.
+	 *    The "user -> user" attack is mitigated by RSB filling on context
+	 *    switch.
 	 *
-	 *    The "user -> user" attack scenario is mitigated by RSB filling.
+	 *    The "guest -> host" attack is mitigated by IBRS or eIBRS.
 	 *
 	 * 2) Poisoned RSB entry
 	 *
 	 *    If the 'next' in-kernel return stack is shorter than 'prev',
 	 *    'next' could be tricked into speculating with a user-poisoned RSB
-	 *    entry.
+	 *    entry.  Speculative Type Confusion ("AMD retbleed") can also
+	 *    create poisoned RSB entries.
 	 *
-	 *    The "user -> kernel" attack scenario is mitigated by SMEP and
-	 *    eIBRS.
+	 *    The "user -> kernel" attack is mitigated by SMEP and eIBRS.
 	 *
-	 *    The "user -> user" scenario, also known as SpectreBHB, requires
-	 *    RSB clearing.
+	 *    The "user -> user" attack, also known as SpectreBHB, requires RSB
+	 *    clearing.
 	 *
-	 * So to mitigate all cases, unconditionally fill RSB on context
-	 * switches.
-	 *
-	 * FIXME: Is this pointless for retbleed-affected AMD?
+	 *    The "guest -> host" attack is mitigated by eIBRS (not IBRS!) or
+	 *    RSB clearing on vmexit.  Note that eIBRS implementations with
+	 *    X86_BUG_EIBRS_PBRSB still need "lite" RSB clearing which retires
+	 *    a single CALL before the first RET.
 	 */
+
 	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
Josh Poimboeuf Nov. 12, 2024, 1:46 a.m. UTC | #8
On Tue, Nov 12, 2024 at 12:29:28AM +0000, Andrew Cooper wrote:
> This is my take.  On AMD CPUs, there are two unrelated issues to take
> into account:
> 
> 1) SRSO
> 
> Affects anything which doesn't enumerate SRSO_NO, which is all parts to
> date including Zen5.
> 
> SRSO ends up overflowing the RAS with arbitrary BTB targets, such that a
> subsequent genuine RET follows a prediction which never came from a real
> CALL instruction.
> 
> Mitigations for SRSO are either safe-ret, or IBPB-on-entry.  Parts
> without IBPB_RET using IBPB-on-entry need to manually flush the RAS.
> 
> Importantly, SMEP does not protection you against SRSO across the
> user->kernel boundary, because the bad RAS entries are arbitrary.  New
> in Zen5 is the SRSO_U/S_NO bit which says this case can't occur any
> more.  So on Zen5, you can in principle get away without a RAS flush on
> entry.

Updated to mention SRSO:

	/*
	 * In general there are two types of RSB attacks:
	 *
	 * 1) RSB underflow ("Intel Retbleed")
	 *
	 *    Some Intel parts have "bottomless RSB".  When the RSB is empty,
	 *    speculated return targets may come from the branch predictor,
	 *    which could have a user-poisoned BTB or BHB entry.
	 *
	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack is
	 *    mitigated by the IBRS branch prediction isolation properties, so
	 *    the RSB buffer filling wouldn't be necessary to protect against
	 *    this type of attack.
	 *
	 *    The "user -> user" attack is mitigated by RSB filling on context
	 *    switch.
	 *
	 *    The "guest -> host" attack is mitigated by IBRS or eIBRS.
	 *
	 * 2) Poisoned RSB entry
	 *
	 *    If the 'next' in-kernel return stack is shorter than 'prev',
	 *    'next' could be tricked into speculating with a user-poisoned RSB
	 *    entry.  Poisoned RSB entries can also be created by Branch Type
	 *    Confusion ("AMD retbleed") or SRSO.
	 *
	 *    The "user -> kernel" attack is mitigated by SMEP and eIBRS.  AMD
	 *    without SRSO_NO also needs the SRSO mitigation.
	 *
	 *    The "user -> user" attack, also known as SpectreBHB, requires RSB
	 *    clearing.
	 *
	 *    The "guest -> host" attack is mitigated by either eIBRS (not
	 *    IBRS!) or RSB clearing on vmexit.  Note that eIBRS
	 *    implementations with X86_BUG_EIBRS_PBRSB still need "lite" RSB
	 *    clearing which retires a single CALL before the first RET.
	 */


---8<---

From: Josh Poimboeuf <jpoimboe@kernel.org>
Subject: [PATCH] x86/bugs: Update insanely long comment about RSB attacks

The long comment above the setting of X86_FEATURE_RSB_CTXSW is a bit
confusing.  It starts out being about context switching specifically,
but then goes on to describe "user -> kernel" mitigations, which aren't
necessarily limited to context switches.

Clarify that it's about *all* RSB attacks and their mitigations.

For consistency, add the "guest -> host" mitigations as well.  Then the
comment above spectre_v2_determine_rsb_fill_type_at_vmexit() can be
removed and the overall line count is reduced.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 60 +++++++++++++-------------------------
 1 file changed, 20 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 47a01d4028f6..3dd1e504d706 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1581,26 +1581,6 @@ static void __init spec_ctrl_disable_kernel_rrsba(void)
 
 static void __init spectre_v2_determine_rsb_fill_type_at_vmexit(enum spectre_v2_mitigation mode)
 {
-	/*
-	 * Similar to context switches, there are two types of RSB attacks
-	 * after VM exit:
-	 *
-	 * 1) RSB underflow
-	 *
-	 * 2) Poisoned RSB entry
-	 *
-	 * When retpoline is enabled, both are mitigated by filling/clearing
-	 * the RSB.
-	 *
-	 * When IBRS is enabled, while #1 would be mitigated by the IBRS branch
-	 * prediction isolation protections, RSB still needs to be cleared
-	 * because of #2.  Note that SMEP provides no protection here, unlike
-	 * user-space-poisoned RSB entries.
-	 *
-	 * eIBRS should protect against RSB poisoning, but if the EIBRS_PBRSB
-	 * bug is present then a LITE version of RSB protection is required,
-	 * just a single call needs to retire before a RET is executed.
-	 */
 	switch (mode) {
 	case SPECTRE_V2_NONE:
 		return;
@@ -1818,43 +1798,43 @@ static void __init spectre_v2_select_mitigation(void)
 	pr_info("%s\n", spectre_v2_strings[mode]);
 
 	/*
-	 * If Spectre v2 protection has been enabled, fill the RSB during a
-	 * context switch.  In general there are two types of RSB attacks
-	 * across context switches, for which the CALLs/RETs may be unbalanced.
+	 * In general there are two types of RSB attacks:
 	 *
-	 * 1) RSB underflow
+	 * 1) RSB underflow ("Intel Retbleed")
 	 *
 	 *    Some Intel parts have "bottomless RSB".  When the RSB is empty,
 	 *    speculated return targets may come from the branch predictor,
 	 *    which could have a user-poisoned BTB or BHB entry.
 	 *
-	 *    AMD has it even worse: *all* returns are speculated from the BTB,
-	 *    regardless of the state of the RSB.
+	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack is
+	 *    mitigated by the IBRS branch prediction isolation properties, so
+	 *    the RSB buffer filling wouldn't be necessary to protect against
+	 *    this type of attack.
 	 *
-	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
-	 *    scenario is mitigated by the IBRS branch prediction isolation
-	 *    properties, so the RSB buffer filling wouldn't be necessary to
-	 *    protect against this type of attack.
+	 *    The "user -> user" attack is mitigated by RSB filling on context
+	 *    switch.
 	 *
-	 *    The "user -> user" attack scenario is mitigated by RSB filling.
+	 *    The "guest -> host" attack is mitigated by IBRS or eIBRS.
 	 *
 	 * 2) Poisoned RSB entry
 	 *
 	 *    If the 'next' in-kernel return stack is shorter than 'prev',
 	 *    'next' could be tricked into speculating with a user-poisoned RSB
-	 *    entry.
+	 *    entry.  Poisoned RSB entries can also be created by Branch Type
+	 *    Confusion ("AMD retbleed") or SRSO.
 	 *
-	 *    The "user -> kernel" attack scenario is mitigated by SMEP and
-	 *    eIBRS.
+	 *    The "user -> kernel" attack is mitigated by SMEP and eIBRS.  AMD
+	 *    without SRSO_NO also needs the SRSO mitigation.
 	 *
-	 *    The "user -> user" scenario, also known as SpectreBHB, requires
-	 *    RSB clearing.
+	 *    The "user -> user" attack, also known as SpectreBHB, requires RSB
+	 *    clearing.
 	 *
-	 * So to mitigate all cases, unconditionally fill RSB on context
-	 * switches.
-	 *
-	 * FIXME: Is this pointless for retbleed-affected AMD?
+	 *    The "guest -> host" attack is mitigated by either eIBRS (not
+	 *    IBRS!) or RSB clearing on vmexit.  Note that eIBRS
+	 *    implementations with X86_BUG_EIBRS_PBRSB still need "lite" RSB
+	 *    clearing which retires a single CALL before the first RET.
 	 */
+
 	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
Borislav Petkov Nov. 12, 2024, 11:58 a.m. UTC | #9
On Mon, Nov 11, 2024 at 05:46:44PM -0800, Josh Poimboeuf wrote:
> Subject: [PATCH] x86/bugs: Update insanely long comment about RSB attacks

Why don't you stick this insanely long comment in
Documentation/admin-guide/hw-vuln/ while at it?

Its place is hardly in the code. You can point to it from the code tho...
Shah, Amit Nov. 12, 2024, 3:16 p.m. UTC | #10
On Mon, 2024-11-11 at 17:46 -0800, Josh Poimboeuf wrote:
> On Tue, Nov 12, 2024 at 12:29:28AM +0000, Andrew Cooper wrote:
> > This is my take.  On AMD CPUs, there are two unrelated issues to
> > take
> > into account:
> > 
> > 1) SRSO
> > 
> > Affects anything which doesn't enumerate SRSO_NO, which is all
> > parts to
> > date including Zen5.
> > 
> > SRSO ends up overflowing the RAS with arbitrary BTB targets, such
> > that a
> > subsequent genuine RET follows a prediction which never came from a
> > real
> > CALL instruction.
> > 
> > Mitigations for SRSO are either safe-ret, or IBPB-on-entry.  Parts
> > without IBPB_RET using IBPB-on-entry need to manually flush the
> > RAS.
> > 
> > Importantly, SMEP does not protection you against SRSO across the
> > user->kernel boundary, because the bad RAS entries are arbitrary. 
> > New
> > in Zen5 is the SRSO_U/S_NO bit which says this case can't occur any
> > more.  So on Zen5, you can in principle get away without a RAS
> > flush on
> > entry.
> 
> Updated to mention SRSO:
> 
> 	/*
> 	 * In general there are two types of RSB attacks:
> 	 *
> 	 * 1) RSB underflow ("Intel Retbleed")
> 	 *
> 	 *    Some Intel parts have "bottomless RSB".  When the RSB
> is empty,
> 	 *    speculated return targets may come from the branch
> predictor,
> 	 *    which could have a user-poisoned BTB or BHB entry.
> 	 *
> 	 *    When IBRS or eIBRS is enabled, the "user -> kernel"
> attack is
> 	 *    mitigated by the IBRS branch prediction isolation
> properties, so
> 	 *    the RSB buffer filling wouldn't be necessary to
> protect against
> 	 *    this type of attack.
> 	 *
> 	 *    The "user -> user" attack is mitigated by RSB filling
> on context
> 	 *    switch.
> 	 *
> 	 *    The "guest -> host" attack is mitigated by IBRS or
> eIBRS.
> 	 *
> 	 * 2) Poisoned RSB entry
> 	 *
> 	 *    If the 'next' in-kernel return stack is shorter than
> 'prev',
> 	 *    'next' could be tricked into speculating with a user-
> poisoned RSB
> 	 *    entry.  Poisoned RSB entries can also be created by
> Branch Type
> 	 *    Confusion ("AMD retbleed") or SRSO.
> 	 *
> 	 *    The "user -> kernel" attack is mitigated by SMEP and
> eIBRS.  AMD
> 	 *    without SRSO_NO also needs the SRSO mitigation.
> 	 *
> 	 *    The "user -> user" attack, also known as SpectreBHB,
> requires RSB
> 	 *    clearing.
> 	 *
> 	 *    The "guest -> host" attack is mitigated by either
> eIBRS (not
> 	 *    IBRS!) or RSB clearing on vmexit.  Note that eIBRS
> 	 *    implementations with X86_BUG_EIBRS_PBRSB still need
> "lite" RSB
> 	 *    clearing which retires a single CALL before the first
> RET.
> 	 */

Thanks, Josh and Andrew.  This reads well to me.  In the context of
ERAPS, I'll end up adding a couple more sentences there as well.

		Amit
Pawan Gupta Nov. 12, 2024, 9:43 p.m. UTC | #11
On Mon, Nov 11, 2024 at 05:46:44PM -0800, Josh Poimboeuf wrote:
> +	 * 1) RSB underflow ("Intel Retbleed")
>  	 *
>  	 *    Some Intel parts have "bottomless RSB".  When the RSB is empty,
>  	 *    speculated return targets may come from the branch predictor,
>  	 *    which could have a user-poisoned BTB or BHB entry.
>  	 *
> -	 *    AMD has it even worse: *all* returns are speculated from the BTB,
> -	 *    regardless of the state of the RSB.
> +	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack is
> +	 *    mitigated by the IBRS branch prediction isolation properties, so
> +	 *    the RSB buffer filling wouldn't be necessary to protect against
> +	 *    this type of attack.
>  	 *
> -	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
> -	 *    scenario is mitigated by the IBRS branch prediction isolation
> -	 *    properties, so the RSB buffer filling wouldn't be necessary to
> -	 *    protect against this type of attack.
> +	 *    The "user -> user" attack is mitigated by RSB filling on context
> +	 *    switch.

user->user SpectreRSB is also mitigated by IBPB, so RSB filling is
unnecessary when IBPB is issued. Also, when an appication does not opted-in
for IBPB at context switch, spectre-v2 for that app is not mitigated,
filling RSB is only a half measure in that case.

Is RSB filling really serving any purpose for userspace?
Josh Poimboeuf Nov. 13, 2024, 8:21 p.m. UTC | #12
On Tue, Nov 12, 2024 at 01:43:48PM -0800, Pawan Gupta wrote:
> On Mon, Nov 11, 2024 at 05:46:44PM -0800, Josh Poimboeuf wrote:
> > +	 * 1) RSB underflow ("Intel Retbleed")
> >  	 *
> >  	 *    Some Intel parts have "bottomless RSB".  When the RSB is empty,
> >  	 *    speculated return targets may come from the branch predictor,
> >  	 *    which could have a user-poisoned BTB or BHB entry.
> >  	 *
> > -	 *    AMD has it even worse: *all* returns are speculated from the BTB,
> > -	 *    regardless of the state of the RSB.
> > +	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack is
> > +	 *    mitigated by the IBRS branch prediction isolation properties, so
> > +	 *    the RSB buffer filling wouldn't be necessary to protect against
> > +	 *    this type of attack.
> >  	 *
> > -	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
> > -	 *    scenario is mitigated by the IBRS branch prediction isolation
> > -	 *    properties, so the RSB buffer filling wouldn't be necessary to
> > -	 *    protect against this type of attack.
> > +	 *    The "user -> user" attack is mitigated by RSB filling on context
> > +	 *    switch.
> 
> user->user SpectreRSB is also mitigated by IBPB, so RSB filling is
> unnecessary when IBPB is issued. Also, when an appication does not opted-in
> for IBPB at context switch, spectre-v2 for that app is not mitigated,
> filling RSB is only a half measure in that case.
> 
> Is RSB filling really serving any purpose for userspace?

Indeed...

If we don't need to flush RSB for user->user, we'd only need to worry
about protecting the kernel.  Something like so?

  - eIBRS+!PBRSB:	no flush
  - eIBRS+PBRSB:	lite flush
  - everything else:	full flush

i.e., same logic as spectre_v2_determine_rsb_fill_type_at_vmexit(), but
also for context switches.
Josh Poimboeuf Nov. 13, 2024, 9:24 p.m. UTC | #13
On Tue, Nov 12, 2024 at 12:58:11PM +0100, Borislav Petkov wrote:
> On Mon, Nov 11, 2024 at 05:46:44PM -0800, Josh Poimboeuf wrote:
> > Subject: [PATCH] x86/bugs: Update insanely long comment about RSB attacks
> 
> Why don't you stick this insanely long comment in
> Documentation/admin-guide/hw-vuln/ while at it?
> 
> Its place is hardly in the code. You can point to it from the code tho...

There are a lot of subtle details to this $#!tstorm, and IMO we probably
wouldn't be having these discussions in the first place if the comment
lived in the docs, as most people seem to ignore them...
Borislav Petkov Nov. 13, 2024, 9:37 p.m. UTC | #14
On Wed, Nov 13, 2024 at 01:24:40PM -0800, Josh Poimboeuf wrote:
> There are a lot of subtle details to this $#!tstorm, and IMO we probably
> wouldn't be having these discussions in the first place if the comment
> lived in the docs, as most people seem to ignore them...

That's why I'm saying point to the docs from the code. You can't have a big
fat comment in the code about this but everything else in the hw-vuln docs.
Josh Poimboeuf Nov. 14, 2024, 12:43 a.m. UTC | #15
On Wed, Nov 13, 2024 at 10:37:24PM +0100, Borislav Petkov wrote:
> On Wed, Nov 13, 2024 at 01:24:40PM -0800, Josh Poimboeuf wrote:
> > There are a lot of subtle details to this $#!tstorm, and IMO we probably
> > wouldn't be having these discussions in the first place if the comment
> > lived in the docs, as most people seem to ignore them...
> 
> That's why I'm saying point to the docs from the code. You can't have a big
> fat comment in the code about this but everything else in the hw-vuln docs.

But those docs are user facing, describing the "what" for each
vulnerability individually.  They're basically historical documents
which don't evolve over time unless we tweak an interface or add a new
mitigation.

This comment relates to the "why" for the code itself (and its poor
confused developers), taking all the RSB-related vulnerabilities into
account.
Pawan Gupta Nov. 14, 2024, 1:55 a.m. UTC | #16
On Wed, Nov 13, 2024 at 12:21:05PM -0800, Josh Poimboeuf wrote:
> On Tue, Nov 12, 2024 at 01:43:48PM -0800, Pawan Gupta wrote:
> > On Mon, Nov 11, 2024 at 05:46:44PM -0800, Josh Poimboeuf wrote:
> > > +	 * 1) RSB underflow ("Intel Retbleed")
> > >  	 *
> > >  	 *    Some Intel parts have "bottomless RSB".  When the RSB is empty,
> > >  	 *    speculated return targets may come from the branch predictor,
> > >  	 *    which could have a user-poisoned BTB or BHB entry.
> > >  	 *
> > > -	 *    AMD has it even worse: *all* returns are speculated from the BTB,
> > > -	 *    regardless of the state of the RSB.
> > > +	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack is
> > > +	 *    mitigated by the IBRS branch prediction isolation properties, so
> > > +	 *    the RSB buffer filling wouldn't be necessary to protect against
> > > +	 *    this type of attack.
> > >  	 *
> > > -	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
> > > -	 *    scenario is mitigated by the IBRS branch prediction isolation
> > > -	 *    properties, so the RSB buffer filling wouldn't be necessary to
> > > -	 *    protect against this type of attack.
> > > +	 *    The "user -> user" attack is mitigated by RSB filling on context
> > > +	 *    switch.
> > 
> > user->user SpectreRSB is also mitigated by IBPB, so RSB filling is
> > unnecessary when IBPB is issued. Also, when an appication does not opted-in
> > for IBPB at context switch, spectre-v2 for that app is not mitigated,
> > filling RSB is only a half measure in that case.
> > 
> > Is RSB filling really serving any purpose for userspace?
> 
> Indeed...
> 
> If we don't need to flush RSB for user->user, we'd only need to worry
> about protecting the kernel.  Something like so?
> 
>   - eIBRS+!PBRSB:	no flush
>   - eIBRS+PBRSB:	lite flush

Yes for VMexit, but not at kernel entry. PBRSB requires an unbalanced RET,
and it is only a problem until the first retired CALL. At VMexit we do have
unbalanced RET but not at kernel entry.

>   - everything else:	full flush

> i.e., same logic as spectre_v2_determine_rsb_fill_type_at_vmexit(), but
> also for context switches.

Yes, assuming you mean user->kernel switch, and not process context switch.
Josh Poimboeuf Nov. 14, 2024, 2:31 a.m. UTC | #17
On Wed, Nov 13, 2024 at 05:55:05PM -0800, Pawan Gupta wrote:
> > > user->user SpectreRSB is also mitigated by IBPB, so RSB filling is
> > > unnecessary when IBPB is issued. Also, when an appication does not opted-in
> > > for IBPB at context switch, spectre-v2 for that app is not mitigated,
> > > filling RSB is only a half measure in that case.
> > > 
> > > Is RSB filling really serving any purpose for userspace?
> > 
> > Indeed...
> > 
> > If we don't need to flush RSB for user->user, we'd only need to worry
> > about protecting the kernel.  Something like so?
> > 
> >   - eIBRS+!PBRSB:	no flush
> >   - eIBRS+PBRSB:	lite flush
> 
> Yes for VMexit, but not at kernel entry. PBRSB requires an unbalanced RET,
> and it is only a problem until the first retired CALL. At VMexit we do have
> unbalanced RET but not at kernel entry.
> 
> >   - everything else:	full flush
> 
> > i.e., same logic as spectre_v2_determine_rsb_fill_type_at_vmexit(), but
> > also for context switches.
> 
> Yes, assuming you mean user->kernel switch, and not process context switch.

Actually I did mean context switch.  AFAIK we don't need to flush RSB at
kernel entry.

If user->user RSB is already mitigated by IBPB, then at context switch
we only have to worry about user->kernel.  e.g., if 'next' has more (in
kernel) RETs then 'prev' had (in kernel) CALLs, the user could trigger
RSB underflow or corruption inside the kernel after the context switch.

Doesn't eIBRS already protect against that?

For PBRSB, I guess we don't need to worry about that since there would
be at least one kernel CALL before context switch.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 47a01d4028f6..0aa629b5537d 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1828,9 +1828,6 @@  static void __init spectre_v2_select_mitigation(void)
 	 *    speculated return targets may come from the branch predictor,
 	 *    which could have a user-poisoned BTB or BHB entry.
 	 *
-	 *    AMD has it even worse: *all* returns are speculated from the BTB,
-	 *    regardless of the state of the RSB.
-	 *
 	 *    When IBRS or eIBRS is enabled, the "user -> kernel" attack
 	 *    scenario is mitigated by the IBRS branch prediction isolation
 	 *    properties, so the RSB buffer filling wouldn't be necessary to
@@ -1852,8 +1849,6 @@  static void __init spectre_v2_select_mitigation(void)
 	 *
 	 * So to mitigate all cases, unconditionally fill RSB on context
 	 * switches.
-	 *
-	 * FIXME: Is this pointless for retbleed-affected AMD?
 	 */
 	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");