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 <zaara98zaara@gmail.com>
Analysed-by: Sithum Shihara <zaara98zaara@gmail.com>
Fixes: 7d1cd4dbf503 ("vhost-user: introduce virtio API")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

1 files changed, 7 insertions, 0 deletions

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");