linux-kernel - Re: [PATCH v6 1/7] KVM: x86: Deflect unknown MSR accesses to user space

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <186ccace-2fad-3db3-0848-cd272b1a64ba@amazon.com>
Date:   Wed, 16 Sep 2020 11:31:30 +0200
From:   Alexander Graf <graf@...zon.com>
To:     Aaron Lewis <aaronlewis@...gle.com>
CC:     Paolo Bonzini <pbonzini@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        KarimAllah Raslan <karahmed@...zon.de>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        kvm list <kvm@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 1/7] KVM: x86: Deflect unknown MSR accesses to user
 space

Hi Aaron,

Thanks a lot for the amazing review! I've been caught in some other 
things recently, so sorry for the delayed response.

On 03.09.20 21:27, Aaron Lewis wrote:
> 
>> +::
>> +
>> +               /* KVM_EXIT_X86_RDMSR / KVM_EXIT_X86_WRMSR */
>> +               struct {
>> +                       __u8 error; /* user -> kernel */
>> +                       __u8 pad[3];
>> +                       __u32 reason; /* kernel -> user */
>> +                       __u32 index; /* kernel -> user */
>> +                       __u64 data; /* kernel <-> user */
>> +               } msr;
>> +
>> +Used on x86 systems. When the VM capability KVM_CAP_X86_USER_SPACE_MSR is
>> +enabled, MSR accesses to registers that would invoke a #GP by KVM kernel code
>> +will instead trigger a KVM_EXIT_X86_RDMSR exit for reads and KVM_EXIT_X86_WRMSR
>> +exit for writes.
>> +
>> +The "reason" field specifies why the MSR trap occurred. User space will only
>> +receive MSR exit traps when a particular reason was requested during through
>> +ENABLE_CAP. Currently valid exit reasons are:
>> +
>> +       KVM_MSR_EXIT_REASON_INVAL - access to invalid MSRs or reserved bits
> 
> 
> Can we also have ENOENT?
>          KVM_MSR_EXIT_REASON_ENOENT - Unknown MSR

I tried to add that at first, but it gets tricky really fast. Why should 
user space have a vested interest in differentiating between "MSR is not 
implemented" and "MSR is guarded by a CPUID flag and thus not handled" 
or "MSR is guarded by a CAP"?

The more details we reveal, the more likely we're to break ABI 
compatibility.

> 
>>
>> +
>> +For KVM_EXIT_X86_RDMSR, the "index" field tells user space which MSR the guest
>> +wants to read. To respond to this request with a successful read, user space
>> +writes the respective data into the "data" field and must continue guest
>> +execution to ensure the read data is transferred into guest register state.
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 88c593f83b28..4d285bf054fb 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1549,12 +1549,88 @@ int kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data)
>>   }
>>   EXPORT_SYMBOL_GPL(kvm_set_msr);
>>
>> +static int complete_emulated_msr(struct kvm_vcpu *vcpu, bool is_read)
>> +{
>> +       if (vcpu->run->msr.error) {
>> +               kvm_inject_gp(vcpu, 0);
> 
> Add return 1. The RIP doesn’t advance when the instruction raises a fault.

Yikes. Good catch! Thank you!

> 
>>
>> +       } else if (is_read) {
>> +               kvm_rax_write(vcpu, (u32)vcpu->run->msr.data);
>> +               kvm_rdx_write(vcpu, vcpu->run->msr.data >> 32);
>> +       }
>> +
>> +       return kvm_skip_emulated_instruction(vcpu);
>> +}
>> +
>> +static int complete_emulated_rdmsr(struct kvm_vcpu *vcpu)
>> +{
>> +       return complete_emulated_msr(vcpu, true);
>> +}
>> +
>>
>>   /* For KVM_EXIT_INTERNAL_ERROR */
>>   /* Emulate instruction failed. */
>> @@ -412,6 +414,15 @@ struct kvm_run {
>>                          __u64 esr_iss;
>>                          __u64 fault_ipa;
>>                  } arm_nisv;
>> +               /* KVM_EXIT_X86_RDMSR / KVM_EXIT_X86_WRMSR */
>> +               struct {
>> +                       __u8 error; /* user -> kernel */
>> +                       __u8 pad[3];
> 
> __u8 pad[7] to maintain 8 byte alignment?  unless we can get away with
> fewer bits for 'reason' and
> get them from 'pad'.

Why would we need an 8 byte alignment here? I always thought natural u64 
alignment on x86_64 was on 4 bytes?


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879