On 4/28/2019 3:09 PM, Thomas Gleixner wrote:
On Sat, 27 Apr 2019, Xiaoyao Li wrote:
On Thu, 2019-04-25 at 09:42 +0200, Thomas Gleixner wrote:
On Wed, 24 Apr 2019, Fenghua Yu wrote:
+static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
+{
+ u64 host_msr_test_ctl;
+
+ if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
+ return;
Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask.
+ host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
+
+ if (host_msr_test_ctl == vmx->msr_test_ctl) {
This still assumes that the only bit which can be set in the MSR is that
lock detect bit.
+ clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
+ } else {
+ add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
+ host_msr_test_ctl, false);
So what happens here is that if any other bit is set on the host, VMENTER
will happily clear it.
There are two bits of MSR TEST_CTL defined in Intel SDM now, which is bit
29 and bit 31. Bit 31 is not used in kernel, and here we only need to
switch bit 29 between host and guest. So should I also change the name
to atomic_switch_split_lock_detect() to indicate that we only switch bit
29?
No. Just because we ony use the split lock bit now, there is no
jusification to name everything splitlock. This is going to have renamed
when yet another bit is added in the future. The MSR is exposed to the
guest and the restriction of bits happens to be splitlock today.
Got it.
guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl;
That preserves any bits which are not exposed to the guest.
But the way more interesting question is why are you exposing the MSR and
the bit to the guest at all if the host has split lock detection enabled?
That does not make any sense as you basically allow the guest to switch it
off and then launch a slowdown attack. If the host has it enabled, then a
guest has to be treated like any other process and the #AC trap has to be
caught by the hypervisor which then kills the guest.
Only if the host has split lock detection disabled, then you can expose it
and allow the guest to turn it on and handle it on its own.
Indeed, if we use split lock detection for protection purpose, when host
has it enabled we should directly pass it to guest and forbid guest from
disabling it. And only when host disables split lock detection, we can
expose it and allow the guest to turn it on.
?
If it is used for protection purpose, then it should follow what you said and
this feature needs to be disabled by default. Because there are split lock
issues in old/current kernels and BIOS. That will cause the existing guest
booting failure and killed due to those split lock.
Rightfully so.
So, the patch 13 "Enable split lock detection by default" needs to be
removed?
If it is only used for debug purpose, I think it might be OK to enable this
feature by default and make it indepedent between host and guest?
No. It does not make sense.
So I think how to handle this feature between host and guest depends on how we
use it? Once you give me a decision, I will follow it in next version.
As I said: The host kernel makes the decision.
If the host kernel has it enabled then the guest is not allowed to change
it. If the guest triggers an #AC it will be killed.
If the host kernel has it disabled then the guest can enable it for it's
own purposes.
Thanks,
tglx
