kvm: nVMX: off by one in vmx_write_pml_buffer()

Commit Message

Dan Carpenter May 10, 2017, 7:43 p.m. UTC

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 <[e-mail]>

Comments

Bandan Das May 10, 2017, 8:18 p.m. UTC | #1

Dan Carpenter <[e-mail]> 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 <[e-mail]>
>
> 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;
>  		}

Radim Krčmář May 16, 2017, 1:56 p.m. UTC | #2

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 <[e-mail]>

Applied to kvm/master, thanks.

(v1 was deemed better after all.)

Patch

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