| Message ID | 20240213085131.503569-1-aiswarya.cyriac@opensynergy.com |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. | expand |
Hi Michael, Thank you for reviewing. I have updated my response inline On Tue, Feb 13, 2024 at 09:51:30AM +0100, Aiswarya Cyriac wrote: >> Fix the following warning when building virtio_snd driver. >> >> " >> *** CID 1583619: Uninitialized variables (UNINIT) >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() >> 288 >> 289 break; >> 290 } >> 291 >> 292 kfree(tlv); >> 293 >> vvv CID 1583619: Uninitialized variables (UNINIT) >> vvv Using uninitialized value "rc". >> 294 return rc; >> 295 } >> 296 >> 297 /** >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. >> 299 * @snd: VirtIO sound device. >> " >> >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") >I don't know enough about ALSA to say whether the patch is correct. But >the commit log needs work: please, do not "fix warnings" - analyse the >code and explain whether there is a real issue and if yes what is it >and how it can trigger. Is an invalid op_flag ever passed? >If it's just a coverity false positive it might be ok to >work around that but document this. This warning is caused by the absence of the "default" branch in the switch-block, and is a false positive because the kernel calls virtsnd_kctl_tlv_op() only with values for op_flag processed in this block. I will update the fix and send a v2 patch >> --- >> sound/virtio/virtio_kctl.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c >> index 0c6ac74aca1e..d7a160c5db03 100644 >> --- a/sound/virtio/virtio_kctl.c >> +++ b/sound/virtio/virtio_kctl.c >> @@ -286,6 +286,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, >> else >> rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); >> >> + break; >> + default: >> + virtsnd_ctl_msg_unref(msg); >> + rc = -EINVAL; >> + >There's already virtsnd_ctl_msg_unref call above. >Also don't we need virtsnd_ctl_msg_unref on other error paths >such as EFAULT? >Unify error handling to fix it all then? This also need to be handled and virtsnd_ctl_msg_unref needed in case of EFAULT as well. I will update the patch. Thanks, Aiswarya Cyriac Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin EMail: aiswarya.cyriac@opensynergy.com www.opensynergy.com Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B Geschäftsführer/Managing Director: Régis Adjamah
On Wed, Feb 14, 2024 at 09:08:26AM +0000, Aiswarya Cyriac wrote: > Hi Michael, > > Thank you for reviewing. I have updated my response inline > > On Tue, Feb 13, 2024 at 09:51:30AM +0100, Aiswarya Cyriac wrote: > >> Fix the following warning when building virtio_snd driver. > >> > >> " > >> *** CID 1583619: Uninitialized variables (UNINIT) > >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > >> 288 > >> 289 break; > >> 290 } > >> 291 > >> 292 kfree(tlv); > >> 293 > >> vvv CID 1583619: Uninitialized variables (UNINIT) > >> vvv Using uninitialized value "rc". > >> 294 return rc; > >> 295 } > >> 296 > >> 297 /** > >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > >> 299 * @snd: VirtIO sound device. > >> " > >> > >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > > >I don't know enough about ALSA to say whether the patch is correct. But > >the commit log needs work: please, do not "fix warnings" - analyse the > >code and explain whether there is a real issue and if yes what is it > >and how it can trigger. Is an invalid op_flag ever passed? > >If it's just a coverity false positive it might be ok to > >work around that but document this. > > This warning is caused by the absence of the "default" branch in the > switch-block, and is a false positive because the kernel calls > virtsnd_kctl_tlv_op() only with values for op_flag processed in > this block. Well we don't normally have functions validate inputs. In this case I am not really sure we should bother with adding dead code. If you really want to, add BUG_ON. > I will update the fix and send a v2 patch > > >> --- > >> sound/virtio/virtio_kctl.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > >> index 0c6ac74aca1e..d7a160c5db03 100644 > >> --- a/sound/virtio/virtio_kctl.c > >> +++ b/sound/virtio/virtio_kctl.c > >> @@ -286,6 +286,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > >> else > >> rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); > >> > >> + break; > >> + default: > >> + virtsnd_ctl_msg_unref(msg); > >> + rc = -EINVAL; > >> + > > >There's already virtsnd_ctl_msg_unref call above. > >Also don't we need virtsnd_ctl_msg_unref on other error paths > >such as EFAULT? > >Unify error handling to fix it all then? > > This also need to be handled and virtsnd_ctl_msg_unref needed in case of EFAULT as well. > I will update the patch. > > > Thanks, > Aiswarya Cyriac > Software Engineer > > OpenSynergy GmbH > Rotherstr. 20, 10245 Berlin > > EMail: aiswarya.cyriac@opensynergy.com > > www.opensynergy.com > Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B > Geschäftsführer/Managing Director: Régis Adjamah > > ________________________________________ > From: Michael S. Tsirkin <mst@redhat.com> > Sent: Tuesday, February 13, 2024 10:06 AM > To: Aiswarya Cyriac > Cc: jasowang@redhat.com; perex@perex.cz; tiwai@suse.com; linux-kernel@vger.kernel.org; alsa-devel@alsa-project.org; virtualization@lists.linux-foundation.org; virtio-dev@lists.oasis-open.org; Anton Yakovlev; coverity-bot > Subject: Re: [PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. > > On Tue, Feb 13, 2024 at 09:51:30AM +0100, Aiswarya Cyriac wrote: > > Fix the following warning when building virtio_snd driver. > > > > " > > *** CID 1583619: Uninitialized variables (UNINIT) > > sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > > 288 > > 289 break; > > 290 } > > 291 > > 292 kfree(tlv); > > 293 > > vvv CID 1583619: Uninitialized variables (UNINIT) > > vvv Using uninitialized value "rc". > > 294 return rc; > > 295 } > > 296 > > 297 /** > > 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > > 299 * @snd: VirtIO sound device. > > " > > > > Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > > Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > > Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > > Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > > I don't know enough about ALSA to say whether the patch is correct. But > the commit log needs work: please, do not "fix warnings" - analyse the > code and explain whether there is a real issue and if yes what is it > and how it can trigger. Is an invalid op_flag ever passed? > If it's just a coverity false positive it might be ok to > work around that but document this. > > > > --- > > sound/virtio/virtio_kctl.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > > index 0c6ac74aca1e..d7a160c5db03 100644 > > --- a/sound/virtio/virtio_kctl.c > > +++ b/sound/virtio/virtio_kctl.c > > @@ -286,6 +286,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, > > else > > rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); > > > > + break; > > + default: > > + virtsnd_ctl_msg_unref(msg); > > + rc = -EINVAL; > > + > > There's already virtsnd_ctl_msg_unref call above. > Also don't we need virtsnd_ctl_msg_unref on other error paths > such as EFAULT? > Unify error handling to fix it all then? > > > break; > > } > > > > -- > > 2.43.0 > >
On Wed, 14 Feb 2024 12:30:19 +0100, Michael S. Tsirkin wrote: > > On Wed, Feb 14, 2024 at 09:08:26AM +0000, Aiswarya Cyriac wrote: > > Hi Michael, > > > > Thank you for reviewing. I have updated my response inline > > > > On Tue, Feb 13, 2024 at 09:51:30AM +0100, Aiswarya Cyriac wrote: > > >> Fix the following warning when building virtio_snd driver. > > >> > > >> " > > >> *** CID 1583619: Uninitialized variables (UNINIT) > > >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > > >> 288 > > >> 289 break; > > >> 290 } > > >> 291 > > >> 292 kfree(tlv); > > >> 293 > > >> vvv CID 1583619: Uninitialized variables (UNINIT) > > >> vvv Using uninitialized value "rc". > > >> 294 return rc; > > >> 295 } > > >> 296 > > >> 297 /** > > >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > > >> 299 * @snd: VirtIO sound device. > > >> " > > >> > > >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > > >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > > >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > > >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > > >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > > > > >I don't know enough about ALSA to say whether the patch is correct. But > > >the commit log needs work: please, do not "fix warnings" - analyse the > > >code and explain whether there is a real issue and if yes what is it > > >and how it can trigger. Is an invalid op_flag ever passed? > > >If it's just a coverity false positive it might be ok to > > >work around that but document this. > > > > This warning is caused by the absence of the "default" branch in the > > switch-block, and is a false positive because the kernel calls > > virtsnd_kctl_tlv_op() only with values for op_flag processed in > > this block. > > Well we don't normally have functions validate inputs. > In this case I am not really sure we should bother > with adding dead code. If you really want to, add BUG_ON. Please don't use BUG_ON() in such a case... There is no reason to break the whole operation. thanks, Takashi
On Wed, 14 Feb 2024 12:30:19 +0100, Michael S. Tsirkin wrote: >> >> On Wed, Feb 14, 2024 at 09:08:26AM +0000, Aiswarya Cyriac wrote: >> > Hi Michael, >> > >> > Thank you for reviewing. I have updated my response inline >> > >> > On Tue, Feb 13, 2024 at 09:51:30AM +0100, Aiswarya Cyriac wrote: >> > >> Fix the following warning when building virtio_snd driver. >> > >> >> > >> " >> > >> *** CID 1583619: Uninitialized variables (UNINIT) >> > >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() >> > >> 288 >> > >> 289 break; >> > >> 290 } >> > >> 291 >> > >> 292 kfree(tlv); >> > >> 293 >> > >> vvv CID 1583619: Uninitialized variables (UNINIT) >> > >> vvv Using uninitialized value "rc". >> > >> 294 return rc; >> > >> 295 } >> > >> 296 >> > >> 297 /** >> > >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. >> > >> 299 * @snd: VirtIO sound device. >> > >> " >> > >> >> > >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> >> > >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> >> > >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> >> > >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") >> > >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") >> > >> > >I don't know enough about ALSA to say whether the patch is correct. But >> > >the commit log needs work: please, do not "fix warnings" - analyse the >> > >code and explain whether there is a real issue and if yes what is it >> > >and how it can trigger. Is an invalid op_flag ever passed? >> > >If it's just a coverity false positive it might be ok to >> > >work around that but document this. >> > >> > This warning is caused by the absence of the "default" branch in the >> > switch-block, and is a false positive because the kernel calls >> > virtsnd_kctl_tlv_op() only with values for op_flag processed in >> > this block. >> >> Well we don't normally have functions validate inputs. >> In this case I am not really sure we should bother >> with adding dead code. If you really want to, add BUG_ON. > Please don't use BUG_ON() in such a case... > There is no reason to break the whole operation. How about adding a WARN_ON() on default case?
On Wed, 14 Feb 2024 14:07:40 +0100, Aiswarya Cyriac wrote: > > > On Wed, 14 Feb 2024 12:30:19 +0100, > Michael S. Tsirkin wrote: > >> > >> On Wed, Feb 14, 2024 at 09:08:26AM +0000, Aiswarya Cyriac wrote: > >> > Hi Michael, > >> > > >> > Thank you for reviewing. I have updated my response inline > >> > > >> > On Tue, Feb 13, 2024 at 09:51:30AM +0100, Aiswarya Cyriac wrote: > >> > >> Fix the following warning when building virtio_snd driver. > >> > >> > >> > >> " > >> > >> *** CID 1583619: Uninitialized variables (UNINIT) > >> > >> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op() > >> > >> 288 > >> > >> 289 break; > >> > >> 290 } > >> > >> 291 > >> > >> 292 kfree(tlv); > >> > >> 293 > >> > >> vvv CID 1583619: Uninitialized variables (UNINIT) > >> > >> vvv Using uninitialized value "rc". > >> > >> 294 return rc; > >> > >> 295 } > >> > >> 296 > >> > >> 297 /** > >> > >> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type. > >> > >> 299 * @snd: VirtIO sound device. > >> > >> " > >> > >> > >> > >> Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> > >> > >> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com> > >> > >> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org> > >> > >> Addresses-Coverity-ID: 1583619 ("Uninitialized variables") > >> > >> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls") > >> > > >> > >I don't know enough about ALSA to say whether the patch is correct. But > >> > >the commit log needs work: please, do not "fix warnings" - analyse the > >> > >code and explain whether there is a real issue and if yes what is it > >> > >and how it can trigger. Is an invalid op_flag ever passed? > >> > >If it's just a coverity false positive it might be ok to > >> > >work around that but document this. > >> > > >> > This warning is caused by the absence of the "default" branch in the > >> > switch-block, and is a false positive because the kernel calls > >> > virtsnd_kctl_tlv_op() only with values for op_flag processed in > >> > this block. > >> > >> Well we don't normally have functions validate inputs. > >> In this case I am not really sure we should bother > >> with adding dead code. If you really want to, add BUG_ON. > > > Please don't use BUG_ON() in such a case... > > There is no reason to break the whole operation. > > How about adding a WARN_ON() on default case? That's better :) thanks, Takashi
diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c index 0c6ac74aca1e..d7a160c5db03 100644 --- a/sound/virtio/virtio_kctl.c +++ b/sound/virtio/virtio_kctl.c @@ -286,6 +286,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, else rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false); + break; + default: + virtsnd_ctl_msg_unref(msg); + rc = -EINVAL; + break; }