[GIT PULL] x86/cpu for v5.17

[GIT PULL] x86/cpu for v5.17

[Borislav Petkov]

Hi Linus,

please pull two x86/cpu updates for 5.17.

Thx.

---

The following changes since commit 136057256686de39cc3a07c2e39ef6bc43003ff6:

  Linux 5.16-rc2 (2021-11-21 13:47:39 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/x86_cpu_for_v5.17_rc1

for you to fetch changes up to 244122b4d2e5221e6abd6e21d6a58170104db781:

  x86/lib: Add fast-short-rep-movs check to copy_user_enhanced_fast_string() (2021-12-29 13:46:02 +0100)

----------------------------------------------------------------
- Enable the short string copies for CPUs which support them, in
copy_user_enhanced_fast_string()

- Avoid writing MSR_CSTAR on Intel due to TDX guests raising a #VE trap

----------------------------------------------------------------
Andi Kleen (1):
      x86/cpu: Don't write CSTAR MSR on Intel CPUs

Tony Luck (1):
      x86/lib: Add fast-short-rep-movs check to copy_user_enhanced_fast_string()

 arch/x86/kernel/cpu/common.c | 15 +++++++++++++--
 arch/x86/lib/copy_user_64.S  |  4 ++--
 2 files changed, 15 insertions(+), 4 deletions(-)

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg

Re: [GIT PULL] x86/cpu for v5.17

[Linus Torvalds]

On Mon, Jan 10, 2022 at 3:16 AM Borislav Petkov <bp@suse.de> wrote:
>
> - Avoid writing MSR_CSTAR on Intel due to TDX guests raising a #VE trap

This is all fine, but my reaction to this is that I would have
expected it to be either a wrmsrl_safe(), or using an actual CPU
feature check.

Checking for a particular vendor seems a bit hacky. We generally try
to avoid things like that, don't we?

Not a big deal, I just thought I'd mention it since I reacted to it.
And we don't seem to have those vendor checks in any of the other code
that uses MSR_CSTAR (just grepping for that and seeing it mentioned in
kvm code etc)

             Linus

Re: [GIT PULL] x86/cpu for v5.17

[pr-tracker-bot]

The pull request you sent on Mon, 10 Jan 2022 12:16:39 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/x86_cpu_for_v5.17_rc1

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/25f8c7785e254779fbd2127c4eced81811e8e421

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] x86/cpu for v5.17

[Borislav Petkov]

CCing the folks who were involved in this one...

On Mon, Jan 10, 2022 at 10:18:06AM -0800, Linus Torvalds wrote:
> On Mon, Jan 10, 2022 at 3:16 AM Borislav Petkov <bp@suse.de> wrote:
> >
> > - Avoid writing MSR_CSTAR on Intel due to TDX guests raising a #VE trap
> 
> This is all fine, but my reaction to this is that I would have
> expected it to be either a wrmsrl_safe(), or using an actual CPU
> feature check.
> 
> Checking for a particular vendor seems a bit hacky. We generally try
> to avoid things like that, don't we?
> 
> Not a big deal, I just thought I'd mention it since I reacted to it.
> And we don't seem to have those vendor checks in any of the other code
> that uses MSR_CSTAR (just grepping for that and seeing it mentioned in
> kvm code etc)

Right, the only point for doing the vendor check I see here is, well,
because it is Intel who doesn't have CSTAR, let's check for Intel. But
yeah, we do avoid the vendor checks if it can be helped.

We can do a synthetic X86_FEATURE flag but that would be a waste. So the
_safe thing and keep the comment sounds optimal to me.

I can whip up a patch ontop if people agree.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg

Re: [GIT PULL] x86/cpu for v5.17

[Linus Torvalds]

On Mon, Jan 10, 2022 at 10:35 AM Borislav Petkov <bp@suse.de> wrote:
>
> Right, the only point for doing the vendor check I see here is, well,
> because it is Intel who doesn't have CSTAR, let's check for Intel. But
> yeah, we do avoid the vendor checks if it can be helped.
>
> We can do a synthetic X86_FEATURE flag but that would be a waste. So the
> _safe thing and keep the comment sounds optimal to me.

I agree that a new feature flag for just this would seem a bit
wasteful, and just using wrmsrl_safe() would seem to be the natural
thing to do.

Particularly since that's literally what the wrmsrl's around that
thing do (ie MSR_IA32_SYSENTER_CS and friends). So that vendor check
really stands out as being the odd man out.

               Linus

Re: [GIT PULL] x86/cpu for v5.17

[Dave Hansen]

On 1/10/22 10:35, Borislav Petkov wrote:
>>
>> Not a big deal, I just thought I'd mention it since I reacted to it.
>> And we don't seem to have those vendor checks in any of the other code
>> that uses MSR_CSTAR (just grepping for that and seeing it mentioned in
>> kvm code etc)
> Right, the only point for doing the vendor check I see here is, well,
> because it is Intel who doesn't have CSTAR, let's check for Intel. But
> yeah, we do avoid the vendor checks if it can be helped.
> 
> We can do a synthetic X86_FEATURE flag but that would be a waste. So the
> _safe thing and keep the comment sounds optimal to me.
> 
> I can whip up a patch ontop if people agree.

There are four basic options here for TDX:

1. Paper over the #VE in the #VE handler itself
2. Do a check for TDX at the wrmsr(MSR_CSTAR) site
3. Do a check for X86_VENDOR_INTEL at the wrmsr(MSR_CSTAR) site
4. Use wrmsr*_safe() and rely on #VE -> fixup_exception()

TDX originally did #1, passed over #2 and settled on #3 because of a 
comment:

	It's an obvious optimization (avoiding the WRMSR with a
	conditional) without TDX because the write is pointless
	independent of TDX." [1]

I think doing wrmsr*_safe() is OK.  But, on TDX systems, it will end up 
taking a weird route:

       WRMSR -> #VE -> hypercall -> ve_raise_fault() -> fixup_exception()

instead of the "normal" _safe route which goes:

       WRMSR -> #GP -> ... -> fixup_exception()

So, we should probably make sure wrmsr*_safe() is fine on TDX before we 
subject ourselves to any additional churn.  Kirill, can you test that out?

1. https://lore.kernel.org/all/87sfvljf5q.ffs@tglx/

Re: [GIT PULL] x86/cpu for v5.17

[Linus Torvalds]

On Mon, Jan 10, 2022 at 12:10 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> There are four basic options here for TDX:
>
> 1. Paper over the #VE in the #VE handler itself

Ahh, I saw it, but didn't really react to the fact that unlike the
other 'wrmsrl_safe()' cases, it takes #VE instead of #GP.

I do think that perhaps just doing fixup_exception() in the #VE
handler is the most obvious case. It's not like exceptions are meant
to be somehow specific to #GP.

But hey, I don't really care that deeply. I just reacted to this all
looking odd, and I've already done the pull. So it's not like I'm
NAK'ing the whole vendor test, it was just surprising to me.

So I don't want people to feel like they have to do that wrmsrl_safe()
thing, or add a feature flag or anything. I see why it happened now,
and I may think it's a bit odd still, but it's really not a huge deal.

                   Linus

Re: [GIT PULL] x86/cpu for v5.17

[Sean Christopherson]

On Mon, Jan 10, 2022, Dave Hansen wrote:
> On 1/10/22 10:35, Borislav Petkov wrote:
> > > 
> > > Not a big deal, I just thought I'd mention it since I reacted to it.
> > > And we don't seem to have those vendor checks in any of the other code
> > > that uses MSR_CSTAR (just grepping for that and seeing it mentioned in
> > > kvm code etc)
> > Right, the only point for doing the vendor check I see here is, well,
> > because it is Intel who doesn't have CSTAR, let's check for Intel. But
> > yeah, we do avoid the vendor checks if it can be helped.
> > 
> > We can do a synthetic X86_FEATURE flag but that would be a waste. So the
> > _safe thing and keep the comment sounds optimal to me.
> > 
> > I can whip up a patch ontop if people agree.
> 
> There are four basic options here for TDX:
> 
> 1. Paper over the #VE in the #VE handler itself
> 2. Do a check for TDX at the wrmsr(MSR_CSTAR) site
> 3. Do a check for X86_VENDOR_INTEL at the wrmsr(MSR_CSTAR) site
> 4. Use wrmsr*_safe() and rely on #VE -> fixup_exception()
> 
> TDX originally did #1, passed over #2 and settled on #3 because of a
> comment:
> 
> 	It's an obvious optimization (avoiding the WRMSR with a
> 	conditional) without TDX because the write is pointless
> 	independent of TDX." [1]
> 
> I think doing wrmsr*_safe() is OK.  But, on TDX systems, it will end up
> taking a weird route:
> 
>       WRMSR -> #VE -> hypercall -> ve_raise_fault() -> fixup_exception()
> 
> instead of the "normal" _safe route which goes:
> 
>       WRMSR -> #GP -> ... -> fixup_exception()
> 
> So, we should probably make sure wrmsr*_safe() is fine on TDX before we
> subject ourselves to any additional churn.  Kirill, can you test that out?

wrmsr*_safe() isn't, uh, safe.  The issue is that the #VE -> hypercall will expose
information about the guest kernel's layout (address of a kernel symbol) to the
untrusted hypervisor, and in theory could be used to attack the guest.  Whether
or not the kernel's layout is all that valuable of a secret is debatable, but it's
hard to argue that the kernel should knowingly expose such information to the host.

Most VMMs probably won't even signal failure on the hypercall, i.e. the kernel will
never do fixup_exception(), the real motivation is to avoid the hypercall.

The argument against papering over the fault in the #VE handler is that there's
exactly one wrmsr(MSR_CSTAR) in the entire kernel, i.e. it's a wash from a code
perspective, and avoiding the wrmsr entirely will allow the #VE handler to WARN
if something else in the kernel ends up writing MSR_CSTAR.