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 <[email protected]> --- 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) { -- 2.55.0
