From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90E74C47092 for ; Wed, 2 Jun 2021 20:47:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7595B613DE for ; Wed, 2 Jun 2021 20:47:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229552AbhFBUsv (ORCPT ); Wed, 2 Jun 2021 16:48:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38900 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229541AbhFBUsu (ORCPT ); Wed, 2 Jun 2021 16:48:50 -0400 Received: from mail-ej1-x636.google.com (mail-ej1-x636.google.com [IPv6:2a00:1450:4864:20::636]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F2C7CC06174A for ; Wed, 2 Jun 2021 13:46:51 -0700 (PDT) Received: by mail-ej1-x636.google.com with SMTP id g8so5884237ejx.1 for ; Wed, 02 Jun 2021 13:46:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fmjdGsZgyfhH82V3nYsUARig/lnO3sllBQpi/gDMFvc=; b=MuvgcTuAIXOaGGA++L/YavJDqGoEu5mG11y7jbFWn+bjS0yuutVVvCSh4/wsETk1cj irDindvdBjVVC5Nypb04oqK5y9kvwErAOXUzBg/D5+uuE6jmwwFmKdrUxjftl6g9XUpw iZ4AVRmt6rkwr2r0x+v6JlPsB1M2TpcIbf3t4t051jj2McFmGwZ9Pp7WEjFULKlG6fY/ RT410/1rOrww8zNzjLW91sxSjKjDDx5/KB5gk2ueLb5xDwQf3FVui57Vq3jYZuAO5Wji qSpqbeI9wNbA64Toaoc6mJ6u7x8aeKU+E+v6bhOdEoH3+902WGdj/Xcrpkcc6rAcFONW CFCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fmjdGsZgyfhH82V3nYsUARig/lnO3sllBQpi/gDMFvc=; b=CDp1yU5p6gkPs0Thdr4T4+3RW6NaiXi28YALcSA2+y8KJfaB5SW87SgUnez+GW/o1e t/+3C0Z41852cYh2fDX4wJjW46V5oGzRvvZg7+oGbO7xhlJAOpZpPiT9PlRdDoYo0/qP Aeze20ODjGPKKzXL0raZ2dxzD0gKmoe4d/pjKD1Qy3ZIkXfxT3ZkGxvwXQeE45ZU83Dw k0w7otfaTGJMekoTa0a8cqCGLNBX4oHFYPrACBxlIxWuTJEtxZCSi7l6hPQH6ow6sjX+ m8+t8N2+Ebh2OiYXSe5brcCqLV7u1y82qORvaQNCidsTTe3MtmaNfvEpLezZS3cPQBnt Scrg== X-Gm-Message-State: AOAM530ON2pmY8TToy1FxXf6LeTVRadwVpagxq8+zjnyx6W9zn3+8oGG iJ0nP+ShwUkaT0wkENe0iSttG/jFcrS1UKP+QjNL X-Google-Smtp-Source: ABdhPJx/n1S5UyuIfGVXGC2QsiIR2Cy08Xe7mAk9pLAL6gbKDdUveE6w4bsXPazI8bBpKsR9b5qc2FdUOfr3XdARSsY= X-Received: by 2002:a17:906:17d8:: with SMTP id u24mr36904730eje.106.1622666810379; Wed, 02 Jun 2021 13:46:50 -0700 (PDT) MIME-Version: 1.0 References: <162163367115.8379.8459012634106035341.stgit@sifl> <162163379461.8379.9691291608621179559.stgit@sifl> <20210602172924.GM447005@madcap2.tricolour.ca> In-Reply-To: <20210602172924.GM447005@madcap2.tricolour.ca> From: Paul Moore Date: Wed, 2 Jun 2021 16:46:39 -0400 Message-ID: Subject: Re: [RFC PATCH 2/9] audit, io_uring, io-wq: add some basic audit support to io_uring To: Richard Guy Briggs Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org, linux-audit@redhat.com, io-uring@vger.kernel.org, linux-fsdevel@vger.kernel.org, Kumar Kartikeya Dwivedi , Jens Axboe , Alexander Viro Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: io-uring@vger.kernel.org On Wed, Jun 2, 2021 at 1:29 PM Richard Guy Briggs 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: > > > > % > > > > Thanks to Richard Guy Briggs for review and feedback. > > > > Signed-off-by: Paul Moore > > --- > > 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 > > #include > > #include > > +#include > > > > #define CREATE_TRACE_POINTS > > #include > > @@ -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-" set at the top of > io_sq_thread(). I think this is more syscall/io_uring confusion. -- paul moore www.paul-moore.com