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

[Chinmay Rath <rathc@linux.ibm.com>]

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 <rathc@linux.ibm.com>
Reported-by: Lazymio <mio@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) {