On Fri, Feb 02, 2024 at 03:11:48PM +0100, Laurent Vivier wrote:Check the buffer address is correctly in the mmap'ed memory. Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- packet.c | 6 +++++ packet.h | 2 ++ tap.c | 39 +++++++++++++++++++++++++++---- tap.h | 1 + vhost_user.c | 66 ++++++++++++++++++++++++++++++++-------------------- 5 files changed, 84 insertions(+), 30 deletions(-) diff --git a/packet.c b/packet.c index af2a539a1794..3c5fc39df6d7 100644 --- a/packet.c +++ b/packet.c @@ -25,6 +25,12 @@ static int packet_check_range(const struct pool *p, size_t offset, size_t len, const char *start, const char *func, int line) { + ASSERT(p->buf); + + if (p->buf_size == 0)So, IIUC, you're using p->buf_size == 0 essentially as a flag to indicate that this packet pool is reference packets in an external (i.e.. guest for vhost-user) pool rather than passt allocated memory. Some comments on the data structure definition, and/or some "theory of operation" comments describing the two cases would probably help to make it easier to follow.+ return vu_packet_check_range((void *)p->buf, offset, len, start, + func, line); + if (start < p->buf) { if (func) { trace("add packet start %p before buffer start %p, " diff --git a/packet.h b/packet.h index 8377dcf678bb..0aec6d9410aa 100644 --- a/packet.h +++ b/packet.h @@ -22,6 +22,8 @@ struct pool { struct iovec pkt[1]; }; +int vu_packet_check_range(void *buf, size_t offset, size_t len, + const char *start, const char *func, int line); void packet_add_do(struct pool *p, size_t len, const char *start, const char *func, int line); void *packet_get_do(const struct pool *p, const size_t idx, diff --git a/tap.c b/tap.c index c2a917bc00ca..930e48689497 100644 --- a/tap.c +++ b/tap.c @@ -626,7 +626,7 @@ resume: if (!eh) continue; if (ntohs(eh->h_proto) == ETH_P_ARP) { - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); packet_add(pkt, l2_len, (char *)eh); arp(c, pkt); @@ -656,7 +656,7 @@ resume: continue; if (iph->protocol == IPPROTO_ICMP) { - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); if (c->no_icmp) continue; @@ -675,7 +675,7 @@ resume: continue; if (iph->protocol == IPPROTO_UDP) { - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); packet_add(pkt, l2_len, (char *)eh); if (dhcp(c, pkt)) @@ -815,7 +815,7 @@ resume: } if (proto == IPPROTO_ICMPV6) { - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); if (c->no_icmp) continue; @@ -839,7 +839,7 @@ resume: uh = (struct udphdr *)l4h; if (proto == IPPROTO_UDP) { - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); packet_add(pkt, l4_len, l4h); @@ -1291,6 +1291,23 @@ static void tap_sock_tun_init(struct ctx *c) epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); } +void tap_sock_update_buf(void *base, size_t size) +{ + int i; + + pool_tap4_storage.buf = base; + pool_tap4_storage.buf_size = size; + pool_tap6_storage.buf = base; + pool_tap6_storage.buf_size = size; + + for (i = 0; i < TAP_SEQS; i++) { + tap4_l4[i].p.buf = base; + tap4_l4[i].p.buf_size = size; + tap6_l4[i].p.buf = base; + tap6_l4[i].p.buf_size = size; + } +} + /** * tap_sock_init() - Create and set up AF_UNIX socket or tuntap file descriptor * @c: Execution context @@ -1302,10 +1319,22 @@ void tap_sock_init(struct ctx *c) pool_tap4_storage = PACKET_INIT(pool_tap4, TAP_MSGS, pkt_buf, sz); pool_tap6_storage = PACKET_INIT(pool_tap6, TAP_MSGS, pkt_buf, sz); + if (c->mode == MODE_VU) { + pool_tap4_storage.buf = NULL; + pool_tap4_storage.buf_size = 0; + pool_tap6_storage.buf = NULL; + pool_tap6_storage.buf_size = 0; + } for (i = 0; i < TAP_SEQS; i++) { tap4_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz); tap6_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz); + if (c->mode == MODE_VU) { + tap4_l4[i].p.buf = NULL; + tap4_l4[i].p.buf_size = 0; + tap6_l4[i].p.buf = NULL; + tap6_l4[i].p.buf_size = 0; + }Can't you use your tap_sock_update_buf() function above to do this initialization?} if (c->fd_tap != -1) { /* Passed as --fd */ diff --git a/tap.h b/tap.h index ee839d4f09dc..6823c9b32313 100644 --- a/tap.h +++ b/tap.h @@ -82,6 +82,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, void tap_handler_passt(struct ctx *c, uint32_t events, const struct timespec *now); void tap_sock_reset(struct ctx *c); +void tap_sock_update_buf(void *base, size_t size); void tap_sock_init(struct ctx *c); void pool_flush_all(void); void tap_handler_all(struct ctx *c, const struct timespec *now); diff --git a/vhost_user.c b/vhost_user.c index 2acd72398e3a..9cc07c8312c0 100644 --- a/vhost_user.c +++ b/vhost_user.c @@ -334,6 +334,25 @@ static bool map_ring(VuDev *vdev, VuVirtq *vq) return !(vq->vring.desc && vq->vring.used && vq->vring.avail); } +int vu_packet_check_range(void *buf, size_t offset, size_t len, const char *start, + const char *func, int line) +{ + VuDevRegion *dev_region; +Ah.. and if IIUC, in the indirect buffer case, the buf pointer in the pool is a pointer to a vector of VuDevRegion rather than a buffer. I think I'd prefer to see struct pool changed to include a union to make it clear that there are two quite different interpretations of the buf pointer.+ for (dev_region = buf; dev_region->mmap_addr; dev_region++) { + if ((char *)dev_region->mmap_addr <= start && + start + offset + len < (char *)dev_region->mmap_addr + + dev_region->mmap_offset + + dev_region->size) + return 0; + } + if (func) { + trace("cannot find region, %s:%i", func, line); + } + + return -1; +} + /* * #syscalls:passt mmap munmap */ @@ -400,6 +419,12 @@ static bool vu_set_mem_table_exec(VuDev *vdev, } } + /* XXX */What's this XXX for?+ ASSERT(vdev->nregions < VHOST_USER_MAX_RAM_SLOTS - 1); + vdev->regions[vdev->nregions].mmap_addr = 0; /* mark EOF for vu_packet_check_range() */ + + tap_sock_update_buf(vdev->regions, 0);If you use a union, you could make the pool point to a while VuDev with nregions as well as the actual region list and bounds check without needing this hack.+ return false; } @@ -650,8 +675,8 @@ static void vu_handle_tx(VuDev *vdev, int index) VuVirtq *vq = &vdev->vq[index]; int hdrlen = vdev->hdrlen; struct timespec now; - char *p; - size_t n; + unsigned int indexes[VIRTQUEUE_MAX_SIZE]; + int count; if (index % 2 != VHOST_USER_TX_QUEUE) { debug("index %d is not an TX queue", index); @@ -660,14 +685,11 @@ static void vu_handle_tx(VuDev *vdev, int index) clock_gettime(CLOCK_MONOTONIC, &now); - p = pkt_buf; - pool_flush_all(); + count = 0; while (1) { VuVirtqElement *elem; - unsigned int out_num; - struct iovec sg[VIRTQUEUE_MAX_SIZE], *out_sg; ASSERT(index == VHOST_USER_TX_QUEUE); elem = vu_queue_pop(vdev, vq, sizeof(VuVirtqElement), buffer[index]); @@ -675,32 +697,26 @@ static void vu_handle_tx(VuDev *vdev, int index) break; } - out_num = elem->out_num; - out_sg = elem->out_sg; - if (out_num < 1) { + if (elem->out_num < 1) {The change from out_num local to elem->out_num seems like an unrelated stylistic change that could be folded into the earlier patch.debug("virtio-net header not in first element"); break; } + ASSERT(elem->out_num == 1); - if (hdrlen) { - unsigned sg_num; - - sg_num = iov_copy(sg, ARRAY_SIZE(sg), out_sg, out_num, - hdrlen, -1); - out_num = sg_num; - out_sg = sg; - } - - n = iov_to_buf(out_sg, out_num, 0, p, TAP_BUF_FILL); - - packet_add_all(c, n, p); - - p += n; + packet_add_all(c, elem->out_sg[0].iov_len - hdrlen, + (char *)elem->out_sg[0].iov_base + hdrlen); + indexes[count] = elem->index; + count++; + } + tap_handler_all(c, &now); - vu_queue_push(vdev, vq, elem, 0); + if (count) { + int i; + for (i = 0; i < count; i++) + vu_queue_fill_by_index(vdev, vq, indexes[i], 0, i); + vu_queue_flush(vdev, vq, count); vu_queue_notify(vdev, vq); } - tap_handler_all(c, &now); } void vu_kick_cb(struct ctx *c, union epoll_ref ref)-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson