Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096

[Liran Alon <liran.alon@oracle.com>]

> On 16 Jul 2019, at 23:02, Singh, Brijesh <brijesh.singh@amd.com> wrote:
> 
> 
> 
> On 7/16/19 2:34 PM, Liran Alon wrote:
>> 
>> 
>>> On 16 Jul 2019, at 22:28, Singh, Brijesh <brijesh.singh@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 7/16/19 12:35 PM, Liran Alon wrote:
>>>> 
>>>> 
>>>>> On 16 Jul 2019, at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> 
>>>>> On 16/07/19 18:56, Liran Alon wrote:
>>>>>> If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist,
>>>>>> then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP
>>>>>> violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks.
>>>>>> 
>>>>>> Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata.
>>>>> 
>>>>> Under the conditions in the code, if CPL were <3 then the SMAP fault
>>>>> would have been sent to the guest.
>>>>> But I agree that if we need to
>>>>> change it to check host CR4, then the CPL of the guest should not be
>>>>> checked.
>>>> 
>>>> Yep.
>>>> Well it all depends on how AMD CPU actually works.
>>>> We need some clarification from AMD but for sure the current code in KVM is not only wrong, but probably have never been tested. :P
>>>> 
>>>> Looking for further clarifications from AMD before submitting v2…
>>>> 
>>> 
>>> When this errata is hit, the CPU will be at CPL3. From hardware
>>> point-of-view the below sequence happens:
>>> 
>>> 1. CPL3 guest hits reserved bit NPT fault (MMIO access)
>> 
>> Why CPU needs to be at CPL3?
>> The requirement for SMAP should be that this page is user-accessible in guest page-tables.
>> Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0.
>> 
> 
> We are discussing reserved NPF so we need to be at CPL3.

I don’t see the connection between a reserved #NPF and the need to be at CPL3.
A vCPU can execute at CPL<3 a page that is mapped user-accessible in guest page-tables in case CR4.SMEP=0
and then instruction will execute successfully and can dereference a page that is mapped in NPT using an entry with a reserved bit set.
Thus, reserved #NPF will be raised while vCPU is at CPL<3 and DecodeAssist microcode will still raise SMAP violation
as CR4.SMAP=1 and microcode perform data-fetch with CPL<3. This leading exactly to Errata condition as far as I understand.

> 
>>> 
>>> 2. Microcode uses special opcode which attempts to read data using the
>>> CPL0 privileges. The microcode read CS:RIP, when it hits SMAP fault,
>>> it gives up and returns no instruction bytes.
>>> 
>>> (Note: vCPU is still at CPL3)
>> 
>> So at this point guest vCPU CR4.SMAP is what matters right? Not host CR4.SMAP.
>> 
> 
> Yes, its guest vCPU SMAP.
> 
>>> 
>>> 3. CPU causes #VMEXIT for original fault address.
>>> 
>>> The SMAP fault occurred while we are still in guest context. It will be
>>> nice to have code test example to triggers this errata.
>> 
>> I can write such code in kvm-unit-tests for you to run on relevant hardware if you have such a machine present.
>> I don’t have relevant machine with me and therefore I wrote a disclaimer I couldn’t test it in cover letter.
>> 
> 
> I have required hardware and should be able to run some test for you.

Ok. Will write such kvm-unit-test and Cc you.

> 
>> So to sum-up what KVM needs to do:
>> 1) Check guest vCPU CR4.SMAP is set to 1. (As I fixed in this commit).
>> 2) Remove the check for CPL==3. If we really want to be pedantic, we can parse guest page-tables to see if PTE have U/S bit set to 1.
> 
> 
> Remember in the case of SEV guest, the page-tables are encrypted with
> the guest specific key and we will not be able to walk it to inspect
> the U/S bit. We want to detect the errata and if its SEV guest then
> we can't do much, for non-SEV fallback to instruction decode which
> will walk the guest page-tables to fetch the instruction bytes.
> 
> 
>> What do you think?
>> 
>> -Liran
>> 
>>> 
>>>> -Liran
>>>> 
>>>>> 
>>>>> Paolo
>>>>> 
>>>>>> It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1.
>>>>> 
>>>> 
>>