diff mbox series

[1/2] nVMX x86: Add a check for reserved bits [11:0] of PML address

Message ID 20180926181812.30679-2-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series [1/2] nVMX x86: Add a check for reserved bits [11:0] of PML address | expand

Commit Message

Krish Sadhukhan Sept. 26, 2018, 6:18 p.m. UTC 
According to section "Checks on VMX Controls" in Intel SDM vol 3C, bits 11:0
of the PML address must be 0.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
---
 arch/x86/include/asm/vmx.h | 2 ++
 arch/x86/kvm/vmx.c         | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Sept. 26, 2018, 6:56 p.m. UTC | #1 
On Wed, Sep 26, 2018 at 02:18:11PM -0400, Krish Sadhukhan wrote:
> According to section "Checks on VMX Controls" in Intel SDM vol 3C, bits 11:0
> of the PML address must be 0.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  arch/x86/include/asm/vmx.h | 2 ++
>  arch/x86/kvm/vmx.c         | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 9527ba5..2c118ad 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -339,6 +339,8 @@ enum vmcs_field {
>  	HOST_RIP                        = 0x00006c16,
>  };
>  
> +#define	PML_ADDRESS_RESV_BITS		0xfff
> +
>  /*
>   * Interruption-information format
>   */
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 533a327..49e707d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11712,7 +11712,8 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>  	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) {
>  		if (!nested_cpu_has_ept(vmcs12) ||
>  		    !IS_ALIGNED(address, 4096)  ||
> -		    address >> maxphyaddr)
> +		    address >> maxphyaddr ||
> +		    address & PML_ADDRESS_RESV_BITS)

This check is handled by "!IS_ALIGNED(address, 4096)".  There's also
existing unit test coverage for this, albeit it's not what one would
describe as exhaustive, e.g.:

	test_vmcs_page_addr(name, encoding, ignored,
			    xfail_beyond_mapped_ram, PAGE_SIZE - 1);

Is there a failure associated with this series or was this prompted
by inspection?

>  			return -EINVAL;
>  	}
>  
> -- 
> 2.9.5
>
Jim Mattson Sept. 26, 2018, 7:18 p.m. UTC | #2 
On Wed, Sep 26, 2018 at 11:56 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:

> This check is handled by "!IS_ALIGNED(address, 4096)".  There's also
> existing unit test coverage for this, albeit it's not what one would
> describe as exhaustive, e.g.:
>
>         test_vmcs_page_addr(name, encoding, ignored,
>                             xfail_beyond_mapped_ram, PAGE_SIZE - 1);
>

If that's a line from test_vmcs_page_values, there are quite a few
more interesting values tested in that function, including each
individual bit.  However, I don't see a caller of
test_vmcs_page_reference() for the PMLADDR field.
Jim Mattson Sept. 26, 2018, 8:03 p.m. UTC | #3 
On Wed, Sep 26, 2018 at 11:18 AM, Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
> According to section "Checks on VMX Controls" in Intel SDM vol 3C, bits 11:0
> of the PML address must be 0.
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  arch/x86/include/asm/vmx.h | 2 ++
>  arch/x86/kvm/vmx.c         | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 9527ba5..2c118ad 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -339,6 +339,8 @@ enum vmcs_field {
>         HOST_RIP                        = 0x00006c16,
>  };
>
> +#define        PML_ADDRESS_RESV_BITS           0xfff
> +
>  /*
>   * Interruption-information format
>   */
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 533a327..49e707d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11712,7 +11712,8 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>         if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) {
>                 if (!nested_cpu_has_ept(vmcs12) ||
>                     !IS_ALIGNED(address, 4096)  ||
> -                   address >> maxphyaddr)
> +                   address >> maxphyaddr ||
> +                   address & PML_ADDRESS_RESV_BITS)

As Sean points out, the reserved bit check is redundant. Rather than
open-coding the checks, this should be probably just be:
        if (!nested_cpu_has_ept(vmcs12) ||
            !page_address_valid(vcpu, vmcs12->pml_address))
Krish Sadhukhan Sept. 26, 2018, 9:15 p.m. UTC | #4 
On 09/26/2018 01:03 PM, Jim Mattson wrote:
> On Wed, Sep 26, 2018 at 11:18 AM, Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> According to section "Checks on VMX Controls" in Intel SDM vol 3C, bits 11:0
>> of the PML address must be 0.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
>> ---
>>   arch/x86/include/asm/vmx.h | 2 ++
>>   arch/x86/kvm/vmx.c         | 3 ++-
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
>> index 9527ba5..2c118ad 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -339,6 +339,8 @@ enum vmcs_field {
>>          HOST_RIP                        = 0x00006c16,
>>   };
>>
>> +#define        PML_ADDRESS_RESV_BITS           0xfff
>> +
>>   /*
>>    * Interruption-information format
>>    */
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 533a327..49e707d 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -11712,7 +11712,8 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>>          if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) {
>>                  if (!nested_cpu_has_ept(vmcs12) ||
>>                      !IS_ALIGNED(address, 4096)  ||
>> -                   address >> maxphyaddr)
>> +                   address >> maxphyaddr ||
>> +                   address & PML_ADDRESS_RESV_BITS)
> As Sean points out, the reserved bit check is redundant. Rather than

Agreed.

> open-coding the checks, this should be probably just be:
>          if (!nested_cpu_has_ept(vmcs12) ||
>              !page_address_valid(vcpu, vmcs12->pml_address))

Yes, we should use page_address_valid().  Also, we should use 
nested_cpu_has_pml() instead of using nested_cpu_has2():

      if (nested_cpu_has_pml() && (!nested_cpu_has_ept(vmcs12) ||
             !page_address_valid(vcpu, vmcs12->pml_address))
     .            return -EINVAL;
Jim Mattson Sept. 26, 2018, 9:46 p.m. UTC | #5 
On Wed, Sep 26, 2018 at 2:15 PM, Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:

>      if (nested_cpu_has_pml() && (!nested_cpu_has_ept(vmcs12) ||
>             !page_address_valid(vcpu, vmcs12->pml_address))
>     .            return -EINVAL;

Perhaps this would be more readable in the style of
nested_vmx_check_msr_bitmap_controls:

if (!nested_cpu_has_pml(vmcs12))
        return 0;

if (!nested_cpu_has_ept(vmcs12) ||
    !page_address_valid(vcpu, vmcs12->pml_address))
        return -EINVAL;

return 0;
diff mbox series

Patch

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 9527ba5..2c118ad 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -339,6 +339,8 @@  enum vmcs_field {
 	HOST_RIP                        = 0x00006c16,
 };
 
+#define	PML_ADDRESS_RESV_BITS		0xfff
+
 /*
  * Interruption-information format
  */
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 533a327..49e707d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11712,7 +11712,8 @@  static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
 	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) {
 		if (!nested_cpu_has_ept(vmcs12) ||
 		    !IS_ALIGNED(address, 4096)  ||
-		    address >> maxphyaddr)
+		    address >> maxphyaddr ||
+		    address & PML_ADDRESS_RESV_BITS)
 			return -EINVAL;
 	}