Re: [RFC PATCH 2/9] audit, io_uring, io-wq: add some basic audit support to io_uring

[Richard Guy Briggs <rgb@redhat.com>]

On 2021-06-02 13:46, Paul Moore wrote:
> On Wed, Jun 2, 2021 at 1:29 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-05-21 17:49, Paul Moore wrote:
> > > WARNING - This is a work in progress and should not be merged
> > > anywhere important.  It is almost surely not complete, and while it
> > > probably compiles it likely hasn't been booted and will do terrible
> > > things.  You have been warned.
> > >
> > > This patch adds basic auditing to io_uring operations, regardless of
> > > their context.  This is accomplished by allocating audit_context
> > > structures for the io-wq worker and io_uring SQPOLL kernel threads
> > > as well as explicitly auditing the io_uring operations in
> > > io_issue_sqe().  The io_uring operations are audited using a new
> > > AUDIT_URINGOP record, an example is shown below:
> > >
> > >   % <TODO - insert AUDIT_URINGOP record example>
> > >
> > > Thanks to Richard Guy Briggs for review and feedback.
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > ---
> > >  fs/io-wq.c                 |    4 +
> > >  fs/io_uring.c              |   11 +++
> > >  include/linux/audit.h      |   17 ++++
> > >  include/uapi/linux/audit.h |    1
> > >  kernel/audit.h             |    2 +
> > >  kernel/auditsc.c           |  173 ++++++++++++++++++++++++++++++++++++++++++++
> > >  6 files changed, 208 insertions(+)
> 
> ...
> 
> > > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > > index e481ac8a757a..e9941d1ad8fd 100644
> > > --- a/fs/io_uring.c
> > > +++ b/fs/io_uring.c
> > > @@ -78,6 +78,7 @@
> > >  #include <linux/task_work.h>
> > >  #include <linux/pagemap.h>
> > >  #include <linux/io_uring.h>
> > > +#include <linux/audit.h>
> > >
> > >  #define CREATE_TRACE_POINTS
> > >  #include <trace/events/io_uring.h>
> > > @@ -6105,6 +6106,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
> > >       if (req->work.creds && req->work.creds != current_cred())
> > >               creds = override_creds(req->work.creds);
> > >
> > > +     if (req->opcode < IORING_OP_LAST)
> > > +             audit_uring_entry(req->opcode);
> 
> Note well the override_creds() call right above the audit code that is
> being added, it will be important later in this email.
> 
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index cc89e9f9a753..729849d41631 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -1536,6 +1562,52 @@ static void audit_log_proctitle(void)
> > >       audit_log_end(ab);
> > >  }
> > >
> > > +/**
> > > + * audit_log_uring - generate a AUDIT_URINGOP record
> > > + * @ctx: the audit context
> > > + */
> > > +static void audit_log_uring(struct audit_context *ctx)
> > > +{
> > > +     struct audit_buffer *ab;
> > > +     const struct cred *cred;
> > > +
> > > +     /*
> > > +      * TODO: What do we log here?  I'm tossing in a few things to start the
> > > +      *       conversation, but additional thought needs to go into this.
> > > +      */
> > > +
> > > +     ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_URINGOP);
> > > +     if (!ab)
> > > +             return;
> > > +     cred = current_cred();
> >
> > This may need to be req->work.creds.  I haven't been following if the
> > io_uring thread inherited the user task's creds (and below, comm and
> > exe).
> 
> Nope, we're good.  See the existing code in io_issue_sqe() :)
> 
> > > +     audit_log_format(ab, "uring_op=%d", ctx->uring_op);
> >
> > arch is stored below in __audit_uring_entry() and never used in the
> > AUDIT_CTX_URING case.  That assignment can either be dropped or printed
> > before uring_op similar to the SYSCALL record.
> 
> Good point, I'll drop the code that records the arch from _entry(); it
> is really only useful to give the appropriate context if needed for
> other things in the audit stream, and that isn't the case like it is
> with syscalls.
> 
> > There aren't really any arg[0-3] to print.
> 
> Which is why I didn't print them.
> 
> > io_uring_register and io_uring_setup() args are better covered by other
> > records.  io_uring_enter() has 6 args and the last two aren't covered by
> > SYSCALL anyways.
> 
>  ???
> 
> I think you are confusing the io_uring ops with syscalls; they are
> very different things from an audit perspective and the io_uring
> auditing is not intended to replace syscall records.  The
> io_uring_setup() and io_uring_enter() syscalls will be auditing just
> as any other syscalls would be using the existing syscall audit code.
> 
> > > +     if (ctx->return_valid != AUDITSC_INVALID)
> > > +             audit_log_format(ab, " success=%s exit=%ld",
> > > +                              (ctx->return_valid == AUDITSC_SUCCESS ?
> > > +                               "yes" : "no"),
> > > +                              ctx->return_code);
> > > +     audit_log_format(ab,
> > > +                      " items=%d"
> > > +                      " ppid=%d pid=%d auid=%u uid=%u gid=%u"
> > > +                      " euid=%u suid=%u fsuid=%u"
> > > +                      " egid=%u sgid=%u fsgid=%u",
> > > +                      ctx->name_count,
> > > +                      task_ppid_nr(current),
> > > +                      task_tgid_nr(current),
> > > +                      from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > > +                      from_kuid(&init_user_ns, cred->uid),
> > > +                      from_kgid(&init_user_ns, cred->gid),
> > > +                      from_kuid(&init_user_ns, cred->euid),
> > > +                      from_kuid(&init_user_ns, cred->suid),
> > > +                      from_kuid(&init_user_ns, cred->fsuid),
> > > +                      from_kgid(&init_user_ns, cred->egid),
> > > +                      from_kgid(&init_user_ns, cred->sgid),
> > > +                      from_kgid(&init_user_ns, cred->fsgid));
> >
> > The audit session ID is still important, relevant and qualifies auid.
> > In keeping with the SYSCALL record format, I think we want to keep
> > ses=audit_get_sessionid(current) in here.
> 
> This might be another case of syscall/io_uring confusion.  An io_uring
> op doesn't necessarily have an audit session ID or an audit UID in the
> conventional sense; for example think about SQPOLL works, shared
> rings, etc.

Right, but those syscalls are what instigate io_uring operations, so
whatever process starts that operation, or gets handed that handle
should be tracked with auid and sessionid (the two work together to
track) unless we can easily track io_uring ops to connect them to a
previous setup syscall.  If we see a need to keep the auid, then the
sessionid goes with it.

> > I'm pretty sure we also want to keep comm= and exe= too, but may have to
> > reach into req->task to get it.  There are two values for comm possible,
> > one from the original task and second "iou-sqp-<pid>" set at the top of
> > io_sq_thread().
> 
> I think this is more syscall/io_uring confusion.

I wouldn't call them confusion but rather parallels, attributing a
particular subject to an action.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635