Hi
On Fri, Jan 5, 2024 at 12:34 AM Volker Rümelin <vr_q...@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_q...@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.lur...@redhat.com>
