linux-kernel - Re: [PATCH v4 08/19] ublk: move offset check out of __ublk_check_and_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADUfDZrAx-ALGpNckfZOwR2LUqQMYud9cb14bMp1SW_E12isLg@mail.gmail.com>
Date: Mon, 12 Jan 2026 10:29:23 -0800
From: Caleb Sander Mateos <csander@...estorage.com>
To: alex+zkern@...olabs.com
Cc: Ming Lei <ming.lei@...hat.com>, Jens Axboe <axboe@...nel.dk>, Shuah Khan <shuah@...nel.org>, 
	linux-block@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Stanley Zhang <stazhang@...estorage.com>, 
	Uday Shankar <ushankar@...estorage.com>, 
	"Martin K . Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCH v4 08/19] ublk: move offset check out of __ublk_check_and_get_req()

On Mon, Jan 12, 2026 at 10:17 AM Alexander Atanasov <alex@...olabs.com> wrote:
>
> 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 <csander@...estorage.com>
> > Reviewed-by: Ming Lei <ming.lei@...hat.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.

This is the existing behavior in __ublk_check_and_get_req(). I agree
allowing buf_off == blk_rq_bytes(req) seems odd, but changing it now
could break ublk servers relying on the current behavior.

Best,
Caleb

>
> 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
>