Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191229164830.62144-1-asteinhauser@google.com> <20200121113317.GH7808@zn.tnic>
In-Reply-To: <20200121113317.GH7808@zn.tnic>
From:   Anthony Steinhauser <asteinhauser@google.com>
Date:   Tue, 21 Jan 2020 03:57:50 -0800
Message-ID: <CAN_oZf1bGgXB2fNh8aAJybtPf2ajCGFSzP+7at87X-iW2kq3Hg@mail.gmail.com>
Subject: Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented
To:     Borislav Petkov <bp@alien8.de>
Cc:     Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        mingo@redhat.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Your change seems to remove exactly the distinction which Thomas
pointed out because SPECTRE_V2_USER_UNAVAILABLE would not
differentiate between STIBP mitigation not available and STIBP not
used because SMT is not possible. Otherwise your modification looks
fine to me.

On Tue, Jan 21, 2020 at 3:33 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Dec 29, 2019 at 08:48:30AM -0800, Anthony Steinhauser wrote:
> > @@ -602,7 +603,7 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
> >  static void __init
> >  spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> >  {
> > -     enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
> > +     enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_UNAVAILABLE;
> >       bool smt_possible = IS_ENABLED(CONFIG_SMP);
> >       enum spectre_v2_user_cmd cmd;
>
> So here in the code, right under this line we check IBPB and STIBP and
> whether SMT is force_disabled/possible and set smt_possible if not. We
> parse cmdline, pick apart selection, etc...
>
> > @@ -616,6 +617,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> >       cmd = spectre_v2_parse_user_cmdline(v2_cmd);
> >       switch (cmd) {
> >       case SPECTRE_V2_USER_CMD_NONE:
> > +             mode = SPECTRE_V2_USER_DISABLED;
> >               goto set_mode;
> >       case SPECTRE_V2_USER_CMD_FORCE:
> >               mode = SPECTRE_V2_USER_STRICT;
> > @@ -676,7 +678,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> >        * mode.
> >        */
> >       if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
> > -             mode = SPECTRE_V2_USER_NONE;
> > +             mode = SPECTRE_V2_USER_UNAVAILABLE;
>
> ... but here we do that evaluation again. But I think that *if* the
> required hw support is not there - either SMT is not possible or STIBP
> not present - then there's no real need to parse the cmdline and do all
> that.
>
> IOW, the filtering out of the cases where the user can't do any changes
> due to not present hw should be concentrated at the function entry and
> mode left at SPECTRE_V2_USER_UNAVAILABLE.
>
> IOW 2, unless I'm not missing some of the gazillion use cases with this
> ;-\ I think that check needs to be moved up and integrated into the
> entry checks. I.e., this ontop or a separate patch...:
>
> ---
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 2e9299816530..ffe5e4fa4611 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -618,8 +618,10 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
>                 return;
>
>         if (cpu_smt_control == CPU_SMT_FORCE_DISABLED ||
> -           cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
> +           cpu_smt_control == CPU_SMT_NOT_SUPPORTED) {
>                 smt_possible = false;
> +               return;
> +       }
>
>         cmd = spectre_v2_parse_user_cmdline(v2_cmd);
>         switch (cmd) {
> @@ -679,13 +681,6 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
>         /* If enhanced IBRS is enabled no STIBP required */
>         if (spectre_v2_enabled == SPECTRE_V2_IBRS_ENHANCED)
>                 return;
> -
> -       /*
> -        * If SMT is not possible or STIBP is not available clear the STIBP
> -        * mode.
> -        */
> -       if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
> -               mode = SPECTRE_V2_USER_UNAVAILABLE;
>  set_mode:
>         spectre_v2_user = mode;
>         /* Only print the STIBP mode when SMT possible */
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette