[PATCH v2 0/3] ublk: harden user buffer handling

[PATCH v2 0/3] ublk: harden user buffer handling

[Caleb Sander Mateos]

For ublk devices not using user copy or zero copy, the ublk driver
copies request data to/from an arbitrary buffer provided by the
userspace ublk server on each I/O.

Fix several bugs where the kernel driver is overly trusting of the
userspace buffer. Ensure that an invalid userspace buffer at worst
results in a failed or requeued I/O and can't trigger panics or
undefined behavior.

v2:
- Check import_ubuf() return value (Ming)
- Avoid calling blk_update_request() with nr_bytes=0

v1: https://lore.kernel.org/linux-block/20260727162450.2921500-1-csander@purestorage.com/

Caleb Sander Mateos (3):
  ublk: check import_ubuf() return value
  ublk: check for ublk_unmap_io() returning 0
  ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

 drivers/block/ublk_drv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.54.0

[PATCH v2 1/3] ublk: check import_ubuf() return value

[Caleb Sander Mateos]

import_ubuf() can fail if the address range (provided by the userspace
ublk server) is outside the allowed user address space. Return that 0
bytes were copied if import_ubuf() fails rather than passing an
uninitialized struct iov_iter to ublk_copy_user_pages().

Fixes: 981f95a571e3 ("ublk: cleanup ublk_copy_user_pages")
Reported-by: Ming Lei <tom.leiming@gmail.com>
Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 4ca6ec738c93..098e046505ad 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1473,11 +1473,14 @@ static unsigned int ublk_map_io(const struct ublk_queue *ubq,
 	 */
 	if (ublk_need_map_req(req)) {
 		struct iov_iter iter;
 		const int dir = ITER_DEST;
 
-		import_ubuf(dir, u64_to_user_ptr(io->buf.addr), rq_bytes, &iter);
+		if (import_ubuf(dir, u64_to_user_ptr(io->buf.addr), rq_bytes,
+				&iter) < 0)
+			return 0;
+
 		return ublk_copy_user_pages(req, 0, &iter, dir);
 	}
 	return rq_bytes;
 }
 
@@ -1494,11 +1497,14 @@ static unsigned int ublk_unmap_io(bool need_map,
 		struct iov_iter iter;
 		const int dir = ITER_SOURCE;
 
 		WARN_ON_ONCE(io->res > rq_bytes);
 
-		import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res, &iter);
+		if (import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res,
+				&iter) < 0)
+			return 0;
+
 		return ublk_copy_user_pages(req, 0, &iter, dir);
 	}
 	return rq_bytes;
 }
 
-- 
2.54.0

[PATCH v2 2/3] ublk: check for ublk_unmap_io() returning 0

[Caleb Sander Mateos]

If the userspace ublk server passes an unmapped address as the data
buffer for a completed ublk read, ublk_unmap_io() will return 0
indicating no bytes could be copied. Currently, this will result in
calling blk_update_request() with nr_bytes=0, which doesn't seem
supported. Fail the I/O with BLK_STS_IOERR in this case instead.

Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 098e046505ad..2cbc359dc196 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1588,12 +1588,18 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
 	/*
 	 * Extremely impossible since we got data filled in just before
 	 *
 	 * Re-read simply for this unlikely case.
 	 */
-	if (unlikely(unmapped_bytes < io->res))
+	if (unlikely(unmapped_bytes < io->res)) {
+		if (unlikely(!unmapped_bytes)) {
+			res = BLK_STS_IOERR;
+			goto exit;
+		}
+
 		io->res = unmapped_bytes;
+	}
 
 	/*
 	 * Run bio->bi_end_io() with softirqs disabled. If the final fput
 	 * happens off this path, then that will prevent ublk's blkdev_release()
 	 * from being called on current's task work, see fput() implementation.
-- 
2.54.0

[PATCH v2 3/3] ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

[Caleb Sander Mateos]

io->res is set from struct ublksrv_io_cmd's result field, which is
controlled by the ublk server process, without any validation. It's thus
possible for userspace to trigger the io->res > rq_bytes warning.
ublk_copy_user_pages() already limits the copy length to the request
data length, so drop the warning.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 2cbc359dc196..8101c2a73efd 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1495,12 +1495,10 @@ static unsigned int ublk_unmap_io(bool need_map,
 
 	if (ublk_need_unmap_req(req)) {
 		struct iov_iter iter;
 		const int dir = ITER_SOURCE;
 
-		WARN_ON_ONCE(io->res > rq_bytes);
-
 		if (import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res,
 				&iter) < 0)
 			return 0;
 
 		return ublk_copy_user_pages(req, 0, &iter, dir);
-- 
2.54.0