If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr
function, it should return with error status.
Addresses-Coverity: 1494124 ("Uninitialized scalar variable")
Signed-off-by: Ameer Hamza <[email protected]>
---
arch/x86/kvm/x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e0aa4dd53c7f..55b90c185717 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5001,7 +5001,7 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu,
void __user *argp)
{
struct kvm_device_attr attr;
- int r;
+ int r = -EINVAL;
if (copy_from_user(&attr, argp, sizeof(attr)))
return -EFAULT;
--
2.25.1
Ameer Hamza <[email protected]> writes:
> If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr
> function, it should return with error status.
>
> Addresses-Coverity: 1494124 ("Uninitialized scalar variable")
>
> Signed-off-by: Ameer Hamza <[email protected]>
> ---
> arch/x86/kvm/x86.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0aa4dd53c7f..55b90c185717 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5001,7 +5001,7 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu,
> void __user *argp)
> {
> struct kvm_device_attr attr;
> - int r;
> + int r = -EINVAL;
>
> if (copy_from_user(&attr, argp, sizeof(attr)))
> return -EFAULT;
The reported issue is not real, kvm_vcpu_ioctl_device_attr() is never
called with anything but [KVM_HAS_DEVICE_ATTR, KVM_GET_DEVICE_ATTR,
KVM_SET_DEVICE_ATTR] as 'ioctl' and the switch below covers all
three. Instead of initializing 'r' we could've added a 'default' case to
the switch, either returning something like EINVAL or just BUG(). Hope
it'll silence coverity.
--
Vitaly
On Mon, Dec 06, 2021 at 10:06:26AM +0100, Vitaly Kuznetsov wrote:
> Ameer Hamza <[email protected]> writes:
>
> > If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr
> > function, it should return with error status.
> >
> > Addresses-Coverity: 1494124 ("Uninitialized scalar variable")
> >
> > Signed-off-by: Ameer Hamza <[email protected]>
> > ---
> > arch/x86/kvm/x86.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e0aa4dd53c7f..55b90c185717 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5001,7 +5001,7 @@ static int kvm_vcpu_ioctl_device_attr(struct kvm_vcpu *vcpu,
> > void __user *argp)
> > {
> > struct kvm_device_attr attr;
> > - int r;
> > + int r = -EINVAL;
> >
> > if (copy_from_user(&attr, argp, sizeof(attr)))
> > return -EFAULT;
>
> The reported issue is not real, kvm_vcpu_ioctl_device_attr() is never
> called with anything but [KVM_HAS_DEVICE_ATTR, KVM_GET_DEVICE_ATTR,
> KVM_SET_DEVICE_ATTR] as 'ioctl' and the switch below covers all
> three. Instead of initializing 'r' we could've added a 'default' case to
> the switch, either returning something like EINVAL or just BUG(). Hope
> it'll silence coverity.
Thank you for your kind response. I agree with you and I think its
logical to add default case here. Let me update the patch.
Best Regards,
Hamza
If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr
function, it should return with error status.
Addresses-Coverity: 1494124 ("Uninitialized scalar variable")
Signed-off-by: Ameer Hamza <[email protected]>
---
Added default case to return EINV for undefined ioctl number
---
arch/x86/kvm/x86.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e0aa4dd53c7f..e6e00f997b1f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5019,6 +5019,8 @@ 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:
+ r = -EINVAL;
}
return r;
--
2.25.1
On Mon, Dec 06, 2021, Ameer Hamza wrote:
> If undefined ioctl number is passed to the kvm_vcpu_ioctl_device_attr
> function, it should return with error status.
No, if anything KVM should do KVM_BUG_ON() and return -EIO, because @ioctl is
completely KVM controlled. But I'd personally prefer we leave it as is, there's
one call site that very clearly invokes the helper with only the three ioctls.
It's not a strong preference though.
On Mon, Dec 06, 2021 at 03:37:43PM +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
> > function, it should return with error status.
>
> No, if anything KVM should do KVM_BUG_ON() and return -EIO, because @ioctl is
> completely KVM controlled. But I'd personally prefer we leave it as is, there's
> one call site that very clearly invokes the helper with only the three ioctls.
> It's not a strong preference though.
Thank you for your response. I agree with you, but I think in my
opinion, it would be nice to resolve coverity warning. Let me update the
patch according to your suggestions anyway.
Thanks,
Hamza.
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 <[email protected]>
---
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;
}
return r;
--
2.25.1
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > > ---
> > > > 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.