diff mbox series

[v4] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.

Message ID 20240216100643.688590-1-aiswarya.cyriac@opensynergy.com
State New
Headers
Series [v4] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. |

Commit Message

Aiswarya Cyriac Feb. 16, 2024, 10:06 a.m. UTC 
  This commit fixes 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.
"

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.

Also, this commit unifies the cleanup path for all possible control
paths in the callback function.

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")
---
 sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin Feb. 16, 2024, 11:27 a.m. UTC | #1 
On Fri, Feb 16, 2024 at 11:06:43AM +0100, Aiswarya Cyriac wrote:
> This commit fixes 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.
> "
> 
> 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.
> 
> Also, this commit unifies the cleanup path for all possible control
> paths in the callback function.
> 
> 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")



> ---
>  sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> index 0c6ac74aca1e..7aa79c05b464 100644
> --- a/sound/virtio/virtio_kctl.c
> +++ b/sound/virtio/virtio_kctl.c
> @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
>  
>  	tlv = kzalloc(size, GFP_KERNEL);
>  	if (!tlv) {
> -		virtsnd_ctl_msg_unref(msg);
> -		return -ENOMEM;
> +		rc = -ENOMEM;
> +		goto on_msg_unref;
>  	}
>  
>  	sg_init_one(&sg, tlv, size);
> @@ -281,14 +281,25 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
>  			hdr->hdr.code =
>  				cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
>  
> -		if (copy_from_user(tlv, utlv, size))
> +		if (copy_from_user(tlv, utlv, size)) {
>  			rc = -EFAULT;
> -		else
> +			goto on_msg_unref;
> +		} else {
>  			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
> +		}
>  
>  		break;
> +	default:
> +		rc = -EINVAL;
> +		/* We never get here - we listed all values for op_flag */
> +		WARN_ON(1);
> +		goto on_msg_unref;
>  	}
> +	kfree(tlv);
> +	return rc;
>  
> +on_msg_unref:
> +	virtsnd_ctl_msg_unref(msg);
>  	kfree(tlv);
>  
>  	return rc;

I don't really like adding code for a false-positive but ALSA
maintainers seem to like this. If yes, this seems like as good
a way as any to do it.

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> -- 
> 2.43.2
Takashi Iwai Feb. 16, 2024, 1:42 p.m. UTC | #2 
On Fri, 16 Feb 2024 12:27:48 +0100,
Michael S. Tsirkin wrote:
> 
> On Fri, Feb 16, 2024 at 11:06:43AM +0100, Aiswarya Cyriac wrote:
> > This commit fixes 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.
> > "
> > 
> > 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.
> > 
> > Also, this commit unifies the cleanup path for all possible control
> > paths in the callback function.
> > 
> > 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")
> 
> 
> 
> > ---
> >  sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> > index 0c6ac74aca1e..7aa79c05b464 100644
> > --- a/sound/virtio/virtio_kctl.c
> > +++ b/sound/virtio/virtio_kctl.c
> > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> >  
> >  	tlv = kzalloc(size, GFP_KERNEL);
> >  	if (!tlv) {
> > -		virtsnd_ctl_msg_unref(msg);
> > -		return -ENOMEM;
> > +		rc = -ENOMEM;
> > +		goto on_msg_unref;
> >  	}
> >  
> >  	sg_init_one(&sg, tlv, size);
> > @@ -281,14 +281,25 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> >  			hdr->hdr.code =
> >  				cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
> >  
> > -		if (copy_from_user(tlv, utlv, size))
> > +		if (copy_from_user(tlv, utlv, size)) {
> >  			rc = -EFAULT;
> > -		else
> > +			goto on_msg_unref;
> > +		} else {
> >  			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
> > +		}
> >  
> >  		break;
> > +	default:
> > +		rc = -EINVAL;
> > +		/* We never get here - we listed all values for op_flag */
> > +		WARN_ON(1);
> > +		goto on_msg_unref;
> >  	}
> > +	kfree(tlv);
> > +	return rc;
> >  
> > +on_msg_unref:
> > +	virtsnd_ctl_msg_unref(msg);
> >  	kfree(tlv);
> >  
> >  	return rc;
> 
> I don't really like adding code for a false-positive but ALSA
> maintainers seem to like this. If yes, this seems like as good
> a way as any to do it.

Err, no, you misunderstood the situation.

I took the v1 patch quickly because:
- It was with Anton's SOB, who is another maintainer of the driver
- I assumed you lost interest in this driver since you haven't reacted
  to the previous patches for long time
- The change there was small and simple enough

Now, it grows unnecessarily large, and yet you complained.  Why should
I take it, then?

This is a subtle cosmetic issue that isn't worth for wasting too much
time and energy.  If we want to shut up the compile warning, and this
is a case where it can't happen, just put the "default:" to the
existing case.  If you want to be user-friendly, put some comment.
That's all.  It'll be a one-liner.

OTOH, if we do care and want to catch any potential logical mistake,
you can put WARN().  But, this doesn't have to go out as an error.
Simply putting WARN() for the default and going through would work,
too.

Or we can keep this lengthy changes if we want, too.

So, I really don't mind which way to fix as long as it works correctly
(and doesn't look too ugly).  Please make agreement among you guys,
and resubmit if needed.


thanks,

Takashi
Michael S. Tsirkin Feb. 16, 2024, 1:48 p.m. UTC | #3 
On Fri, Feb 16, 2024 at 02:42:37PM +0100, Takashi Iwai wrote:
> On Fri, 16 Feb 2024 12:27:48 +0100,
> Michael S. Tsirkin wrote:
> > 
> > On Fri, Feb 16, 2024 at 11:06:43AM +0100, Aiswarya Cyriac wrote:
> > > This commit fixes 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.
> > > "
> > > 
> > > 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.
> > > 
> > > Also, this commit unifies the cleanup path for all possible control
> > > paths in the callback function.
> > > 
> > > 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")
> > 
> > 
> > 
> > > ---
> > >  sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> > > index 0c6ac74aca1e..7aa79c05b464 100644
> > > --- a/sound/virtio/virtio_kctl.c
> > > +++ b/sound/virtio/virtio_kctl.c
> > > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> > >  
> > >  	tlv = kzalloc(size, GFP_KERNEL);
> > >  	if (!tlv) {
> > > -		virtsnd_ctl_msg_unref(msg);
> > > -		return -ENOMEM;
> > > +		rc = -ENOMEM;
> > > +		goto on_msg_unref;
> > >  	}
> > >  
> > >  	sg_init_one(&sg, tlv, size);
> > > @@ -281,14 +281,25 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> > >  			hdr->hdr.code =
> > >  				cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
> > >  
> > > -		if (copy_from_user(tlv, utlv, size))
> > > +		if (copy_from_user(tlv, utlv, size)) {
> > >  			rc = -EFAULT;
> > > -		else
> > > +			goto on_msg_unref;
> > > +		} else {
> > >  			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
> > > +		}
> > >  
> > >  		break;
> > > +	default:
> > > +		rc = -EINVAL;
> > > +		/* We never get here - we listed all values for op_flag */
> > > +		WARN_ON(1);
> > > +		goto on_msg_unref;
> > >  	}
> > > +	kfree(tlv);
> > > +	return rc;
> > >  
> > > +on_msg_unref:
> > > +	virtsnd_ctl_msg_unref(msg);
> > >  	kfree(tlv);
> > >  
> > >  	return rc;
> > 
> > I don't really like adding code for a false-positive but ALSA
> > maintainers seem to like this. If yes, this seems like as good
> > a way as any to do it.
> 
> Err, no, you misunderstood the situation.
> 
> I took the v1 patch quickly because:
> - It was with Anton's SOB, who is another maintainer of the driver
> - I assumed you lost interest in this driver since you haven't reacted
>   to the previous patches for long time
> - The change there was small and simple enough
> 
> Now, it grows unnecessarily large, and yet you complained.  Why should
> I take it, then?
> 
> This is a subtle cosmetic issue that isn't worth for wasting too much
> time and energy.  If we want to shut up the compile warning, and this
> is a case where it can't happen, just put the "default:" to the
> existing case.  If you want to be user-friendly, put some comment.
> That's all.  It'll be a one-liner.
> 
> OTOH, if we do care and want to catch any potential logical mistake,
> you can put WARN().  But, this doesn't have to go out as an error.
> Simply putting WARN() for the default and going through would work,
> too.
> 
> Or we can keep this lengthy changes if we want, too.
> 
> So, I really don't mind which way to fix as long as it works correctly
> (and doesn't look too ugly).  Please make agreement among you guys,
> and resubmit if needed.
> 
> 
> thanks,
> 
> Takashi

OK sorry about too verbose.  I mean since Anton wants it, I ack this.

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Takashi Iwai Feb. 16, 2024, 2:02 p.m. UTC | #4 
On Fri, 16 Feb 2024 14:48:10 +0100,
Michael S. Tsirkin wrote:
> 
> On Fri, Feb 16, 2024 at 02:42:37PM +0100, Takashi Iwai wrote:
> > On Fri, 16 Feb 2024 12:27:48 +0100,
> > Michael S. Tsirkin wrote:
> > > 
> > > On Fri, Feb 16, 2024 at 11:06:43AM +0100, Aiswarya Cyriac wrote:
> > > > This commit fixes 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.
> > > > "
> > > > 
> > > > 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.
> > > > 
> > > > Also, this commit unifies the cleanup path for all possible control
> > > > paths in the callback function.
> > > > 
> > > > 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")
> > > 
> > > 
> > > 
> > > > ---
> > > >  sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
> > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> > > > index 0c6ac74aca1e..7aa79c05b464 100644
> > > > --- a/sound/virtio/virtio_kctl.c
> > > > +++ b/sound/virtio/virtio_kctl.c
> > > > @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> > > >  
> > > >  	tlv = kzalloc(size, GFP_KERNEL);
> > > >  	if (!tlv) {
> > > > -		virtsnd_ctl_msg_unref(msg);
> > > > -		return -ENOMEM;
> > > > +		rc = -ENOMEM;
> > > > +		goto on_msg_unref;
> > > >  	}
> > > >  
> > > >  	sg_init_one(&sg, tlv, size);
> > > > @@ -281,14 +281,25 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> > > >  			hdr->hdr.code =
> > > >  				cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
> > > >  
> > > > -		if (copy_from_user(tlv, utlv, size))
> > > > +		if (copy_from_user(tlv, utlv, size)) {
> > > >  			rc = -EFAULT;
> > > > -		else
> > > > +			goto on_msg_unref;
> > > > +		} else {
> > > >  			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
> > > > +		}
> > > >  
> > > >  		break;
> > > > +	default:
> > > > +		rc = -EINVAL;
> > > > +		/* We never get here - we listed all values for op_flag */
> > > > +		WARN_ON(1);
> > > > +		goto on_msg_unref;
> > > >  	}
> > > > +	kfree(tlv);
> > > > +	return rc;
> > > >  
> > > > +on_msg_unref:
> > > > +	virtsnd_ctl_msg_unref(msg);
> > > >  	kfree(tlv);
> > > >  
> > > >  	return rc;
> > > 
> > > I don't really like adding code for a false-positive but ALSA
> > > maintainers seem to like this. If yes, this seems like as good
> > > a way as any to do it.
> > 
> > Err, no, you misunderstood the situation.
> > 
> > I took the v1 patch quickly because:
> > - It was with Anton's SOB, who is another maintainer of the driver
> > - I assumed you lost interest in this driver since you haven't reacted
> >   to the previous patches for long time
> > - The change there was small and simple enough
> > 
> > Now, it grows unnecessarily large, and yet you complained.  Why should
> > I take it, then?
> > 
> > This is a subtle cosmetic issue that isn't worth for wasting too much
> > time and energy.  If we want to shut up the compile warning, and this
> > is a case where it can't happen, just put the "default:" to the
> > existing case.  If you want to be user-friendly, put some comment.
> > That's all.  It'll be a one-liner.
> > 
> > OTOH, if we do care and want to catch any potential logical mistake,
> > you can put WARN().  But, this doesn't have to go out as an error.
> > Simply putting WARN() for the default and going through would work,
> > too.
> > 
> > Or we can keep this lengthy changes if we want, too.
> > 
> > So, I really don't mind which way to fix as long as it works correctly
> > (and doesn't look too ugly).  Please make agreement among you guys,
> > and resubmit if needed.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> OK sorry about too verbose.  I mean since Anton wants it, I ack this.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Alright, I applied this one.  Thanks!


Takashi
diff mbox series

Patch

diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
index 0c6ac74aca1e..7aa79c05b464 100644
--- a/sound/virtio/virtio_kctl.c
+++ b/sound/virtio/virtio_kctl.c
@@ -253,8 +253,8 @@  static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
 
 	tlv = kzalloc(size, GFP_KERNEL);
 	if (!tlv) {
-		virtsnd_ctl_msg_unref(msg);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto on_msg_unref;
 	}
 
 	sg_init_one(&sg, tlv, size);
@@ -281,14 +281,25 @@  static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
 			hdr->hdr.code =
 				cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
 
-		if (copy_from_user(tlv, utlv, size))
+		if (copy_from_user(tlv, utlv, size)) {
 			rc = -EFAULT;
-		else
+			goto on_msg_unref;
+		} else {
 			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
+		}
 
 		break;
+	default:
+		rc = -EINVAL;
+		/* We never get here - we listed all values for op_flag */
+		WARN_ON(1);
+		goto on_msg_unref;
 	}
+	kfree(tlv);
+	return rc;
 
+on_msg_unref:
+	virtsnd_ctl_msg_unref(msg);
 	kfree(tlv);
 
 	return rc;