diff mbox series

[3/7,v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

Message ID 20210412215611.110095-4-krish.sadhukhan@oracle.com (mailing list archive)
State New
Headers show
Series KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests | expand

Commit Message

Krish Sadhukhan April 12, 2021, 9:56 p.m. UTC
According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
bitmaps. Therefore setting/unssetting these bits has no effect as far as
VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
verifying hardware behavior.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/nested.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Paolo Bonzini April 17, 2021, 2:18 p.m. UTC | #1
On 12/04/21 23:56, Krish Sadhukhan wrote:
> According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
> bitmaps. Therefore setting/unssetting these bits has no effect as far as
> VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
> verifying hardware behavior.

Tests should only verify KVM behavior, in order to verify hardware 
behavior you have to run them on bare metal.

Still, the patch is okay.

Paolo

> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>   arch/x86/kvm/svm/nested.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ae53ae46ebca..fd42c8b7f99a 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>   
>   	/* Copy it here because nested_svm_check_controls will check it.  */
>   	svm->nested.ctl.asid           = control->asid;
> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>   }
>   
>   /*
>
Sean Christopherson April 20, 2021, 8 p.m. UTC | #2
On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
> According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
> bitmaps. Therefore setting/unssetting these bits has no effect as far as
> VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
> verifying hardware behavior.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/svm/nested.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ae53ae46ebca..fd42c8b7f99a 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>  
>  	/* Copy it here because nested_svm_check_controls will check it.  */
>  	svm->nested.ctl.asid           = control->asid;
> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;

This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned address.
The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.

I also don't think svm->nested.ctl.msrpm_base_pa makes its way to hardware; IIUC,
it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets loaded into
the "real" VMCB is vmcb02->control.msrpm_base_pa.

>  }
>  
>  /*
> -- 
> 2.27.0
>
Krish Sadhukhan April 22, 2021, 5:50 p.m. UTC | #3
On 4/20/21 1:00 PM, Sean Christopherson wrote:
> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>> According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
>> bitmaps. Therefore setting/unssetting these bits has no effect as far as
>> VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
>> verifying hardware behavior.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>   arch/x86/kvm/svm/nested.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index ae53ae46ebca..fd42c8b7f99a 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>   
>>   	/* Copy it here because nested_svm_check_controls will check it.  */
>>   	svm->nested.ctl.asid           = control->asid;
>> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned address.
> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>
> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to hardware; IIUC,
> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets loaded into
> the "real" VMCB is vmcb02->control.msrpm_base_pa.


Not sure if there's a problem with my patch as such, but upon inspecting 
the code, I see something missing:

     In nested_load_control_from_vmcb12(), we are not really loading 
msrpm_base_pa from vmcb12 even     though the name of the function 
suggests so.

     Then nested_vmcb_check_controls() checks msrpm_base_pa from 
'nested.ctl' which doesn't have         the copy from vmcb12.

     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of 
msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.

     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.


Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?

>>   }
>>   
>>   /*
>> -- 
>> 2.27.0
>>
Krish Sadhukhan April 22, 2021, 5:52 p.m. UTC | #4
On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
>
> On 4/20/21 1:00 PM, Sean Christopherson wrote:
>> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>>> According to APM vol 2, hardware ignores the low 12 bits in MSRPM 
>>> and IOPM
>>> bitmaps. Therefore setting/unssetting these bits has no effect as 
>>> far as
>>> VMRUN is concerned. Also, setting/unsetting these bits prevents 
>>> tests from
>>> verifying hardware behavior.
>>>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> ---
>>>   arch/x86/kvm/svm/nested.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>>> index ae53ae46ebca..fd42c8b7f99a 100644
>>> --- a/arch/x86/kvm/svm/nested.c
>>> +++ b/arch/x86/kvm/svm/nested.c
>>> @@ -287,8 +287,6 @@ static void 
>>> nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>>         /* Copy it here because nested_svm_check_controls will check 
>>> it.  */
>>>       svm->nested.ctl.asid           = control->asid;
>>> -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>>> -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned 
>> address.
>> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>>
>> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to 
>> hardware; IIUC,
>> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets 
>> loaded into
>> the "real" VMCB is vmcb02->control.msrpm_base_pa.
>
>
> Not sure if there's a problem with my patch as such, but upon 
> inspecting the code, I see something missing:
>
>     In nested_load_control_from_vmcb12(), we are not really loading 
> msrpm_base_pa from vmcb12 even     though the name of the function 
> suggests so.
>
>     Then nested_vmcb_check_controls() checks msrpm_base_pa from 
> 'nested.ctl' which doesn't have         the copy from vmcb12.
>
>     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of 
> msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
>
>     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
>
>
> Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?

Sorry, I meant to say:  Aren't we actually using msrpm_base_pa from 
vmcb01 instead of vmcb12 ?
>
>>>   }
>>>     /*
>>> -- 
>>> 2.27.0
>>>
Krish Sadhukhan April 22, 2021, 5:56 p.m. UTC | #5
On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
>
> On 4/20/21 1:00 PM, Sean Christopherson wrote:
>> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>>> According to APM vol 2, hardware ignores the low 12 bits in MSRPM 
>>> and IOPM
>>> bitmaps. Therefore setting/unssetting these bits has no effect as 
>>> far as
>>> VMRUN is concerned. Also, setting/unsetting these bits prevents 
>>> tests from
>>> verifying hardware behavior.
>>>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> ---
>>>   arch/x86/kvm/svm/nested.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>>> index ae53ae46ebca..fd42c8b7f99a 100644
>>> --- a/arch/x86/kvm/svm/nested.c
>>> +++ b/arch/x86/kvm/svm/nested.c
>>> @@ -287,8 +287,6 @@ static void 
>>> nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>>         /* Copy it here because nested_svm_check_controls will check 
>>> it.  */
>>>       svm->nested.ctl.asid           = control->asid;
>>> -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>>> -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned 
>> address.
>> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>>
>> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to 
>> hardware; IIUC,
>> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets 
>> loaded into
>> the "real" VMCB is vmcb02->control.msrpm_base_pa.
>
>
> Not sure if there's a problem with my patch as such, but upon 
> inspecting the code, I see something missing:
>
>     In nested_load_control_from_vmcb12(), we are not really loading 
> msrpm_base_pa from vmcb12 even     though the name of the function 
> suggests so.
>
>     Then nested_vmcb_check_controls() checks msrpm_base_pa from 
> 'nested.ctl' which doesn't have         the copy from vmcb12.
>
>     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of 
> msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
>
>     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
>
>
> Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?


Sorry, I meant to say,  "from vmcb01 instead of vmcb12"

>
>>>   }
>>>     /*
>>> -- 
>>> 2.27.0
>>>
Sean Christopherson April 22, 2021, 6:01 p.m. UTC | #6
On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
> 
> On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
> > 
> > On 4/20/21 1:00 PM, Sean Christopherson wrote:
> > > On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
> > > > According to APM vol 2, hardware ignores the low 12 bits in
> > > > MSRPM and IOPM
> > > > bitmaps. Therefore setting/unssetting these bits has no effect
> > > > as far as
> > > > VMRUN is concerned. Also, setting/unsetting these bits prevents
> > > > tests from
> > > > verifying hardware behavior.
> > > > 
> > > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > > > ---
> > > >   arch/x86/kvm/svm/nested.c | 2 --
> > > >   1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > index ae53ae46ebca..fd42c8b7f99a 100644
> > > > --- a/arch/x86/kvm/svm/nested.c
> > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > @@ -287,8 +287,6 @@ static void
> > > > nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> > > >         /* Copy it here because nested_svm_check_controls will
> > > > check it.  */
> > > >       svm->nested.ctl.asid           = control->asid;
> > > > -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> > > > -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
> > > This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned
> > > address.
> > > The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
> > > 
> > > I also don't think svm->nested.ctl.msrpm_base_pa makes its way to
> > > hardware; IIUC,
> > > it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets
> > > loaded into
> > > the "real" VMCB is vmcb02->control.msrpm_base_pa.
> > 
> > 
> > Not sure if there's a problem with my patch as such, but upon inspecting
> > the code, I see something missing:
> > 
> >     In nested_load_control_from_vmcb12(), we are not really loading
> > msrpm_base_pa from vmcb12 even     though the name of the function
> > suggests so.
> > 
> >     Then nested_vmcb_check_controls() checks msrpm_base_pa from
> > 'nested.ctl' which doesn't have         the copy from vmcb12.
> > 
> >     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of
> > msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
> > 
> >     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
> > 
> > 
> > Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?
> 
> 
> Sorry, I meant to say,  "from vmcb01 instead of vmcb12"

The bitmap that's shoved into hardware comes from vmcb02, the bitmap that KVM
reads to merge into _that_ bitmap comes from vmcb12.

static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
{
	/*
	 * This function merges the msr permission bitmaps of kvm and the
	 * nested vmcb. It is optimized in that it only merges the parts where
	 * the kvm msr permission bitmap may contain zero bits
	 */
	int i;

	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
		return true;

	for (i = 0; i < MSRPM_OFFSETS; i++) {
		u32 value, p;
		u64 offset;

		if (msrpm_offsets[i] == 0xffffffff)
			break;

		p      = msrpm_offsets[i];
		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);

		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
			return false;

		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
	}

	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm)); <- This is vmcb02

	return true;
}
Krish Sadhukhan April 23, 2021, 1:12 a.m. UTC | #7
On 4/22/21 11:01 AM, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>> On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
>>> On 4/20/21 1:00 PM, Sean Christopherson wrote:
>>>> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>>>>> According to APM vol 2, hardware ignores the low 12 bits in
>>>>> MSRPM and IOPM
>>>>> bitmaps. Therefore setting/unssetting these bits has no effect
>>>>> as far as
>>>>> VMRUN is concerned. Also, setting/unsetting these bits prevents
>>>>> tests from
>>>>> verifying hardware behavior.
>>>>>
>>>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>>> ---
>>>>>    arch/x86/kvm/svm/nested.c | 2 --
>>>>>    1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>>>>> index ae53ae46ebca..fd42c8b7f99a 100644
>>>>> --- a/arch/x86/kvm/svm/nested.c
>>>>> +++ b/arch/x86/kvm/svm/nested.c
>>>>> @@ -287,8 +287,6 @@ static void
>>>>> nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>>>>          /* Copy it here because nested_svm_check_controls will
>>>>> check it.  */
>>>>>        svm->nested.ctl.asid           = control->asid;
>>>>> -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>>>>> -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>>>> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned
>>>> address.
>>>> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>>>>
>>>> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to
>>>> hardware; IIUC,
>>>> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets
>>>> loaded into
>>>> the "real" VMCB is vmcb02->control.msrpm_base_pa.
>>>
>>> Not sure if there's a problem with my patch as such, but upon inspecting
>>> the code, I see something missing:
>>>
>>>      In nested_load_control_from_vmcb12(), we are not really loading
>>> msrpm_base_pa from vmcb12 even     though the name of the function
>>> suggests so.
>>>
>>>      Then nested_vmcb_check_controls() checks msrpm_base_pa from
>>> 'nested.ctl' which doesn't have         the copy from vmcb12.
>>>
>>>      Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of
>>> msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
>>>
>>>      Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
>>>
>>>
>>> Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?
>>
>> Sorry, I meant to say,  "from vmcb01 instead of vmcb12"
> The bitmap that's shoved into hardware comes from vmcb02, the bitmap that KVM
> reads to merge into _that_ bitmap comes from vmcb12.
>
> static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
> {
> 	/*
> 	 * This function merges the msr permission bitmaps of kvm and the
> 	 * nested vmcb. It is optimized in that it only merges the parts where
> 	 * the kvm msr permission bitmap may contain zero bits
> 	 */
> 	int i;
>
> 	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> 		return true;
>
> 	for (i = 0; i < MSRPM_OFFSETS; i++) {
> 		u32 value, p;
> 		u64 offset;
>
> 		if (msrpm_offsets[i] == 0xffffffff)
> 			break;
>
> 		p      = msrpm_offsets[i];
> 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>
> 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
> 			return false;
>
> 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
> 	}
>
> 	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm)); <- This is vmcb02
>
> 	return true;
> }

Sorry, I somehow missed the call to copy_vmcb_control_area() in 
nested_load_control_from_vmcb12() where we are actually getting the 
msrpm_base_pa from vmcb12. Thanks for the explanation.

Getting back to your concern that this patch breaks 
nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some 
bits in 11:0 are set, the hardware is anyway going to ignore those bits, 
irrespective of whether we clear them (before my patch) or pass them as 
is (my patch) and therefore what L1 thinks as a valid address will 
effectively be an invalid address to the hardware. The only difference 
my patch makes is it enables tests to verify hardware behavior. Am 
missing something ?
Sean Christopherson April 23, 2021, 3:56 p.m. UTC | #8
On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
> On 4/22/21 11:01 AM, Sean Christopherson wrote:
> > 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
> > 
> > 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
> > 			return false;
> > 
> > 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2

... 
 
> Getting back to your concern that this patch breaks
> nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some bits
> in 11:0 are set, the hardware is anyway going to ignore those bits,
> irrespective of whether we clear them (before my patch) or pass them as is
> (my patch) and therefore what L1 thinks as a valid address will effectively
> be an invalid address to the hardware. The only difference my patch makes is
> it enables tests to verify hardware behavior. Am missing something ?

See the above snippet where KVM reads the effectively vmcb12->msrpm to merge L1's
desires with KVM's desires.  By removing the code that ensures
svm->nested.ctl.msrpm_base_pa is page aligned, the above offset calculation will
be wrong.
Paolo Bonzini April 23, 2021, 8:31 p.m. UTC | #9
On 23/04/21 17:56, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>> On 4/22/21 11:01 AM, Sean Christopherson wrote:
>>> 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>>>
>>> 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
>>> 			return false;
>>>
>>> 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
> 
> ...
>   
>> Getting back to your concern that this patch breaks
>> nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some bits
>> in 11:0 are set, the hardware is anyway going to ignore those bits,
>> irrespective of whether we clear them (before my patch) or pass them as is
>> (my patch) and therefore what L1 thinks as a valid address will effectively
>> be an invalid address to the hardware. The only difference my patch makes is
>> it enables tests to verify hardware behavior. Am missing something ?
> 
> See the above snippet where KVM reads the effectively vmcb12->msrpm to merge L1's
> desires with KVM's desires.  By removing the code that ensures
> svm->nested.ctl.msrpm_base_pa is page aligned, the above offset calculation will
> be wrong.

In fact the kvm-unit-test you sent was also wrong for this same reason 
when it was testing addresses near the end of the physical address space.

Paolo
Krish Sadhukhan April 26, 2021, 9:59 p.m. UTC | #10
On 4/23/21 1:31 PM, Paolo Bonzini wrote:
> On 23/04/21 17:56, Sean Christopherson wrote:
>> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>>> On 4/22/21 11:01 AM, Sean Christopherson wrote:
>>>>         offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>>>>
>>>>         if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- 
>>>> This reads vmcb12
>>>>             return false;
>>>>
>>>>         svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge 
>>>> vmcb12's bitmap to KVM's bitmap for L2
>>
>> ...
>>> Getting back to your concern that this patch breaks
>>> nested_svm_vmrun_msrpm().  If L1 passes a valid address in which 
>>> some bits
>>> in 11:0 are set, the hardware is anyway going to ignore those bits,
>>> irrespective of whether we clear them (before my patch) or pass them 
>>> as is
>>> (my patch) and therefore what L1 thinks as a valid address will 
>>> effectively
>>> be an invalid address to the hardware. The only difference my patch 
>>> makes is
>>> it enables tests to verify hardware behavior. Am missing something ?
>>
>> See the above snippet where KVM reads the effectively vmcb12->msrpm 
>> to merge L1's
>> desires with KVM's desires.  By removing the code that ensures
>> svm->nested.ctl.msrpm_base_pa is page aligned, the above offset 
>> calculation will
>> be wrong.
>
> In fact the kvm-unit-test you sent was also wrong for this same reason 
> when it was testing addresses near the end of the physical address space.
>
> Paolo
>
It seems to me that we should clear bits 11:0 in 
nested_svm_vmrun_msrpm() where we are forming  msrpm_base_pa address for 
vmcb02.  nested_svm_check_bitmap_pa() aligns the address passed to it, 
before checking it.

Should I send a patch for this ?
Sean Christopherson April 26, 2021, 10:07 p.m. UTC | #11
On Mon, Apr 26, 2021, Krish Sadhukhan wrote:
> 
> On 4/23/21 1:31 PM, Paolo Bonzini wrote:
> > On 23/04/21 17:56, Sean Christopherson wrote:
> > > On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
> > > > On 4/22/21 11:01 AM, Sean Christopherson wrote:
> > > > >         offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
> > > > > 
> > > > >         if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value,
> > > > > 4)) <- This reads vmcb12
> > > > >             return false;
> > > > > 
> > > > >         svm->nested.msrpm[p] = svm->msrpm[p] | value; <-
> > > > > Merge vmcb12's bitmap to KVM's bitmap for L2
> > > 
> > > ...
> > > > Getting back to your concern that this patch breaks
> > > > nested_svm_vmrun_msrpm().  If L1 passes a valid address in which
> > > > some bits
> > > > in 11:0 are set, the hardware is anyway going to ignore those bits,
> > > > irrespective of whether we clear them (before my patch) or pass
> > > > them as is
> > > > (my patch) and therefore what L1 thinks as a valid address will
> > > > effectively
> > > > be an invalid address to the hardware. The only difference my
> > > > patch makes is
> > > > it enables tests to verify hardware behavior. Am missing something ?
> > > 
> > > See the above snippet where KVM reads the effectively vmcb12->msrpm
> > > to merge L1's
> > > desires with KVM's desires.  By removing the code that ensures
> > > svm->nested.ctl.msrpm_base_pa is page aligned, the above offset
> > > calculation will
> > > be wrong.
> > 
> > In fact the kvm-unit-test you sent was also wrong for this same reason
> > when it was testing addresses near the end of the physical address
> > space.
> > 
> > Paolo
> > 
> It seems to me that we should clear bits 11:0 in nested_svm_vmrun_msrpm()
> where we are forming  msrpm_base_pa address for vmcb02. 
> nested_svm_check_bitmap_pa() aligns the address passed to it, before
> checking it.
> 
> Should I send a patch for this ?

I don't see any reason to leave the bits set in svm->nested.ctl.msrpm_base_pa
any longer than they absolutely need to be set, i.e.
nested_load_control_from_vmcb12() feels like the best place to clear the offset.

If we really want to change something, we could WARN in the consistency check on
an unaligned address, e.g.

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 540d43ba2cf4..56e109d2ea7f 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -220,7 +220,7 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
  */
 static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size)
 {
-       u64 addr = PAGE_ALIGN(pa);
+       WARN_ON_ONCE(!PAGE_ALIGNED(pa));

        return kvm_vcpu_is_legal_gpa(vcpu, addr) &&
            kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index ae53ae46ebca..fd42c8b7f99a 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -287,8 +287,6 @@  static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 
 	/* Copy it here because nested_svm_check_controls will check it.  */
 	svm->nested.ctl.asid           = control->asid;
-	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
-	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
 }
 
 /*