linux-audit.redhat.com archive mirror
 help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	selinux@vger.kernel.org, linux-security-module@vger.kernel.org,
	linux-audit@redhat.com,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	linux-fsdevel@vger.kernel.org, io-uring@vger.kernel.org,
	Alexander Viro <viro@zeniv.linux.org.uk>
Subject: Re: [RFC PATCH 2/9] audit, io_uring, io-wq: add some basic audit support to io_uring
Date: Wed, 2 Jun 2021 16:46:39 -0400	[thread overview]
Message-ID: <CAHC9VhS0sy_Y8yx4uiZeJhAf_a94ipt1EbE16BOVv6tXtWkgMg@mail.gmail.com> (raw)
In-Reply-To: <20210602172924.GM447005@madcap2.tricolour.ca>

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.

> 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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


  reply	other threads:[~2021-06-02 20:47 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-21 21:49 [RFC PATCH 0/9] Add LSM access controls and auditing to io_uring Paul Moore
2021-05-21 21:49 ` [RFC PATCH 1/9] audit: prepare audit_context for use in calling contexts beyond syscalls Paul Moore
2021-05-21 21:49 ` [RFC PATCH 2/9] audit, io_uring, io-wq: add some basic audit support to io_uring Paul Moore
2021-05-22  0:22   ` [RFC PATCH 2/9] audit,io_uring,io-wq: " Pavel Begunkov
2021-05-22  2:36     ` Paul Moore
2021-05-23 20:26       ` Pavel Begunkov
2021-05-24 19:59         ` Paul Moore
2021-05-25  8:27           ` Pavel Begunkov
2021-05-25 14:53             ` Paul Moore
2021-05-26  1:11           ` Jens Axboe
2021-05-26  2:04             ` Paul Moore
2021-05-26 10:19               ` Pavel Begunkov
2021-05-26 14:38                 ` Paul Moore
2021-05-26 15:11                   ` [RFC PATCH 2/9] audit, io_uring, io-wq: " Steve Grubb
2021-05-26 15:17                   ` [RFC PATCH 2/9] audit,io_uring,io-wq: " Stefan Metzmacher
2021-05-26 15:49                     ` Richard Guy Briggs
2021-05-26 17:22                       ` Jens Axboe
2021-05-27 17:27                         ` Richard Guy Briggs
2021-05-26 15:49                     ` Victor Stewart
2021-05-26 16:38                       ` Casey Schaufler
2021-05-26 17:15               ` Jens Axboe
2021-05-26 17:31                 ` Jens Axboe
2021-05-26 17:54                   ` Jens Axboe
2021-05-26 18:01                     ` Jens Axboe
2021-05-26 18:44                       ` Paul Moore
2021-05-26 18:57                         ` Pavel Begunkov
2021-05-26 19:10                           ` Paul Moore
2021-05-26 19:44                         ` Jens Axboe
2021-05-26 20:19                           ` Paul Moore
2021-05-28 16:02                             ` Paul Moore
2021-06-02  8:26                               ` Pavel Begunkov
2021-06-02 15:46                                 ` Richard Guy Briggs
2021-06-03 10:39                                   ` Pavel Begunkov
2021-06-02 19:46                                 ` Paul Moore
2021-06-03 10:51                                   ` Pavel Begunkov
2021-06-03 15:54                                     ` Casey Schaufler
2021-06-03 15:54                               ` Jens Axboe
2021-06-04  5:04                                 ` Paul Moore
2021-05-26 18:38                     ` Paul Moore
2021-06-02 17:29   ` [RFC PATCH 2/9] audit, io_uring, io-wq: " Richard Guy Briggs
2021-06-02 20:46     ` Paul Moore [this message]
2021-08-25  1:21       ` Richard Guy Briggs
2021-08-25 19:41         ` Paul Moore
2021-05-21 21:50 ` [RFC PATCH 3/9] audit: dev/test patch to force io_uring auditing Paul Moore
2021-05-21 21:50 ` [RFC PATCH 4/9] audit: add filtering for io_uring records Paul Moore
2021-05-28 22:35   ` Richard Guy Briggs
2021-05-30 15:26     ` Paul Moore
2021-05-31 13:44       ` Richard Guy Briggs
2021-06-02  1:40         ` Paul Moore
2021-06-02 15:37           ` Richard Guy Briggs
2021-06-02 17:20             ` Paul Moore
2021-05-31 13:44       ` [PATCH 1/2] audit: add filtering for io_uring records, addendum Richard Guy Briggs
2021-05-31 16:08         ` kernel test robot
2021-05-31 17:38         ` kernel test robot
2021-06-07 23:15         ` Paul Moore
2021-06-08 12:55           ` Richard Guy Briggs
2021-06-09  2:45             ` Paul Moore
2021-05-31 13:44       ` [PATCH 2/2] audit: block PERM fields being used with io_uring filtering Richard Guy Briggs
2021-05-21 21:50 ` [RFC PATCH 5/9] fs: add anon_inode_getfile_secure() similar to anon_inode_getfd_secure() Paul Moore
2021-05-21 21:50 ` [RFC PATCH 6/9] io_uring: convert io_uring to the secure anon inode interface Paul Moore
2021-05-21 21:50 ` [RFC PATCH 7/9] lsm,io_uring: add LSM hooks to io_uring Paul Moore
2021-05-26 14:48   ` Stefan Metzmacher
2021-05-26 20:45     ` Paul Moore
2021-05-21 21:50 ` [RFC PATCH 8/9] selinux: add support for the io_uring access controls Paul Moore
2021-05-21 21:50 ` [RFC PATCH 9/9] Smack: Brutalist io_uring support with debug Paul Moore
2021-05-22  0:53 ` [RFC PATCH 0/9] Add LSM access controls and auditing to io_uring Tetsuo Handa
2021-05-22  2:06   ` Paul Moore
2021-05-26 15:00 ` Jeff Moyer
2021-05-26 18:49   ` Paul Moore
2021-05-26 19:07     ` Jeff Moyer
2021-05-26 19:10       ` Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHC9VhS0sy_Y8yx4uiZeJhAf_a94ipt1EbE16BOVv6tXtWkgMg@mail.gmail.com \
    --to=paul@paul-moore.com \
    --cc=axboe@kernel.dk \
    --cc=io-uring@vger.kernel.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=memxor@gmail.com \
    --cc=rgb@redhat.com \
    --cc=selinux@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).