untrusted comment: verify with openbsd-69-base.pub RWQQsAemppS46BYc4N/jYj1zQ4EDUTeuu6Nmvy3JIdpWvvbUU7btsoZJgBCY2pHjsosFRcU5hja+siCqDb3kjsUJP2oHx1fu1wk= OpenBSD 6.9 errata 003, May 18, 2021: vmd guest virtio drivers could cause stack overflows by crafting invalid virtio descriptor lengths. Apply by doing: signify -Vep /etc/signify/openbsd-69-base.pub -x 003_vmd.patch.sig \ -m - | (cd /usr/src && patch -p0) And then rebuild and install vmd(8) and vmctl(8): cd /usr/src/usr.sbin/vmd make obj make make install cd /usr/src/usr.sbin/vmctl make obj make make install Index: usr.sbin/vmd/vioscsi.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/vioscsi.c,v diff -u -p -r1.15 vioscsi.c --- usr.sbin/vmd/vioscsi.c 3 Mar 2021 01:27:54 -0000 1.15 +++ usr.sbin/vmd/vioscsi.c 12 May 2021 00:42:02 -0000 @@ -186,10 +186,16 @@ vioscsi_free_info(struct ioinfo *info) } static struct ioinfo * -vioscsi_start_read(struct vioscsi_dev *dev, off_t block, ssize_t n_blocks) +vioscsi_start_read(struct vioscsi_dev *dev, off_t block, size_t n_blocks) { struct ioinfo *info; + /* Limit to 64M for now */ + if (n_blocks * VIOSCSI_BLOCK_SIZE_CDROM > (1 << 26)) { + log_warnx("%s: read size exceeded 64M", __func__); + return (NULL); + } + info = calloc(1, sizeof(*info)); if (!info) goto nomem; @@ -237,7 +243,7 @@ vioscsi_handle_tur(struct vioscsi_dev *d vioscsi_prepare_resp(&resp, VIRTIO_SCSI_S_OK, SCSI_OK, 0, 0, 0); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status data @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -295,7 +301,7 @@ vioscsi_handle_inquiry(struct vioscsi_de "idx %d req_idx %d global_idx %d", __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_inq; @@ -310,7 +316,8 @@ vioscsi_handle_inquiry(struct vioscsi_de __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, inq_data, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, inq_data, + sizeof(struct scsi_inquiry_data))) { log_warnx("%s: unable to write inquiry" " response to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -337,7 +344,7 @@ vioscsi_handle_mode_sense(struct vioscsi uint8_t mode_page_ctl; uint8_t mode_page_code; uint8_t *mode_reply; - uint8_t mode_reply_len; + uint8_t mode_reply_len = 0; struct scsi_mode_sense *mode_sense; memset(&resp, 0, sizeof(resp)); @@ -356,7 +363,7 @@ vioscsi_handle_mode_sense(struct vioscsi * mode sense header is 4 bytes followed * by a variable page * ERR_RECOVERY_PAGE is 12 bytes - * CDVD_CAPABILITIES_PAGE is 27 bytes + * CDVD_CAPABILITIES_PAGE is 32 bytes */ switch (mode_page_code) { case ERR_RECOVERY_PAGE: @@ -376,7 +383,7 @@ vioscsi_handle_mode_sense(struct vioscsi *(mode_reply + 7) = MODE_READ_RETRY_COUNT; break; case CDVD_CAPABILITIES_PAGE: - mode_reply_len = 31; + mode_reply_len = 36; mode_reply = (uint8_t*)calloc(mode_reply_len, sizeof(uint8_t)); if (mode_reply == NULL) @@ -403,11 +410,10 @@ vioscsi_handle_mode_sense(struct vioscsi DPRINTF("%s: writing resp to 0x%llx size %d " "at local idx %d req_idx %d global_idx %d", - __func__, acct->resp_desc->addr, acct->resp_desc->len, + __func__, acct->resp_desc->addr, mode_reply_len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK" " resp status data @ 0x%llx", __func__, acct->resp_desc->addr); @@ -421,12 +427,11 @@ vioscsi_handle_mode_sense(struct vioscsi DPRINTF("%s: writing mode_reply to 0x%llx " "size %d at local idx %d req_idx %d " - "global_idx %d",__func__, acct->resp_desc->addr, - acct->resp_desc->len, acct->resp_idx, acct->req_idx, - acct->idx); + "global_idx %d", __func__, acct->resp_desc->addr, + mode_reply_len, acct->resp_idx, acct->req_idx, acct->idx); if (write_mem(acct->resp_desc->addr, mode_reply, - acct->resp_desc->len)) { + mode_reply_len)) { log_warnx("%s: unable to write " "mode_reply to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -452,8 +457,7 @@ mode_sense_error: acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); goto mode_sense_out; @@ -478,7 +482,7 @@ vioscsi_handle_mode_sense_big(struct vio uint8_t mode_page_ctl; uint8_t mode_page_code; uint8_t *mode_reply; - uint8_t mode_reply_len; + uint8_t mode_reply_len = 0; uint16_t mode_sense_len; struct scsi_mode_sense_big *mode_sense_10; @@ -499,7 +503,7 @@ vioscsi_handle_mode_sense_big(struct vio * mode sense header is 8 bytes followed * by a variable page * ERR_RECOVERY_PAGE is 12 bytes - * CDVD_CAPABILITIES_PAGE is 27 bytes + * CDVD_CAPABILITIES_PAGE is 32 bytes */ switch (mode_page_code) { case ERR_RECOVERY_PAGE: @@ -519,7 +523,7 @@ vioscsi_handle_mode_sense_big(struct vio *(mode_reply + 11) = MODE_READ_RETRY_COUNT; break; case CDVD_CAPABILITIES_PAGE: - mode_reply_len = 35; + mode_reply_len = 40; mode_reply = (uint8_t*)calloc(mode_reply_len, sizeof(uint8_t)); if (mode_reply == NULL) @@ -549,8 +553,7 @@ vioscsi_handle_mode_sense_big(struct vio __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK" " resp status data @ 0x%llx", __func__, acct->resp_desc->addr); @@ -564,11 +567,11 @@ vioscsi_handle_mode_sense_big(struct vio DPRINTF("%s: writing mode_reply to 0x%llx " "size %d at local idx %d req_idx %d global_idx %d", - __func__, acct->resp_desc->addr, acct->resp_desc->len, + __func__, acct->resp_desc->addr, mode_reply_len, acct->resp_idx, acct->req_idx, acct->idx); if (write_mem(acct->resp_desc->addr, mode_reply, - acct->resp_desc->len)) { + mode_reply_len)) { log_warnx("%s: unable to write " "mode_reply to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -594,8 +597,7 @@ mode_sense_big_error: acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); goto mode_sense_big_out; @@ -666,7 +668,7 @@ vioscsi_handle_read_capacity(struct vios __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_read_capacity; @@ -682,7 +684,7 @@ vioscsi_handle_read_capacity(struct vios acct->resp_idx, acct->req_idx, acct->idx); if (write_mem(acct->resp_desc->addr, r_cap_data, - acct->resp_desc->len)) { + sizeof(struct scsi_read_cap_data))) { log_warnx("%s: unable to write read_cap_data" " response to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -741,7 +743,7 @@ vioscsi_handle_read_capacity_16(struct v __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status " "data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_read_capacity_16; @@ -757,7 +759,7 @@ vioscsi_handle_read_capacity_16(struct v acct->resp_idx, acct->req_idx, acct->idx); if (write_mem(acct->resp_desc->addr, r_cap_data_16, - acct->resp_desc->len)) { + sizeof(struct scsi_read_cap_data_16))) { log_warnx("%s: unable to write read_cap_data_16" " response to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -804,8 +806,7 @@ vioscsi_handle_report_luns(struct vioscs acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR " "status data @ 0x%llx", __func__, acct->resp_desc->addr); @@ -820,7 +821,7 @@ vioscsi_handle_report_luns(struct vioscs } - reply_rpl = calloc(1, sizeof(*reply_rpl)); + reply_rpl = calloc(1, sizeof(struct vioscsi_report_luns_data)); if (reply_rpl == NULL) { log_warnx("%s: cannot alloc reply_rpl", __func__); @@ -841,7 +842,7 @@ vioscsi_handle_report_luns(struct vioscs "idx %d req_idx %d global_idx %d", __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_rpl; @@ -856,7 +857,8 @@ vioscsi_handle_report_luns(struct vioscs __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, reply_rpl, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, reply_rpl, + sizeof(struct vioscsi_report_luns_data))) { log_warnx("%s: unable to write reply_rpl" " response to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -906,8 +908,7 @@ vioscsi_handle_read_6(struct vioscsi_dev acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR " "status data @ 0x%llx", __func__, acct->resp_desc->addr); @@ -942,8 +943,7 @@ vioscsi_handle_read_6(struct vioscsi_dev acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR " "status data @ 0x%llx", __func__, acct->resp_desc->addr); @@ -969,7 +969,7 @@ vioscsi_handle_read_6(struct vioscsi_dev __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status " "data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_read_6; @@ -984,8 +984,7 @@ vioscsi_handle_read_6(struct vioscsi_dev __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, read_buf, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, read_buf, info->len)) { log_warnx("%s: unable to write read_buf to gpa @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -1014,6 +1013,7 @@ vioscsi_handle_read_10(struct vioscsi_de off_t chunk_offset; struct ioinfo *info; struct scsi_rw_10 *read_10; + size_t chunk_len = 0; memset(&resp, 0, sizeof(resp)); read_10 = (struct scsi_rw_10 *)(req->cdb); @@ -1037,8 +1037,7 @@ vioscsi_handle_read_10(struct vioscsi_de acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -1072,8 +1071,7 @@ vioscsi_handle_read_10(struct vioscsi_de acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -1098,7 +1096,7 @@ vioscsi_handle_read_10(struct vioscsi_de __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status " "data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_read_10; @@ -1120,8 +1118,16 @@ vioscsi_handle_read_10(struct vioscsi_de __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, - read_buf + chunk_offset, acct->resp_desc->len)) { + /* Check we don't read beyond read_buf boundaries. */ + if (acct->resp_desc->len > info->len - chunk_offset) { + log_warnx("%s: descriptor length beyond read_buf len", + __func__); + chunk_len = info->len - chunk_offset; + } else + chunk_len = acct->resp_desc->len; + + if (write_mem(acct->resp_desc->addr, read_buf + chunk_offset, + chunk_len)) { log_warnx("%s: unable to write read_buf" " to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -1164,7 +1170,7 @@ vioscsi_handle_prevent_allow(struct vios dev->locked = dev->locked ? 0 : 1; - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status data @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -1193,7 +1199,8 @@ vioscsi_handle_mechanism_status(struct v mech_status_len = (uint16_t)_2btol(mech_status->length); DPRINTF("%s: MECH_STATUS Len %u", __func__, mech_status_len); - mech_status_header = calloc(1, sizeof(*mech_status_header)); + mech_status_header = calloc(1, + sizeof(struct scsi_mechanism_status_header)); if (mech_status_header == NULL) goto mech_out; @@ -1206,7 +1213,7 @@ vioscsi_handle_mechanism_status(struct v acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_mech; @@ -1217,7 +1224,7 @@ vioscsi_handle_mechanism_status(struct v &(acct->resp_idx)); if (write_mem(acct->resp_desc->addr, mech_status_header, - acct->resp_desc->len)) { + sizeof(struct scsi_mechanism_status_header))) { log_warnx("%s: unable to write " "mech_status_header response to " "gpa @ 0x%llx", @@ -1272,8 +1279,7 @@ vioscsi_handle_read_toc(struct vioscsi_d acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); goto read_toc_out; @@ -1347,7 +1353,7 @@ vioscsi_handle_read_toc(struct vioscsi_d __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status data @ 0x%llx", __func__, acct->resp_desc->addr); goto read_toc_out; @@ -1362,8 +1368,7 @@ vioscsi_handle_read_toc(struct vioscsi_d __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, toc_data, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, toc_data, sizeof(toc_data))) { log_warnx("%s: unable to write toc descriptor data @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -1400,8 +1405,7 @@ vioscsi_handle_read_disc_info(struct vio acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); } else { @@ -1441,8 +1445,7 @@ vioscsi_handle_gesn(struct vioscsi_dev * acct->resp_desc = vioscsi_next_ring_desc(acct->desc, acct->req_desc, &(acct->resp_idx)); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set ERR status data @ 0x%llx", __func__, acct->resp_desc->addr); goto gesn_out; @@ -1480,7 +1483,7 @@ vioscsi_handle_gesn(struct vioscsi_dev * __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to write OK resp status " "data @ 0x%llx", __func__, acct->resp_desc->addr); goto gesn_out; @@ -1495,8 +1498,7 @@ vioscsi_handle_gesn(struct vioscsi_dev * __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, gesn_reply, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, gesn_reply, sizeof(gesn_reply))) { log_warnx("%s: unable to write gesn_reply" " response to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -1625,8 +1627,7 @@ vioscsi_handle_get_config(struct vioscsi __func__, acct->resp_desc->addr, acct->resp_desc->len, acct->resp_idx, acct->req_idx, acct->idx); - if (write_mem(acct->resp_desc->addr, &resp, - acct->resp_desc->len)) { + if (write_mem(acct->resp_desc->addr, &resp, sizeof(resp))) { log_warnx("%s: unable to set Ok status data @ 0x%llx", __func__, acct->resp_desc->addr); goto free_get_config; @@ -1642,7 +1643,7 @@ vioscsi_handle_get_config(struct vioscsi acct->resp_idx, acct->req_idx, acct->idx); if (write_mem(acct->resp_desc->addr, get_conf_reply, - acct->resp_desc->len)) { + G_CONFIG_REPLY_SIZE)) { log_warnx("%s: unable to write get_conf_reply" " response to gpa @ 0x%llx", __func__, acct->resp_desc->addr); @@ -2081,7 +2082,7 @@ vioscsi_notifyq(struct vioscsi_dev *dev) { uint64_t q_gpa; uint32_t vr_sz; - int ret; + int cnt, ret; char *vr; struct virtio_scsi_req_hdr req; struct virtio_scsi_res_hdr resp; @@ -2123,8 +2124,15 @@ vioscsi_notifyq(struct vioscsi_dev *dev) goto out; } + cnt = 0; while (acct.idx != (acct.avail->idx & VIOSCSI_QUEUE_MASK)) { + /* Guard against infinite descriptor chains */ + if (++cnt >= VIOSCSI_QUEUE_SIZE) { + log_warnx("%s: invalid descriptor table", __func__); + goto out; + } + acct.req_idx = acct.avail->ring[acct.idx] & VIOSCSI_QUEUE_MASK; acct.req_desc = &(acct.desc[acct.req_idx]); @@ -2138,7 +2146,7 @@ vioscsi_notifyq(struct vioscsi_dev *dev) } /* Read command from descriptor ring */ - if (read_mem(acct.req_desc->addr, &req, acct.req_desc->len)) { + if (read_mem(acct.req_desc->addr, &req, sizeof(req))) { log_warnx("%s: command read_mem error @ 0x%llx", __func__, acct.req_desc->addr); goto out; @@ -2170,8 +2178,13 @@ vioscsi_notifyq(struct vioscsi_dev *dev) vioscsi_prepare_resp(&resp, VIRTIO_SCSI_S_BAD_TARGET, SCSI_OK, 0, 0, 0); + if (acct.resp_desc->len > sizeof(resp)) { + log_warnx("%s: invalid descriptor length", + __func__); + goto out; + } if (write_mem(acct.resp_desc->addr, &resp, - acct.resp_desc->len)) { + sizeof(resp))) { log_warnx("%s: unable to write BAD_TARGET" " resp status data @ 0x%llx", __func__, acct.resp_desc->addr); Index: usr.sbin/vmd/virtio.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v diff -u -p -u -p -r1.84.2.1 virtio.c --- usr.sbin/vmd/virtio.c 3 May 2021 20:12:10 -0000 1.84.2.1 +++ usr.sbin/vmd/virtio.c 13 May 2021 20:57:26 -0000 @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -86,6 +87,8 @@ vioblk_cmd_name(uint32_t type) static void dump_descriptor_chain(struct vring_desc *desc, int16_t dxx) { + unsigned int cnt = 0; + log_debug("descriptor chain @ %d", dxx); do { log_debug("desc @%d addr/len/flags/next = 0x%llx / 0x%x " @@ -96,6 +99,15 @@ dump_descriptor_chain(struct vring_desc desc[dxx].flags, desc[dxx].next); dxx = desc[dxx].next; + + /* + * Dump up to the max number of descriptor for the largest + * queue we support, which currently is VIONET_QUEUE_SIZE. + */ + if (++cnt >= VIONET_QUEUE_SIZE) { + log_warnx("%s: descriptor table invalid", __func__); + return; + } } while (desc[dxx].flags & VRING_DESC_F_NEXT); log_debug("desc @%d addr/len/flags/next = 0x%llx / 0x%x / 0x%x " @@ -220,8 +232,7 @@ viornd_notifyq(void) if (rnd_data != NULL) { arc4random_buf(rnd_data, desc[avail->ring[aidx]].len); - if (write_mem(desc[avail->ring[aidx]].addr, - rnd_data, desc[avail->ring[aidx]].len)) { + if (write_mem(desc[avail->ring[aidx]].addr, rnd_data, sz)) { log_warnx("viornd: can't write random data @ " "0x%llx", desc[avail->ring[aidx]].addr); @@ -349,10 +360,16 @@ vioblk_free_info(struct ioinfo *info) } static struct ioinfo * -vioblk_start_read(struct vioblk_dev *dev, off_t sector, ssize_t sz) +vioblk_start_read(struct vioblk_dev *dev, off_t sector, size_t sz) { struct ioinfo *info; + /* Limit to 64M for now */ + if (sz > (1 << 26)) { + log_warnx("%s: read size exceeded 64M", __func__); + return (NULL); + } + info = calloc(1, sizeof(*info)); if (!info) goto nomem; @@ -393,9 +410,16 @@ vioblk_start_write(struct vioblk_dev *de { struct ioinfo *info; + /* Limit to 64M for now */ + if (len > (1 << 26)) { + log_warnx("%s: write size exceeded 64M", __func__); + return (NULL); + } + info = calloc(1, sizeof(*info)); if (!info) goto nomem; + info->buf = malloc(len); if (info->buf == NULL) goto nomem; @@ -403,7 +427,7 @@ vioblk_start_write(struct vioblk_dev *de info->offset = sector * VIRTIO_BLK_SECTOR_SIZE; info->file = &dev->file; - if (read_mem(addr, info->buf, len)) { + if (read_mem(addr, info->buf, info->len)) { vioblk_free_info(info); return NULL; } @@ -431,7 +455,6 @@ vioblk_finish_write(struct ioinfo *info) /* * XXX in various cases, ds should be set to VIRTIO_BLK_S_IOERR, if we can - * XXX cant trust ring data from VM, be extra cautious. */ int vioblk_notifyq(struct vioblk_dev *dev) @@ -440,7 +463,7 @@ vioblk_notifyq(struct vioblk_dev *dev) uint32_t vr_sz; uint16_t idx, cmd_desc_idx, secdata_desc_idx, ds_desc_idx; uint8_t ds; - int ret; + int cnt, ret; off_t secbias; char *vr; struct vring_desc *desc, *cmd_desc, *secdata_desc, *ds_desc; @@ -495,7 +518,7 @@ vioblk_notifyq(struct vioblk_dev *dev) } /* Read command from descriptor ring */ - if (read_mem(cmd_desc->addr, &cmd, cmd_desc->len)) { + if (read_mem(cmd_desc->addr, &cmd, sizeof(cmd))) { log_warnx("vioblk: command read_mem error @ 0x%llx", cmd_desc->addr); goto out; @@ -513,14 +536,14 @@ vioblk_notifyq(struct vioblk_dev *dev) goto out; } + cnt = 0; secbias = 0; do { struct ioinfo *info; const uint8_t *secdata; info = vioblk_start_read(dev, - cmd.sector + secbias, - (ssize_t)secdata_desc->len); + cmd.sector + secbias, secdata_desc->len); /* read the data, use current data descriptor */ secdata = vioblk_finish_read(info); @@ -532,7 +555,7 @@ vioblk_notifyq(struct vioblk_dev *dev) } if (write_mem(secdata_desc->addr, secdata, - secdata_desc->len)) { + secdata_desc->len)) { log_warnx("can't write sector " "data to gpa @ 0x%llx", secdata_desc->addr); @@ -549,13 +572,20 @@ vioblk_notifyq(struct vioblk_dev *dev) secdata_desc_idx = secdata_desc->next & VIOBLK_QUEUE_MASK; secdata_desc = &desc[secdata_desc_idx]; + + /* Guard against infinite chains */ + if (++cnt >= VIOBLK_QUEUE_SIZE) { + log_warnx("%s: descriptor table " + "invalid", __func__); + goto out; + } } while (secdata_desc->flags & VRING_DESC_F_NEXT); ds_desc_idx = secdata_desc_idx; ds_desc = secdata_desc; ds = VIRTIO_BLK_S_OK; - if (write_mem(ds_desc->addr, &ds, ds_desc->len)) { + if (write_mem(ds_desc->addr, &ds, sizeof(ds))) { log_warnx("can't write device status data @ " "0x%llx", ds_desc->addr); dump_descriptor_chain(desc, cmd_desc_idx); @@ -594,6 +624,7 @@ vioblk_notifyq(struct vioblk_dev *dev) goto out; } + cnt = 0; secbias = 0; do { struct ioinfo *info; @@ -626,13 +657,20 @@ vioblk_notifyq(struct vioblk_dev *dev) secdata_desc_idx = secdata_desc->next & VIOBLK_QUEUE_MASK; secdata_desc = &desc[secdata_desc_idx]; + + /* Guard against infinite chains */ + if (++cnt >= VIOBLK_QUEUE_SIZE) { + log_warnx("%s: descriptor table " + "invalid", __func__); + goto out; + } } while (secdata_desc->flags & VRING_DESC_F_NEXT); ds_desc_idx = secdata_desc_idx; ds_desc = secdata_desc; ds = VIRTIO_BLK_S_OK; - if (write_mem(ds_desc->addr, &ds, ds_desc->len)) { + if (write_mem(ds_desc->addr, &ds, sizeof(ds))) { log_warnx("wr vioblk: can't write device " "status data @ 0x%llx", ds_desc->addr); dump_descriptor_chain(desc, cmd_desc_idx); @@ -658,7 +696,7 @@ vioblk_notifyq(struct vioblk_dev *dev) ds_desc = &desc[ds_desc_idx]; ds = VIRTIO_BLK_S_OK; - if (write_mem(ds_desc->addr, &ds, ds_desc->len)) { + if (write_mem(ds_desc->addr, &ds, sizeof(ds))) { log_warnx("fl vioblk: " "can't write device status " "data @ 0x%llx", ds_desc->addr); @@ -1075,7 +1113,7 @@ vionet_update_qs(struct vionet_dev *dev) * Must be called with dev->mutex acquired. */ int -vionet_enq_rx(struct vionet_dev *dev, char *pkt, ssize_t sz, int *spc) +vionet_enq_rx(struct vionet_dev *dev, char *pkt, size_t sz, int *spc) { uint64_t q_gpa; uint32_t vr_sz; @@ -1083,7 +1121,7 @@ vionet_enq_rx(struct vionet_dev *dev, ch ptrdiff_t off; int ret; char *vr; - ssize_t rem; + size_t rem; struct vring_desc *desc, *pkt_desc, *hdr_desc; struct vring_avail *avail; struct vring_used *used; @@ -1092,6 +1130,11 @@ vionet_enq_rx(struct vionet_dev *dev, ch ret = 0; + if (sz < 1 || sz > IP_MAXPACKET + ETHER_HDR_LEN) { + log_warn("%s: invalid packet size", __func__); + return (0); + } + if (!(dev->cfg.device_status & VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK)) return ret; @@ -1155,14 +1198,14 @@ vionet_enq_rx(struct vionet_dev *dev, ch if (rem >= sz) { if (write_mem(hdr_desc->addr + sizeof(struct virtio_net_hdr), - pkt, sz)) { + pkt, sz)) { log_warnx("vionet: rx enq packet write_mem error @ " "0x%llx", pkt_desc->addr); goto out; } } else { /* Fallback to pkt_desc descriptor */ - if ((uint64_t)pkt_desc->len >= (uint64_t)sz) { + if (pkt_desc->len >= sz) { /* Must be not readable */ if ((pkt_desc->flags & VRING_DESC_F_WRITE) == 0) { log_warnx("unexpected readable rx desc %d", @@ -1231,7 +1274,7 @@ vionet_rx(struct vionet_dev *dev) if (errno != EAGAIN) log_warn("unexpected read error on vionet " "device"); - } else if (sz != 0) { + } else if (sz > 0) { eh = (struct ether_header *)buf; if (!dev->lockedmac || sz < ETHER_HDR_LEN || ETHER_IS_MULTICAST(eh->ether_dhost) || @@ -1384,17 +1427,15 @@ vionet_notifyq(struct vionet_dev *dev) /* * Must be called with dev->mutex acquired. - * - * XXX cant trust ring data from VM, be extra cautious. */ int vionet_notify_tx(struct vionet_dev *dev) { uint64_t q_gpa; uint32_t vr_sz; - uint16_t idx, pkt_desc_idx, hdr_desc_idx, dxx; - size_t pktsz; + uint16_t idx, pkt_desc_idx, hdr_desc_idx, dxx, cnt; + size_t pktsz, chunk_size = 0; ssize_t dhcpsz = 0; int ret, num_enq, ofs, spc; char *vr, *pkt, *dhcppkt; struct vring_desc *desc, *pkt_desc, *hdr_desc; @@ -1439,10 +1480,23 @@ vionet_notify_tx(struct vionet_dev *dev) hdr_desc = &desc[hdr_desc_idx]; pktsz = 0; + cnt = 0; dxx = hdr_desc_idx; do { pktsz += desc[dxx].len; dxx = desc[dxx].next; + + /* + * Virtio 1.0, cs04, section 2.4.5: + * "The number of descriptors in the table is defined + * by the queue size for this virtqueue: this is the + * maximum possible descriptor chain length." + */ + if (++cnt >= VIONET_QUEUE_SIZE) { + log_warnx("%s: descriptor table invalid", + __func__); + goto out; + } } while (desc[dxx].flags & VRING_DESC_F_NEXT); pktsz += desc[dxx].len; @@ -1450,11 +1504,12 @@ vionet_notify_tx(struct vionet_dev *dev) /* Remove virtio header descriptor len */ pktsz -= hdr_desc->len; - /* - * XXX check sanity pktsz - * XXX too long and > PAGE_SIZE checks - * (PAGE_SIZE can be relaxed to 16384 later) - */ + /* Only allow buffer len < max IP packet + Ethernet header */ + if (pktsz > IP_MAXPACKET + ETHER_HDR_LEN) { + log_warnx("%s: invalid packet size %lu", __func__, + pktsz); + goto out; + } pkt = malloc(pktsz); if (pkt == NULL) { log_warn("malloc error alloc packet buf"); @@ -1473,9 +1528,16 @@ vionet_notify_tx(struct vionet_dev *dev) goto out; } + /* Check we don't read beyond allocated pktsz */ + if (pkt_desc->len > pktsz - ofs) { + log_warnx("%s: descriptor len past pkt len", + __func__); + chunk_size = pktsz - ofs - pkt_desc->len; + } else + chunk_size = pkt_desc->len; + /* Read packet from descriptor ring */ - if (read_mem(pkt_desc->addr, pkt + ofs, - pkt_desc->len)) { + if (read_mem(pkt_desc->addr, pkt + ofs, chunk_size)) { log_warnx("vionet: packet read_mem error " "@ 0x%llx", pkt_desc->addr); goto out; @@ -1493,9 +1555,15 @@ vionet_notify_tx(struct vionet_dev *dev) goto out; } + /* Check we don't read beyond allocated pktsz */ + if (pkt_desc->len > pktsz - ofs) { + log_warnx("%s: descriptor len past pkt len", __func__); + chunk_size = pktsz - ofs - pkt_desc->len; + } else + chunk_size = pkt_desc->len; + /* Read packet from descriptor ring */ - if (read_mem(pkt_desc->addr, pkt + ofs, - pkt_desc->len)) { + if (read_mem(pkt_desc->addr, pkt + ofs, chunk_size)) { log_warnx("vionet: packet read_mem error @ " "0x%llx", pkt_desc->addr); goto out; Index: usr.sbin/vmd/virtio.h =================================================================== RCS file: /cvs/src/usr.sbin/vmd/virtio.h,v diff -u -p -u -p -r1.37 virtio.h --- usr.sbin/vmd/virtio.h 29 Mar 2021 23:37:01 -0000 1.37 +++ usr.sbin/vmd/virtio.h 13 May 2021 20:58:14 -0000 @@ -298,7 +298,7 @@ int vionet_notifyq(struct vionet_dev *); void vionet_notify_rx(struct vionet_dev *); int vionet_notify_tx(struct vionet_dev *); void vionet_process_rx(uint32_t); -int vionet_enq_rx(struct vionet_dev *, char *, ssize_t, int *); +int vionet_enq_rx(struct vionet_dev *, char *, size_t, int *); void vionet_set_hostmac(struct vmd_vm *, unsigned int, uint8_t *); int vmmci_io(int, uint16_t, uint32_t *, uint8_t *, void *, uint8_t);