Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

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

On Tue, Jan 23, 2024 at 5:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/23/24 3:40 PM, Jens Axboe wrote:
> > On 1/23/24 3:35 PM, Jens Axboe wrote:
> >>
> >> On Tue, 23 Jan 2024 16:55:02 -0500, Paul Moore wrote:
> >>> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
> >>> command to take into account the security implications of making an
> >>> io_uring-private file descriptor generally accessible to a userspace
> >>> task.
> >>>
> >>> The first change in this patch is to enable auditing of the FD_INSTALL
> >>> operation as installing a file descriptor into a task's file descriptor
> >>> table is a security relevant operation and something that admins/users
> >>> may want to audit.
> >>>
> >>> [...]
> >>
> >> Applied, thanks!
> >>
> >> [1/1] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL
> >>       commit: 16bae3e1377846734ec6b87eee459c0f3551692c
> >
> > So after doing that and writing the test case and testing it, it dawned
> > on me that we should potentially allow the current task creds. And to
> > make matters worse, this is indeed what happens if eg the application
> > would submit this with IOSQE_ASYNC or if it was part of a linked series
> > and we marked it async.
> >
> > While I originally reasoned for why this is fine as it'd be silly to
> > register your current creds and then proceed to pass in that personality,
> > I do think that we should probably handle that case and clearly separate
> > the case of "we assigned creds from the submitting task because we're
> > handing it to a thread" vs "the submitting task asked for other creds
> > that were previously registered".
> >
> > I'll take a look and see what works the best here.
>
> Actually, a quick look and it's fine, the usual async offload will do
> the right thing. So let's just keep it as-is, I don't think there's any
> point to complicating this for some theoretically-valid-but-obscure use
> case!

Perhaps the one case where REQ_F_CREDS is our friend for FD_INSTALL ;)

> FWIW, the test case is here, and I'll augment it now to add IOSQE_ASYNC
> as well just to cover all the bases.
>
> https://git.kernel.dk/cgit/liburing/commit/?id=bc576ca398661b266d3e4a4f5db3a9cf7f33fe62

Great, thanks!

-- 
paul-moore.com