[Devel] [PATCH vz10 0/4] vhost-blk: error- and boundary-handling fixes

[Konstantin Khorenko]

While reviewing the recent "vhost-blk: stop fetching descriptors on
VHOST_BLK_SET_BACKEND(-1)" patch I went over the surrounding error paths
in drivers/vhost/blk.c and found a few latent bugs.  None of them are
introduced by that patch - they all date back to the original vhost-blk
import (40a5928ec730) - but they share a common theme: a return value or
a userspace-supplied value is not validated the way the code assumes,
and the mistake stays invisible during normal operation.  This series
fixes them.  No functional change for the well-behaved fast paths.

  1. fix NULL deref on bad fd in VHOST_BLK_SET_BACKEND
     vhost_blk_set_backend() checks fget() with IS_ERR(), but fget()
     signals failure by returning NULL, not an ERR_PTR.  IS_ERR(NULL) is
     false, so a bad (non-negative) fd slips through and the next line
     dereferences it (file->f_mapping->host) and oopses.  Test for NULL
     and return -EBADF.

  2. fix out-of-bounds req[] access on vhost_get_vq_desc() error
     [most important] vhost_blk_handle_guest_kick() stores the
     vhost_get_vq_desc() return value in a u16 head.  On error the
     function returns a negative errno, which truncates to a large
     positive u16, so the "head < 0" check is always false and the bogus
     value is used to index blk_vq->req[head] - a wild out-of-bounds
     access tens of MB past the allocation.  Reachable by a guest that
     corrupts its avail ring / descriptor chain.  Declare head as int.

  3. re-validate vq index in vhost_blk_setup() (double-fetch)
     vhost_blk_setup() uses a userspace-supplied vq index as an array
     index into blk->vqs[] without bounds checking.  It is validated by
     vhost_vring_ioctl() first, but that is a separate read of the same
     userspace memory; a second thread of the same process can rewrite
     the index between the two reads (classic double-fetch / TOCTOU).
     Re-validate against VHOST_BLK_VQ_MAX where it is used.  This is
     local hardening on the kernel/userspace boundary - the trigger
     needs an already-privileged fd owner (a host-side VMM, not the
     guest) winning a narrow race - not a guest-triggerable escape.

  4. return int, not size_t, from the bounce-buffer copy helpers
     vhost_blk_move_{req_to_bb,bb_to_req}() are typed size_t but
     "return -EINVAL".  It works today only by truncation/non-zeroness
     in the callers; switch the return type to int.  No functional
     change - cleanup that removes a future foot-gun.

Patches 1-3 are correctness/memory-safety fixes; patch 4 is cleanup.
All four are independent and could be applied in any order.

Konstantin Khorenko (4):
  vhost-blk: fix NULL deref on bad fd in VHOST_BLK_SET_BACKEND
  vhost-blk: fix out-of-bounds req[] access on vhost_get_vq_desc() error
  vhost-blk: re-validate vq index in vhost_blk_setup() (double-fetch)
  vhost-blk: return int, not size_t, from the bounce-buffer copy helpers

 drivers/vhost/blk.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

-- 
2.43.0