On Thu, Feb 13, 2025 at 8:30 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> This isn't generally necessary, but conditions have been observed where
> SQE data is accessed from the original SQE after prep has been done and
> outside of the initial issue. Opcode prep handlers must ensure that any
> SQE related data is stable beyond the prep phase, but uring_cmd is a bit
> special in how it handles the SQE which makes it susceptible to reading
> stale data. If the application has reused the SQE before the original
> completes, then that can lead to data corruption.
>
> Down the line we can relax this again once uring_cmd has been sanitized
> a bit, and avoid unnecessarily copying the SQE.
>
> Reported-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> Reviewed-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>
> ---
>
> V2:
> - Pass in SQE for copy, and drop helper for copy
v2 looks good to me. You might add "Fixes: 5eff57fa9f3a", since we
know it fixes the potential SQE corruption in the link and drain
cases.
>
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 8af7780407b7..e6701b7aa147 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -165,15 +165,6 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, u64 res2,
> }
> EXPORT_SYMBOL_GPL(io_uring_cmd_done);
>
> -static void io_uring_cmd_cache_sqes(struct io_kiocb *req)
> -{
> - struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
> - struct io_uring_cmd_data *cache = req->async_data;
> -
> - memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
> - ioucmd->sqe = cache->sqes;
> -}
> -
> static int io_uring_cmd_prep_setup(struct io_kiocb *req,
> const struct io_uring_sqe *sqe)
> {
> @@ -185,10 +176,15 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
> return -ENOMEM;
> cache->op_data = NULL;
>
> - ioucmd->sqe = sqe;
> - /* defer memcpy until we need it */
> - if (unlikely(req->flags & REQ_F_FORCE_ASYNC))
> - io_uring_cmd_cache_sqes(req);
> + /*
> + * Unconditionally cache the SQE for now - this is only needed for
> + * requests that go async, but prep handlers must ensure that any
> + * sqe data is stable beyond prep. Since uring_cmd is special in
> + * that it doesn't read in per-op data, play it safe and ensure that
> + * any SQE data is stable beyond prep. This can later get relaxed.
> + */
> + memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
> + ioucmd->sqe = cache->sqes;
> return 0;
> }
>
> @@ -251,16 +247,8 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
> }
>
> ret = file->f_op->uring_cmd(ioucmd, issue_flags);
> - if (ret == -EAGAIN) {
> - struct io_uring_cmd_data *cache = req->async_data;
> -
> - if (ioucmd->sqe != cache->sqes)
> - io_uring_cmd_cache_sqes(req);
> - return -EAGAIN;
> - } else if (ret == -EIOCBQUEUED) {
> - return -EIOCBQUEUED;
> - }
> -
> + if (ret == -EAGAIN || ret == -EIOCBQUEUED)
> + return ret;
> if (ret < 0)
> req_set_fail(req);
> io_req_uring_cleanup(req, issue_flags);
>
> --
> Jens Axboe
>
