On 6/19/26 20:57, Konstantin Khorenko wrote:
vhost_blk_setup() uses the virtqueue index supplied by userspace as an
array index into blk->vqs[] without bounds checking:
if (copy_from_user(&s, argp, sizeof(s)))
return -EFAULT;
if (blk->vqs[s.index].req) /* s.index unchecked */
return 0;
blk->vqs[s.index].req = kvmalloc(...);
blk->vqs[] is a fixed array of VHOST_BLK_VQ_MAX (32) inline
struct vhost_blk_vq elements embedded in struct vhost_blk. sizeof()
of each element is large (it contains iov[UIO_MAXIOV]), so an
out-of-range s.index points many megabytes outside the allocation:
reading blk->vqs[s.index].req typically oopses, and if that address
happens to be mapped and reads as NULL, the following assignment
performs a wild kernel write -> memory corruption.
At first glance s.index looks safe because vhost_blk_setup() is only
reached from the VHOST_SET_VRING_NUM path:
ret = vhost_vring_ioctl(&blk->dev, ioctl, argp);
if (!ret && ioctl == VHOST_SET_VRING_NUM)
ret = vhost_blk_setup(blk, argp);
and vhost_vring_ioctl() -> vhost_get_vq_from_user() already validates
the index (idx >= dev->nvqs -> -ENOBUFS) and even applies
array_index_nospec().
The catch is that this is a classic double-fetch (TOCTOU, CWE-367).
argp is a pointer into the *caller's* address space. The struct is
read from it twice:
1. vhost_get_vq_from_user() does get_user(idx, idxp) and validates it,
and vhost_vring_set_num() copies and validates the same struct to
set vq->num;
2. vhost_blk_setup() does a *second* copy_from_user() of the same
struct and uses s.index and s.num directly.
blk->dev.mutex is held across both reads, but that mutex only serializes
other in-kernel ioctls on this fd. It does not stop a *different thread
of the same process* (which shares the address space) from overwriting
the struct between the two reads with a plain userspace store - that
thread never enters the kernel at all:
Thread T1: Thread T2 (same process):
state->index = 0;
ioctl(fd, VHOST_SET_VRING_NUM, state)
get_user(index) == 0 -> OK
state->index = 0x41414141;
copy_from_user(&s, argp)
s.index == 0x41414141
blk->vqs[0x41414141] <- out of bounds
So the values validated in step 1 are not necessarily the values used in
step 2. Both userspace-controlled fields of the second fetch need care:
- s.index: re-validate it against VHOST_BLK_VQ_MAX right where it is
used. The bounds check is a predictable branch, so also clamp the
index with array_index_nospec() so a misprediction cannot
speculatively index blk->vqs[] out of range - matching what
vhost_get_vq_from_user() already does for the first read.
- s.num: it is the element count of the per-request array
blk->vqs[s.index].req, but that array is later indexed by the
descriptor head, which is bounded by vq->num (0 .. vq->num - 1), not
by s.num:
req = &blk_vq->req[head]; /* in vhost_blk_req_handle() */
vq->num was set from vhost_vring_set_num()'s own validated copy. A
raced s.num smaller than vq->num would leave req[] too small and the
head index would run off the end; a raced s.num == 0 makes kvmalloc()
return ZERO_SIZE_PTR, which passes the !req check and faults on first
use. Size req[] from the validated vq->num instead of from the racy
s.num; that is also the correct count, since the array is indexed by
head in the ring range.
Practical exposure is limited: the trigger requires a process that
already owns the vhost-blk fd (a host-side VMM, not the guest) and that
wins a narrow race window, so this is local hardening on the
kernel/userspace boundary rather than a guest-triggerable escape. It is
still memory-unsafe and must be fixed.
Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk
guests")
Feature: vhost-blk: in-kernel accelerator for virtio-blk guests
Signed-off-by: Konstantin Khorenko <[email protected]>
Reviewed-by: Vladimir Riabchun <[email protected]>
---
Changes since v1 ("vhost-blk: re-validate vq index in vhost_blk_setup()
(double-fetch)"):
v2:
- Also harden s.index against a speculative out-of-bounds access: after
the bounds check, clamp the index with array_index_nospec(). The
bounds branch is almost always not-taken, so the CPU may speculatively
index blk->vqs[] with an out-of-range s.index before the check
resolves; this matches what vhost_get_vq_from_user() already does for
the first read. (Suggested on review.)
- Close the s.num half of the same double-fetch, which v1 missed. v1's
commit message claimed s.num "needs no such treatment ... just fails
with -ENOMEM"; that is wrong. s.num is read by the same second
copy_from_user() and sizes blk->vqs[].req, which is later indexed by
the descriptor head in 0 .. vq->num - 1 (vq->num comes from the first,
validated fetch in vhost_vring_set_num()). A raced s.num < vq->num
leaves req[] too small -> out-of-bounds req[head] in
vhost_blk_req_handle(); a raced s.num == 0 makes kvmalloc() return
ZERO_SIZE_PTR, which passes the !req check and faults on first use.
v2 sizes req[] from the validated vq->num and ignores the racy s.num.
- Add #include <linux/nospec.h> for array_index_nospec().
- Reword the subject ("vq index" -> "vq setup") and the commit message
to cover both s.index and s.num.
No functional change on the legitimate, non-racing path: a correct VMM
passes s.num == vq->num so req[] keeps the same size, and a valid s.index
is left unchanged by array_index_nospec().
drivers/vhost/blk.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index c03945ac04234..db10434f3ae18 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -21,6 +21,7 @@
#include <linux/kthread.h>
#include <linux/blkdev.h>
#include <linux/llist.h>
+#include <linux/nospec.h>
#include "vhost.h"
@@ -860,14 +861,42 @@ static long vhost_blk_reset_owner(struct vhost_blk *blk)
static int vhost_blk_setup(struct vhost_blk *blk, void __user *argp)
{
struct vhost_vring_state s;
+ struct vhost_virtqueue *vq;
if (copy_from_user(&s, argp, sizeof(s)))
return -EFAULT;
+ /*
+ * s.index was validated by vhost_vring_ioctl() before we got here,
+ * but that was a separate read of the same userspace memory. This is
+ * a fresh copy_from_user(), so re-validate it before using it as an
+ * array index (see the commit message for the double-fetch race).
+ */
+ if (s.index >= VHOST_BLK_VQ_MAX)
+ return -ENOBUFS;
+ /*
+ * The bounds check above is a predictable branch, so the CPU may
+ * speculatively index blk->vqs[] with an out-of-range s.index before
+ * it resolves. Clamp the index for speculation, as
+ * vhost_get_vq_from_user() already does for the first read.
+ */
+ s.index = array_index_nospec(s.index, VHOST_BLK_VQ_MAX);
+ vq = &blk->vqs[s.index].vq;
+
if (blk->vqs[s.index].req)
return 0;
- blk->vqs[s.index].req = kvmalloc(sizeof(struct vhost_blk_req) * s.num, GFP_KERNEL);
+ /*
+ * Size the request array from vq->num, the ring size the kernel
+ * validated and stored in vhost_vring_set_num(). s.num comes from the
+ * same second copy_from_user() as s.index and is equally racy: a
+ * concurrent userspace store could shrink it below vq->num, leaving
+ * req[] too small for the head indices (0 .. vq->num - 1) later used
+ * in vhost_blk_req_handle(). vq->num is always non-zero and a power
+ * of two here, so it is the safe and correct count.
+ */
+ blk->vqs[s.index].req = kvmalloc(sizeof(struct vhost_blk_req) * vq->num,
+ GFP_KERNEL);
if (!blk->vqs[s.index].req)
return -ENOMEM;
--
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
