| 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
| ||
|
Date: Sat, 29 Feb 2020 10:42:26 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Sean Christopherson <sean.j.christopherson@...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 2/28/2020 11:44 PM, Sean Christopherson wrote:
> 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.
Fair enough.
> 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.
OK. I'll go this way.
> 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
>>
Powered by blists - more mailing lists