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=-18.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS 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 A9346C43460 for ; Fri, 23 Apr 2021 10:52:07 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D81446128B for ; Fri, 23 Apr 2021 10:52:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D81446128B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ubuntu.com Authentication-Results: mail.kernel.org; spf=tempfail smtp.mailfrom=linux-audit-bounces@redhat.com Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-579-COw4-zQbOyOu1GhQv0h3SQ-1; Fri, 23 Apr 2021 06:52:03 -0400 X-MC-Unique: COw4-zQbOyOu1GhQv0h3SQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 13C128030A1; Fri, 23 Apr 2021 10:51:59 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CB95A1349A; Fri, 23 Apr 2021 10:51:58 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id 7F8821806D1A; Fri, 23 Apr 2021 10:51:58 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 13N7ufdX022672 for ; Fri, 23 Apr 2021 03:56:41 -0400 Received: by smtp.corp.redhat.com (Postfix) id 7F75E2157FD7; Fri, 23 Apr 2021 07:56:41 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast03.extmail.prod.ext.rdu2.redhat.com [10.11.55.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7A0382157FDF for ; Fri, 23 Apr 2021 07:56:37 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C227D8002E2 for ; Fri, 23 Apr 2021 07:56:37 +0000 (UTC) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-495-33-j-2B8NG2z2mkWHdCY9g-1; Fri, 23 Apr 2021 03:56:33 -0400 X-MC-Unique: 33-j-2B8NG2z2mkWHdCY9g-1 Received: by mail.kernel.org (Postfix) with ESMTPSA id 57E79611C2; Fri, 23 Apr 2021 07:48:34 +0000 (UTC) Date: Fri, 23 Apr 2021 09:48:31 +0200 From: Christian Brauner To: Richard Guy Briggs Subject: Re: [PATCH 1/2] audit: add support for the openat2 syscall Message-ID: <20210423074831.lc4jqqtyuun2fnws@wittgenstein> References: <49510cacfb5fbbaa312a4a389f3a6619675007ab.1616031035.git.rgb@redhat.com> <20210318104843.uiga6tmmhn5wfhbs@wittgenstein> <20210318120801.GK3141668@madcap2.tricolour.ca> <20210423023408.GB2174828@madcap2.tricolour.ca> MIME-Version: 1.0 In-Reply-To: <20210423023408.GB2174828@madcap2.tricolour.ca> X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-loop: linux-audit@redhat.com X-Mailman-Approved-At: Fri, 23 Apr 2021 06:50:23 -0400 Cc: linux-s390@vger.kernel.org, linux-ia64@vger.kernel.org, linux-parisc@vger.kernel.org, x86@kernel.org, LKML , sparclinux@vger.kernel.org, Aleksa Sarai , Linux-Audit Mailing List , Alexander Viro , linux-alpha@vger.kernel.org, linux-fsdevel@vger.kernel.org, Eric Paris , linuxppc-dev@lists.ozlabs.org X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=linux-audit-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Thu, Apr 22, 2021 at 10:34:08PM -0400, Richard Guy Briggs wrote: > On 2021-03-18 08:08, Richard Guy Briggs wrote: > > On 2021-03-18 11:48, Christian Brauner wrote: > > > [+Cc Aleksa, the author of openat2()] > > > > Ah! Thanks for pulling in Aleksa. I thought I caught everyone... > > > > > and a comment below. :) > > > > Same... > > > > > On Wed, Mar 17, 2021 at 09:47:17PM -0400, Richard Guy Briggs wrote: > > > > The openat2(2) syscall was added in kernel v5.6 with commit fddb5d430ad9 > > > > ("open: introduce openat2(2) syscall") > > > > > > > > Add the openat2(2) syscall to the audit syscall classifier. > > > > > > > > See the github issue > > > > https://github.com/linux-audit/audit-kernel/issues/67 > > > > > > > > Signed-off-by: Richard Guy Briggs > > > > --- > > > > arch/alpha/kernel/audit.c | 2 ++ > > > > arch/ia64/kernel/audit.c | 2 ++ > > > > arch/parisc/kernel/audit.c | 2 ++ > > > > arch/parisc/kernel/compat_audit.c | 2 ++ > > > > arch/powerpc/kernel/audit.c | 2 ++ > > > > arch/powerpc/kernel/compat_audit.c | 2 ++ > > > > arch/s390/kernel/audit.c | 2 ++ > > > > arch/s390/kernel/compat_audit.c | 2 ++ > > > > arch/sparc/kernel/audit.c | 2 ++ > > > > arch/sparc/kernel/compat_audit.c | 2 ++ > > > > arch/x86/ia32/audit.c | 2 ++ > > > > arch/x86/kernel/audit_64.c | 2 ++ > > > > kernel/auditsc.c | 3 +++ > > > > lib/audit.c | 4 ++++ > > > > lib/compat_audit.c | 4 ++++ > > > > 15 files changed, 35 insertions(+) > > > > > > > > diff --git a/arch/alpha/kernel/audit.c b/arch/alpha/kernel/audit.c > > > > index 96a9d18ff4c4..06a911b685d1 100644 > > > > --- a/arch/alpha/kernel/audit.c > > > > +++ b/arch/alpha/kernel/audit.c > > > > @@ -42,6 +42,8 @@ int audit_classify_syscall(int abi, unsigned syscall) > > > > return 3; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/arch/ia64/kernel/audit.c b/arch/ia64/kernel/audit.c > > > > index 5192ca899fe6..5eaa888c8fd3 100644 > > > > --- a/arch/ia64/kernel/audit.c > > > > +++ b/arch/ia64/kernel/audit.c > > > > @@ -43,6 +43,8 @@ int audit_classify_syscall(int abi, unsigned syscall) > > > > return 3; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/arch/parisc/kernel/audit.c b/arch/parisc/kernel/audit.c > > > > index 9eb47b2225d2..fc721a7727ba 100644 > > > > --- a/arch/parisc/kernel/audit.c > > > > +++ b/arch/parisc/kernel/audit.c > > > > @@ -52,6 +52,8 @@ int audit_classify_syscall(int abi, unsigned syscall) > > > > return 3; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/arch/parisc/kernel/compat_audit.c b/arch/parisc/kernel/compat_audit.c > > > > index 20c39c9d86a9..fc6d35918c44 100644 > > > > --- a/arch/parisc/kernel/compat_audit.c > > > > +++ b/arch/parisc/kernel/compat_audit.c > > > > @@ -35,6 +35,8 @@ int parisc32_classify_syscall(unsigned syscall) > > > > return 3; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 1; > > > > } > > > > diff --git a/arch/powerpc/kernel/audit.c b/arch/powerpc/kernel/audit.c > > > > index a2dddd7f3d09..8f32700b0baa 100644 > > > > --- a/arch/powerpc/kernel/audit.c > > > > +++ b/arch/powerpc/kernel/audit.c > > > > @@ -54,6 +54,8 @@ int audit_classify_syscall(int abi, unsigned syscall) > > > > return 4; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/arch/powerpc/kernel/compat_audit.c b/arch/powerpc/kernel/compat_audit.c > > > > index 55c6ccda0a85..ebe45534b1c9 100644 > > > > --- a/arch/powerpc/kernel/compat_audit.c > > > > +++ b/arch/powerpc/kernel/compat_audit.c > > > > @@ -38,6 +38,8 @@ int ppc32_classify_syscall(unsigned syscall) > > > > return 4; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 1; > > > > } > > > > diff --git a/arch/s390/kernel/audit.c b/arch/s390/kernel/audit.c > > > > index d395c6c9944c..d964cb94cfaf 100644 > > > > --- a/arch/s390/kernel/audit.c > > > > +++ b/arch/s390/kernel/audit.c > > > > @@ -54,6 +54,8 @@ int audit_classify_syscall(int abi, unsigned syscall) > > > > return 4; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/arch/s390/kernel/compat_audit.c b/arch/s390/kernel/compat_audit.c > > > > index 444fb1f66944..f7b32933ce0e 100644 > > > > --- a/arch/s390/kernel/compat_audit.c > > > > +++ b/arch/s390/kernel/compat_audit.c > > > > @@ -39,6 +39,8 @@ int s390_classify_syscall(unsigned syscall) > > > > return 4; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 1; > > > > } > > > > diff --git a/arch/sparc/kernel/audit.c b/arch/sparc/kernel/audit.c > > > > index a6e91bf34d48..b6dcca9c6520 100644 > > > > --- a/arch/sparc/kernel/audit.c > > > > +++ b/arch/sparc/kernel/audit.c > > > > @@ -55,6 +55,8 @@ int audit_classify_syscall(int abi, unsigned int syscall) > > > > return 4; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/arch/sparc/kernel/compat_audit.c b/arch/sparc/kernel/compat_audit.c > > > > index 10eeb4f15b20..d2652a1083ad 100644 > > > > --- a/arch/sparc/kernel/compat_audit.c > > > > +++ b/arch/sparc/kernel/compat_audit.c > > > > @@ -39,6 +39,8 @@ int sparc32_classify_syscall(unsigned int syscall) > > > > return 4; > > > > case __NR_execve: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 1; > > > > } > > > > diff --git a/arch/x86/ia32/audit.c b/arch/x86/ia32/audit.c > > > > index 6efe6cb3768a..57a02ade5503 100644 > > > > --- a/arch/x86/ia32/audit.c > > > > +++ b/arch/x86/ia32/audit.c > > > > @@ -39,6 +39,8 @@ int ia32_classify_syscall(unsigned syscall) > > > > case __NR_execve: > > > > case __NR_execveat: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 1; > > > > } > > > > diff --git a/arch/x86/kernel/audit_64.c b/arch/x86/kernel/audit_64.c > > > > index 83d9cad4e68b..39de1e021258 100644 > > > > --- a/arch/x86/kernel/audit_64.c > > > > +++ b/arch/x86/kernel/audit_64.c > > > > @@ -53,6 +53,8 @@ int audit_classify_syscall(int abi, unsigned syscall) > > > > case __NR_execve: > > > > case __NR_execveat: > > > > return 5; > > > > + case __NR_openat2: > > > > + return 6; > > > > default: > > > > return 0; > > > > } > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > index 8bb9ac84d2fb..f5616e70d129 100644 > > > > --- a/kernel/auditsc.c > > > > +++ b/kernel/auditsc.c > > > > @@ -76,6 +76,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > #include "audit.h" > > > > > > > > @@ -195,6 +196,8 @@ static int audit_match_perm(struct audit_context *ctx, int mask) > > > > return ((mask & AUDIT_PERM_WRITE) && ctx->argv[0] == SYS_BIND); > > > > case 5: /* execve */ > > > > return mask & AUDIT_PERM_EXEC; > > > > + case 6: /* openat2 */ > > > > + return mask & ACC_MODE((u32)((struct open_how *)ctx->argv[2])->flags); > > > > > > That looks a bit dodgy. Maybe sm like the below would be a bit better? > > > > Ah, ok, fair enough, since original flags use a u32 and this was picked > > as u64 for alignment. It was just occurring to me last night that I > > might have the dubious honour of being the first usage of 0%llo format > > specifier in the kernel... ;-) > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index 47fb48f42c93..531e882a5096 100644 > > > --- a/kernel/auditsc.c > > > +++ b/kernel/auditsc.c > > > @@ -159,6 +159,7 @@ static const struct audit_nfcfgop_tab audit_nfcfgs[] = { > > > > > > static int audit_match_perm(struct audit_context *ctx, int mask) > > > { > > > + struct open_how *openat2; > > > unsigned n; > > > if (unlikely(!ctx)) > > > return 0; > > > @@ -195,6 +196,12 @@ static int audit_match_perm(struct audit_context *ctx, int mask) > > > return ((mask & AUDIT_PERM_WRITE) && ctx->argv[0] == SYS_BIND); > > > case 5: /* execve */ > > > return mask & AUDIT_PERM_EXEC; > > > + case 6: /* openat2 */ > > > + openat2 = ctx->argv[2]; > > > + if (upper_32_bits(openat2->flags)) > > > + pr_warn("Some sensible warning about unknown flags"); > > > + > > > + return mask & ACC_MODE(lower_32_bits(openat2->flags)); > > > default: > > > return 0; > > > } > > > > > > (Ideally we'd probably notice at build-time that we've got flags > > > exceeding 32bits. Could probably easily been done by exposing an all > > > flags macro somewhere and then we can place a BUILD_BUG_ON() or sm into > > > such places.) > > open_how arguments are translated to open_flags which is limited to 32 bits. > > This code is shared with the other open functions that are limited to 32 bits > in open_flags. openat2 was created to avoid the limitations of openat, so at > some point it isn't unreasonable that flags exceed 32 bits, but open_flags > would have to be modified at that point to accommodate. > > This value is handed in from userspace, and could be handed in without being > defined in the kernel, so those values need to be properly checked regardless > of the flags defined in the kernel. > > The openat2 syscall claims to check all flags but no check is done on the top > 32 bits. Hm, I think this is an oversight because of the different semantics for openat() and openat2(). We should check that no upper 32 bits are set for openat2(). That's the intended semantics. For old openat() we can't error on unknown flags because it has traditionally ignored unknown flags. > > build_open_flags() assigns how->flags to an int, effectively dropping the top > 32 bits, before being checked against ~VALID_OPEN_FLAGS. This happens after > audit mode filtering, but has the same result. Right. That's at bug we should return an error to userspace. We do for any unkown values that fall within the lower 32 bit range so it's silly to ignore unknown values in the upper 32 bit range. > > Audit mode filtering using ACC_MODE() already masks out all but the lowest two > bits with O_ACCMODE, so there is no danger of overflowing a u32. > > tomoyo_check_open_permission() assigns ACC_MODE() to u8 without a check. > > All FMODE_* flags are clamped at u32. > > 6 bits remain at top and 4 bits just above O_ACCMODE, so there is no immediate > danger of overflow and if any additional mode bits are needed they are > available. > 000377777703 used > 037777777777 available > 10 bits remaining > > So, I don't think a check at this point in the code is useful, but do agree Maybe but note that a defensive posture here might be a good thing instead of tripping over the issue later. > that there should be some changes and checks added in sys_openat2 and > build_open_flags(). > > > Also noticed: It looks like fddb5d430ad9f left in VALID_UPGRADE_FLAGS for > how->upgrade_mask that was removed. This may be used at a later date, but at > this point is dead code. I'll take a look now. Christian -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit