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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.