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

[Andrew Cooper <amc96@xxxxxxxx>]

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