Re: [PATCH v4 08/19] ublk: move offset check out of __ublk_check_and_get_req()

[Alexander Atanasov]

On 8.01.26 11:19, Caleb Sander Mateos wrote:
__ublk_check_and_get_req() checks that the passed in offset is within
the data length of the specified ublk request. However, only user copy
(ublk_check_and_get_req()) supports accessing ublk request data at a
nonzero offset. Zero-copy buffer registration (ublk_register_io_buf())
always passes 0 for the offset, so the check is unnecessary. Move the
check from __ublk_check_and_get_req() to ublk_check_and_get_req().
Signed-off-by: Caleb Sander Mateos <csan...@purestorage.com>
Reviewed-by: Ming Lei <ming....@redhat.com>
---
  drivers/block/ublk_drv.c | 16 +++++++++-------
  1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index e7697dc4a812..8eefb838b563 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -253,11 +253,11 @@ struct ublk_params_header {
  

[snip]

@@ -2603,13 +2603,10 @@ static inline struct request 
*__ublk_check_and_get_req(struct ublk_device *ub,
                goto fail_put;
  
  	if (!ublk_rq_has_data(req))
                goto fail_put;
  
-	if (offset > blk_rq_bytes(req))
-               goto fail_put;
-
        return req;
  fail_put:
        ublk_put_req_ref(io, req);
        return NULL;
  }
@@ -2687,14 +2684,19 @@ ublk_user_copy(struct kiocb *iocb, struct iov_iter 
*iter, int dir)
  
  	if (tag >= ub->dev_info.queue_depth)
                return -EINVAL;
  
  	io = &ubq->ios[tag];
-       req = __ublk_check_and_get_req(ub, q_id, tag, io, buf_off);
+       req = __ublk_check_and_get_req(ub, q_id, tag, io);
        if (!req)
                return -EINVAL;
  
+	if (buf_off > blk_rq_bytes(req)) {
+               ret = -EINVAL;
+               goto out;
+       }
+

Offset is zero based, bytes are count so it should be >= here.

It will work this way but for buf_off == blk_rq_bytes(req) user will get 
0 instead of EINVAL.

static size_t ublk_copy_user_pages(const struct request *req,
                unsigned offset, struct iov_iter *uiter, int dir)
{
        size_t done = 0;
...
        rq_for_each_segment(bv, req, iter) {
...
                if (offset >= bv.bv_len) {
                        offset -= bv.bv_len; // bv_len is same as 
blk_rq_bytes(req)
                        continue; // this breaks the loop when ==
                }
...
        }
        return done; // done is never incremented
}

        if (!ublk_check_ubuf_dir(req, dir)) {
                ret = -EACCES;
                goto out;
        }


--
have fun,
alex