linux-kernel - Re: [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200228154453.GC2329@linux.intel.com>
Date:   Fri, 28 Feb 2020 07:44:53 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Xiaoyao Li <xiaoyao.li@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: Remove superfluous brackets in
 kvm_arch_dev_ioctl()

On Fri, Feb 28, 2020 at 01:25:27PM +0800, Xiaoyao Li wrote:
> Remove unnecessary brackets from the case statements in

They aren't unnecessary, e.g. simply taking away brackets without
refactoring the code will break the build.

> kvm_arch_dev_ioctl().
> 
> The brackets are visually confusing and error-prone, e.g., brackets of

They're confusing when they're broken, but IMO they're a non-issue when
used correctly, which is the vast majority of the time.  I wouldn't say I
love brackets, but for me it's preferrable to having a big pile of
variables at the top of the function.  I also find having the struct type
in the case helpful, e.g. it's easy to figure out which struct corresponds
to which ioctl in the API.

And despite this being the second instance of this style of bug in KVM in
the last few months, I don't think it's fair to call brackets error-prone.
These are literally the only two times I've ever seen this class of bug.

Regardless, using brackets to create a new scope in a case statement is a
widely used pattern:

  $ git grep case | grep ": {" | wc -l
  1954

Eliminating all current and future uses isn't realistic.

A better way to help prevent these type of bugs from being introduced
would be to teach checkpatch to issue a warning if a set of brackets
encapsulates a case statement.

Fixing this particular bug is then a small patch, and maybe throw in an
opportunistic cleanup to add a "break" in the default path.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2103101eca78..f059697bf61e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3485,6 +3485,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
                        goto out;
                r = 0;
                break;
+       }
        case KVM_GET_MSR_FEATURE_INDEX_LIST: {
                struct kvm_msr_list __user *user_msr_list = argp;
                struct kvm_msr_list msr_list;
@@ -3510,9 +3511,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
        case KVM_GET_MSRS:
                r = msr_io(NULL, argp, do_get_msr_feature, 1);
                break;
-       }
        default:
                r = -EINVAL;
+               break;
        }
 out:
        return r;

> case KVM_X86_GET_MCE_CAP_SUPPORTED accidently includes case
> KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS.
> Signed-off-by: Xiaoyao Li <xiaoyao.li@...el.com>
> ---
>  arch/x86/kvm/x86.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..9efd693189df 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3412,14 +3412,16 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  long kvm_arch_dev_ioctl(struct file *filp,
>  			unsigned int ioctl, unsigned long arg)
>  {
> -	void __user *argp = (void __user *)arg;
> +	struct kvm_msr_list __user *user_msr_list;
> +	struct kvm_cpuid2 __user *cpuid_arg;
> +	struct kvm_msr_list msr_list;
> +	struct kvm_cpuid2 cpuid;
> +	unsigned int n;
>  	long r;
>  
>  	switch (ioctl) {
> -	case KVM_GET_MSR_INDEX_LIST: {
> -		struct kvm_msr_list __user *user_msr_list = argp;
> -		struct kvm_msr_list msr_list;
> -		unsigned n;
> +	case KVM_GET_MSR_INDEX_LIST:
> +		user_msr_list = (void __user *)arg;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
> @@ -3441,11 +3443,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  			goto out;
>  		r = 0;
>  		break;
> -	}
>  	case KVM_GET_SUPPORTED_CPUID:
> -	case KVM_GET_EMULATED_CPUID: {
> -		struct kvm_cpuid2 __user *cpuid_arg = argp;
> -		struct kvm_cpuid2 cpuid;
> +	case KVM_GET_EMULATED_CPUID:
> +		cpuid_arg = (void __user *)arg;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
> @@ -3461,18 +3461,15 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  			goto out;
>  		r = 0;
>  		break;
> -	}
> -	case KVM_X86_GET_MCE_CAP_SUPPORTED: {
> +	case KVM_X86_GET_MCE_CAP_SUPPORTED:
>  		r = -EFAULT;
> -		if (copy_to_user(argp, &kvm_mce_cap_supported,
> +		if (copy_to_user((void __user *)arg, &kvm_mce_cap_supported,
>  				 sizeof(kvm_mce_cap_supported)))
>  			goto out;
>  		r = 0;
>  		break;
> -	case KVM_GET_MSR_FEATURE_INDEX_LIST: {
> -		struct kvm_msr_list __user *user_msr_list = argp;
> -		struct kvm_msr_list msr_list;
> -		unsigned int n;
> +	case KVM_GET_MSR_FEATURE_INDEX_LIST:
> +		user_msr_list = (void __user *)arg;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
> @@ -3490,11 +3487,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  			goto out;
>  		r = 0;
>  		break;
> -	}
>  	case KVM_GET_MSRS:
> -		r = msr_io(NULL, argp, do_get_msr_feature, 1);
> +		r = msr_io(NULL, (void __user *)arg, do_get_msr_feature, 1);
>  		break;
> -	}
>  	default:
>  		r = -EINVAL;
>  	}
> -- 
> 2.19.1
>