Re: [RFC PATCH v3 2/5] io_uring/cmd: zero-init pdu in io_uring_cmd_prep() to avoid UB

[Sidong Yang <sidong.yang@furiosa.ai>]

On Mon, Sep 08, 2025 at 12:45:58PM -0700, Caleb Sander Mateos wrote:
> On Sat, Sep 6, 2025 at 7:28 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >
> > On Tue, Sep 02, 2025 at 08:31:00AM -0700, Caleb Sander Mateos wrote:
> > > On Tue, Sep 2, 2025 at 3:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > > >
> > > > On Mon, Sep 01, 2025 at 05:34:28PM -0700, Caleb Sander Mateos wrote:
> > > > > On Fri, Aug 22, 2025 at 5:56 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > > > > >
> > > > > > The pdu field in io_uring_cmd may contain stale data when a request
> > > > > > object is recycled from the slab cache. Accessing uninitialized or
> > > > > > garbage memory can lead to undefined behavior in users of the pdu.
> > > > > >
> > > > > > Ensure the pdu buffer is cleared during io_uring_cmd_prep() so that
> > > > > > each command starts from a well-defined state. This avoids exposing
> > > > > > uninitialized memory and prevents potential misinterpretation of data
> > > > > > from previous requests.
> > > > > >
> > > > > > No functional change is intended other than guaranteeing that pdu is
> > > > > > always zero-initialized before use.
> > > > > >
> > > > > > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> > > > > > ---
> > > > > >  io_uring/uring_cmd.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > > > > > index 053bac89b6c0..2492525d4e43 100644
> > > > > > --- a/io_uring/uring_cmd.c
> > > > > > +++ b/io_uring/uring_cmd.c
> > > > > > @@ -203,6 +203,7 @@ int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> > > > > >         if (!ac)
> > > > > >                 return -ENOMEM;
> > > > > >         ioucmd->sqe = sqe;
> > > > > > +       memset(&ioucmd->pdu, 0, sizeof(ioucmd->pdu));
> > > > >
> > > > > Adding this overhead to every existing uring_cmd() implementation is
> > > > > unfortunate. Could we instead track the initialized/uninitialized
> > > > > state by using different types on the Rust side? The io_uring_cmd
> > > > > could start as an IoUringCmd, where the PDU field is MaybeUninit,
> > > > > write_pdu<T>() could return a new IoUringCmdPdu<T> that guarantees the
> > > > > PDU has been initialized.
> > > >
> > > > I've found a flag IORING_URING_CMD_REISSUE that we could initialize
> > > > the pdu. In uring_cmd callback, we can fill zero when it's not reissued.
> > > > But I don't know that we could call T::default() in miscdevice. If we
> > > > make IoUringCmdPdu<T>, MiscDevice also should be MiscDevice<T>.
> > > >
> > > > How about assign a byte in pdu for checking initialized? In uring_cmd(),
> > > > We could set a byte flag that it's not initialized. And we could return
> > > > error that it's not initialized in read_pdu().
> > >
> > > Could we do the zero-initialization (or T::default()) in
> > > MiscdeviceVTable::uring_cmd() if the IORING_URING_CMD_REISSUE flag
> > > isn't set (i.e. on the initial issue)? That way, we avoid any
> > > performance penalty for the existing C uring_cmd() implementations.
> > > I'm not quite sure what you mean by "assign a byte in pdu for checking
> > > initialized".
> >
> > Sure, we could fill zero when it's the first time uring_cmd called with
> > checking the flag. I would remove this commit for next version. I also
> > suggests that we would provide the method that read_pdu() and write_pdu().
> > In read_pdu() I want to check write_pdu() is called before. So along the
> > 20 bytes for pdu, maybe we could use a bytes for the flag that pdu is
> > initialized?
> 
> Not sure what you mean about "20 bytes for pdu".
> It seems like it would be preferable to enforce that write_pdu() has
> been called before read_pdu() using the Rust type system instead of a
> runtime check. I was thinking a signature like fn write_pdu(cmd:
> IoUringCmd, value: T) -> IoUringCmdPdu<T>. Do you feel there's a
> reason that wouldn't work and a runtime check would be necessary?

I didn't think about make write_pdu() to return IoUringCmdPdu<T> before.
I think it's good way to pdu is safe without adding a new generic param for
MiscDevice. write_pdu() would return IoUringCmdPdu<T> and it could call
IoUringCmdPdu<T>::pdu(&mut self) -> &mut T safely maybe.

> 
> >
> > But maybe I would introduce a new struct that has Pin<&mut IoUringCmd> and
> > issue_flags. How about some additional field for pdu is initialized like below?
> >
> > struct IoUringCmdArgs {
> >   ioucmd: Pin<&mut IoUringCmd>,
> >   issue_flags: u32,
> >   pdu_initialized: bool,
> > }
> 
> One other thing I realized is that issue_flags should come from the
> *current* context rather than the context the uring_cmd() callback was
> called in. For example, if io_uring_cmd_done() is called from task
> work context, issue_flags should match the issue_flags passed to the
> io_uring_cmd_tw_t callback, not the issue_flags originally passed to
> the uring_cmd() callback. So it probably makes more sense to decouple
> issue_flags from the (owned) IoUringCmd. I think you could pass it by
> reference (&IssueFlags) or with a phantom reference lifetime
> (IssueFlags<'_>) to the Rust uring_cmd() and task work callbacks to
> ensure it can't be used after those callbacks have returned.

I have had no idea about task work context. I agree with you that
it would be better to separate issue_flags from IoUringCmd. So, 
IoUringCmdArgs would have a only field Pin<&mut IoUringCmd>?

> 
> Best,
> Caleb