vmd(8): guest virtio drivers can cause stack & buffer overflows

A vmd guest can craft invalid virtio descriptor lengths resulting
in reading and writing beyond stack-allocated buffer lengths providing
an escape vector to the host.

Instead of allowing the guest to dictate read/write lengths, this
commit has vmd just use compile-time lengths based on the source
or destination object sizes. For instances where vmd's virtio
implementation can't use this method, such as reading packets from
the vionet device, cap each read with a pre-computed max chunk size.

Reported by Maxime Villard.

Tested with help from Mischa Peters, OK mlarkin@

diff --git a/usr.sbin/vmd/vioscsi.c b/usr.sbin/vmd/vioscsi.c
index 8e37c74..2263c2f 100644 (file)
--- a/usr.sbin/vmd/vioscsi.c
+++ b/usr.sbin/vmd/vioscsi.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vioscsi.c,v 1.17 2021/04/22 18:40:21 dv Exp $  */
+/*     $OpenBSD: vioscsi.c,v 1.18 2021/05/18 11:06:43 dv Exp $  */
 
 /*
  * Copyright (c) 2017 Carlos Cardenas <ccardenas@openbsd.org>
@@ -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 *dev, struct virtio_scsi_req_hdr *req,
 
        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_dev *dev,
            "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_dev *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, 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_dev *dev,
        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_dev *dev,
                 * 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_dev *dev,
                        *(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_dev *dev,
 
                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_dev *dev,
 
                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 vioscsi_dev *dev,
        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 vioscsi_dev *dev,
                 * 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 vioscsi_dev *dev,
                        *(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 vioscsi_dev *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);
@@ -564,11 +567,11 @@ vioscsi_handle_mode_sense_big(struct vioscsi_dev *dev,
 
                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 vioscsi_dev *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_capacity;
@@ -682,7 +684,7 @@ vioscsi_handle_read_capacity(struct vioscsi_dev *dev,
            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 vioscsi_dev *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_capacity_16;
@@ -757,7 +759,7 @@ vioscsi_handle_read_capacity_16(struct vioscsi_dev *dev,
            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 vioscsi_dev *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);
@@ -820,7 +821,7 @@ vioscsi_handle_report_luns(struct vioscsi_dev *dev,
 
        }
 
-       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 vioscsi_dev *dev,
            "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 vioscsi_dev *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, 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 *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 *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 *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 *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_dev *dev,
        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_dev *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);
                } else {
@@ -1072,8 +1071,7 @@ vioscsi_handle_read_10(struct vioscsi_dev *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);
                } else {
@@ -1098,7 +1096,7 @@ vioscsi_handle_read_10(struct vioscsi_dev *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_10;
@@ -1120,8 +1118,16 @@ vioscsi_handle_read_10(struct vioscsi_dev *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 + 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 vioscsi_dev *dev,
 
        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 vioscsi_dev *dev,
        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 vioscsi_dev *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 free_mech;
@@ -1217,7 +1224,7 @@ vioscsi_handle_mechanism_status(struct vioscsi_dev *dev,
            &(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_dev *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 read_toc_out;
@@ -1347,7 +1353,7 @@ vioscsi_handle_read_toc(struct vioscsi_dev *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 read_toc_out;
@@ -1362,8 +1368,7 @@ vioscsi_handle_read_toc(struct vioscsi_dev *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, 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 vioscsi_dev *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);
        } else {
@@ -1441,8 +1445,7 @@ vioscsi_handle_gesn(struct vioscsi_dev *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 *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 *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_dev *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 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_dev *dev,
            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);
@@ -2145,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;
@@ -2177,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);

diff --git a/usr.sbin/vmd/virtio.c b/usr.sbin/vmd/virtio.c
index b6d707f..ad55f5d 100644 (file)
--- a/usr.sbin/vmd/virtio.c
+++ b/usr.sbin/vmd/virtio.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: virtio.c,v 1.86 2021/04/22 18:40:21 dv Exp $  */
+/*     $OpenBSD: virtio.c,v 1.87 2021/05/18 11:06:43 dv Exp $  */
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -232,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);
@@ -365,6 +364,12 @@ 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;
@@ -405,9 +410,16 @@ vioblk_start_write(struct vioblk_dev *dev, off_t sector,
 {
        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;
@@ -415,7 +427,7 @@ vioblk_start_write(struct vioblk_dev *dev, off_t sector,
        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;
        }
@@ -443,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)
@@ -507,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;
@@ -544,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);
@@ -574,7 +585,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
                        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);
@@ -659,7 +670,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
                        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);
@@ -685,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);
@@ -1119,7 +1130,7 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, size_t sz, int *spc)
 
        ret = 0;
 
-       if (sz < 1) {
+       if (sz < 1 || sz > IP_MAXPACKET + ETHER_HDR_LEN) {
                log_warn("%s: invalid packet size", __func__);
                return (0);
        }
@@ -1173,7 +1184,7 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, size_t sz, int *spc)
 
        /* Write out virtio header */
        if (write_mem(hdr_desc->addr, &hdr, sizeof(struct virtio_net_hdr))) {
-               log_warnx("vionet: rx enq header write_mem error @ "
+           log_warnx("vionet: rx enq header write_mem error @ "
                    "0x%llx", hdr_desc->addr);
                goto out;
        }
@@ -1187,7 +1198,7 @@ vionet_enq_rx(struct vionet_dev *dev, char *pkt, size_t sz, int *spc)
 
        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;
@@ -1416,8 +1427,6 @@ 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)
@@ -1425,7 +1434,7 @@ 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, cnt;
-       size_t pktsz;
+       size_t pktsz, chunk_size = 0;
        ssize_t dhcpsz;
        int ret, num_enq, ofs, spc;
        char *vr, *pkt, *dhcppkt;
@@ -1520,9 +1529,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;
@@ -1540,9 +1556,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;