Re: [PATCH v9 09/17] x86/split_lock: Handle #AC exception for split lock

Xiaoyao Li
2019-10-17T01:24:03Z

On 10/17/2019 1:42 AM, Sean Christopherson wrote:
> On Wed, Oct 16, 2019 at 09:23:37AM -0700, Sean Christopherson wrote:
>> On Wed, Oct 16, 2019 at 05:43:53PM +0200, Paolo Bonzini wrote:
>>> On 16/10/19 17:41, Sean Christopherson wrote:
>>>> On Wed, Oct 16, 2019 at 04:08:14PM +0200, Paolo Bonzini wrote:
>>>>> SIGBUS (actually a new KVM_EXIT_INTERNAL_ERROR result from KVM_RUN is
>>>>> better, but that's the idea) is for when you're debugging guests.
>>>>> Global disable (or alternatively, disable SMT) is for production use.
>>>>
>>>> Alternatively, for guests without split-lock #AC enabled, what if KVM were
>>>> to emulate the faulting instruction with split-lock detection temporarily
>>>> disabled?
>>>
>>> Yes we can get fancy, but remember that KVM is not yet supporting
>>> emulation of locked instructions. Adding it is possible but shouldn't
>>> be in the critical path for the whole feature.
>>
>> Ah, didn't realize that. I'm surprised emulating all locks with cmpxchg
>> doesn't cause problems (or am I misreading the code?). Assuming I'm
>> reading the code correctly, the #AC path could kick all other vCPUS on
>> emulation failure and then retry emulation to "guarantee" success. Though
>> that's starting to build quite the house of cards.
>
> Ugh, doesn't the existing emulation behavior create another KVM issue?
> KVM uses a locked cmpxchg in emulator_cmpxchg_emulated() and the address
> is guest controlled, e.g. a guest could coerce the host into disabling
> split-lock detection via the host's #AC handler by triggering emulation
> and inducing an #AC in the emulator.
>

Exactly right.

I have tested with force_emulation_prefix. It did go into the #AC
handler and disable the split-lock detection in host.

However, without force_emulation_prefix enabled, I'm not sure whether
malicious guest can create the case causing the emulation with a lock
prefix and going to the emulator_cmpxchg_emulated().
I found it impossible without force_emulation_prefix enabled and I'm not
familiar with emulation at all. If I missed something, please let me know.

>>> How would you disable split-lock detection temporarily? Just tweak
>>> MSR_TEST_CTRL for the time of running the one instruction, and cross
>>> fingers that the sibling doesn't notice?
>>
>> Tweak MSR_TEST_CTRL, with logic to handle the scenario where split-lock
>> detection is globally disable during emulation (so KVM doesn't
>> inadvertantly re-enable it).
>>
>> There isn't much for the sibling to notice. The kernel would temporarily
>> allow split-locks on the sibling, but that's a performance issue and isn't
>> directly fatal. A missed #AC in the host kernel would only delay the
>> inevitable global disabling of split-lock. A missed #AC in userspace would
>> again just delay the inevitable SIGBUS.