Re: [PATCH 4/5 v4] nSVM: Test addresses of MSR and IO permissions maps

[Sean Christopherson <seanjc@google.com>]

On Wed, Mar 24, 2021, Krish Sadhukhan wrote:
> According to section "Canonicalization and Consistency Checks" in APM vol 2,
> the following guest state is illegal:
> 
>     "The MSR or IOIO intercept tables extend to a physical address that
>      is greater than or equal to the maximum supported physical address."
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  x86/svm_tests.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/x86/svm_tests.c b/x86/svm_tests.c
> index 29a0b59..70442d2 100644
> --- a/x86/svm_tests.c
> +++ b/x86/svm_tests.c
> @@ -2304,6 +2304,33 @@ static void test_dr(void)
>  	vmcb->save.dr7 = dr_saved;
>  }
>  
> +/*
> + * If the MSR or IOIO intercept table extends to a physical address that
> + * is greater than or equal to the maximum supported physical address, the
> + * guest state is illegal.
> + *
> + * [ APM vol 2]
> + */
> +static void test_msrpm_iopm_bitmap_addrs(void)
> +{
> +	u64 addr_spill_beyond_ram =

FWIW, it's not "beyond ram", it's beyond the legal physical address space.  E.g.
the address can point at stuff other than RAM and be perfectly legal from a
consistency check perspective.

> +	    (u64)(((u64)1 << cpuid_maxphyaddr()) - 4096);

It'd be nice to also check a straight legal address, and an address that
straddles the high address => 0.

> +
> +	/* MSR bitmap address */
> +	vmcb->control.intercept |= 1ULL << INTERCEPT_MSR_PROT;
> +	vmcb->control.msrpm_base_pa = addr_spill_beyond_ram;
> +	report(svm_vmrun() == SVM_EXIT_ERR, "Test MSRPM address: %lx",
> +	    addr_spill_beyond_ram);
> +	vmcb->control.intercept &= ~(1ULL << INTERCEPT_MSR_PROT);
> +
> +	/* MSR bitmap address */
> +	vmcb->control.intercept |= 1ULL << INTERCEPT_IOIO_PROT;
> +	vmcb->control.msrpm_base_pa = addr_spill_beyond_ram;

Wrong bitmap.

> +	report(svm_vmrun() == SVM_EXIT_ERR, "Test IOPM address: %lx",
> +	    addr_spill_beyond_ram);
> +	vmcb->control.intercept &= ~(1ULL << INTERCEPT_IOIO_PROT);

The control should be save/restored, assuming the intercepts were clear will
cause reproducibility issues for other tests.

> +}
> +
>  static void svm_guest_state_test(void)
>  {
>  	test_set_guest(basic_guest_main);
> @@ -2313,6 +2340,7 @@ static void svm_guest_state_test(void)
>  	test_cr3();
>  	test_cr4();
>  	test_dr();
> +	test_msrpm_iopm_bitmap_addrs();
>  }
>  
>  
> -- 
> 2.27.0
>