Re: [PULL 1/1] virtio-scsi: pass the same cdb_size to virtio_scsi_pop_req and virtio_scsi_handle_cmd_req_prepare

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 09, 2026 at 12:33:09PM +0200, Paolo Bonzini wrote:
> Ensure that there is no allocation/usage mismatch when requests
> are processed in virtio_scsi_handle_cmd_vq.  To do this,
> retrieve the value once and pass it to both functions.
> 
> For other calls to virtio_scsi_pop_req the extra size
> can be 0, because control and event requests fit
> entirely in VirtIOSCSIReq.
> 
> Reported-by: Jihe Wang <wangjihe.mail@gmail.com>
> Tested-by: Jihe Wang <wangjihe.mail@gmail.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)

This issue is tagged CVE-2026-5763   - if possible can we get that
in the commit message before merging.

> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 774968d8c70..6c737680119 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -227,16 +227,16 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
>      return 0;
>  }
>  
> -static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq, QemuMutex *vq_lock)
> +static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq, size_t extra_req_size,
> +                                          QemuMutex *vq_lock)
>  {
> -    VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
>      VirtIOSCSIReq *req;
>  
>      if (vq_lock) {
>          qemu_mutex_lock(vq_lock);
>      }
>  
> -    req = virtqueue_pop(vq, sizeof(VirtIOSCSIReq) + vs->cdb_size);
> +    req = virtqueue_pop(vq, sizeof(VirtIOSCSIReq) + extra_req_size);
>  
>      if (vq_lock) {
>          qemu_mutex_unlock(vq_lock);
> @@ -682,7 +682,7 @@ static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
>  {
>      VirtIOSCSIReq *req;
>  
> -    while ((req = virtio_scsi_pop_req(s, vq, &s->ctrl_lock))) {
> +    while ((req = virtio_scsi_pop_req(s, vq, 0, &s->ctrl_lock))) {
>          virtio_scsi_handle_ctrl_req(s, req);
>      }
>  }
> @@ -850,13 +850,14 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
>      virtio_scsi_complete_cmd_req(req);
>  }
>  
> -static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
> +static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req,
> +                                              size_t cdb_size)
>  {
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>      SCSIDevice *d;
>      int rc;
>  
> -    rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
> +    rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + cdb_size,
>                                 sizeof(VirtIOSCSICmdResp) + vs->sense_size);
>      if (rc < 0) {
>          if (rc == -ENOTSUP) {
> @@ -878,7 +879,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
>      }
>      req->sreq = scsi_req_new(d, req->req.cmd.tag,
>                               virtio_scsi_get_lun(req->req.cmd.lun),
> -                             req->req.cmd.cdb, vs->cdb_size, req);
> +                             req->req.cmd.cdb, cdb_size, req);
>  
>      if (req->sreq->cmd.mode != SCSI_XFER_NONE
>          && (req->sreq->cmd.mode != req->mode ||
> @@ -913,12 +914,15 @@ static void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>      QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
>  
>      do {
> +        VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
> +        size_t cdb_size = qatomic_read(&vs->cdb_size);
> +
>          if (suppress_notifications) {
>              virtio_queue_set_notification(vq, 0);
>          }
>  
> -        while ((req = virtio_scsi_pop_req(s, vq, NULL))) {
> -            ret = virtio_scsi_handle_cmd_req_prepare(s, req);
> +        while ((req = virtio_scsi_pop_req(s, vq, cdb_size, NULL))) {
> +            ret = virtio_scsi_handle_cmd_req_prepare(s, req, cdb_size);
>              if (!ret) {
>                  QTAILQ_INSERT_TAIL(&reqs, req, next);
>              } else if (ret == -EINVAL) {
> @@ -989,7 +993,7 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
>      }
>  
>      vs->sense_size = virtio_ldl_p(vdev, &scsiconf->sense_size);
> -    vs->cdb_size = virtio_ldl_p(vdev, &scsiconf->cdb_size);
> +    qatomic_set(&vs->cdb_size, virtio_ldl_p(vdev, &scsiconf->cdb_size));
>  }
>  
>  static uint64_t virtio_scsi_get_features(VirtIODevice *vdev,
> @@ -1050,7 +1054,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s,
>          return;
>      }
>  
> -    req = virtio_scsi_pop_req(s, vs->event_vq, &s->event_lock);
> +    req = virtio_scsi_pop_req(s, vs->event_vq, 0, &s->event_lock);
>      WITH_QEMU_LOCK_GUARD(&s->event_lock) {
>          if (!req) {
>              s->events_dropped = true;
> -- 
> 2.53.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|