Re: [PATCH v2 1/2] x86/speculation: Allow enabling STIBP with legacy IBRS

[KP Singh]

On Wed, Feb 22, 2023 at 9:48 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Wed, Feb 22, 2023 at 09:16:21AM -0800, KP Singh wrote:
> > Thanks for iterating. I think your commit description and rewrite
> > omits a few key subtleties which I have tried to reinforce in both the
> > commit log and the comments.
> >
> > Q: What does STIBP have to do with IBRS?
> > A: Setting the IBRS bit implicitly enables STIBP / some form of cross
> > thread protection.
>
> That belongs in the docs, if you want to explain this properly.
>
> > Q: Why does it work with eIBRS?
> > A: Because we set the IBRS bit once and leave it set when using eIBRS
>
> Also docs.
>
> > I think this subtlety should be reinforced in the commit description
> > and code comments so that we don't get it wrong again. Your commit
> > does answer this one (thanks!)
>
> Commit messages are fine when explaining *why* a change is being done.
> What is even finer is when you put a lenghtier explanation in our
> documentation so that people can actually find it. Finding text in
> commit messages is harder...

Sure, I think the docs do already cover it, but I sort of disagree
with your statement around the commit message. I feel the more context
you can add in the commit message, the better it is. When I am looking
at the change log, it would be helpful to have the information that I
mentioned in the Q&A. Small things like, "eIBRS needs the IBRS bit set
which also enables cross-thread protections" is a very important
context for this patch IMHO. Without this one is just left head
scratching and scrambling to read lengthy docs and processor manuals.

But again, totally your call. Others, feel free to chime in.

>
> > Q: Why does it not work with the way the kernel currently implements
> > legacy IBRS?
> > A: Because the kernel clears the bit on returning to user space.
>
> From the commit message:
>
>     However, on return to userspace, the IBRS bit is cleared for performance
>     reasons. That leaves userspace threads vulnerable to cross-thread
>     predictions influence against which STIBP protects.

This sort of loosely implies that the IBRS bit also enables
cross-thread protections. Can you atleast add this one explicitly?

"Setting the IBRS bit also enables cross thread protections"

>
> > The reason why I refactored this into a separate helper was to
> > document the subtleties I mentioned above and anchor them to one place
> > as the function is used in 2 places. But this is a maintainer's
> > choice, so it's your call :)
>
> The less code gets added in that thing, the better. Not yet another
> helper pls.

Sure, your call.

>
> > I do agree with Pawan that it's worth adding a pr_info about what the
> > kernel is doing about STIBP.
>
> STIBP status gets dumped through stibp_state().

Not at the stage when the kernel decides to drop the STIBP protection
when eIBRS is enabled. If we had this information when we had a
positive POC, it would have been much easier to figure out what's
going on here.

- KP

>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette