diff mbox series

[for-6.8,v4,1/3] LoongArch: KVM: Fix input validation of _kvm_get_cpucfg and kvm_check_cpucfg

Message ID 20240222105109.2042732-2-kernel@xen0n.name
State New
Headers
Series KVM: LoongArch: Fix wrong CPUCFG ID handling |

Commit Message

WANG Xuerui Feb. 22, 2024, 10:51 a.m. UTC 
  From: WANG Xuerui <git@xen0n.name>

The range check for the CPUCFG ID is wrong (should have been a ||
instead of &&) and useless in effect, so fix the obvious mistake.

Furthermore, the juggling of the temp return value is unnecessary,
because it is semantically equivalent and more readable to just
return at every switch case's end. This is done too to avoid potential
bugs in the future related to the unwanted complexity.

Also, the return value of _kvm_get_cpucfg is meant to be checked, but
this was not done, so bad CPUCFG IDs wrongly fall back to the default
case and 0 is incorrectly returned; check the return value to fix the
UAPI behavior.

While at it, also remove the redundant range check in kvm_check_cpucfg,
because out-of-range CPUCFG IDs are already rejected by the -EINVAL
as returned by _kvm_get_cpucfg.

Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support")
Fixes: 118e10cd893d ("LoongArch: KVM: Add LASX (256bit SIMD) support")
Signed-off-by: WANG Xuerui <git@xen0n.name>
---
 arch/loongarch/kvm/vcpu.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

Comments

maobibo Feb. 22, 2024, 11:57 a.m. UTC | #1 
On 2024/2/22 下午6:51, WANG Xuerui wrote:
> From: WANG Xuerui <git@xen0n.name>
> 
> The range check for the CPUCFG ID is wrong (should have been a ||
> instead of &&) and useless in effect, so fix the obvious mistake.
> 
> Furthermore, the juggling of the temp return value is unnecessary,
> because it is semantically equivalent and more readable to just
> return at every switch case's end. This is done too to avoid potential
> bugs in the future related to the unwanted complexity.
> 
> Also, the return value of _kvm_get_cpucfg is meant to be checked, but
> this was not done, so bad CPUCFG IDs wrongly fall back to the default
> case and 0 is incorrectly returned; check the return value to fix the
> UAPI behavior.
> 
> While at it, also remove the redundant range check in kvm_check_cpucfg,
> because out-of-range CPUCFG IDs are already rejected by the -EINVAL
> as returned by _kvm_get_cpucfg.
> 
> Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support")
> Fixes: 118e10cd893d ("LoongArch: KVM: Add LASX (256bit SIMD) support")
> Signed-off-by: WANG Xuerui <git@xen0n.name>
> ---
>   arch/loongarch/kvm/vcpu.c | 35 ++++++++++++++++++-----------------
>   1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 27701991886d..7fd32de6656b 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -300,9 +300,7 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
>   
>   static int _kvm_get_cpucfg(int id, u64 *v)
>   {
> -	int ret = 0;
> -
> -	if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
> +	if (id < 0 || id >= KVM_MAX_CPUCFG_REGS)
>   		return -EINVAL;
>   
>   	switch (id) {
> @@ -324,32 +322,35 @@ static int _kvm_get_cpucfg(int id, u64 *v)
>   		if (cpu_has_lasx)
>   			*v |= CPUCFG2_LASX;
>   
> -		break;
> +		return 0;
>   	default:
> -		ret = -EINVAL;
> -		break;
> +		/*
> +		 * No restrictions on other valid CPUCFG IDs' values, but
> +		 * CPUCFG data is limited to 32 bits as the LoongArch ISA
> +		 * manual says (Volume 1, Section 2.2.10.5 "CPUCFG").
> +		 */
> +		*v = U32_MAX;
> +		return 0;
>   	}
> -	return ret;
>   }
>   
>   static int kvm_check_cpucfg(int id, u64 val)
>   {
> -	u64 mask;
> -	int ret = 0;
> -
> -	if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
> -		return -EINVAL;
> +	u64 mask = 0;
> +	int ret;
>   
> -	if (_kvm_get_cpucfg(id, &mask))
> +	ret = _kvm_get_cpucfg(id, &mask);
> +	if (ret)
>   		return ret;
>   
> +	if (val & ~mask)
> +		/* Unsupported features and/or the higher 32 bits should not be set */
> +		return -EINVAL;
> +
>   	switch (id) {
>   	case 2:
>   		/* CPUCFG2 features checking */
> -		if (val & ~mask)
> -			/* The unsupported features should not be set */
> -			ret = -EINVAL;
> -		else if (!(val & CPUCFG2_LLFTP))
> +		if (!(val & CPUCFG2_LLFTP))
>   			/* The LLFTP must be set, as guest must has a constant timer */
>   			ret = -EINVAL;
>   		else if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
> 

Thanks for your contributions -:)

Reviewed-by: Bibo Mao <maobibo@loongson.cn>
diff mbox series

Patch

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 27701991886d..7fd32de6656b 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -300,9 +300,7 @@  static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
 
 static int _kvm_get_cpucfg(int id, u64 *v)
 {
-	int ret = 0;
-
-	if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
+	if (id < 0 || id >= KVM_MAX_CPUCFG_REGS)
 		return -EINVAL;
 
 	switch (id) {
@@ -324,32 +322,35 @@  static int _kvm_get_cpucfg(int id, u64 *v)
 		if (cpu_has_lasx)
 			*v |= CPUCFG2_LASX;
 
-		break;
+		return 0;
 	default:
-		ret = -EINVAL;
-		break;
+		/*
+		 * No restrictions on other valid CPUCFG IDs' values, but
+		 * CPUCFG data is limited to 32 bits as the LoongArch ISA
+		 * manual says (Volume 1, Section 2.2.10.5 "CPUCFG").
+		 */
+		*v = U32_MAX;
+		return 0;
 	}
-	return ret;
 }
 
 static int kvm_check_cpucfg(int id, u64 val)
 {
-	u64 mask;
-	int ret = 0;
-
-	if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
-		return -EINVAL;
+	u64 mask = 0;
+	int ret;
 
-	if (_kvm_get_cpucfg(id, &mask))
+	ret = _kvm_get_cpucfg(id, &mask);
+	if (ret)
 		return ret;
 
+	if (val & ~mask)
+		/* Unsupported features and/or the higher 32 bits should not be set */
+		return -EINVAL;
+
 	switch (id) {
 	case 2:
 		/* CPUCFG2 features checking */
-		if (val & ~mask)
-			/* The unsupported features should not be set */
-			ret = -EINVAL;
-		else if (!(val & CPUCFG2_LLFTP))
+		if (!(val & CPUCFG2_LLFTP))
 			/* The LLFTP must be set, as guest must has a constant timer */
 			ret = -EINVAL;
 		else if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))