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.
