| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<LV3PR12MB926553F1FBF1D6B5A04C308A94802@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Tue, 29 Apr 2025 14:11:05 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Borislav Petkov <bp@...en8.de>
CC: Thomas Gleixner <tglx@...utronix.de>, Peter Zijlstra
<peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>, Pawan Gupta
<pawan.kumar.gupta@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>, Dave
Hansen <dave.hansen@...ux.intel.com>, "x86@...nel.org" <x86@...nel.org>, "H .
Peter Anvin" <hpa@...or.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 11/16] x86/bugs: Restructure spectre_v2_user mitigation
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Tuesday, April 29, 2025 3:47 AM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Peter Zijlstra <peterz@...radead.org>;
> Josh Poimboeuf <jpoimboe@...nel.org>; Pawan Gupta
> <pawan.kumar.gupta@...ux.intel.com>; Ingo Molnar <mingo@...hat.com>; Dave
> Hansen <dave.hansen@...ux.intel.com>; x86@...nel.org; H . Peter Anvin
> <hpa@...or.com>; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v5 11/16] x86/bugs: Restructure spectre_v2_user mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Fri, Apr 18, 2025 at 11:17:16AM -0500, David Kaplan wrote:
> > @@ -217,6 +214,11 @@ void __init cpu_select_mitigations(void)
> > * choices.
> > */
> > retbleed_update_mitigation();
> > + /*
> > + * spectre_v2_user_update_mitigation() depends on
> > + * retbleed_update_mitigation().
> > + */
>
> Why aren't you keeping the reason for the dependency from the above comment?
>
> That's important when we need to touch this code again...
>
> > + spectre_v2_user_update_mitigation();
> > mds_update_mitigation();
> > taa_update_mitigation();
> > mmio_update_mitigation();
> > @@ -224,6 +226,7 @@ void __init cpu_select_mitigations(void)
> >
> > spectre_v1_apply_mitigation();
> > retbleed_apply_mitigation();
> > + spectre_v2_user_apply_mitigation();
> > mds_apply_mitigation();
> > taa_apply_mitigation();
> > mmio_apply_mitigation();
> > @@ -1374,6 +1377,8 @@ enum spectre_v2_mitigation_cmd {
> > SPECTRE_V2_CMD_IBRS,
> > };
> >
> > +static enum spectre_v2_mitigation_cmd spectre_v2_cmd __ro_after_init
> > += SPECTRE_V2_CMD_AUTO;
> > +
> > enum spectre_v2_user_cmd {
> > SPECTRE_V2_USER_CMD_NONE,
> > SPECTRE_V2_USER_CMD_AUTO,
> > @@ -1412,31 +1417,19 @@ static void __init spec_v2_user_print_cond(const
> char *reason, bool secure)
> > pr_info("spectre_v2_user=%s forced on command line.\n",
> > reason); }
> >
> > -static __ro_after_init enum spectre_v2_mitigation_cmd spectre_v2_cmd;
> > -
> > static enum spectre_v2_user_cmd __init
> > spectre_v2_parse_user_cmdline(void)
>
> Lemme unbreak that silly thing while here...
>
> > {
> > - enum spectre_v2_user_cmd mode;
> > char arg[20];
> > int ret, i;
> >
> > - mode = IS_ENABLED(CONFIG_MITIGATION_SPECTRE_V2) ?
> > - SPECTRE_V2_USER_CMD_AUTO : SPECTRE_V2_USER_CMD_NONE;
> > -
> > - switch (spectre_v2_cmd) {
> > - case SPECTRE_V2_CMD_NONE:
> > + if (cpu_mitigations_off() ||
> > + !IS_ENABLED(CONFIG_MITIGATION_SPECTRE_V2))
> > return SPECTRE_V2_USER_CMD_NONE;
> > - case SPECTRE_V2_CMD_FORCE:
> > - return SPECTRE_V2_USER_CMD_FORCE;
> > - default:
> > - break;
> > - }
> >
> > ret = cmdline_find_option(boot_command_line, "spectre_v2_user",
> > arg, sizeof(arg));
> > if (ret < 0)
> > - return mode;
> > + return SPECTRE_V2_USER_CMD_AUTO;
> >
> > for (i = 0; i < ARRAY_SIZE(v2_user_options); i++) {
> > if (match_option(arg, ret, v2_user_options[i].option)) {
> > @@ -1447,7 +1440,7 @@ spectre_v2_parse_user_cmdline(void)
> > }
> >
> > pr_err("Unknown user space protection option (%s). Switching to default\n",
> arg);
> > - return mode;
> > + return SPECTRE_V2_USER_CMD_AUTO;
> > }
> >
> > static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation
> > mode) @@ -1458,7 +1451,6 @@ static inline bool
> > spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) static void
> > __init
> > spectre_v2_user_select_mitigation(void)
>
> That too.
>
> > {
> > - enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
> > enum spectre_v2_user_cmd cmd;
>
> Might as well get rid of that one.
>
> > if (!boot_cpu_has(X86_FEATURE_IBPB) &&
> > !boot_cpu_has(X86_FEATURE_STIBP)) @@ -1467,48 +1459,65 @@
> spectre_v2_user_select_mitigation(void)
> > cmd = spectre_v2_parse_user_cmdline();
> > switch (cmd) {
> > case SPECTRE_V2_USER_CMD_NONE:
> > - goto set_mode;
> > + return;
> > case SPECTRE_V2_USER_CMD_FORCE:
> > - mode = SPECTRE_V2_USER_STRICT;
> > + spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
> > + spectre_v2_user_stibp = SPECTRE_V2_USER_STRICT;
>
> Those should be aligned at the '=' sign for better readability.
>
> ...
>
> IOW, all the changes ontop:
>
> ---
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index
> afea9179acdd..dc75195760ca 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -214,9 +214,11 @@ void __init cpu_select_mitigations(void)
> * choices.
> */
> retbleed_update_mitigation();
> +
> /*
> * spectre_v2_user_update_mitigation() depends on
> - * retbleed_update_mitigation().
> + * retbleed_update_mitigation(), specifically the STIBP
> + * selection is forced for UNRET or IBPB.
> */
> spectre_v2_user_update_mitigation();
> mds_update_mitigation();
> @@ -1422,8 +1424,7 @@ static void __init spec_v2_user_print_cond(const char
> *reason, bool secure)
> pr_info("spectre_v2_user=%s forced on command line.\n", reason); }
>
> -static enum spectre_v2_user_cmd __init
> -spectre_v2_parse_user_cmdline(void)
> +static enum spectre_v2_user_cmd __init
> +spectre_v2_parse_user_cmdline(void)
> {
> char arg[20];
> int ret, i;
> @@ -1453,29 +1454,25 @@ static inline bool spectre_v2_in_ibrs_mode(enum
> spectre_v2_mitigation mode)
> return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS; }
>
> -static void __init
> -spectre_v2_user_select_mitigation(void)
> +static void __init spectre_v2_user_select_mitigation(void)
> {
> - enum spectre_v2_user_cmd cmd;
> -
> if (!boot_cpu_has(X86_FEATURE_IBPB) &&
> !boot_cpu_has(X86_FEATURE_STIBP))
> return;
>
> - cmd = spectre_v2_parse_user_cmdline();
> - switch (cmd) {
> + switch (spectre_v2_parse_user_cmdline()) {
> case SPECTRE_V2_USER_CMD_NONE:
> return;
> case SPECTRE_V2_USER_CMD_FORCE:
> - spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
> + spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
> spectre_v2_user_stibp = SPECTRE_V2_USER_STRICT;
> break;
> case SPECTRE_V2_USER_CMD_AUTO:
> case SPECTRE_V2_USER_CMD_PRCTL:
> - spectre_v2_user_ibpb = SPECTRE_V2_USER_PRCTL;
> + spectre_v2_user_ibpb = SPECTRE_V2_USER_PRCTL;
> spectre_v2_user_stibp = SPECTRE_V2_USER_PRCTL;
> break;
> case SPECTRE_V2_USER_CMD_PRCTL_IBPB:
> - spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
> + spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
> spectre_v2_user_stibp = SPECTRE_V2_USER_PRCTL;
> break;
> case SPECTRE_V2_USER_CMD_SECCOMP:
>
That all looks good to me.
Thanks --David Kaplan
Powered by blists - more mailing lists