Message-ID: <448ebe52-f8d5-1e5a-c128-dfa3feab725f@amd.com>
Date:   Wed, 19 May 2021 08:19:47 -0500
From:   "Saripalli, RK" <rsaripal@....com>
To:     Pawan Gupta <writetopawan@...il.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        Jonathan Corbet <corbet@....net>, bsd@...hat.com
Subject: Re: [v6 1/1] x86/bugs: Implement mitigation for Predictive Store
 Forwarding



On 5/19/2021 12:38 AM, Pawan Gupta wrote:
> On 17.05.2021 17:00, Ramakrishna Saripalli wrote:
>> From: Ramakrishna Saripalli <rk.saripalli@....com>
>>
>> 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@....com>
>> ---
>> .../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

Powered by blists - more mailing lists