From 265a9f96ef3cfa54d2b66ec74e60a8152c6bb0ab Mon Sep 17 00:00:00 2001 From: Sithum Shihara Date: Sat, 10 Oct 2026 08:33:54 +0200 Subject: virtio: Limit number of mapped iovec entries to maximum available entries Based on an original patch and investigation by Sithum Shihara: limit the number of host iovec entries we use to map guest descriptors to the bound provided to virtqueue_map_desc(). If we don't, Sithum reports, on the vu_handle_tx() path, we can exceed the VIRTQUEUE_MAX_SIZE limit for the out_sg array, with some very specific configurations of memory regions. This is harmless as we could only manage to write one additional entry, 16 bytes (the size of a struct iovec), which invariably ends up into padding memory. This is however definitely not intended, and could potentially turn into robustness issues, even though we couldn't demonstrate any availability impact either, as the FORTIFY_SOURCE checks don't trigger here, because the virtqueue_map_desc() loop uses a direct assignment. We tried to achieve larger writes, which, in theory, we could try to obtain by having multiple exceeding iovec entries (not just 1), but, in practice, without modifying QEMU sources, we couldn't achieve the desired QEMU configuration and the submission of that kind of guest descriptor. However, as an abundance of caution, we considered what could happen if we had achieved that. Sticking to vu_handle_tx() as a caller, which appears to be the only interesting case, the size of the worst-case write would correspond to 30 split iovec entries, of 16 bytes each (struct iovec), that is, 480 bytes. Out of those, 256 bytes would be written to the padding area, and we would be left with 224 bytes that can overwrite up to 7 'elem' entries (and we would need to cross 16 boundaries to reach this point, which, as stated, appears unlikely to be achievable). The values written into these 'elem' array would be corrupted in a specific, predictable way, as they are the return values of vu_gpa_to_va(), but, relevantly, not of the first 8 unintended calls (because those results are written to the padding area), and, as a result, the guest can't control their values, because the 'len' parameter for the non-first calls is fixed and corresponding to the size of the memory regions. We couldn't find any writing pattern for those entries, provided a given starting 'pa' argument, that would generate a valid 'index' field as a result. The resulting values of these 7 'elem' entries can't be used as a vector to achieve arbitrary code execution either (not claimed by Sithum, and we also failed to achieve this), because they don't contain any pointer to be executed, and they can't possibly be used for heap spraying because they can't be possibly written to any heap area (passt doesn't do dynamic memory allocation). We looked into other calling paths for virtqueue_map_desc() as well, but couldn't find any way to trigger a similar out-of-bound write, because they all have external or internal bounds that prevent or limit the iovec entry overflow (to the point where we don't even reach the 'elem' array). Regardless of that, add the missing check for correctness and robustness. Ideally, we would have liked to have a rate-limited warning here, to avoid the risk of a malicious guest flooding the logs, but the call path from where we could obtain a current timestamp is rather long, and we don't expect this configuration to be actually used in any practical setup: stick to a debug() call for simplicity. We would like to thank Sithum Shihara for the precious and insightful investigation, and for sharing his results in a very clear and honest way. Reported-by: Sithum Shihara Analysed-by: Sithum Shihara Fixes: 7d1cd4dbf503 ("vhost-user: introduce virtio API") Signed-off-by: Stefano Brivio --- virtio.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/virtio.c b/virtio.c index d7016cc..689da66 100644 --- a/virtio.c +++ b/virtio.c @@ -409,6 +409,13 @@ static bool virtqueue_map_desc(const struct vu_dev *dev, while (sz) { uint64_t len = sz; + if (num_sg >= max_num_sg) { + debug( +"vhost-user: Too many iovec entries for guest descriptor (%u >= %u), discard", + num_sg, max_num_sg); + return false; + } + iov[num_sg].iov_base = vu_gpa_to_va(dev, &len, pa); if (iov[num_sg].iov_base == NULL) die("vhost-user: invalid address for buffers"); -- cgit v1.2.3