Re: io_uring: worker thread NULL dereference during openat op

[Paul Moore <paul@paul-moore.com>]

On Tue, Apr 16, 2024 at 9:45 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> On 4/16/24 04:29, Paul Moore wrote:
> > On Mon, Apr 15, 2024 at 7:26 PM Dan Clash <daclash@linux.microsoft.com> wrote:
> >>
> >> Below is a test program that causes multiple io_uring worker threads to
> >> hit a NULL dereference while executing openat ops.

...

> > Thanks for the well documented bug report!
> >
> > That's interesting, it looks like audit_inode() is potentially being
> > passed a filename struct with a NULL name field (filename::name ==
> > NULL).  Given the IOSQE_ASYNC and what looks like io_uring calling
> > getname() from within the __io_openat_prep() function, I suspect the
> > issue is that we aren't associating the filename information we
> > collect in getname() with the proper audit_context().  In other words,
> > we do the getname() in one context, and then the actual open operation
> > in another, and the audit filename info is lost in the switch.
> >
> > I think this is related to another issue that Jens and I have been
> > discussing relating to connect() and sockaddrs.  We had been hoping
> > that the issue we were seeing with sockaddrs was just a special case
> > with connect, but it doesn't look like that is the case.
> >
> > I'm going to be a bit busy this week with conferences, but given the
> > previous discussions with Jens as well as this new issue, I suspect
> > that we are going to need to do some work to support creation of a
> > thin, or lazily setup, audit_context that we can initialize in the
> > io_uring prep routines for use by getname(), move_addr_to_kernel(),
> > etc., store in the io_kiocb struct, and then fully setup in
> > audit_uring_entry().
>
> I'd prefer not to leak that much into the io_uring's hot path. I
> don't understand specifics of the problem, but let me throw
> some ideas:
>
> 1) Each io_uring request has a reference to the task it was
> submitted from, i.e. req->task, can you use the context from
> the submitter task? E.g.
>
> audit_ctx = req->task->audit_context
>
> io_uring explicitly lists all tasks using it, and you can easily
> hook in there and initialise audit so that req->ctx->audit_context
> is always set.

In a few cases, see my previous comments for examples, the work done
in the io_uring prep functions needs to be associated with the work
done in the actual operation, e.g. openat, connect, etc.  The reason
for this is that we need to carry over some bits of state from the
prep portion to the operation itself so that it can be included in the
audit_uring_entry()/_exit() region.  Unfortunately there are a number
of reasons why we can't leverage the submitter's audit_context here,
but most of the reasons essentially come down to the disconnected and
async nature of io_uring operations and the separation between the
prep work and actual operation.

> 2) Can we initialise audit for each io-wq task when we create
> them? We can also try to share audit ctx b/w iowq tasks and
> the task they were created for.

There is still the issue of connecting each individual prep work state
with the associated operation within the audit_uring_entry()/_exit()
region.

-- 
paul-moore.com