linux-kernel - Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl

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] [day] [month] [year] [list]
Message-ID: <CADUfDZrwFe+CJPXtKO8fTVU7wq8gRDcEMeCUGsbH9TTwNV7f3Q@mail.gmail.com>
Date: Mon, 2 Feb 2026 07:38:32 -0800
From: Caleb Sander Mateos <csander@...estorage.com>
To: Ming Lei <ming.lei@...hat.com>
Cc: Jens Axboe <axboe@...nel.dk>, Govindarajulu Varadarajan <govind.varadar@...il.com>, 
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd

On Fri, Jan 30, 2026 at 6:08 PM Ming Lei <ming.lei@...hat.com> wrote:
>
> On Fri, Jan 30, 2026 at 10:14:13AM -0700, Caleb Sander Mateos wrote:
> > struct ublksrv_ctrl_cmd is part of the io_uring_sqe, which may lie in
> > userspace-mapped memory. It's racy to access its fields with normal
> > loads, as userspace may write to them concurrently. Use READ_ONCE() to
> > copy the ublksrv_ctrl_cmd from the io_uring_sqe to the stack. Use the
> > local copy in place of the one in the io_uring_sqe.
> >
> > Fixes: 87213b0d847c ("ublk: allow non-blocking ctrl cmds in IO_URING_F_NONBLOCK issue")
> > Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> > ---
> >  drivers/block/ublk_drv.c | 56 ++++++++++++++++++++++------------------
> >  1 file changed, 31 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 01088194c8d3..8122b012a7ae 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -4729,16 +4729,15 @@ static int ublk_ctrl_del_dev(struct ublk_device **p_ub, bool wait)
> >       if (wait && wait_event_interruptible(ublk_idr_wq, ublk_idr_freed(idx)))
> >               return -EINTR;
> >       return 0;
> >  }
> >
> > -static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
> > +static inline void ublk_ctrl_cmd_dump(u32 cmd_op,
> > +                                   const struct ublksrv_ctrl_cmd *header)
> >  {
> > -     const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
> > -
> >       pr_devel("%s: cmd_op %x, dev id %d qid %d data %llx buf %llx len %u\n",
> > -                     __func__, cmd->cmd_op, header->dev_id, header->queue_id,
> > +                     __func__, cmd_op, header->dev_id, header->queue_id,
> >                       header->data[0], header->addr, header->len);
> >  }
> >
> >  static void ublk_ctrl_stop_dev(struct ublk_device *ub)
> >  {
> > @@ -5117,13 +5116,12 @@ static int ublk_char_dev_permission(struct ublk_device *ub,
> >       path_put(&path);
> >       return err;
> >  }
> >
> >  static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> > -             struct io_uring_cmd *cmd)
> > +             u32 cmd_op, struct ublksrv_ctrl_cmd *header)
> >  {
> > -     struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)io_uring_sqe_cmd(cmd->sqe);
> >       bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
> >       void __user *argp = (void __user *)(unsigned long)header->addr;
> >       char *dev_path = NULL;
> >       int ret = 0;
> >       int mask;
> > @@ -5135,11 +5133,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> >                * The new added command of UBLK_CMD_GET_DEV_INFO2 includes
> >                * char_dev_path in payload too, since userspace may not
> >                * know if the specified device is created as unprivileged
> >                * mode.
> >                */
> > -             if (_IOC_NR(cmd->cmd_op) != UBLK_CMD_GET_DEV_INFO2)
> > +             if (_IOC_NR(cmd_op) != UBLK_CMD_GET_DEV_INFO2)
> >                       return 0;
> >       }
> >
> >       /*
> >        * User has to provide the char device path for unprivileged ublk
> > @@ -5156,11 +5154,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> >       dev_path = memdup_user_nul(argp, header->dev_path_len);
> >       if (IS_ERR(dev_path))
> >               return PTR_ERR(dev_path);
> >
> >       ret = -EINVAL;
> > -     switch (_IOC_NR(cmd->cmd_op)) {
> > +     switch (_IOC_NR(cmd_op)) {
> >       case UBLK_CMD_GET_DEV_INFO:
> >       case UBLK_CMD_GET_DEV_INFO2:
> >       case UBLK_CMD_GET_QUEUE_AFFINITY:
> >       case UBLK_CMD_GET_PARAMS:
> >       case (_IOC_NR(UBLK_U_CMD_GET_FEATURES)):
> > @@ -5186,11 +5184,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> >       if (!ret) {
> >               header->len -= header->dev_path_len;
> >               header->addr += header->dev_path_len;
> >       }
> >       pr_devel("%s: dev id %d cmd_op %x uid %d gid %d path %s ret %d\n",
> > -                     __func__, ub->ub_number, cmd->cmd_op,
> > +                     __func__, ub->ub_number, cmd_op,
> >                       ub->dev_info.owner_uid, ub->dev_info.owner_gid,
> >                       dev_path, ret);
> >  exit:
> >       kfree(dev_path);
> >       return ret;
> > @@ -5210,11 +5208,13 @@ static bool ublk_ctrl_uring_cmd_may_sleep(u32 cmd_op)
> >  }
> >
> >  static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
> >               unsigned int issue_flags)
> >  {
> > -     const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
> > +     /* May point to userspace-mapped memory */
> > +     const struct ublksrv_ctrl_cmd *ub_src = io_uring_sqe_cmd(cmd->sqe);
> > +     struct ublksrv_ctrl_cmd header;
>
> It is cleaner to initialize header variable here, otherwise this patch
> looks fine:

It wouldn't be correct to dereference ub_src here; see the prior patch
"ublk: Validate SQE128 flag before accessing the cmd".

Best,
Caleb