Mailing List Archive: [PATCH 1/2] x86/vmx: implement Bus Lock detection

[PATCH 1/2] x86/vmx: implement Bus Lock detection

May 17, 2022, 6:21 AM

Post #1 of 4 (6 views)

Add support for enabling Bus Lock Detection on Intel systems. Such
detection works by triggering a vmexit, which is enough of a pause to
prevent a guest from abusing of the Bus Lock.

Add an extra perf counter to track the number of Bus Locks detected.
This is done because Bus Locks can also be reported by setting the bit
26 in the exit reason field, so also account for those.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/hvm/vmx/vmcs.c | 4 +++-
xen/arch/x86/hvm/vmx/vmx.c | 18 ++++++++++++++++++
xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 3 +++
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 2 ++
xen/arch/x86/include/asm/perfc_defn.h | 4 +++-
5 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 56fed2db03..d388e6729c 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -209,6 +209,7 @@ static void __init vmx_display_features(void)
P(cpu_has_vmx_virt_exceptions, "Virtualisation Exceptions");
P(cpu_has_vmx_pml, "Page Modification Logging");
P(cpu_has_vmx_tsc_scaling, "TSC Scaling");
+ P(cpu_has_vmx_bus_lock_detection, "Bus Lock Detection");
#undef P

if ( !printed )
@@ -318,7 +319,8 @@ static int vmx_init_vmcs_config(bool bsp)
SECONDARY_EXEC_ENABLE_VM_FUNCTIONS |
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
SECONDARY_EXEC_XSAVES |
- SECONDARY_EXEC_TSC_SCALING);
+ SECONDARY_EXEC_TSC_SCALING |
+ SECONDARY_EXEC_BUS_LOCK_DETECTION);
if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
if ( opt_vpid_enabled )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d03e78bf0d..02cc7a2023 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4053,6 +4053,16 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)

if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
return vmx_failed_vmentry(exit_reason, regs);
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
+ {
+ /*
+ * Delivery of Bus Lock VM exit was pre-empted by a higher priority VM
+ * exit.
+ */
+ exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
+ if ( exit_reason != EXIT_REASON_BUS_LOCK )
+ perfc_incr(buslock);
+ }

if ( v->arch.hvm.vmx.vmx_realmode )
{
@@ -4549,6 +4559,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
vmx_handle_descriptor_access(exit_reason);
break;

+ case EXIT_REASON_BUS_LOCK:
+ perfc_incr(buslock);
+ /*
+ * Nothing to do: just taking a vmexit should be enough of a pause to
+ * prevent a VM from crippling the host with bus locks.
+ */
+ break;
+
case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
case EXIT_REASON_INVPCID:
/* fall through */
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index 9119aa8536..5d3edc1642 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -266,6 +266,7 @@ extern u32 vmx_vmentry_control;
#define SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS 0x00040000
#define SECONDARY_EXEC_XSAVES 0x00100000
#define SECONDARY_EXEC_TSC_SCALING 0x02000000
+#define SECONDARY_EXEC_BUS_LOCK_DETECTION 0x40000000
extern u32 vmx_secondary_exec_control;

#define VMX_EPT_EXEC_ONLY_SUPPORTED 0x00000001
@@ -345,6 +346,8 @@ extern u64 vmx_ept_vpid_cap;
(vmx_secondary_exec_control & SECONDARY_EXEC_XSAVES)
#define cpu_has_vmx_tsc_scaling \
(vmx_secondary_exec_control & SECONDARY_EXEC_TSC_SCALING)
+#define cpu_has_vmx_bus_lock_detection \
+ (vmx_secondary_exec_control & SECONDARY_EXEC_BUS_LOCK_DETECTION)

#define VMCS_RID_TYPE_MASK 0x80000000

diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 8eedf59155..03995701a1 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -159,6 +159,7 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
* Exit Reasons
*/
#define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000
+#define VMX_EXIT_REASONS_BUS_LOCK (1u << 26)

#define EXIT_REASON_EXCEPTION_NMI 0
#define EXIT_REASON_EXTERNAL_INTERRUPT 1
@@ -219,6 +220,7 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
#define EXIT_REASON_PML_FULL 62
#define EXIT_REASON_XSAVES 63
#define EXIT_REASON_XRSTORS 64
+#define EXIT_REASON_BUS_LOCK 74
/* Remember to also update VMX_PERF_EXIT_REASON_SIZE! */

/*
diff --git a/xen/arch/x86/include/asm/perfc_defn.h b/xen/arch/x86/include/asm/perfc_defn.h
index b07063b7d8..d6eb661940 100644
--- a/xen/arch/x86/include/asm/perfc_defn.h
+++ b/xen/arch/x86/include/asm/perfc_defn.h
@@ -6,7 +6,7 @@ PERFCOUNTER_ARRAY(exceptions, "exceptions", 32)

#ifdef CONFIG_HVM

-#define VMX_PERF_EXIT_REASON_SIZE 65
+#define VMX_PERF_EXIT_REASON_SIZE 75
#define VMEXIT_NPF_PERFC 143
#define SVM_PERF_EXIT_REASON_SIZE (VMEXIT_NPF_PERFC + 1)
PERFCOUNTER_ARRAY(vmexits, "vmexits",
@@ -125,4 +125,6 @@ PERFCOUNTER(realmode_exits, "vmexits from realmode")

PERFCOUNTER(pauseloop_exits, "vmexits from Pause-Loop Detection")

+PERFCOUNTER(buslock, "Bus Locks Detected")
+
/*#endif*/ /* __XEN_PERFC_DEFN_H__ */
--
2.36.0

Re: [PATCH 1/2] x86/vmx: implement Bus Lock detection [ In reply to ]

Andrew.Cooper3 at citrix

May 18, 2022, 3:50 PM

Post #2 of 4 (6 views)

Permalink

On 17/05/2022 14:21, Roger Pau Monne wrote:
> Add support for enabling Bus Lock Detection on Intel systems. Such
> detection works by triggering a vmexit, which is enough of a pause to
> prevent a guest from abusing of the Bus Lock.

"which is enough of a" is a bit firmer than ideal. "which Andy says
will be ok" is perhaps more accurate.

Perhaps "which ought to be enough" ?

A buslock here or there is no problem, and non-malicious software
appears to be devoid of buslocks (hardly surprising - it would be a hard
error on other architectures), but a malicious piece of userspace can
trivially cripple the system.

Forcing a vmexit on every buslock limits an attacker to one buslock per
however long a vmentry/exit cycle takes.

> Add an extra perf counter to track the number of Bus Locks detected.

extra Xen perf counter.

Because other hypervisors use actual perf counters to emulate this
ability on current hardware. Maybe something we should consider...

> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index d03e78bf0d..02cc7a2023 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4053,6 +4053,16 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>
> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> return vmx_failed_vmentry(exit_reason, regs);
> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> + {
> + /*
> + * Delivery of Bus Lock VM exit was pre-empted by a higher priority VM
> + * exit.
> + */
> + exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> + if ( exit_reason != EXIT_REASON_BUS_LOCK )
> + perfc_incr(buslock);

I'm pretty sure you can drop the if, and do the perfc_incr()
unconditionally. You won't get EXIT_REASON_BUS_LOCK |
VMX_EXIT_REASONS_BUS_LOCK given that wording in the ISE.

To test, Intel has PENDING_DBG which interferes with most easy attempts
to create the condition, but how about this.

Load an LDT, misaligned across a cacheline boundary, and set #DB's %cs
to LDT[0] with a clear access bit, then execute an `icebp` instruction.
The atomic write to set the access bit is a 4-byte access typically.

This should cause the #DB intercept to trigger on the same instantaneous
boundary that generated the buslock.

Otherwise, LGTM.

~Andrew

Re: [PATCH 1/2] x86/vmx: implement Bus Lock detection [ In reply to ]

roger.pau at citrix

May 19, 2022, 5:21 AM

Post #3 of 4 (5 views)

Permalink

On Wed, May 18, 2022 at 10:50:02PM +0000, Andrew Cooper wrote:
> On 17/05/2022 14:21, Roger Pau Monne wrote:
> > Add support for enabling Bus Lock Detection on Intel systems. Such
> > detection works by triggering a vmexit, which is enough of a pause to
> > prevent a guest from abusing of the Bus Lock.
>
> "which is enough of a" is a bit firmer than ideal. "which Andy says
> will be ok" is perhaps more accurate.
>
> Perhaps "which ought to be enough" ?
>
> A buslock here or there is no problem, and non-malicious software
> appears to be devoid of buslocks (hardly surprising - it would be a hard
> error on other architectures), but a malicious piece of userspace can
> trivially cripple the system.
>
> Forcing a vmexit on every buslock limits an attacker to one buslock per
> however long a vmentry/exit cycle takes.
>
> > Add an extra perf counter to track the number of Bus Locks detected.
>
> extra Xen perf counter.
>
> Because other hypervisors use actual perf counters to emulate this
> ability on current hardware. Maybe something we should consider...
>
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index d03e78bf0d..02cc7a2023 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -4053,6 +4053,16 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >
> > if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> > return vmx_failed_vmentry(exit_reason, regs);
> > + if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> > + {
> > + /*
> > + * Delivery of Bus Lock VM exit was pre-empted by a higher priority VM
> > + * exit.
> > + */
> > + exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> > + if ( exit_reason != EXIT_REASON_BUS_LOCK )
> > + perfc_incr(buslock);
>
> I'm pretty sure you can drop the if, and do the perfc_incr()
> unconditionally. You won't get EXIT_REASON_BUS_LOCK |
> VMX_EXIT_REASONS_BUS_LOCK given that wording in the ISE.

I though the same, but got a EXIT_REASON_BUS_LOCK |
VMX_EXIT_REASONS_BUS_LOCK fairly easy by simply doing a xchg over a
cache line boundary.

I think at least on the model I'm testing it looks like
VMX_EXIT_REASONS_BUS_LOCK is added unconditionally, regardless of
whether the vmexit itself is already EXIT_REASON_BUS_LOCK.

> To test, Intel has PENDING_DBG which interferes with most easy attempts
> to create the condition, but how about this.
>
> Load an LDT, misaligned across a cacheline boundary, and set #DB's %cs
> to LDT[0] with a clear access bit, then execute an `icebp` instruction.
> The atomic write to set the access bit is a 4-byte access typically.
>
> This should cause the #DB intercept to trigger on the same instantaneous
> boundary that generated the buslock.
>
> Otherwise, LGTM.

If you agree with the above I will modify the commit message and
resend.

Thanks, Roger.

Re: [PATCH 1/2] x86/vmx: implement Bus Lock detection [ In reply to ]

amc96 at srcf

May 19, 2022, 6:05 AM

Post #4 of 4 (5 views)

Permalink

On 19/05/2022 13:21, Roger Pau Monné wrote:
> On Wed, May 18, 2022 at 10:50:02PM +0000, Andrew Cooper wrote:
>> On 17/05/2022 14:21, Roger Pau Monne wrote:
>>> Add support for enabling Bus Lock Detection on Intel systems. Such
>>> detection works by triggering a vmexit, which is enough of a pause to
>>> prevent a guest from abusing of the Bus Lock.
>> "which is enough of a" is a bit firmer than ideal. "which Andy says
>> will be ok" is perhaps more accurate.
>>
>> Perhaps "which ought to be enough" ?
>>
>> A buslock here or there is no problem, and non-malicious software
>> appears to be devoid of buslocks (hardly surprising - it would be a hard
>> error on other architectures), but a malicious piece of userspace can
>> trivially cripple the system.
>>
>> Forcing a vmexit on every buslock limits an attacker to one buslock per
>> however long a vmentry/exit cycle takes.
>>
>>> Add an extra perf counter to track the number of Bus Locks detected.
>> extra Xen perf counter.
>>
>> Because other hypervisors use actual perf counters to emulate this
>> ability on current hardware. Maybe something we should consider...
>>
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index d03e78bf0d..02cc7a2023 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -4053,6 +4053,16 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>
>>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
>>> return vmx_failed_vmentry(exit_reason, regs);
>>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
>>> + {
>>> + /*
>>> + * Delivery of Bus Lock VM exit was pre-empted by a higher priority VM
>>> + * exit.
>>> + */
>>> + exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
>>> + if ( exit_reason != EXIT_REASON_BUS_LOCK )
>>> + perfc_incr(buslock);
>> I'm pretty sure you can drop the if, and do the perfc_incr()
>> unconditionally. You won't get EXIT_REASON_BUS_LOCK |
>> VMX_EXIT_REASONS_BUS_LOCK given that wording in the ISE.
> I though the same, but got a EXIT_REASON_BUS_LOCK |
> VMX_EXIT_REASONS_BUS_LOCK fairly easy by simply doing a xchg over a
> cache line boundary.
>
> I think at least on the model I'm testing it looks like
> VMX_EXIT_REASONS_BUS_LOCK is added unconditionally, regardless of
> whether the vmexit itself is already EXIT_REASON_BUS_LOCK.

Hmm, in which case you've found either an SDP bug, or a documentation bug.

Lets follow up with Intel and try to identify which it is.

~Andrew