* [GIT PULL] x86/cpu for v5.17 @ 2022-01-10 11:16 Borislav Petkov 2022-01-10 18:18 ` Linus Torvalds 2022-01-10 18:27 ` pr-tracker-bot 0 siblings, 2 replies; 8+ messages in thread From: Borislav Petkov @ 2022-01-10 11:16 UTC (permalink / raw) To: Linus Torvalds; +Cc: x86-ml, lkml 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 11:16 [GIT PULL] x86/cpu for v5.17 Borislav Petkov @ 2022-01-10 18:18 ` Linus Torvalds 2022-01-10 18:35 ` Borislav Petkov 2022-01-10 18:27 ` pr-tracker-bot 1 sibling, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2022-01-10 18:18 UTC (permalink / raw) To: Borislav Petkov; +Cc: x86-ml, lkml 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 18:18 ` Linus Torvalds @ 2022-01-10 18:35 ` Borislav Petkov 2022-01-10 19:32 ` Linus Torvalds 2022-01-10 20:10 ` Dave Hansen 0 siblings, 2 replies; 8+ messages in thread From: Borislav Petkov @ 2022-01-10 18:35 UTC (permalink / raw) To: Linus Torvalds Cc: x86-ml, lkml, Andi Kleen, Kuppuswamy Sathyanarayanan, Tony Luck 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 18:35 ` Borislav Petkov @ 2022-01-10 19:32 ` Linus Torvalds 2022-01-10 20:10 ` Dave Hansen 1 sibling, 0 replies; 8+ messages in thread From: Linus Torvalds @ 2022-01-10 19:32 UTC (permalink / raw) To: Borislav Petkov Cc: x86-ml, lkml, Andi Kleen, Kuppuswamy Sathyanarayanan, Tony Luck 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 18:35 ` Borislav Petkov 2022-01-10 19:32 ` Linus Torvalds @ 2022-01-10 20:10 ` Dave Hansen 2022-01-10 20:15 ` Linus Torvalds 2022-01-11 22:48 ` Sean Christopherson 1 sibling, 2 replies; 8+ messages in thread From: Dave Hansen @ 2022-01-10 20:10 UTC (permalink / raw) To: Borislav Petkov, Linus Torvalds Cc: x86-ml, lkml, Andi Kleen, Kuppuswamy Sathyanarayanan, Tony Luck, Shutemov, Kirill 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 20:10 ` Dave Hansen @ 2022-01-10 20:15 ` Linus Torvalds 2022-01-11 22:48 ` Sean Christopherson 1 sibling, 0 replies; 8+ messages in thread From: Linus Torvalds @ 2022-01-10 20:15 UTC (permalink / raw) To: Dave Hansen Cc: Borislav Petkov, x86-ml, lkml, Andi Kleen, Kuppuswamy Sathyanarayanan, Tony Luck, Shutemov, Kirill 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 20:10 ` Dave Hansen 2022-01-10 20:15 ` Linus Torvalds @ 2022-01-11 22:48 ` Sean Christopherson 1 sibling, 0 replies; 8+ messages in thread From: Sean Christopherson @ 2022-01-11 22:48 UTC (permalink / raw) To: Dave Hansen Cc: Borislav Petkov, Linus Torvalds, x86-ml, lkml, Andi Kleen, Kuppuswamy Sathyanarayanan, Tony Luck, Shutemov, Kirill 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] x86/cpu for v5.17 2022-01-10 11:16 [GIT PULL] x86/cpu for v5.17 Borislav Petkov 2022-01-10 18:18 ` Linus Torvalds @ 2022-01-10 18:27 ` pr-tracker-bot 1 sibling, 0 replies; 8+ messages in thread From: pr-tracker-bot @ 2022-01-10 18:27 UTC (permalink / raw) To: Borislav Petkov; +Cc: Linus Torvalds, x86-ml, lkml 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-11 22:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-10 11:16 [GIT PULL] x86/cpu for v5.17 Borislav Petkov 2022-01-10 18:18 ` Linus Torvalds 2022-01-10 18:35 ` Borislav Petkov 2022-01-10 19:32 ` Linus Torvalds 2022-01-10 20:10 ` Dave Hansen 2022-01-10 20:15 ` Linus Torvalds 2022-01-11 22:48 ` Sean Christopherson 2022-01-10 18:27 ` pr-tracker-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).