3:46 p.m.
On Wed, Jun 2, 2021 at 1:29 PM Richard Guy Briggs <rgb(a)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(a)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