From: dv Date: Thu, 22 Apr 2021 18:40:21 +0000 (+0000) Subject: vmd(8): guard against bad virtio drivers X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=3688c158de84bfd1b433891c75e9b5d6034c892d;p=openbsd vmd(8): guard against bad virtio drivers Add protections against guests with bad virtio-{blk,net,scsi} drivers, specifically avoiding invalid descriptor chains and invalid vionet packet sizes. This helps prevent possible lockup of the host vm process due to a spinning device event loop thread. Also fix an unneeded cast in the vioblk handling in case of invalid buffer lengths. OK mlarkin@ --- diff --git a/usr.sbin/vmd/vioscsi.c b/usr.sbin/vmd/vioscsi.c index 31a2e299cdd..8e37c745ace 100644 --- a/usr.sbin/vmd/vioscsi.c +++ b/usr.sbin/vmd/vioscsi.c @@ -1,4 +1,4 @@ -/* $OpenBSD: vioscsi.c,v 1.16 2021/04/22 10:45:21 dv Exp $ */ +/* $OpenBSD: vioscsi.c,v 1.17 2021/04/22 18:40:21 dv Exp $ */ /* * Copyright (c) 2017 Carlos Cardenas @@ -2081,7 +2081,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 +2123,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]); diff --git a/usr.sbin/vmd/virtio.c b/usr.sbin/vmd/virtio.c index 9fcb752ea4d..b6d707fbed8 100644 --- a/usr.sbin/vmd/virtio.c +++ b/usr.sbin/vmd/virtio.c @@ -1,4 +1,4 @@ -/* $OpenBSD: virtio.c,v 1.85 2021/04/21 18:27:36 dv Exp $ */ +/* $OpenBSD: virtio.c,v 1.86 2021/04/22 18:40:21 dv Exp $ */ /* * Copyright (c) 2015 Mike Larkin @@ -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, int16_t dxx) 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 " @@ -349,7 +361,7 @@ 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; @@ -440,7 +452,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; @@ -513,14 +525,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); @@ -549,6 +561,13 @@ 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; @@ -594,6 +613,7 @@ vioblk_notifyq(struct vioblk_dev *dev) goto out; } + cnt = 0; secbias = 0; do { struct ioinfo *info; @@ -626,6 +646,13 @@ 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; @@ -1397,7 +1424,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; + uint16_t idx, pkt_desc_idx, hdr_desc_idx, dxx, cnt; size_t pktsz; ssize_t dhcpsz; int ret, num_enq, ofs, spc; @@ -1445,10 +1472,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; @@ -1456,11 +1496,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");