Re: [PATCH] hw/scsi: validate IU buffer bounds in vscsi_preprocess_desc()

[Chinmay Rath]

On 8/19/26 15:28, Chinmay Rath wrote:

cdb_offset and local_desc values are dependent on the guest. If the value
is large enough, it can lead to an out-of-bounds read in vscsi_fetch_desc().

Add an 'avail' constant in vscsi_preprocess_desc() representing the number of
bytes available in srp_cmd.add_data[] and use it to prevent out-of-bound reads.

Make all callers of vscsi_preprocess_desc() check its return value and
handle failure accordingly.

Closes: https://gitlab.com/qemu-project/qemu/-/work_items/4162
Signed-off-by: Chinmay Rath <ra...@linux.ibm.com>

Reported-by: Lazymio <m...@lazym.io>

---
  hw/scsi/spapr_vscsi.c | 43 ++++++++++++++++++++++++++++++++++++++-----
  1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index b4c8f94d22..9fef224727 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -484,6 +484,8 @@ static int data_out_desc_size(struct srp_cmd *cmd)
  static int vscsi_preprocess_desc(vscsi_req *req)
  {
      struct srp_cmd *cmd = &req_iu(req)->srp.cmd;
+    /* bytes available for descriptors behind srp_cmd.add_data */
+    const unsigned avail = SRP_MAX_IU_LEN - offsetof(struct srp_cmd, add_data);

req->cdb_offset = cmd->add_cdb_len & ~3; @@ -498,16 +500,35 @@ static int vscsi_preprocess_desc(vscsi_req *req)

      case SRP_NO_DATA_DESC:
          break;
      case SRP_DATA_DESC_DIRECT:
+        if (req->cdb_offset + sizeof(struct srp_direct_buf) > avail) {
+            fprintf(stderr,
+                    "vscsi_preprocess_desc: direct desc out of bounds\n");
+            return -1;
+        }
          req->total_desc = req->local_desc = 1;
          break;
      case SRP_DATA_DESC_INDIRECT: {
-        struct srp_indirect_buf *ind_tmp = (struct srp_indirect_buf *)
-                (cmd->add_data + req->cdb_offset);
+        struct srp_indirect_buf *ind_tmp;
+
+        if (req->cdb_offset + sizeof(struct srp_indirect_buf) > avail) {
+            fprintf(stderr,
+                    "vscsi_preprocess_desc: indirect desc out of bounds\n");
+            return -1;
+        }
+        ind_tmp = (struct srp_indirect_buf *)(cmd->add_data + req->cdb_offset);

req->total_desc = be32_to_cpu(ind_tmp->table_desc.len) /

                            sizeof(struct srp_direct_buf);
          req->local_desc = req->writing ? cmd->data_out_desc_cnt :
                            cmd->data_in_desc_cnt;
+
+        /* desc_list[] entries must also fit inside the IU buffer */
+        if (req->local_desc * sizeof(struct srp_direct_buf) >
+            avail - req->cdb_offset - sizeof(struct srp_indirect_buf)) {
+            fprintf(stderr,
+                    "vscsi_preprocess_desc: local_desc out of bounds\n");
+            return -1;
+        }
          break;
      }
      default:
@@ -725,7 +746,11 @@ static void vscsi_inquiry_no_target(VSCSIState *s, 
vscsi_req *req)
      memcpy(&resp_data[8], "QEMU    ", 8);

req->writing = 0;

-    vscsi_preprocess_desc(req);
+    if (vscsi_preprocess_desc(req) < 0) {
+        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
+        vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
+        return;
+    }
      rc = vscsi_srp_transfer_data(s, req, 0, resp_data, len);
      if (rc < 0) {
          vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
@@ -775,7 +800,12 @@ static void vscsi_report_luns(VSCSIState *s, vscsi_req 
*req)
          i += 8;
      }

- vscsi_preprocess_desc(req);

+    if (vscsi_preprocess_desc(req) < 0) {
+        g_free(resp_data);
+        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
+        vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
+        return;
+    }
      rc = vscsi_srp_transfer_data(s, req, 0, resp_data, len);
      g_free(resp_data);
      if (rc < 0) {
@@ -823,7 +853,10 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
          req->writing = (n < 1);

/* Preprocess RDMA descriptors */

-        vscsi_preprocess_desc(req);
+        if (vscsi_preprocess_desc(req) < 0) {
+            scsi_req_cancel(req->sreq);
+            return 1;
+        }

/* Get transfer direction and initiate transfer */

          if (n > 0) {