[PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Dan Carpenter]

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;

Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

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;
 		}

[PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Dan Carpenter]

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;

Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

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;
 		}

Re: [PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Bandan Das]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> 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;
>
> Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> 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) {

This is actually an overflow check and as the counter decrements, we hit
0xffff which will satisfy guest_pml_index > PML_ENTITY_NUM.

However, a buggy guest hypervisor can write 512 to guest_pml_index and in that
case, we need to inject a pml full event which won't happen if we don't use >=.
So, your patch is correct, I would only suggest that the commit message be
modified to reflect this condition.

Bandan

>  			vmx->nested.pml_full = true;
>  			return 1;
>  		}

Re: [PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Bandan Das]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> 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;
>
> Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> 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) {

This is actually an overflow check and as the counter decrements, we hit
0xffff which will satisfy guest_pml_index > PML_ENTITY_NUM.

However, a buggy guest hypervisor can write 512 to guest_pml_index and in that
case, we need to inject a pml full event which won't happen if we don't use >=.
So, your patch is correct, I would only suggest that the commit message be
modified to reflect this condition.

Bandan

>  			vmx->nested.pml_full = true;
>  			return 1;
>  		}

[PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Dan Carpenter]

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.

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;
 		}

[PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Dan Carpenter]

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.

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;
 		}

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Paolo Bonzini]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Paolo Bonzini]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Dan Carpenter]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Dan Carpenter]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Paolo Bonzini]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Paolo Bonzini]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Bandan Das]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Bandan Das]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Paolo Bonzini]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Paolo Bonzini]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Bandan Das]

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

Re: [PATCH v2] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Bandan Das]

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

Re: [PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Radim Krčmář]

2017-05-10 22:43+0300, Dan Carpenter:
> 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;
> 
> Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied to kvm/master, thanks.

(v1 was deemed better after all.)

Re: [PATCH] kvm: nVMX: off by one in vmx_write_pml_buffer()

[Radim Krčmář]

2017-05-10 22:43+0300, Dan Carpenter:
> 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;
> 
> Fixes: c5f983f6e845 ("nVMX: Implement emulated Page Modification Logging")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied to kvm/master, thanks.

(v1 was deemed better after all.)