Re: [PATCH 05/10] hw/audio/virtio-sound: return correct command response size

[Marc-André Lureau]

Hi

On Fri, Jan 5, 2024 at 12:34 AM Volker Rümelin <vr_qemu@t-online.de> wrote:
>
> The payload size returned by command VIRTIO_SND_R_PCM_INFO is
> wrong. The code in process_cmd() assumes that all commands
> return only a virtio_snd_hdr payload, but some commands like
> VIRTIO_SND_R_PCM_INFO may return an additional payload.
>
> Add a zero initialized payload_size variable to struct
> virtio_snd_ctrl_command to allow for additional payloads.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  hw/audio/virtio-snd.c         | 7 +++++--
>  include/hw/audio/virtio-snd.h | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index a2817a64b5..9f3269d72b 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -262,12 +262,13 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
>          memset(&pcm_info[i].padding, 0, 5);
>      }
>
> +    cmd->payload_size = sizeof(virtio_snd_pcm_info) * count;
>      cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
>      iov_from_buf(cmd->elem->in_sg,
>                   cmd->elem->in_num,
>                   sizeof(virtio_snd_hdr),
>                   pcm_info,
> -                 sizeof(virtio_snd_pcm_info) * count);
> +                 cmd->payload_size);

iov_from_buf() return value should probably be checked to match the
expected written size..

The earlier check for iov_size() is then probably redundant. Hmm, it
checks for req.size, which should probably match
sizeof(virtio_snd_pcm_info) too.

>  }
>
>  /*
> @@ -805,7 +806,8 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
>                   0,
>                   &cmd->resp,
>                   sizeof(virtio_snd_hdr));
> -    virtqueue_push(cmd->vq, cmd->elem, sizeof(virtio_snd_hdr));
> +    virtqueue_push(cmd->vq, cmd->elem,
> +                   sizeof(virtio_snd_hdr) + cmd->payload_size);
>      virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
>  }
>
> @@ -856,6 +858,7 @@ static void virtio_snd_handle_ctrl(VirtIODevice *vdev, 
> VirtQueue *vq)
>          cmd->elem = elem;
>          cmd->vq = vq;
>          cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
> +        /* implicit cmd->payload_size = 0; */
>          QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next);
>          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>      }
> diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
> index d1a68444d0..d6e08bd30d 100644
> --- a/include/hw/audio/virtio-snd.h
> +++ b/include/hw/audio/virtio-snd.h
> @@ -210,6 +210,7 @@ struct virtio_snd_ctrl_command {
>      VirtQueue *vq;
>      virtio_snd_hdr ctrl;
>      virtio_snd_hdr resp;
> +    size_t payload_size;
>      QTAILQ_ENTRY(virtio_snd_ctrl_command) next;
>  };
>  #endif
> --
> 2.35.3
>

otherwise, lgtm. Should it be backported to -stable ?
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>