[Devel] [PATCH vz10 2/4] vhost-blk: fix out-of-bounds req[] access on vhost_get_vq_desc() error

[Konstantin Khorenko]

vhost_blk_handle_guest_kick() declares the descriptor head as

	u16 head;

and then does

	head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
				 &out, &in, NULL, NULL);
	if (unlikely(head < 0))
		break;
	if (unlikely(head == vq->num)) {
		...

vhost_get_vq_desc() returns an int with three distinct outcomes:

  - a negative errno (-EFAULT / -EINVAL) when the avail ring or a
    descriptor is malformed,
  - vq->num when there is nothing available right now,
  - the descriptor head index (0 .. vq->num - 1) on success.

Storing that int into a u16 breaks the error case in two ways:

  1. The negative errno is truncated to a large positive 16-bit value
     (e.g. -EINVAL (-22) becomes 0xffea == 65514).
  2. "head < 0" is therefore a comparison of an unsigned value against
     zero, which is always false (the compiler folds it away under
     -Wtype-limits).  The error branch is dead code.

So on a vhost_get_vq_desc() failure we neither break nor hit the
"head == vq->num" branch (65514 does not equal the ring size).  Control
falls through and the bogus value is used as an array index in
vhost_blk_req_handle():

	req = &blk_vq->req[head];	/* blk_vq->req has vq->num entries */

blk_vq->req is allocated with vq->num elements, so indexing it with
~65514 is a wild out-of-bounds access tens of megabytes past the
allocation; the subsequent stores to req->blk_vq, req->head, etc.
corrupt unrelated kernel memory.

The bug is only reachable when vhost_get_vq_desc() actually returns an
error, i.e. when the guest presents a corrupted avail ring or descriptor
chain.  Normal operation is unaffected because a valid head (0 ..
vq->num - 1) fits in a u16 and the "head == vq->num" empty-ring sentinel
also fits, which is why this has gone unnoticed.

Declare head as int.  Then the "head < 0" error check works, the
"head == vq->num" comparison is unchanged (head is non-negative there),
and assigning it to req->head (u16) stays safe because a valid head is
always < vq->num <= 0xffff.

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 <khorenko at virtuozzo.com>
---
 drivers/vhost/blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index ed9b7041893f2..61de56cd73a0a 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -502,7 +502,7 @@ static void vhost_blk_handle_guest_kick(struct vhost_work *work)
 	struct iov_iter iter;
 	int in, out, ret;
 	struct file *f;
-	u16 head;
+	int head;
 
 	vq = container_of(work, struct vhost_virtqueue, poll.work);
 	blk = container_of(vq->dev, struct vhost_blk, dev);
-- 
2.43.0