Re: [PATCH 1/5] x86/cpufeatures: Define feature bits to support mitigation of PSF
On 4/9/2021 3:19 PM, Borislav Petkov wrote: > On Fri, Apr 09, 2021 at 02:45:23PM -0500, Saripalli, RK wrote: >> Yes, these options should be fine for now. >> Like you said, if we get the need to add prctl and seccomp, I can always do >> that later. >> >> What do you think auto should default to?. >> In SSBD case, I believe auto defaults to prctl or seccomp. >> Since we will not have that here, we should choose something for auto. > > Or not add it yet. Just have "on" and "off" for now. OK > > Which begs the question should this be controllable by the mitigations= > switch too? > > I wanna say, let's have people evaluate and play with it first and > we can add it to that switch later. As long as we don't change the > user-visible controls - if anything we'll be extending them later, > potentially - we should be fine usage-wise and from user visibility POV. Agreed. We can add this later. > >> All the other mitigation x86 mitigation code goes into kernel/cpu/bugs.c. >> I think psf_cmdline() or equivalent also belongs there and not in >> kernel/cpu/amd.c. > > It being AMD-specific, it can dwell in amd.c initially. OK > > Thx. >
Re: [PATCH 1/5] x86/cpufeatures: Define feature bits to support mitigation of PSF
On Fri, Apr 09, 2021 at 02:45:23PM -0500, Saripalli, RK wrote: > Yes, these options should be fine for now. > Like you said, if we get the need to add prctl and seccomp, I can always do > that later. > > What do you think auto should default to?. > In SSBD case, I believe auto defaults to prctl or seccomp. > Since we will not have that here, we should choose something for auto. Or not add it yet. Just have "on" and "off" for now. Which begs the question should this be controllable by the mitigations= switch too? I wanna say, let's have people evaluate and play with it first and we can add it to that switch later. As long as we don't change the user-visible controls - if anything we'll be extending them later, potentially - we should be fine usage-wise and from user visibility POV. > All the other mitigation x86 mitigation code goes into kernel/cpu/bugs.c. > I think psf_cmdline() or equivalent also belongs there and not in > kernel/cpu/amd.c. It being AMD-specific, it can dwell in amd.c initially. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 1/5] x86/cpufeatures: Define feature bits to support mitigation of PSF
Boris, thank you.
On 4/9/2021 2:39 PM, Borislav Petkov wrote:
> On Fri, Apr 09, 2021 at 01:22:49PM -0500, Saripalli, RK wrote:
>>> And I think you don't need this one either if we do a "light" controls
>>> thing but lemme look at the rest first.
>
> Ok, and what I mean with "lite" version is something like this below
> which needs finishing and testing.
>
> Initially, it could support the cmdline params:
>
> predict_store_fwd={on,off,auto}
>
> to give people the opportunity to experiment with the feature.
>
> If it turns out that prctl and seccomp per-task toggling is needed then
> sure, we can extend but I don't see the reason for a whole separate set
> of options yet. Especially is ssbd already controls this.
>
> AFAICT, of course and if I'm not missing some other aspect here.
>
> Thx.
Yes, these options should be fine for now.
Like you said, if we get the need to add prctl and seccomp, I can always do
that later.
What do you think auto should default to?.
In SSBD case, I believe auto defaults to prctl or seccomp.
Since we will not have that here, we should choose something for auto.
>
> ---
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 2d11384dc9ab..226b73700f88 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1165,3 +1165,22 @@ void set_dr_addr_mask(unsigned long mask, int dr)
> break;
> }
> }
> +
> +static int __init psf_cmdline(char *str)
> +{
> + if (!boot_cpu_has(X86_FEATURE_PSFD))
> + return 0;
> +
> + if (!str)
> + return -EINVAL;
> +
> + if (!strcmp(str, "off")) {
> + x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
> + setup_clear_cpu_cap(X86_FEATURE_PSFD);
> + }
> +
> + return 0;
> +}
> +early_param("predict_store_fwd", psf_cmdline);
> +
> +
>
All the other mitigation x86 mitigation code goes into kernel/cpu/bugs.c.
I think psf_cmdline() or equivalent also belongs there and not in
kernel/cpu/amd.c.
Looking forward to your feedback.
Thanks,
RK
Re: [PATCH 1/5] x86/cpufeatures: Define feature bits to support mitigation of PSF
On Fri, Apr 09, 2021 at 01:22:49PM -0500, Saripalli, RK wrote:
> > And I think you don't need this one either if we do a "light" controls
> > thing but lemme look at the rest first.
Ok, and what I mean with "lite" version is something like this below
which needs finishing and testing.
Initially, it could support the cmdline params:
predict_store_fwd={on,off,auto}
to give people the opportunity to experiment with the feature.
If it turns out that prctl and seccomp per-task toggling is needed then
sure, we can extend but I don't see the reason for a whole separate set
of options yet. Especially is ssbd already controls this.
AFAICT, of course and if I'm not missing some other aspect here.
Thx.
---
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 2d11384dc9ab..226b73700f88 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1165,3 +1165,22 @@ void set_dr_addr_mask(unsigned long mask, int dr)
break;
}
}
+
+static int __init psf_cmdline(char *str)
+{
+ if (!boot_cpu_has(X86_FEATURE_PSFD))
+ return 0;
+
+ if (!str)
+ return -EINVAL;
+
+ if (!strcmp(str, "off")) {
+ x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
+ setup_clear_cpu_cap(X86_FEATURE_PSFD);
+ }
+
+ return 0;
+}
+early_param("predict_store_fwd", psf_cmdline);
+
+
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 1/5] x86/cpufeatures: Define feature bits to support mitigation of PSF
On 4/9/2021 12:41 PM, Borislav Petkov wrote: > On Tue, Apr 06, 2021 at 10:50:00AM -0500, Ramakrishna Saripalli wrote: >> diff --git a/arch/x86/include/asm/cpufeatures.h >> b/arch/x86/include/asm/cpufeatures.h >> index cc96e26d69f7..21e7f8d0d7d9 100644 >> --- a/arch/x86/include/asm/cpufeatures.h >> +++ b/arch/x86/include/asm/cpufeatures.h >> @@ -201,7 +201,7 @@ >> #define X86_FEATURE_INVPCID_SINGLE ( 7*32+ 7) /* Effectively INVPCID && >> CR4.PCIDE=1 */ >> #define X86_FEATURE_HW_PSTATE ( 7*32+ 8) /* AMD HW-PState */ >> #define X86_FEATURE_PROC_FEEDBACK ( 7*32+ 9) /* AMD ProcFeedbackInterface >> */ >> -/* FREE!( 7*32+10) */ >> +#define X86_FEATURE_PSFD( 7*32+10) /* Predictive Store Forward >> Disable */ > > You don't need this one... Boris, I added this bit so we could detect later that PSF is supported. But since PSF is AMD specific for now, I guess I will go along with your suggestions. > >> #define X86_FEATURE_PTI ( 7*32+11) /* Kernel Page Table >> Isolation enabled */ >> #define X86_FEATURE_RETPOLINE ( 7*32+12) /* "" Generic >> Retpoline mitigation for Spectre variant 2 */ >> #define X86_FEATURE_RETPOLINE_AMD ( 7*32+13) /* "" AMD Retpoline >> mitigation for Spectre variant 2 */ >> @@ -309,6 +309,7 @@ >> #define X86_FEATURE_AMD_SSBD(13*32+24) /* "" Speculative >> Store Bypass Disable */ >> #define X86_FEATURE_VIRT_SSBD (13*32+25) /* Virtualized >> Speculative Store Bypass Disable */ >> #define X86_FEATURE_AMD_SSB_NO (13*32+26) /* "" Speculative >> Store Bypass is fixed in hardware. */ >> +#define X86_FEATURE_AMD_PSFD(13*32+28) /* "" Predictive >> Store Forward Disable */ > > ... when you have this one. And this one is AMD-specific so you can just > as well call it X86_FEATURE_PSFD and remove the "". Ok > >> >> /* Thermal and Power Management Leaf, CPUID level 0x0006 (EAX), word 14 >> */ >> #define X86_FEATURE_DTHERM (14*32+ 0) /* Digital Thermal Sensor */ >> @@ -428,5 +429,6 @@ >> #define X86_BUG_TAA X86_BUG(22) /* CPU is affected by TSX >> Async Abort(TAA) */ >> #define X86_BUG_ITLB_MULTIHIT X86_BUG(23) /* CPU may incur >> MCE during certain page attribute changes */ >> #define X86_BUG_SRBDS X86_BUG(24) /* CPU may leak RNG >> bits if not mitigated */ >> +#define X86_BUG_PSF X86_BUG(25) /* CPU is affected by >> Predictive Store Forwarding attack */ > > And I think you don't need this one either if we do a "light" controls > thing but lemme look at the rest first. > Ok. > Thx. >
Re: [PATCH 1/5] x86/cpufeatures: Define feature bits to support mitigation of PSF
On Tue, Apr 06, 2021 at 10:50:00AM -0500, Ramakrishna Saripalli wrote: > diff --git a/arch/x86/include/asm/cpufeatures.h > b/arch/x86/include/asm/cpufeatures.h > index cc96e26d69f7..21e7f8d0d7d9 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -201,7 +201,7 @@ > #define X86_FEATURE_INVPCID_SINGLE ( 7*32+ 7) /* Effectively INVPCID && > CR4.PCIDE=1 */ > #define X86_FEATURE_HW_PSTATE( 7*32+ 8) /* AMD HW-PState */ > #define X86_FEATURE_PROC_FEEDBACK( 7*32+ 9) /* AMD ProcFeedbackInterface > */ > -/* FREE!( 7*32+10) */ > +#define X86_FEATURE_PSFD ( 7*32+10) /* Predictive Store Forward > Disable */ You don't need this one... > #define X86_FEATURE_PTI ( 7*32+11) /* Kernel Page Table > Isolation enabled */ > #define X86_FEATURE_RETPOLINE( 7*32+12) /* "" Generic > Retpoline mitigation for Spectre variant 2 */ > #define X86_FEATURE_RETPOLINE_AMD( 7*32+13) /* "" AMD Retpoline > mitigation for Spectre variant 2 */ > @@ -309,6 +309,7 @@ > #define X86_FEATURE_AMD_SSBD (13*32+24) /* "" Speculative Store > Bypass Disable */ > #define X86_FEATURE_VIRT_SSBD(13*32+25) /* Virtualized > Speculative Store Bypass Disable */ > #define X86_FEATURE_AMD_SSB_NO (13*32+26) /* "" Speculative > Store Bypass is fixed in hardware. */ > +#define X86_FEATURE_AMD_PSFD (13*32+28) /* "" Predictive Store > Forward Disable */ ... when you have this one. And this one is AMD-specific so you can just as well call it X86_FEATURE_PSFD and remove the "". > > /* Thermal and Power Management Leaf, CPUID level 0x0006 (EAX), word 14 > */ > #define X86_FEATURE_DTHERM (14*32+ 0) /* Digital Thermal Sensor */ > @@ -428,5 +429,6 @@ > #define X86_BUG_TAA X86_BUG(22) /* CPU is affected by TSX > Async Abort(TAA) */ > #define X86_BUG_ITLB_MULTIHITX86_BUG(23) /* CPU may incur > MCE during certain page attribute changes */ > #define X86_BUG_SRBDSX86_BUG(24) /* CPU may leak RNG > bits if not mitigated */ > +#define X86_BUG_PSF X86_BUG(25) /* CPU is affected by > Predictive Store Forwarding attack */ And I think you don't need this one either if we do a "light" controls thing but lemme look at the rest first. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
