diff mbox series

KVM: arm64: kvm_reset_vcpu() return code incorrect with SVE

Message ID 20200617104339.35094-1-steven.price@arm.com
State New, archived
Headers show
Series KVM: arm64: kvm_reset_vcpu() return code incorrect with SVE | expand

Commit Message

Steven Price June 17, 2020, 10:43 a.m. UTC
If SVE is enabled then 'ret' can be assigned the return value of
kvm_vcpu_enable_sve() which may be 0 causing future "goto out" sites to
erroneously return 0 on failure rather than -EINVAL as expected.

Remove the initialisation of 'ret' and make setting the return value
explicit to avoid this situation in the future.

Fixes: 9a3cdf26e336 ("KVM: arm64/sve: Allow userspace to enable SVE for vcpus")
Reported-by: James Morse <james.morse@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
---
The problematic chunk isn't visible in the diff, so reproduced here:

	if (!kvm_arm_vcpu_sve_finalized(vcpu)) {
		if (test_bit(KVM_ARM_VCPU_SVE, vcpu->arch.features)) {
			ret = kvm_vcpu_enable_sve(vcpu);
			if (ret)
				goto out;
		}
	} else {
		kvm_vcpu_reset_sve(vcpu);
	}

 arch/arm64/kvm/reset.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Marc Zyngier June 17, 2020, 10:47 a.m. UTC | #1
Hi Steven,

On 2020-06-17 11:43, Steven Price wrote:
> If SVE is enabled then 'ret' can be assigned the return value of
> kvm_vcpu_enable_sve() which may be 0 causing future "goto out" sites to
> erroneously return 0 on failure rather than -EINVAL as expected.
> 
> Remove the initialisation of 'ret' and make setting the return value
> explicit to avoid this situation in the future.
> 
> Fixes: 9a3cdf26e336 ("KVM: arm64/sve: Allow userspace to enable SVE for 
> vcpus")
> Reported-by: James Morse <james.morse@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> The problematic chunk isn't visible in the diff, so reproduced here:
> 
> 	if (!kvm_arm_vcpu_sve_finalized(vcpu)) {
> 		if (test_bit(KVM_ARM_VCPU_SVE, vcpu->arch.features)) {
> 			ret = kvm_vcpu_enable_sve(vcpu);
> 			if (ret)
> 				goto out;
> 		}
> 	} else {
> 		kvm_vcpu_reset_sve(vcpu);
> 	}
> 
>  arch/arm64/kvm/reset.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index d3b209023727..f1057603b756 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -245,7 +245,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu 
> *vcpu)
>   */
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  {
> -	int ret = -EINVAL;
> +	int ret;
>  	bool loaded;
>  	u32 pstate;
> 
> @@ -269,15 +269,19 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> 
>  	if (test_bit(KVM_ARM_VCPU_PTRAUTH_ADDRESS, vcpu->arch.features) ||
>  	    test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, vcpu->arch.features)) {
> -		if (kvm_vcpu_enable_ptrauth(vcpu))
> +		if (kvm_vcpu_enable_ptrauth(vcpu)) {
> +			ret = -EINVAL;
>  			goto out;
> +		}
>  	}
> 
>  	switch (vcpu->arch.target) {
>  	default:
>  		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> -			if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1))
> +			if (cpus_have_const_cap(ARM64_HAS_32BIT_EL1)) {

Do you really mean this? Seems counter-productive... :-(

> +				ret = -EINVAL;
>  				goto out;
> +			}
>  			pstate = VCPU_RESET_PSTATE_SVC;
>  		} else {
>  			pstate = VCPU_RESET_PSTATE_EL1;

Thanks,

         M.
Steven Price June 17, 2020, 10:50 a.m. UTC | #2
On 17/06/2020 11:47, Marc Zyngier wrote:
> Hi Steven,
> 
> On 2020-06-17 11:43, Steven Price wrote:
>> If SVE is enabled then 'ret' can be assigned the return value of
>> kvm_vcpu_enable_sve() which may be 0 causing future "goto out" sites to
>> erroneously return 0 on failure rather than -EINVAL as expected.
>>
>> Remove the initialisation of 'ret' and make setting the return value
>> explicit to avoid this situation in the future.
>>
>> Fixes: 9a3cdf26e336 ("KVM: arm64/sve: Allow userspace to enable SVE 
>> for vcpus")
>> Reported-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> The problematic chunk isn't visible in the diff, so reproduced here:
>>
>>     if (!kvm_arm_vcpu_sve_finalized(vcpu)) {
>>         if (test_bit(KVM_ARM_VCPU_SVE, vcpu->arch.features)) {
>>             ret = kvm_vcpu_enable_sve(vcpu);
>>             if (ret)
>>                 goto out;
>>         }
>>     } else {
>>         kvm_vcpu_reset_sve(vcpu);
>>     }
>>
>>  arch/arm64/kvm/reset.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index d3b209023727..f1057603b756 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -245,7 +245,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu 
>> *vcpu)
>>   */
>>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>  {
>> -    int ret = -EINVAL;
>> +    int ret;
>>      bool loaded;
>>      u32 pstate;
>>
>> @@ -269,15 +269,19 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>
>>      if (test_bit(KVM_ARM_VCPU_PTRAUTH_ADDRESS, vcpu->arch.features) ||
>>          test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, vcpu->arch.features)) {
>> -        if (kvm_vcpu_enable_ptrauth(vcpu))
>> +        if (kvm_vcpu_enable_ptrauth(vcpu)) {
>> +            ret = -EINVAL;
>>              goto out;
>> +        }
>>      }
>>
>>      switch (vcpu->arch.target) {
>>      default:
>>          if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
>> -            if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1))
>> +            if (cpus_have_const_cap(ARM64_HAS_32BIT_EL1)) {
> 
> Do you really mean this? Seems counter-productive... :-(

Clearly not... I'm really not sure how I managed to screw that up so 
badly :(

I'm glad someone is awake!

Sorry about that,

Steve

>> +                ret = -EINVAL;
>>                  goto out;
>> +            }
>>              pstate = VCPU_RESET_PSTATE_SVC;
>>          } else {
>>              pstate = VCPU_RESET_PSTATE_EL1;
> 
> Thanks,
> 
>          M.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index d3b209023727..f1057603b756 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -245,7 +245,7 @@  static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
  */
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
-	int ret = -EINVAL;
+	int ret;
 	bool loaded;
 	u32 pstate;
 
@@ -269,15 +269,19 @@  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 
 	if (test_bit(KVM_ARM_VCPU_PTRAUTH_ADDRESS, vcpu->arch.features) ||
 	    test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, vcpu->arch.features)) {
-		if (kvm_vcpu_enable_ptrauth(vcpu))
+		if (kvm_vcpu_enable_ptrauth(vcpu)) {
+			ret = -EINVAL;
 			goto out;
+		}
 	}
 
 	switch (vcpu->arch.target) {
 	default:
 		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
-			if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1))
+			if (cpus_have_const_cap(ARM64_HAS_32BIT_EL1)) {
+				ret = -EINVAL;
 				goto out;
+			}
 			pstate = VCPU_RESET_PSTATE_SVC;
 		} else {
 			pstate = VCPU_RESET_PSTATE_EL1;