| Message ID | 20170510204302.ilb7bs3pbr6h7d7u@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/05/2017 22:43, Dan Carpenter wrote: > There are PML_ENTITY_NUM elements in the pml_address[] array so the > > should be >= or we write beyond the end of the array when we do: > > pml_address[vmcs12->guest_pml_index--] = gpa; > > This causes a static checker warning but the runtime impact is minimal. > The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a > buggy hypervisor. The v1 commit message is better actually. You can always replace "buggy" with "malicious". It's a 8 byte write and bits 12-45 of the datum are controlled by the attacker. It's pretty bad (and embarrassing - I'm not sure why I was super-sure that PML_ENTITY_NUM was 511 rather than 512). Paolo
On Thu, May 11, 2017 at 09:31:17AM +0200, Paolo Bonzini wrote: > > > On 10/05/2017 22:43, Dan Carpenter wrote: > > There are PML_ENTITY_NUM elements in the pml_address[] array so the > > > should be >= or we write beyond the end of the array when we do: > > > > pml_address[vmcs12->guest_pml_index--] = gpa; > > > > This causes a static checker warning but the runtime impact is minimal. > > The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a > > buggy hypervisor. > > The v1 commit message is better actually. You can always replace > "buggy" with "malicious". Can't the hypervisor already basically do what it wants? regards, dan carpenter
On 11/05/2017 09:42, Dan Carpenter wrote: > On Thu, May 11, 2017 at 09:31:17AM +0200, Paolo Bonzini wrote: >> >> >> On 10/05/2017 22:43, Dan Carpenter wrote: >>> There are PML_ENTITY_NUM elements in the pml_address[] array so the > >>> should be >= or we write beyond the end of the array when we do: >>> >>> pml_address[vmcs12->guest_pml_index--] = gpa; >>> >>> This causes a static checker warning but the runtime impact is minimal. >>> The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a >>> buggy hypervisor. >> >> The v1 commit message is better actually. You can always replace >> "buggy" with "malicious". > > Can't the hypervisor already basically do what it wants? Here you have a nested hypervisor, that can force the host hypervisor to do a kmap and write at offset 4096 inside (well, actually outside...) that kmap. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 10/05/2017 22:43, Dan Carpenter wrote: >> There are PML_ENTITY_NUM elements in the pml_address[] array so the > >> should be >= or we write beyond the end of the array when we do: >> >> pml_address[vmcs12->guest_pml_index--] = gpa; Actually, we can never write beyond the end when we do pml_address[vmcs12->guest_pml_index--] = gpa (which happens in the host hypervisor btw). I think this should be changed. >> This causes a static checker warning but the runtime impact is minimal. >> The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a >> buggy hypervisor. > > The v1 commit message is better actually. You can always replace > "buggy" with "malicious". I agree, they are interchangeable but what's the worst that can happen ? L1 killing itself ? Bandan > It's a 8 byte write and bits 12-45 of the datum are controlled by the > attacker. It's pretty bad (and embarrassing - I'm not sure why I was > super-sure that PML_ENTITY_NUM was 511 rather than 512). > > Paolo
On 11/05/2017 15:56, Bandan Das wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 10/05/2017 22:43, Dan Carpenter wrote: >>> There are PML_ENTITY_NUM elements in the pml_address[] array so the > >>> should be >= or we write beyond the end of the array when we do: >>> >>> pml_address[vmcs12->guest_pml_index--] = gpa; > > Actually, we can never write beyond the end when we do > pml_address[vmcs12->guest_pml_index--] = gpa (which happens in the > host hypervisor btw). I think this should be changed. If vmcs12->guest_pml_index is 512 it will write beyond the end without Dan's patch. >>> This causes a static checker warning but the runtime impact is minimal. >>> The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a >>> buggy hypervisor. >> >> The v1 commit message is better actually. You can always replace >> "buggy" with "malicious". > > I agree, they are interchangeable but what's the worst that can happen ? > L1 killing itself ? L0 writing 8 bytes in kernel memory outside the bounds of L1's memory. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/05/2017 15:56, Bandan Das wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 10/05/2017 22:43, Dan Carpenter wrote: >>>> There are PML_ENTITY_NUM elements in the pml_address[] array so the > >>>> should be >= or we write beyond the end of the array when we do: >>>> >>>> pml_address[vmcs12->guest_pml_index--] = gpa; >> >> Actually, we can never write beyond the end when we do >> pml_address[vmcs12->guest_pml_index--] = gpa (which happens in the >> host hypervisor btw). I think this should be changed. > > If vmcs12->guest_pml_index is 512 it will write beyond the end without > Dan's patch. Oops, sorry! I misread, the assignment is taking place here as well. v1 is fine. Thanks, Bandan >>>> This causes a static checker warning but the runtime impact is minimal. >>>> The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a >>>> buggy hypervisor. >>> >>> The v1 commit message is better actually. You can always replace >>> "buggy" with "malicious". >> >> I agree, they are interchangeable but what's the worst that can happen ? >> L1 killing itself ? > > L0 writing 8 bytes in kernel memory outside the bounds of L1's memory. > > Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index c6f4ad44aa95..7698e8f321bf 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -11213,7 +11213,7 @@ static int vmx_write_pml_buffer(struct kvm_vcpu *vcpu) if (!nested_cpu_has_pml(vmcs12)) return 0; - if (vmcs12->guest_pml_index > PML_ENTITY_NUM) { + if (vmcs12->guest_pml_index >= PML_ENTITY_NUM) { vmx->nested.pml_full = true; return 1; }
There are PML_ENTITY_NUM elements in the pml_address[] array so the > should be >= or we write beyond the end of the array when we do: pml_address[vmcs12->guest_pml_index--] = gpa; This causes a static checker warning but the runtime impact is minimal. The ->guest_pml_index variable can only be set to PML_ENTITY_NUM by a buggy hypervisor. Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: update the changelog to say that this is a low severity bug.