Re: [v6 1/1] x86/bugs: Implement mitigation for Predictive Store Forwarding
From: Saripalli, RK
Date: Wed May 19 2021 - 09:19:54 EST
On 5/19/2021 12:38 AM, Pawan Gupta wrote:
> On 17.05.2021 17:00, Ramakrishna Saripalli wrote:
>> From: Ramakrishna Saripalli <rk.saripalli@xxxxxxx>
>>
>> Certain AMD processors feature a new technology called Predictive Store
>> Forwarding (PSF).
>>
>> PSF is a micro-architectural optimization designed to improve the
>> performance of code execution by predicting dependencies between
>> loads and stores.
>>
>> Incorrect PSF predictions can occur due to two reasons.
>>
>> - It is possible that the load/store pair may have had dependency for
>> a while but the dependency has stopped because the address in the
>> load/store pair has changed.
>>
>> - Second source of incorrect PSF prediction can occur because of an alias
>> in the PSF predictor structure stored in the microarchitectural state.
>> PSF predictor tracks load/store pair based on portions of instruction
>> pointer. It is possible that a load/store pair which does have a
>> dependency may be aliased by another load/store pair which does not have
>> the same dependency. This can result in incorrect speculation.
>>
>> Software may be able to detect this aliasing and perform side-channel
>> attacks.
>>
>> All CPUs that implement PSF provide one bit to disable this feature.
>> If the bit to disable this feature is available, it means that the CPU
>> implements PSF feature and is therefore vulnerable to PSF risks.
>>
>> The bits that are introduced
>>
>> X86_FEATURE_PSFD: CPUID_Fn80000008_EBX[28] ("PSF disable")
>> If this bit is 1, CPU implements PSF and PSF control
>> via SPEC_CTRL_MSR is supported in the CPU.
>>
>> All AMD processors that support PSF implement a bit in
>> SPEC_CTRL MSR (0x48) to disable or enable Predictive Store
>> Forwarding.
>>
>> PSF control introduces a new kernel parameter called
>> predictive_store_fwd_disable.
>>
>> Kernel parameter predictive_store_fwd_disable has the following values
>>
>> - on. Disable PSF on all CPUs.
>>
>> - off. Enable PSF on all CPUs.
>> This is also the default setting.
>>
>> Signed-off-by: Ramakrishna Saripalli<rk.saripalli@xxxxxxx>
>> ---
>> .../admin-guide/kernel-parameters.txt | 5 +
>> arch/x86/include/asm/cpufeatures.h | 1 +
>> arch/x86/include/asm/msr-index.h | 2 +
>> arch/x86/include/asm/nospec-branch.h | 6 ++
>> arch/x86/kernel/cpu/bugs.c | 94 +++++++++++++++++++
>> 5 files changed, 108 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 04545725f187..a5f694dccb24 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3940,6 +3940,11 @@
>> Format: {"off"}
>> Disable Hardware Transactional Memory
>>
>> + predictive_store_fwd_disable= [X86] This option controls PSF.
>> + off - Turns on PSF.
>> + on - Turns off PSF.
>> + default : off.
>> +
>> preempt= [KNL]
>> Select preemption mode if you have CONFIG_PREEMPT_DYNAMIC
>> none - Limited to cond_resched() calls
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index cc96e26d69f7..078f46022293 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -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_PSFD (13*32+28) /* Predictive Store Forward Disable */
>>
>> /* Thermal and Power Management Leaf, CPUID level 0x00000006 (EAX), word 14 */
>> #define X86_FEATURE_DTHERM (14*32+ 0) /* Digital Thermal Sensor */
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 546d6ecf0a35..f569918c8754 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -51,6 +51,8 @@
>> #define SPEC_CTRL_STIBP BIT(SPEC_CTRL_STIBP_SHIFT) /* STIBP mask */
>> #define SPEC_CTRL_SSBD_SHIFT 2 /* Speculative Store Bypass Disable bit */
>> #define SPEC_CTRL_SSBD BIT(SPEC_CTRL_SSBD_SHIFT) /* Speculative Store Bypass Disable */
>> +#define SPEC_CTRL_PSFD_SHIFT 7
>> +#define SPEC_CTRL_PSFD BIT(SPEC_CTRL_PSFD_SHIFT) /* Predictive Store Forwarding Disable */
>>
>> #define MSR_IA32_PRED_CMD 0x00000049 /* Prediction Command */
>> #define PRED_CMD_IBPB BIT(0) /* Indirect Branch Prediction Barrier */
>> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
>> index cb9ad6b73973..1231099e5c07 100644
>> --- a/arch/x86/include/asm/nospec-branch.h
>> +++ b/arch/x86/include/asm/nospec-branch.h
>> @@ -198,6 +198,12 @@ enum ssb_mitigation {
>> SPEC_STORE_BYPASS_SECCOMP,
>> };
>>
>> +/* The Predictive Store forward control variant */
>> +enum psf_mitigation {
>> + PREDICTIVE_STORE_FORWARD_NONE,
>> + PREDICTIVE_STORE_FORWARD_DISABLE,
>> +};
>> +
>> extern char __indirect_thunk_start[];
>> extern char __indirect_thunk_end[];
>>
>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>> index d41b70fe4918..c80ef4d86b72 100644
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -38,6 +38,7 @@
>> static void __init spectre_v1_select_mitigation(void);
>> static void __init spectre_v2_select_mitigation(void);
>> static void __init ssb_select_mitigation(void);
>> +static void __init psf_select_mitigation(void);
>> static void __init l1tf_select_mitigation(void);
>> static void __init mds_select_mitigation(void);
>> static void __init mds_print_mitigation(void);
>> @@ -107,6 +108,7 @@ void __init check_bugs(void)
>> spectre_v1_select_mitigation();
>> spectre_v2_select_mitigation();
>> ssb_select_mitigation();
>> + psf_select_mitigation();
>> l1tf_select_mitigation();
>> mds_select_mitigation();
>> taa_select_mitigation();
>> @@ -1195,6 +1197,98 @@ static void ssb_select_mitigation(void)
>> pr_info("%s\n", ssb_strings[ssb_mode]);
>> }
>>
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "Predictive Store Forward: " fmt
>> +
>> +static enum psf_mitigation psf_mode __ro_after_init = PREDICTIVE_STORE_FORWARD_NONE;
>> +
>> +/* The kernel command line selection */
>> +enum psf_mitigation_cmd {
>> + PREDICTIVE_STORE_FORWARD_CMD_NONE,
>> + PREDICTIVE_STORE_FORWARD_CMD_ON,
>> +};
>> +
>> +static const char * const psf_strings[] = {
>> + [PREDICTIVE_STORE_FORWARD_NONE] = "Vulnerable",
>> + [PREDICTIVE_STORE_FORWARD_DISABLE] = "Mitigation: Predictive Store Forward disabled",
>> +};
>> +
>> +static const struct {
>> + const char *option;
>> + enum psf_mitigation_cmd cmd;
>> +} psf_mitigation_options[] __initconst = {
>> + { "on", PREDICTIVE_STORE_FORWARD_CMD_ON }, /* Disable Speculative Store Bypass */
>> + { "off", PREDICTIVE_STORE_FORWARD_CMD_NONE }, /* Don't touch Speculative Store Bypass */
>> +};
>> +
>> +static enum psf_mitigation_cmd __init psf_parse_cmdline(void)
>> +{
>> + enum psf_mitigation_cmd cmd = PREDICTIVE_STORE_FORWARD_CMD_NONE;
>> + char arg[20];
>> + int ret, i;
>> +
>> + ret = cmdline_find_option(boot_command_line, "predictive_store_fwd_disable",
>> + arg, sizeof(arg));
>> + if (ret < 0)
>> + return PREDICTIVE_STORE_FORWARD_CMD_NONE;
>> +
>> + for (i = 0; i < ARRAY_SIZE(psf_mitigation_options); i++) {
>> + if (!match_option(arg, ret, psf_mitigation_options[i].option))
>> + continue;
>> +
>> + cmd = psf_mitigation_options[i].cmd;
>> + break;
>> + }
>> +
>> + if (i >= ARRAY_SIZE(psf_mitigation_options)) {
>> + pr_err("unknown option (%s). Switching to AUTO select\n", arg);
>> + return PREDICTIVE_STORE_FORWARD_CMD_NONE;
>> + }
>> +
>> + return cmd;
>> +}
>> +
>> +static enum psf_mitigation __init __psf_select_mitigation(void)
>> +{
>> + enum psf_mitigation mode = PREDICTIVE_STORE_FORWARD_NONE;
>> + enum psf_mitigation_cmd cmd;
>> +
>> + if (!boot_cpu_has(X86_FEATURE_PSFD))
>> + return mode;
>> +
>> + cmd = psf_parse_cmdline();
>> +
>> + switch (cmd) {
>> + case PREDICTIVE_STORE_FORWARD_CMD_ON:
>> + mode = PREDICTIVE_STORE_FORWARD_DISABLE;
>> + break;
>> + default:
>> + mode = PREDICTIVE_STORE_FORWARD_NONE;
>> + break;
>> + }
>> +
>> + x86_spec_ctrl_mask |= SPEC_CTRL_PSFD;
>> +
>> + if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
>> + mode = PREDICTIVE_STORE_FORWARD_DISABLE;
>> +
>> + if (mode == PREDICTIVE_STORE_FORWARD_DISABLE) {
>> + setup_force_cpu_cap(X86_FEATURE_PSFD);
>
> Why do we need to force set X86_FEATURE_PSFD here? Control will not
> reach here if this is not already set (because of boot_cpu_has() check
> at function entry).
>
>> + x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
>> + wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>> + }
>> +
>> + return mode;
>> +}
>> +
>> +static void psf_select_mitigation(void)
>> +{
>> + psf_mode = __psf_select_mitigation();
>> +
>> + if (boot_cpu_has(X86_FEATURE_PSFD))
>> + pr_info("%s\n", psf_strings[psf_mode]);
>> +}
>
> For an on/off control this patch looks like an overkill. I think we can
> simplify it as below:
>
> static void psf_select_mitigation(void)
> {
> if (!boot_cpu_has(X86_FEATURE_PSFD))
> return;
>
> x86_spec_ctrl_mask |= SPEC_CTRL_PSFD;
>
> if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
> psf_mode = PREDICTIVE_STORE_FORWARD_DISABLE;
>
> if (psf_mode == PREDICTIVE_STORE_FORWARD_DISABLE) {
> x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
> wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> }
>
> pr_info("%s\n", psf_strings[psf_mode]);
> }
>
> static int __init psfd_cmdline(char *str)
> {
> if (!boot_cpu_has(X86_FEATURE_PSFD))
> return;
>
> if (!str)
> return -EINVAL;
>
> if (!strcmp(str, "on"))
> psf_mode = PREDICTIVE_STORE_FORWARD_DISABLE;
>
> return 0;
> }
> early_param("predictive_store_fwd_disable", psfd_cmdline);
I agree that on/off can be simple like what you suggested.
My patches version 2 to 5 were in fact structured this way
but implemented in kernel/cpu/amd.c as it was deemed that that was
the file logically speaking to put these changes in.
Later reviews suggested that since this mitigation is similar to spec_control_bypass_disable
(although it is a subset of the above), that it is better to have this in kernel/cpu/bugs.c and
structured similar to spec_control_bypass_disable hence this patchset.
>
> ---
> Thanks,
> Pawan