Date:   Mon, 20 Feb 2023 15:31:40 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     KP Singh <kpsingh@kernel.org>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Dave Hansen <dave.hansen@intel.com>
Cc:     Josh Poimboeuf <jpoimboe@kernel.org>, linux-kernel@vger.kernel.org,
        pjt@google.com, evn@google.com, tglx@linutronix.de,
        mingo@redhat.com, dave.hansen@linux.intel.com, x86@kernel.org,
        hpa@zytor.com, peterz@infradead.org,
        pawan.kumar.gupta@linux.intel.com, kim.phillips@amd.com,
        alexandre.chartre@oracle.com, daniel.sneddon@linux.intel.com,
        =?utf-8?B?Sm9zw6k=?= Oliveira <joseloliveira11@gmail.com>,
        Rodrigo Branco <rodrigo@kernelhacking.com>,
        Alexandra Sandulescu <aesa@google.com>,
        Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH RESEND] x86/speculation: Fix user-mode spectre-v2
 protection with KERNEL_IBRS
Message-ID: <Y/OETPFTJbGtCADM@zn.tnic>
References: <20230220120127.1975241-1-kpsingh@kernel.org>
 <20230220121350.aidsipw3kd4rsyss@treble>
 <CACYkzJ5L9MLuE5Jz+z5-NJCCrUqTbgKQkXSqnQnCfTD_WNA7_Q@mail.gmail.com>
 <CACYkzJ6n=-tobhX0ONQhjHSgmnNjWnNe_dZnEOGtD8Y6S3RHbA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACYkzJ6n=-tobhX0ONQhjHSgmnNjWnNe_dZnEOGtD8Y6S3RHbA@mail.gmail.com>
Precedence: bulk

Drop stable@.

On Mon, Feb 20, 2023 at 04:34:02AM -0800, KP Singh wrote:
> > > On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
> > > > +static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode)
> > > > +{
> > > > +     /* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
> > > > +      *
> > > > +      * However, With KERNEL_IBRS, the IBRS bit is cleared on return
> > > > +      * to user and the user-mode code needs to be able to enable protection
> > > > +      * from cross-thread training, either by always enabling STIBP or
> > > > +      * by enabling it via prctl.
> > > > +      */
> > > > +     return (spectre_v2_in_ibrs_mode(mode) &&
> > > > +             !cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));
> > > > +}
> > >
> > > The comments and code confused me, they both seem to imply some
> > > distinction between IBRS and KERNEL_IBRS, but in the kernel those are
> > > functionally the same thing.  e.g., the kernel doesn't have a user IBRS
> > > mode.
> > >
> > > And, unless I'm missing some subtlety here, it seems to be a convoluted
> > > way of saying that eIBRS doesn't need STIBP in user space.
> 
> Actually, there is a subtlety, STIBP is not needed in IBRS and eIBRS
> however, with KERNEL_IBRS we only enable IBRS (by setting and
> unsetting the IBRS bit of SPEC_CTL) in the kernel context and this is
> why we need to allow STIBP in userspace. If it were for pure IBRS, we
> would not need it either (based on my reading). Now, with
> spectre_v2_in_ibrs_mode, as per the current implementation implies
> that KERNEL_IBRS is chosen, but this may change. Also, I would also
> prefer to have it more readable in the sense that:
> 
> "If the kernel decides to write 0 to the IBRS bit on returning to the
> user, STIBP needs to to be allowed in user space"

From:

https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/indirect-branch-restricted-speculation.html

"If IA32_SPEC_CTRL.IBRS is already 1 before a transition to a more
privileged predictor mode, some processors may allow the predicted
targets of indirect branches executed in that predictor mode to be
controlled by software that executed before the transition. Software can
avoid this by using WRMSR on the IA32_SPEC_CTRL MSR to set the IBRS bit
to 1 after any such transition, regardless of the bit’s previous value.
It is not necessary to clear the bit first; writing it with a value of
1 after the transition suffices, regardless of the bit’s original
value."

I'd love it if we could simplify our code by not caring of the IBRS bit
when returning to user but I'm afraid that it ain't that simple.

This above probably wants to say that you need to write 1 on CPL change
because it has a flushing behavior of killing user prediction entries.

Lemme add Andy and dhansen for clarification.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette