| Message ID | 20211206164503.135917-1-amhamza.mgc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] KVM: x86: fix for missing initialization of return status variable | expand |
On Mon, Dec 06, 2021, Ameer Hamza wrote: > If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr > ioctl, we should trigger KVM_BUG_ON() and return with EIO to silent > coverity warning. > > Addresses-Coverity: 1494124 ("Uninitialized scalar variable") > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> > --- > Changes in v3: > Added KVM_BUG_ON() as default case and returned -EIO > --- > arch/x86/kvm/x86.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index e0aa4dd53c7f..b37068f847ff 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5019,6 +5019,9 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu, > case KVM_SET_DEVICE_ATTR: > r = kvm_arch_tsc_set_attr(vcpu, &attr); > break; > + default: > + KVM_BUG_ON(1, vcpu->kvm); > + r = -EIO; At least have a break; if we're going to be pedantic about things. > } > > return r; > -- > 2.25.1 >
On Mon, Dec 06, 2021 at 05:02:01PM +0000, Sean Christopherson wrote: > On Mon, Dec 06, 2021, Ameer Hamza wrote: > > If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr > > ioctl, we should trigger KVM_BUG_ON() and return with EIO to silent > > coverity warning. > > > > Addresses-Coverity: 1494124 ("Uninitialized scalar variable") > > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> > > --- > > Changes in v3: > > Added KVM_BUG_ON() as default case and returned -EIO > > --- > > arch/x86/kvm/x86.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index e0aa4dd53c7f..b37068f847ff 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -5019,6 +5019,9 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu, > > case KVM_SET_DEVICE_ATTR: > > r = kvm_arch_tsc_set_attr(vcpu, &attr); > > break; > > + default: > > + KVM_BUG_ON(1, vcpu->kvm); > > + r = -EIO; > > At least have a > > break; > > if we're going to be pedantic about things. I just started as a contributer in this community and trying to fix issues found by static analyzer tools. If you think that's not necessary, its totally fine :) > > } > > > > return r; > > -- > > 2.25.1 > >
On Mon, Dec 06, 2021, Ameer Hamza wrote: > On Mon, Dec 06, 2021 at 05:02:01PM +0000, Sean Christopherson wrote: > > On Mon, Dec 06, 2021, Ameer Hamza wrote: > > > If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr > > > ioctl, we should trigger KVM_BUG_ON() and return with EIO to silent > > > coverity warning. > > > > > > Addresses-Coverity: 1494124 ("Uninitialized scalar variable") > > > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> > > > --- > > > Changes in v3: > > > Added KVM_BUG_ON() as default case and returned -EIO > > > --- > > > arch/x86/kvm/x86.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index e0aa4dd53c7f..b37068f847ff 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -5019,6 +5019,9 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu, > > > case KVM_SET_DEVICE_ATTR: > > > r = kvm_arch_tsc_set_attr(vcpu, &attr); > > > break; > > > + default: > > > + KVM_BUG_ON(1, vcpu->kvm); > > > + r = -EIO; > > > > At least have a > > > > break; > > > > if we're going to be pedantic about things. > I just started as a contributer in this community and trying > to fix issues found by static analyzer tools. If you think that's > not necessary, its totally fine :) (Most) Static analyzers are great, they definitely find real bugs. But they also have a fair number of false positives, e.g. this is a firmly a false positive, so the results of any static analyzer needs to thought about critically, not blindly followed. It's completely understandable that Coverity got tripped up in this case, but that's exactly why having a human vet the bug report is necessary. There is arguably value in having a default statement to ensure future KVM code doesn't end up adding a bad call, which is why I'm not completely opposed to the above addition. Where folks, myself included, get a bit grumpy is when patches are sent to "fix" bug reports from static analyzers without evidence that the submitter has done their due dilegence to understand the code they are changing, e.g. even without any understanding of KVM, a search of kvm_vcpu_ioctl_device_attr() in the code base and reading of the function would have shown that the report was a false positive, albeit a somewhat odd one, and that returning -EINVAL was likely the wrong thing to do. If you're unsure if something is a real bug, please ask a question. Rapid firing patches at the list also makes reviewers grumpy as it again suggests a lack of due dilegence, especially when the patches have typos ("EINV" in v2) and/or have obvious shortcomings (missing "break" in v3). TL;DR: I have no objection whatsover to fixing (potential) bugs found by static analyzers, but please slow down and (a) make sure that it's actually a bug, (b) ask if you're unsure, and (c) do your best to ensure that what you're sending is an overall improvement.
On Mon, Dec 06, 2021 at 06:01:05PM +0000, Sean Christopherson wrote: > On Mon, Dec 06, 2021, Ameer Hamza wrote: > > On Mon, Dec 06, 2021 at 05:02:01PM +0000, Sean Christopherson wrote: > > > On Mon, Dec 06, 2021, Ameer Hamza wrote: > > > > If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr > > > > ioctl, we should trigger KVM_BUG_ON() and return with EIO to silent > > > > coverity warning. > > > > > > > > Addresses-Coverity: 1494124 ("Uninitialized scalar variable") > > > > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> > > > > --- > > > > Changes in v3: > > > > Added KVM_BUG_ON() as default case and returned -EIO > > > > --- > > > > arch/x86/kvm/x86.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > index e0aa4dd53c7f..b37068f847ff 100644 > > > > --- a/arch/x86/kvm/x86.c > > > > +++ b/arch/x86/kvm/x86.c > > > > @@ -5019,6 +5019,9 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu, > > > > case KVM_SET_DEVICE_ATTR: > > > > r = kvm_arch_tsc_set_attr(vcpu, &attr); > > > > break; > > > > + default: > > > > + KVM_BUG_ON(1, vcpu->kvm); > > > > + r = -EIO; > > > > > > At least have a > > > > > > break; > > > > > > if we're going to be pedantic about things. > > I just started as a contributer in this community and trying > > to fix issues found by static analyzer tools. If you think that's > > not necessary, its totally fine :) > > (Most) Static analyzers are great, they definitely find real bugs. But they also > have a fair number of false positives, e.g. this is a firmly a false positive, so > the results of any static analyzer needs to thought about critically, not blindly > followed. It's completely understandable that Coverity got tripped up in this > case, but that's exactly why having a human vet the bug report is necessary. > > There is arguably value in having a default statement to ensure future KVM code > doesn't end up adding a bad call, which is why I'm not completely opposed to the > above addition. > > Where folks, myself included, get a bit grumpy is when patches are sent to "fix" > bug reports from static analyzers without evidence that the submitter has done > their due dilegence to understand the code they are changing, e.g. even without > any understanding of KVM, a search of kvm_vcpu_ioctl_device_attr() in the code > base and reading of the function would have shown that the report was a false > positive, albeit a somewhat odd one, and that returning -EINVAL was likely the > wrong thing to do. If you're unsure if something is a real bug, please ask a > question. > > Rapid firing patches at the list also makes reviewers grumpy as it again suggests > a lack of due dilegence, especially when the patches have typos ("EINV" in v2) > and/or have obvious shortcomings (missing "break" in v3). > > TL;DR: I have no objection whatsover to fixing (potential) bugs found by static > analyzers, but please slow down and (a) make sure that it's actually a bug, (b) > ask if you're unsure, and (c) do your best to ensure that what you're sending is > an overall improvement. Totally agreed with you. Thank you so much for your insights on this. I will keep this into consideration moving forward. Best Regards, Hamza.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e0aa4dd53c7f..b37068f847ff 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5019,6 +5019,9 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu, case KVM_SET_DEVICE_ATTR: r = kvm_arch_tsc_set_attr(vcpu, &attr); break; + default: + KVM_BUG_ON(1, vcpu->kvm); + r = -EIO; } return r;
If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr ioctl, we should trigger KVM_BUG_ON() and return with EIO to silent coverity warning. Addresses-Coverity: 1494124 ("Uninitialized scalar variable") Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> --- Changes in v3: Added KVM_BUG_ON() as default case and returned -EIO --- arch/x86/kvm/x86.c | 3 +++ 1 file changed, 3 insertions(+)