| Message ID | 20240104203422.12308-5-vr_qemu@t-online.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-sound migration part 1 | expand |
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>
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_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. 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.lureau@redhat.com> >
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); } /* @@ -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
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(-)