On Sun, 1 Nov 2020 at 02:05, Tom Lendacky <[email protected]> wrote: > > On 10/29/20 1:51 AM, Anand K Mistry wrote: > > On AMD CPUs which have the feature X86_FEATURE_AMD_STIBP_ALWAYS_ON, > > STIBP is set to on and 'spectre_v2_user_stibp == > > SPECTRE_V2_USER_STRICT_PREFERRED'. At the same time, IBPB can be set to > > conditional. However, this leads to the case where it's impossible to > > turn on IBPB for a process because in the PR_SPEC_DISABLE case in > > ib_prctl_set, the (spectre_v2_user_stibp == > > SPECTRE_V2_USER_STRICT_PREFERRED) condition leads to a return before the > > task flag is set. Similarly, ib_prctl_get will return PR_SPEC_DISABLE > > even though IBPB is set to conditional. > > > > More generally, the following cases are possible: > > 1. STIBP = conditional && IBPB = on for spectre_v2_user=seccomp,ibpb > > 2. STIBP = on && IBPB = conditional for AMD CPUs with > > X86_FEATURE_AMD_STIBP_ALWAYS_ON > > > > The first case functions correctly today, but only because > > spectre_v2_user_ibpb isn't updated to reflect the IBPB mode. > > > > At a high level, this change does one thing. If either STIBP is IBPB is > > s/STIBP is IBPB/STIBP or IBPB/
Oops. Will be fixed in v2. > > > set to conditional, allow the prctl to change the task flag. Also, > > reflect that capability when querying the state. This isn't perfect > > since it doesn't take into account if only STIBP or IBPB is > > unconditionally on. But it allows the conditional feature to work as > > expected, without affecting the unconditional one. > > > > Signed-off-by: Anand K Mistry <[email protected]> > > > > --- > > > > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 18 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > > index d3f0db463f96..fb64e02eed6f 100644 > > --- a/arch/x86/kernel/cpu/bugs.c > > +++ b/arch/x86/kernel/cpu/bugs.c > > @@ -1254,6 +1254,11 @@ static int ssb_prctl_set(struct task_struct *task, > > unsigned long ctrl) > > return 0; > > } > > > > +static bool is_spec_ib_user(enum spectre_v2_user_mitigation mode) > > Maybe something like is_spec_ib_user_controlled() would be a better name. Changed in v2. > > > +{ > > + return mode == SPECTRE_V2_USER_PRCTL || mode == > > SPECTRE_V2_USER_SECCOMP; > > +} > > + > > I like the idea of passing in the mode you want to check, but it appears > they are never used independently. The ibpb and stibp modes are always > checked together in one of the if statements below, so you could make this > a function that checks both modes and just have a single call. I'll leave > that up to the maintainers to see what is preferred. I can see both sides to this. Personally, I think I prefer it as-is since I think it improves readability a bit by making the conditions less complicated whilst not hiding too many details. I'll wait to see what others say before changing this one. > > > static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > > { > > switch (ctrl) { > > @@ -1262,13 +1267,16 @@ static int ib_prctl_set(struct task_struct *task, > > unsigned long ctrl) > > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > > return 0; > > /* > > - * Indirect branch speculation is always disabled in strict > > - * mode. It can neither be enabled if it was force-disabled > > - * by a previous prctl call. > > + * With strict mode for both IBPB and STIBP, the instruction > > + * code paths avoid checking this task flag and instead, > > + * unconditionally run the instruction. However, STIBP and > > IBPB > > + * are independent and either can be set to conditionally > > + * enabled regardless of the mode of the other. If either is > > set > > + * to conditional, allow the task flag to be updated, unless > > it > > + * was force-disabled by a previous prctl call. > > You probably want to reference the STIBP always on mode that allows this > situation. Updated comment in v2 to mention the X86_FEATURE_AMD_STIBP_ALWAYS_ON case. > > > */ > > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED > > || > > + if ((!is_spec_ib_user(spectre_v2_user_ibpb) && > > + !is_spec_ib_user(spectre_v2_user_stibp)) || > > task_spec_ib_force_disable(task)) > > return -EPERM; > > task_clear_spec_ib_disable(task); > > @@ -1283,9 +1291,8 @@ static int ib_prctl_set(struct task_struct *task, > > unsigned long ctrl) > > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > > return -EPERM; > > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > > + if (!is_spec_ib_user(spectre_v2_user_ibpb) && > > + !is_spec_ib_user(spectre_v2_user_stibp)) > > The set function seems reasonable to me. > > > return 0; > > task_set_spec_ib_disable(task); > > if (ctrl == PR_SPEC_FORCE_DISABLE) > > @@ -1351,20 +1358,18 @@ static int ib_prctl_get(struct task_struct *task) > > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > > return PR_SPEC_ENABLE; > > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > > - return PR_SPEC_DISABLE; > > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL || > > - spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL || > > - spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP) { > > + else if (is_spec_ib_user(spectre_v2_user_ibpb) || > > + is_spec_ib_user(spectre_v2_user_stibp)) { > > if (task_spec_ib_force_disable(task)) > > return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE; > > if (task_spec_ib_disable(task)) > > return PR_SPEC_PRCTL | PR_SPEC_DISABLE; > > return PR_SPEC_PRCTL | PR_SPEC_ENABLE; > > - } else > > + } else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > > + return PR_SPEC_DISABLE; > > + else > > The get function also seems reasonable. > > Lets hear what some of the other folks that are familiar with this area > have to say. > > Thanks, > Tom > > > return PR_SPEC_NOT_AFFECTED; > > } > > > > -- Anand K. Mistry Software Engineer Google Australia
