eIBRS protects against RSB underflow/poisoning attacks. Adding
retpoline to the mix doesn't change that. Retpoline has a balanced
CALL/RET anyway.
So the current full RSB filling on VMEXIT with eIBRS+retpoline is
overkill. Disable it (or do the VMEXIT_LITE mitigation if needed).
Suggested-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Reviewed-by: Amit Shah <amit.shah@amd.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
arch/x86/kernel/cpu/bugs.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 47a01d4028f6..68bed17f0980 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1605,20 +1605,20 @@ static void __init spectre_v2_determine_rsb_fill_type_at_vmexit(enum spectre_v2_
case SPECTRE_V2_NONE:
return;
- case SPECTRE_V2_EIBRS_LFENCE:
case SPECTRE_V2_EIBRS:
+ case SPECTRE_V2_EIBRS_LFENCE:
+ case SPECTRE_V2_EIBRS_RETPOLINE:
if (boot_cpu_has_bug(X86_BUG_EIBRS_PBRSB)) {
- setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT_LITE);
pr_info("Spectre v2 / PBRSB-eIBRS: Retire a single CALL on VMEXIT\n");
+ setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT_LITE);
}
return;
- case SPECTRE_V2_EIBRS_RETPOLINE:
case SPECTRE_V2_RETPOLINE:
case SPECTRE_V2_LFENCE:
case SPECTRE_V2_IBRS:
- setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
pr_info("Spectre v2 / SpectreRSB : Filling RSB on VMEXIT\n");
+ setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
return;
}
--
2.47.0On Thu, Nov 21, 2024 at 12:07:18PM -0800, Josh Poimboeuf wrote:
> eIBRS protects against RSB underflow/poisoning attacks. Adding
> retpoline to the mix doesn't change that. Retpoline has a balanced
> CALL/RET anyway.
This is exactly why I've been wanting for us to document our mitigations for
a long time now.
A bunch of statements above for which I can only rhyme up they're correct if
I search for the vendor docs. On the AMD side I've found:
"When Automatic IBRS is enabled, the internal return address stack used for
return address predictions is cleared on VMEXIT."
APM v2, p. 58/119
For the Intel side I'm not that lucky. There's something here:
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/branch-history-injection.html
Or is it this one:
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#inpage-nav-1-3-undefined
Or is this written down explicitly in some other doc?
In any case, I'd like for us to do have a piece of text accompanying such
patches, perhaps here:
Documentation/admin-guide/hw-vuln/spectre.rst
which quotes the vendor docs.
The current thread(s) on the matter already show how much confused we all are
by all the possible mitigation options, uarch speculative dances etc etc.
> So the current full RSB filling on VMEXIT with eIBRS+retpoline is
> overkill. Disable it (or do the VMEXIT_LITE mitigation if needed).
>
> Suggested-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Reviewed-by: Amit Shah <amit.shah@amd.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
> arch/x86/kernel/cpu/bugs.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 47a01d4028f6..68bed17f0980 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1605,20 +1605,20 @@ static void __init spectre_v2_determine_rsb_fill_type_at_vmexit(enum spectre_v2_
> case SPECTRE_V2_NONE:
> return;
>
> - case SPECTRE_V2_EIBRS_LFENCE:
> case SPECTRE_V2_EIBRS:
> + case SPECTRE_V2_EIBRS_LFENCE:
> + case SPECTRE_V2_EIBRS_RETPOLINE:
> if (boot_cpu_has_bug(X86_BUG_EIBRS_PBRSB)) {
> - setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT_LITE);
> pr_info("Spectre v2 / PBRSB-eIBRS: Retire a single CALL on VMEXIT\n");
> + setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT_LITE);
Why are you swapping those?
> }
> return;
>
> - case SPECTRE_V2_EIBRS_RETPOLINE:
> case SPECTRE_V2_RETPOLINE:
> case SPECTRE_V2_LFENCE:
> case SPECTRE_V2_IBRS:
> - setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
> pr_info("Spectre v2 / SpectreRSB : Filling RSB on VMEXIT\n");
> + setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
Ditto?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sat, Nov 30, 2024 at 04:31:25PM +0100, Borislav Petkov wrote:
> On Thu, Nov 21, 2024 at 12:07:18PM -0800, Josh Poimboeuf wrote:
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -1605,20 +1605,20 @@ static void __init spectre_v2_determine_rsb_fill_type_at_vmexit(enum spectre_v2_
> > case SPECTRE_V2_NONE:
> > return;
> >
> > - case SPECTRE_V2_EIBRS_LFENCE:
> > case SPECTRE_V2_EIBRS:
> > + case SPECTRE_V2_EIBRS_LFENCE:
> > + case SPECTRE_V2_EIBRS_RETPOLINE:
> > if (boot_cpu_has_bug(X86_BUG_EIBRS_PBRSB)) {
> > - setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT_LITE);
> > pr_info("Spectre v2 / PBRSB-eIBRS: Retire a single CALL on VMEXIT\n");
> > + setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT_LITE);
>
> Why are you swapping those?
>
> > }
> > return;
> >
> > - case SPECTRE_V2_EIBRS_RETPOLINE:
> > case SPECTRE_V2_RETPOLINE:
> > case SPECTRE_V2_LFENCE:
> > case SPECTRE_V2_IBRS:
> > - setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
> > pr_info("Spectre v2 / SpectreRSB : Filling RSB on VMEXIT\n");
> > + setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
>
> Ditto?
It's more readable that way, similar to how a comment goes before code.
--
Josh
On Sat, Nov 30, 2024 at 04:31:25PM +0100, Borislav Petkov wrote: > On Thu, Nov 21, 2024 at 12:07:18PM -0800, Josh Poimboeuf wrote: > > eIBRS protects against RSB underflow/poisoning attacks. Adding > > retpoline to the mix doesn't change that. Retpoline has a balanced > > CALL/RET anyway. > > This is exactly why I've been wanting for us to document our mitigations for > a long time now. > > A bunch of statements above for which I can only rhyme up they're correct if > I search for the vendor docs. On the AMD side I've found: > > "When Automatic IBRS is enabled, the internal return address stack used for > return address predictions is cleared on VMEXIT." > > APM v2, p. 58/119 > > For the Intel side I'm not that lucky. There's something here: > > https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/branch-history-injection.html > > Or is it this one: > > https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#inpage-nav-1-3-undefined > > Or is this written down explicitly in some other doc? It is in this doc: https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/indirect-branch-restricted-speculation.html "Processors with enhanced IBRS still support the usage model where IBRS is set only in the OS/VMM for OSes that enable SMEP. To do this, such processors will ensure that guest behavior cannot control the RSB after a VM exit once IBRS is set, even if IBRS was not set at the time of the VM exit."
On Mon, Dec 02, 2024 at 03:35:21PM -0800, Pawan Gupta wrote:
> It is in this doc:
>
> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/indirect-branch-restricted-speculation.html
>
I hope those URLs remain more stable than past experience shows.
> "Processors with enhanced IBRS still support the usage model where IBRS is
> set only in the OS/VMM for OSes that enable SMEP. To do this, such
> processors will ensure that guest behavior cannot control the RSB after a
> VM exit once IBRS is set, even if IBRS was not set at the time of the VM
> exit."
ACK, thanks.
Now, can we pls add those excerpts to Documentation/ and point to them from
the code so that it is crystal clear why it is ok?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Dec 03, 2024 at 12:20:15PM +0100, Borislav Petkov wrote: > On Mon, Dec 02, 2024 at 03:35:21PM -0800, Pawan Gupta wrote: > > It is in this doc: > > > > https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/indirect-branch-restricted-speculation.html > > > > I hope those URLs remain more stable than past experience shows. > > > "Processors with enhanced IBRS still support the usage model where IBRS is > > set only in the OS/VMM for OSes that enable SMEP. To do this, such > > processors will ensure that guest behavior cannot control the RSB after a > > VM exit once IBRS is set, even if IBRS was not set at the time of the VM > > exit." > > ACK, thanks. > > Now, can we pls add those excerpts to Documentation/ and point to them from > the code so that it is crystal clear why it is ok? Ok, I'll try to write up a document. I'm thinking it should go in its own return-based-attacks.rst file rather than spectre.rst, which is more of an outdated historical document at this point. And we want this document to actually be read (and kept up to date) by developers instead of mostly ignored like the others. -- Josh
On Thu, Dec 05, 2024 at 03:12:07PM -0800, Josh Poimboeuf wrote:
> Ok, I'll try to write up a document. I'm thinking it should go in its
> own return-based-attacks.rst file rather than spectre.rst, which is more
> of an outdated historical document at this point.
Thanks!
> And we want this document to actually be read (and kept up to date) by
> developers instead of mostly ignored like the others.
Good idea.
Now how do we enforce this?
We could add a rule to checkpatch or to some other patch checking script
like mine here for example :-)
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=vp
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sat, 2024-11-30 at 16:31 +0100, Borislav Petkov wrote: > On Thu, Nov 21, 2024 at 12:07:18PM -0800, Josh Poimboeuf wrote: > > eIBRS protects against RSB underflow/poisoning attacks. Adding > > retpoline to the mix doesn't change that. Retpoline has a balanced > > CALL/RET anyway. > > This is exactly why I've been wanting for us to document our > mitigations for > a long time now. FWIW, I'd say we have fairly decent documentation with commit messages + code + comments in code. > A bunch of statements above for which I can only rhyme up they're > correct if > I search for the vendor docs. On the AMD side I've found: [...] > In any case, I'd like for us to do have a piece of text accompanying > such > patches, perhaps here: > > Documentation/admin-guide/hw-vuln/spectre.rst > > which quotes the vendor docs. If you're saying that we need *additional* documentation that replicates hw manuals and the knowledge we have in our commit + code + comments, that I agree with. I got the feeling earlier, though, that you were saying we need that documentation *instead of* the current comments-within-code, and that didn't sound like the right thing to do. > The current thread(s) on the matter already show how much confused we > all are > by all the possible mitigation options, uarch speculative dances etc > etc. ... and the code flows and looks much better after this commit (for SpectreRSB at least), which is a huge plus. It's important to note that at some point in the past we got vulnerabilities and hw features/quirks one after the other, and we kept tacking mitigation code on top of the existing one -- because that's what you need to do during an embargo period. Now's the moment when we're consolidating it all while taking stock of the overall situation. This looks like a sound way to go about taking a higher-level view and simplifying the code. I doubt we'd want to do things any other way; and much less doing this kind of an exercise during an embargo. Moving comments out of the code will only add to frustration during embargo periods. Just my 2c Amit
On Mon, Dec 02, 2024 at 11:15:24AM +0000, Shah, Amit wrote:
> FWIW, I'd say we have fairly decent documentation with commit messages
> + code + comments in code.
You mean everytime we want to swap back in why we did some mitigation decision
the way we did, we should do git archeology and go on a chase?
I've been doing it for years now and it is an awful experience. Absolutely
certainly can be better.
> If you're saying that we need *additional* documentation that replicates hw
> manuals and the knowledge we have in our commit + code + comments, that
> I agree with.
We need documentation or at least pointers to vendor documentation which
explain why we're doing this exact type of mitigation and why we're not doing
other. Why is it ok to disable SMT, why is it not, for example? This patch is
another good example.
> I got the feeling earlier, though, that you were saying we need that
> documentation *instead of* the current comments-within-code, and that
> didn't sound like the right thing to do.
Comments within the code are fine. Big fat comment which is almost a page long
is pushing it. It can very well exist in Documentation/ and a short comment
can point to it.
And in Documentation/ we can go nuts and explain away...
> ... and the code flows and looks much better after this commit (for
> SpectreRSB at least), which is a huge plus.
Why does it look better?
Because of the move of SPECTRE_V2_EIBRS_RETPOLINE?
> It's important to note that at some point in the past we got vulnerabilities
> and hw features/quirks one after the other, and we kept tacking mitigation
> code on top of the existing one -- because that's what you need to do during
> an embargo period. Now's the moment when we're consolidating it all while
> taking stock of the overall situation.
Yes, that's exactly why I'm pushing for improving the documentation and the
reasoning for each mitigation. Exactly because stuff is not under NDA anymore
and we can do all the debating in the open, and the dust has settled.
> This looks like a sound way to go about taking a higher-level view and
> simplifying the code.
Yap.
And to give you another argument for it: when we clean it up nicely, it'll be
easier to add new mitigations. :)
And no, I don't want more but no one's listening to me anyway...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
© 2016 - 2025 Red Hat, Inc.