...

On Wed, Feb 14, 2024 at 03:01:10PM +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 | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c index 0c6ac74aca1e..40606eb381af 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_cleanup;
   }
  
   sg_init_one(&sg, tlv, size);
  

@@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, case SNDRV_CTL_TLV_OP_READ: hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);

•         /* Since virtsnd_ctl_msg_send() drops the reference, we increase
  

•          * the counter to be consistent with the on_cleanup path.
  

•          */
  

This is not how multi-line comments should look.

Adding overhead here is just a waste of cycles. Instead, separate error handling and normal exit paths. Then you will not need to increase the refcount here.

Ok.

...

•         virtsnd_ctl_msg_ref(msg);
  

•          rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
           if (!rc) {
                   if (copy_to_user(utlv, tlv, size))
  

@@ -281,14 +286,26 @@ 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
  

•         } else {
  

•                 /* Same as the comment above */
  

Same thing. Besides, this kind of cross referencing breaks immediately when someone adds a comment in the middle.

I agree. I will update the patch

...

•                 virtsnd_ctl_msg_ref(msg);
  

•                  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);
  
           break;
   }
  
  

+on_cleanup:

• virtsnd_ctl_msg_unref(msg);
  

•  kfree(tlv);
  
   return rc;
  

on_cleanup is not informative, coding style says: "Choose label names which say what the goto does or why the goto exists."

And saving on duplication here by paying elsewhere does not make sense. So you do this instead:

    kfree(tlv);
    return rc;

on_error: virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc;

This is very ideomatic.

Thanks. I will separate error and non error cases and update 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: Wednesday, February 14, 2024 4:36 PM 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: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.

On Wed, Feb 14, 2024 at 03:01:10PM +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 | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c index 0c6ac74aca1e..40606eb381af 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_cleanup;
  }
  
  sg_init_one(&sg, tlv, size);
  

@@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag, case SNDRV_CTL_TLV_OP_READ: hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);

•         /* Since virtsnd_ctl_msg_send() drops the reference, we increase
  

•          * the counter to be consistent with the on_cleanup path.
  

•          */
  

This is not how multi-line comments should look.

Adding overhead here is just a waste of cycles. Instead, separate error handling and normal exit paths. Then you will not need to increase the refcount here.

•         virtsnd_ctl_msg_ref(msg);
  

•         rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
          if (!rc) {
                  if (copy_to_user(utlv, tlv, size))
  

@@ -281,14 +286,26 @@ 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
  

•         } else {
  

•                 /* Same as the comment above */
  

Same thing. Besides, this kind of cross referencing breaks immediately when someone adds a comment in the middle.

•                 virtsnd_ctl_msg_ref(msg);
  

•                 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);
  
          break;
  }
  
  

+on_cleanup:

• virtsnd_ctl_msg_unref(msg);
  

• kfree(tlv);
  
  return rc;
  

on_cleanup is not informative, coding style says: "Choose label names which say what the goto does or why the goto exists."

And saving on duplication here by paying elsewhere does not make sense. So you do this instead:

kfree(tlv); return rc;

on_error: virtsnd_ctl_msg_unref(msg); kfree(tlv); return rc;

This is very ideomatic.

-- 2.43.0