Am 05.01.24 um 12:36 schrieb Marc-André Lureau:
> 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.
I wouldn't like to change that in this patch. There are more places in
this device and also in other virtio devices where this is done in
exactly the same way.
>
>> }
>>
>> /*
>> @@ -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 ?
Yes, I will cc qemu-stable in my v2 series. While the Linux virtio sound
driver ignores the returned "used length", this is required by the
virtio specification.
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
