Re: [ghak-trim PATCH v1] audit: move put_tree() to avoid trim_trees refcount underflow and UAF
On Mon 23-08-21 22:04:09, Richard Guy Briggs wrote: > AUDIT_TRIM is expected to be idempotent, but multiple executions resulted in a > refcount underflow and use-after-free. > > git bisect fingered commit fb041bb7c0a918b95c6889fc965cdc4a75b4c0ca (2019-11) > ("locking/refcount: Consolidate implementations of refcount_t") > but this patch with its more thorough checking that wasn't in the x86 assembly > code merely exposed a previously existing tree refcount imbalance in the case > of tree trimming code that was refactored with prune_one() to remove a tree > introduced in commit 8432c70062978d9a57bde6715496d585ec520c3e (2018-11) > ("audit: Simplify locking around untag_chunk()") > > Move the put_tree() to cover only the prune_one() case. > > Passes audit-testsuite and 3 passes of "auditctl -t" with at least one > directory watch. > > Fixes: 8432c7006297 ("audit: Simplify locking around untag_chunk()") > Signed-off-by: Richard Guy Briggs > Cc: Jan Kara > Cc: Will Deacon > Cc: Alexander Viro > Cc: Seiji Nishikawa Ah, that was indeed a stupid mistake. Thanks for debugging this! The patch looks good to me. You can add: Reviewed-by: Jan Kara Honza > --- > kernel/audit_tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index b2be4e978ba3..2cd7b5694422 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -593,7 +593,6 @@ static void prune_tree_chunks(struct audit_tree *victim, > bool tagged) > spin_lock(&hash_lock); > } > spin_unlock(&hash_lock); > - put_tree(victim); > } > > /* > @@ -602,6 +601,7 @@ static void prune_tree_chunks(struct audit_tree *victim, > bool tagged) > static void prune_one(struct audit_tree *victim) > { > prune_tree_chunks(victim, false); > + put_tree(victim); > } > > /* trim the uncommitted chunks from tree */ > -- > 2.27.0 > -- Jan Kara SUSE Labs, CR -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal targets") added generic support for AUDIT but that didn't include support for bi-arch like powerpc. Commit 4b58841149dc ("audit: Add generic compat syscall support") added generic support for bi-arch. Convert powerpc to that bi-arch generic audit support. Cc: Paul Moore Cc: Eric Paris Signed-off-by: Christophe Leroy --- Resending v2 with Audit people in Cc v2: - Missing 'git add' for arch/powerpc/include/asm/unistd32.h - Finalised commit description --- arch/powerpc/Kconfig| 5 +- arch/powerpc/include/asm/unistd32.h | 7 +++ arch/powerpc/kernel/Makefile| 3 -- arch/powerpc/kernel/audit.c | 84 - arch/powerpc/kernel/compat_audit.c | 44 --- 5 files changed, 8 insertions(+), 135 deletions(-) create mode 100644 arch/powerpc/include/asm/unistd32.h delete mode 100644 arch/powerpc/kernel/audit.c delete mode 100644 arch/powerpc/kernel/compat_audit.c diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 663766fbf505..5472358609d2 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -163,6 +163,7 @@ config PPC select ARCH_WANT_IRQS_OFF_ACTIVATE_MM select ARCH_WANT_LD_ORPHAN_WARN select ARCH_WEAK_RELEASE_ACQUIRE + select AUDIT_ARCH_COMPAT_GENERIC select BINFMT_ELF select BUILDTIME_TABLE_SORT select CLONE_BACKWARDS @@ -316,10 +317,6 @@ config GENERIC_TBSYNC bool default y if PPC32 && SMP -config AUDIT_ARCH - bool - default y - config GENERIC_BUG bool default y diff --git a/arch/powerpc/include/asm/unistd32.h b/arch/powerpc/include/asm/unistd32.h new file mode 100644 index ..07689897d206 --- /dev/null +++ b/arch/powerpc/include/asm/unistd32.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef _ASM_POWERPC_UNISTD32_H_ +#define _ASM_POWERPC_UNISTD32_H_ + +#include + +#endif /* _ASM_POWERPC_UNISTD32_H_ */ diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index 7be36c1e1db6..825121eba3c2 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -125,9 +125,6 @@ obj-$(CONFIG_PCI) += pci_$(BITS).o $(pci64-y) \ pci-common.o pci_of_scan.o obj-$(CONFIG_PCI_MSI) += msi.o -obj-$(CONFIG_AUDIT)+= audit.o -obj64-$(CONFIG_AUDIT) += compat_audit.o - obj-$(CONFIG_PPC_IO_WORKAROUNDS) += io-workarounds.o obj-y += trace/ diff --git a/arch/powerpc/kernel/audit.c b/arch/powerpc/kernel/audit.c deleted file mode 100644 index a27f3d09.. --- a/arch/powerpc/kernel/audit.c +++ /dev/null @@ -1,84 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -#include -#include -#include -#include - -static unsigned dir_class[] = { -#include -~0U -}; - -static unsigned read_class[] = { -#include -~0U -}; - -static unsigned write_class[] = { -#include -~0U -}; - -static unsigned chattr_class[] = { -#include -~0U -}; - -static unsigned signal_class[] = { -#include -~0U -}; - -int audit_classify_arch(int arch) -{ -#ifdef CONFIG_PPC64 - if (arch == AUDIT_ARCH_PPC) - return 1; -#endif - return 0; -} - -int audit_classify_syscall(int abi, unsigned syscall) -{ -#ifdef CONFIG_PPC64 - extern int ppc32_classify_syscall(unsigned); - if (abi == AUDIT_ARCH_PPC) - return ppc32_classify_syscall(syscall); -#endif - switch(syscall) { - case __NR_open: - return 2; - case __NR_openat: - return 3; - case __NR_socketcall: - return 4; - case __NR_execve: - return 5; - default: - return 0; - } -} - -static int __init audit_classes_init(void) -{ -#ifdef CONFIG_PPC64 - extern __u32 ppc32_dir_class[]; - extern __u32 ppc32_write_class[]; - extern __u32 ppc32_read_class[]; - extern __u32 ppc32_chattr_class[]; - extern __u32 ppc32_signal_class[]; - audit_register_class(AUDIT_CLASS_WRITE_32, ppc32_write_class); - audit_register_class(AUDIT_CLASS_READ_32, ppc32_read_class); - audit_register_class(AUDIT_CLASS_DIR_WRITE_32, ppc32_dir_class); - audit_register_class(AUDIT_CLASS_CHATTR_32, ppc32_chattr_class); - audit_register_class(AUDIT_CLASS_SIGNAL_32, ppc32_signal_class); -#endif - audit_register_class(AUDIT_CLASS_WRITE, write_class); - audit_register_class(AUDIT_CLASS_READ, read_class); - audit_register_class(AUDIT_CLASS_DIR_WRITE, dir_class); - audit_register_class(AUDIT_CLASS_CHATTR, chattr_class); - audit_register_class(AUDIT_CLASS_SIGNAL, signal_class); - return 0; -} - -__initcall(audit_classes_init); diff --git a/arch/powerpc/kernel/compat_audit.c b/arch/powerpc/kernel/compat_audit.c deleted file mode 100644 index 55c6ccda0a85.. --- a/
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler wrote: > > On 8/20/2021 12:06 PM, Paul Moore wrote: > >> Unless you explicitly enable audit on the kernel cmdline, e.g. > >> "audit=1", processes started before userspace enables audit will not > >> have a properly allocated audit_context; see the "if > >> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for > >> the reason why. > > I found a hack-around that no one will like. I changed that check to be > > (likely(!audit_ever_enabled) && !lsm_multiple_contexts()) > > It probably introduces a memory leak and/or performance degradation, > but it has the desired affect. I can't speak for everyone, but I know I don't like that as a solution ;) I imagine such a change would also draw the ire of the never-audit crowd and the distros themselves. However, I understand the need to just get *something* in place so you can continue to test/develop; it's fine to keep that for now, but I'm going to be very disappointed if that line finds its way into the next posted patchset revision. I'm very much open to ideas but my gut feeling is that the end solution is going to be changes to audit_log_start() and audit_log_end(). In my mind the primary reason for this hunch is that support for multiple LSMs[*] needs to be transparent to the various callers in the kernel; this means the existing audit pattern of ... audit_log_start(...); audit_log_format(...); audit_log_end(...); ... should be preserved and be unchanged from what it is now. We've already talked in some general terms about what such changes might look like, but to summarize the previous discussions, I think we would need to think about the following things: * Adding a timestamp/serial field to the audit_buffer struct, it could even be in a union with the audit_context pointer as we would never need both at the same time: if the audit_context ptr is non-NULL you use that, otherwise you use the timestamp. The audit_buffer timestamp would not eliminate the need for the timestamp info in the audit_context struct for what I hope are obvious reasons. * Additional logic in audit_log_end() to generate additional ancillary records for LSM labels. This will likely mean storing the message "type" passed to audit_log_start() in the audit_record struct and using that information in audit_log_end() to decide if a LSM ancillary record is needed. Logistically this would likely mean converting the existing audit_log_end() function into a static/private __audit_log_end() and converting audit_log_end() into something like this: void audit_log_end(ab) { __audit_log_end(ab); // rm the ab cleanup from __audit_log_end if (ab->anc_mlsm) { // generate the multiple lsm record } audit_buffer_free(ab); } * Something else that I'm surely forgetting :) At the end of all this we may find that the "local" audit_context concept is no longer needed. It was originally created to deal with grouping ancillary records with non-syscall records into a single event; while what we are talking about above is different, I believe that our likely solution will also work to solve the original "local" audit_context use case as well. We'll have to see how this goes. [*] I expect that the audit container ID work will have similar issues and need a similar solution, I'm surprised it hasn't come up yet. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
On Tue, Aug 24, 2021 at 9:36 AM Christophe Leroy wrote: > > Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal > targets") added generic support for AUDIT but that didn't include > support for bi-arch like powerpc. > > Commit 4b58841149dc ("audit: Add generic compat syscall support") > added generic support for bi-arch. > > Convert powerpc to that bi-arch generic audit support. > > Cc: Paul Moore > Cc: Eric Paris > Signed-off-by: Christophe Leroy > --- > Resending v2 with Audit people in Cc > > v2: > - Missing 'git add' for arch/powerpc/include/asm/unistd32.h > - Finalised commit description > --- > arch/powerpc/Kconfig| 5 +- > arch/powerpc/include/asm/unistd32.h | 7 +++ > arch/powerpc/kernel/Makefile| 3 -- > arch/powerpc/kernel/audit.c | 84 - > arch/powerpc/kernel/compat_audit.c | 44 --- > 5 files changed, 8 insertions(+), 135 deletions(-) > create mode 100644 arch/powerpc/include/asm/unistd32.h > delete mode 100644 arch/powerpc/kernel/audit.c > delete mode 100644 arch/powerpc/kernel/compat_audit.c Can you explain, in detail please, the testing you have done to verify this patch? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On 8/24/2021 7:45 AM, Paul Moore wrote: > On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler > wrote: >>> On 8/20/2021 12:06 PM, Paul Moore wrote: Unless you explicitly enable audit on the kernel cmdline, e.g. "audit=1", processes started before userspace enables audit will not have a properly allocated audit_context; see the "if (likely(!audit_ever_enabled))" check at the top of audit_alloc() for the reason why. >> I found a hack-around that no one will like. I changed that check to be >> >> (likely(!audit_ever_enabled) && !lsm_multiple_contexts()) >> >> It probably introduces a memory leak and/or performance degradation, >> but it has the desired affect. > I can't speak for everyone, but I know I don't like that as a solution > ;) I imagine such a change would also draw the ire of the never-audit > crowd and the distros themselves. However, I understand the need to > just get *something* in place so you can continue to test/develop; > it's fine to keep that for now, but I'm going to be very disappointed > if that line finds its way into the next posted patchset revision. As I said, it's a hack-around that demonstrates the scope of the problem. Had you expressed enthusiastic approval for it I'd have been very surprised. > I'm very much open to ideas but my gut feeling is that the end > solution is going to be changes to audit_log_start() and > audit_log_end(). In my mind the primary reason for this hunch is that > support for multiple LSMs[*] needs to be transparent to the various > callers in the kernel; this means the existing audit pattern of ... > > audit_log_start(...); > audit_log_format(...); > audit_log_end(...); > > ... should be preserved and be unchanged from what it is now. We've > already talked in some general terms about what such changes might > look like, but to summarize the previous discussions, I think we would > need to think about the following things: I will give this a shot. > > * Adding a timestamp/serial field to the audit_buffer struct, it could > even be in a union with the audit_context pointer as we would never > need both at the same time: if the audit_context ptr is non-NULL you > use that, otherwise you use the timestamp. The audit_buffer timestamp > would not eliminate the need for the timestamp info in the > audit_context struct for what I hope are obvious reasons. > > * Additional logic in audit_log_end() to generate additional ancillary > records for LSM labels. This will likely mean storing the message > "type" passed to audit_log_start() in the audit_record struct and > using that information in audit_log_end() to decide if a LSM ancillary > record is needed. Logistically this would likely mean converting the > existing audit_log_end() function into a static/private > __audit_log_end() and converting audit_log_end() into something like > this: > > void audit_log_end(ab) > { > __audit_log_end(ab); // rm the ab cleanup from __audit_log_end > if (ab->anc_mlsm) { > // generate the multiple lsm record > } > audit_buffer_free(ab); > } > > * Something else that I'm surely forgetting :) > > At the end of all this we may find that the "local" audit_context > concept is no longer needed. It was originally created to deal with > grouping ancillary records with non-syscall records into a single > event; while what we are talking about above is different, I believe > that our likely solution will also work to solve the original "local" > audit_context use case as well. We'll have to see how this goes. I'll keep that in mind as I fiddle about. > [*] I expect that the audit container ID work will have similar issues > and need a similar solution, I'm surprised it hasn't come up yet. Hmm. That effort has been quiet lately. Too quiet. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
On Tue, Aug 24, 2021 at 11:20 AM Casey Schaufler wrote: > On 8/24/2021 7:45 AM, Paul Moore wrote: > > On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler > > wrote: > >>> On 8/20/2021 12:06 PM, Paul Moore wrote: > Unless you explicitly enable audit on the kernel cmdline, e.g. > "audit=1", processes started before userspace enables audit will not > have a properly allocated audit_context; see the "if > (likely(!audit_ever_enabled))" check at the top of audit_alloc() for > the reason why. > >> I found a hack-around that no one will like. I changed that check to be > >> > >> (likely(!audit_ever_enabled) && !lsm_multiple_contexts()) > >> > >> It probably introduces a memory leak and/or performance degradation, > >> but it has the desired affect. > > I can't speak for everyone, but I know I don't like that as a solution > > ;) I imagine such a change would also draw the ire of the never-audit > > crowd and the distros themselves. However, I understand the need to > > just get *something* in place so you can continue to test/develop; > > it's fine to keep that for now, but I'm going to be very disappointed > > if that line finds its way into the next posted patchset revision. > > As I said, it's a hack-around that demonstrates the scope of the > problem. Had you expressed enthusiastic approval for it I'd have > been very surprised. That's okay, you can admit you were trying to catch me not paying attention ;) > > I'm very much open to ideas but my gut feeling is that the end > > solution is going to be changes to audit_log_start() and > > audit_log_end(). In my mind the primary reason for this hunch is that > > support for multiple LSMs[*] needs to be transparent to the various > > callers in the kernel; this means the existing audit pattern of ... > > > > audit_log_start(...); > > audit_log_format(...); > > audit_log_end(...); > > > > ... should be preserved and be unchanged from what it is now. We've > > already talked in some general terms about what such changes might > > look like, but to summarize the previous discussions, I think we would > > need to think about the following things: > > I will give this a shot. Thanks. I'm sure I'm probably missing some detail, but if you get stuck let me know and I'll try to lend a hand. > > [*] I expect that the audit container ID work will have similar issues > > and need a similar solution, I'm surprised it hasn't come up yet. > > Hmm. That effort has been quiet lately. Too quiet. The current delay is intentional and is related to the io_uring work. When Richard and I first became aware of the io_uring issue Richard was in the process of readying his latest revision to the audit container ID patchset and some of the changes he was incorporating, in my opinion, hinted at the io_uring issue, or at least drew more attention to that than I was comfortable seeing posted publicly. Richard discussed this with his management and security response team, and they felt differently. I told Richard that I didn't want to block him posting an update to the patchset, but that I felt it would be The Wrong Thing To Do and if he did post the patchset I would likely ignore it until after the io_uring fix had been posted so as to not draw additional attention to his changes. I can't speak for Richard's mindset, but he appeared anxious to post his changes regardless of my concerns, and he did so shortly afterwards. That's why you haven't seen much progress on this for a while, and why you will see me comment on the latest patchset after the io_uring patches land in -next after the next merge window closes. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
Le 24/08/2021 à 16:47, Paul Moore a écrit : On Tue, Aug 24, 2021 at 9:36 AM Christophe Leroy wrote: Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal targets") added generic support for AUDIT but that didn't include support for bi-arch like powerpc. Commit 4b58841149dc ("audit: Add generic compat syscall support") added generic support for bi-arch. Convert powerpc to that bi-arch generic audit support. Cc: Paul Moore Cc: Eric Paris Signed-off-by: Christophe Leroy --- Resending v2 with Audit people in Cc v2: - Missing 'git add' for arch/powerpc/include/asm/unistd32.h - Finalised commit description --- arch/powerpc/Kconfig| 5 +- arch/powerpc/include/asm/unistd32.h | 7 +++ arch/powerpc/kernel/Makefile| 3 -- arch/powerpc/kernel/audit.c | 84 - arch/powerpc/kernel/compat_audit.c | 44 --- 5 files changed, 8 insertions(+), 135 deletions(-) create mode 100644 arch/powerpc/include/asm/unistd32.h delete mode 100644 arch/powerpc/kernel/audit.c delete mode 100644 arch/powerpc/kernel/compat_audit.c Can you explain, in detail please, the testing you have done to verify this patch? I built ppc64_defconfig and checked that the generated code is functionnaly equivalent. ppc32_classify_syscall() is exactly the same as audit_classify_compat_syscall() except that the later takes the syscall as second argument (ie in r4) whereas the former takes it as first argument (ie in r3). audit_classify_arch() and powerpc audit_classify_syscall() are slightly different between the powerpc version and the generic version because the powerpc version checks whether it is AUDIT_ARCH_PPC or not (ie value 20), while the generic one checks whether it has bit __AUDIT_ARCH_64BIT set or not (__AUDIT_ARCH_64BIT is the sign bit of a word), but taking into account that the abi is either AUDIT_ARCH_PPC, AUDIT_ARCH_PPC64 or AUDIT_ARCH_PPC64LE, the result is the same. If you are asking I guess you saw something wrong ? arch/powerpc/kernel/compat_audit.o: file format elf64-powerpc Disassembly of section .text: <.ppc32_classify_syscall>: 0: 28 03 00 66 cmplwi r3,102 4: 7c 69 1b 78 mr r9,r3 8: 41 82 00 48 beq 50 <.ppc32_classify_syscall+0x50> c: 41 81 00 24 bgt 30 <.ppc32_classify_syscall+0x30> 10: 28 03 00 05 cmplwi r3,5 14: 38 60 00 02 li r3,2 18: 4d 82 00 20 beqlr 1c: 28 09 00 0b cmplwi r9,11 20: 40 82 00 38 bne 58 <.ppc32_classify_syscall+0x58> 24: 38 60 00 05 li r3,5 28: 4e 80 00 20 blr 2c: 60 00 00 00 nop 30: 28 03 01 1e cmplwi r3,286 34: 38 60 00 01 li r3,1 38: 4c 82 00 20 bnelr 3c: 38 60 00 03 li r3,3 40: 4e 80 00 20 blr 44: 60 00 00 00 nop 48: 60 00 00 00 nop 4c: 60 00 00 00 nop 50: 38 60 00 04 li r3,4 54: 4e 80 00 20 blr 58: 38 60 00 01 li r3,1 5c: 4e 80 00 20 blr lib/compat_audit.o: file format elf64-powerpc Disassembly of section .text: <.audit_classify_compat_syscall>: 0: 28 04 00 66 cmplwi r4,102 4: 41 82 00 4c beq 50 <.audit_classify_compat_syscall+0x50> 8: 41 81 00 28 bgt 30 <.audit_classify_compat_syscall+0x30> c: 28 04 00 05 cmplwi r4,5 10: 38 60 00 02 li r3,2 14: 4d 82 00 20 beqlr 18: 28 04 00 0b cmplwi r4,11 1c: 40 82 00 3c bne 58 <.audit_classify_compat_syscall+0x58> 20: 38 60 00 05 li r3,5 24: 4e 80 00 20 blr 28: 60 00 00 00 nop 2c: 60 00 00 00 nop 30: 28 04 01 1e cmplwi r4,286 34: 38 60 00 01 li r3,1 38: 4c 82 00 20 bnelr 3c: 38 60 00 03 li r3,3 40: 4e 80 00 20 blr 44: 60 00 00 00 nop 48: 60 00 00 00 nop 4c: 60 00 00 00 nop 50: 38 60 00 04 li r3,4 54: 4e 80 00 20 blr 58: 38 60 00 01 li r3,1 5c: 4e 80 00 20 blr arch/powerpc/kernel/audit.o: file format elf64-powerpc Disassembly of section .text: <.audit_classify_arch>: 0: 68 63 00 14 xorir3,r3,20 4: 7c 63 00 34 cntlzw r3,r3 8: 54 63 d9 7e rlwinm r3,r3,27,5,31 c: 4e 80 00 20 blr 0010 <.audit_classify_syscall>: 10: 2c 03 00 14 cmpwi r3,20 14: 41 82 00 5c beq 70 <.audit_classify_syscall+0x60> 18: 28 04 00 66 cmplwi r4,102 1c: 41 82 00 44 beq 60 <.audit_classify_syscall+0x50> 20: 41 81 00 20 bgt 40 <.audit_classify_syscall+0x30> 24: 28 04 00 05 cmplwi r4,5 28: 38 60 00 02 li r3,2 2c: 4d 82 00 20 beqlr 30: 28 04 00 0b cmplwi r4,11 34: 40 82 00 64 bne 98 <.audit_classify_syscall+0x88> 38: 38 60 00 05 li r3,5 3c: 4e 80 00 2
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On 2021-08-11 16:48, Paul Moore wrote: > Draft #2 of the patchset which brings auditing and proper LSM access > controls to the io_uring subsystem. The original patchset was posted > in late May and can be found via lore using the link below: > > https://lore.kernel.org/linux-security-module/162163367115.8379.8459012634106035341.stgit@sifl/ > > This draft should incorporate all of the feedback from the original > posting as well as a few smaller things I noticed while playing > further with the code. The big change is of course the selective > auditing in the io_uring op servicing, but that has already been > discussed quite a bit in the original thread so I won't go into > detail here; the important part is that we found a way to move > forward and this draft captures that. For those of you looking to > play with these patches, they are based on Linus' v5.14-rc5 tag and > on my test system they boot and appear to function without problem; > they pass the selinux-testsuite and audit-testsuite and I have not > noticed any regressions in the normal use of the system. If you want > to get a copy of these patches straight from git you can use the > "working-io_uring" branch in the repo below: > > git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > > Beyond the existing test suite tests mentioned above, I've cobbled > together some very basic, very crude tests to exercise some of the > things I care about from a LSM/audit perspective. These tests are > pretty awful (I'm not kidding), but they might be helpful for the > other LSM/audit developers who want to test things: > > https://drop.paul-moore.com/90.kUgq > > There are currently two tests: 'iouring.2' and 'iouring.3'; > 'iouring.1' was lost in a misguided and overzealous 'rm' command. > The first test is standalone and basically tests the SQPOLL > functionality while the second tests sharing io_urings across process > boundaries and the credential/personality sharing mechanism. The > console output of both tests isn't particularly useful, the more > interesting bits are in the audit and LSM specific logs. The > 'iouring.2' command requires no special arguments to run but the > 'iouring.3' test is split into a "server" and "client"; the server > should be run without argument: > > % ./iouring.3s > >>> server started, pid = 11678 > >>> memfd created, fd = 3 > >>> io_uring created; fd = 5, creds = 1 > > ... while the client should be run with two arguments: the first is > the PID of the server process, the second is the "memfd" fd number: > > % ./iouring.3c 11678 3 > >>> client started, server_pid = 11678 server_memfd = 3 > >>> io_urings = 5 (server) / 5 (client) > >>> io_uring ops using creds = 1 > >>> async op result: 36 > >>> async op result: 36 > >>> async op result: 36 > >>> async op result: 36 > >>> START file contents > What is this life if, full of care, > we have no time to stand and stare. > >>> END file contents > > The tests were hacked together from various sources online, > attribution and links to additional info can be found in the test > sources, but I expect these tests to die a fiery death in the not > to distant future as I work to add some proper tests to the SELinux > and audit test suites. > > As I believe these patches should spend a full -rcX cycle in > linux-next, my current plan is to continue to solicit feedback on > these patches while they undergo additional testing (next up is > verification of the audit filter code for io_uring). Assuming no > critical issues are found on the mailing lists or during testing, I > will post a proper patchset later with the idea of merging it into > selinux/next after the upcoming merge window closes. > > Any comments, feedback, etc. are welcome. Thanks for the tests. I have a bunch of userspace patches to add to the last set I posted and these tests will help exercise them. I also have one more kernel patch to post... I'll dive back into that now. I had wanted to post them before now but got distracted with AUDIT_TRIM breakage. > --- > > Casey Schaufler (1): > Smack: Brutalist io_uring support with debug > > Paul Moore (8): > audit: prepare audit_context for use in calling contexts beyond > syscalls > audit,io_uring,io-wq: add some basic audit support to io_uring > audit: dev/test patch to force io_uring auditing > audit: add filtering for io_uring records > fs: add anon_inode_getfile_secure() similar to > anon_inode_getfd_secure() > io_uring: convert io_uring to the secure anon inode interface > lsm,io_uring: add LSM hooks to io_uring > selinux: add support for the io_uring access controls > > > fs/anon_inodes.c| 29 ++ > fs/io-wq.c | 4 + > fs/io_uring.c | 69 +++- > include/linux/anon_inodes.h |
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On Tue, Aug 24, 2021 at 4:57 PM Richard Guy Briggs wrote: > Thanks for the tests. I have a bunch of userspace patches to add to the > last set I posted and these tests will help exercise them. I also have > one more kernel patch to post... I'll dive back into that now. I had > wanted to post them before now but got distracted with AUDIT_TRIM > breakage. If it helps, last week I started working on a little test tool for the audit-testsuite and selinux-testsuite (see attached). It may not be final, but I don't expect too many changes to it before I post the test suite patches; it is definitely usable now. It's inspired by the previous tests, but it uses a much more test suite friendly fork/exec model for testing the sharing of io_urings across process boundaries. Would you mind sharing your latest userspace patches, if not publicly I would be okay with privately off-list; I'm putting together the test suite patches this week and it would be good to make sure I'm using your latest take on the userspace changes. Also, what is the kernel patch? Did you find a bug or is this some new functionality you think might be useful? Both can be important, but the bug is *really* important; even if you don't have a fix for that, just a description of the problem would be good. -- paul moore www.paul-moore.com /* * io_uring test tool to exercise LSM/SELinux and audit kernel code paths * Author: Paul Moore * * Copyright 2021 Microsoft Corporation * * At the time this code was written the best, and most current, source of info * on io_uring seemed to be the liburing sources themselves (link below). The * code below is based on the lessons learned from looking at the liburing * code. * * -> https://github.com/axboe/liburing * * The liburing LICENSE file contains the following: * * Copyright 2020 Jens Axboe * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to * deal in the Software without restriction, including without limitation the * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or * sell copies of the Software, and to permit persons to whom the Software is * furnished to do so, subject to the following conditions: * * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER * DEALINGS IN THE SOFTWARE. * */ /* * BUILDING: * * gcc -o -g -O0 -luring -lrt * * RUNNING: * * The program can be run using the following command lines: * * % prog sqpoll * ... this invocation runs the io_uring SQPOLL test. * * % prog t1 * ... this invocation runs the parent/child io_uring sharing test. * * % prog t1 * ... this invocation runs the parent/child io_uring sharing test with the * child process run in the specified SELinux domain. * */ #include #include #include #include #include #include #include #include #include #include struct urt_config { struct io_uring ring; struct io_uring_params ring_params; int ring_creds; }; #define URING_ENTRIES8 #define URING_SHM_NAME"/iouring_test_4" int selinux_state = -1; #define SELINUX_CTX_MAX512 char selinux_ctx[SELINUX_CTX_MAX] = "\0"; /** * Display an error message and exit * @param msg the error message * * Output @msg to stderr and exit with errno as the exit value. */ void fatal(const char *msg) { const char *str = (msg ? msg : "unknown"); if (!errno) { errno = 1; fprintf(stderr, "%s: unknown error\n", msg); } else perror(str); if (errno < 0) exit(-errno); exit(errno); } /** * Determine if SELinux is enabled and set the internal state * * Attempt to read from /proc/self/attr/current and determine if SELinux is * enabled, store the current context/domain in @selinux_ctx if SELinux is * enabled. We avoid using the libselinux API in order to increase portability * and make it easier for other LSMs to adopt this test. */ int selinux_enabled(void) { int fd = -1; ssize_t ctx_len; char ctx[SELINUX_CTX_MAX]; if (selinux_state >= 0) return selinux_state; /* attempt to get the current context */ fd = open("/proc/self/attr/current", O_RDONLY); if (fd < 0) goto err; ctx_len = read(fd, ctx, SELINUX_CTX_MAX - 1); if (ctx_len <= 0) goto err; close(fd); /* save the current context */ ctx[ctx_len] = '\0'; strcpy(selinux_ctx, ctx); selinux_state = 1; return selinux_state; err: if (fd >= 0) close(fd); selin
Re: [ghak-trim PATCH v1] audit: move put_tree() to avoid trim_trees refcount underflow and UAF
On Mon, Aug 23, 2021 at 10:05 PM Richard Guy Briggs wrote: > > AUDIT_TRIM is expected to be idempotent, but multiple executions resulted in a > refcount underflow and use-after-free. > > git bisect fingered commit fb041bb7c0a918b95c6889fc965cdc4a75b4c0ca (2019-11) > ("locking/refcount: Consolidate implementations of refcount_t") > but this patch with its more thorough checking that wasn't in the x86 assembly > code merely exposed a previously existing tree refcount imbalance in the case > of tree trimming code that was refactored with prune_one() to remove a tree > introduced in commit 8432c70062978d9a57bde6715496d585ec520c3e (2018-11) > ("audit: Simplify locking around untag_chunk()") > > Move the put_tree() to cover only the prune_one() case. > > Passes audit-testsuite and 3 passes of "auditctl -t" with at least one > directory watch. > > Fixes: 8432c7006297 ("audit: Simplify locking around untag_chunk()") > Signed-off-by: Richard Guy Briggs > Cc: Jan Kara > Cc: Will Deacon > Cc: Alexander Viro > Cc: Seiji Nishikawa > --- > kernel/audit_tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) First a quick comment about the commit description, when referencing specific commits in the description please use the same format that you used in the "Fixes:" tag; it makes the description easier to read. No need to resend, I fixed it when I merged your patch, but something to keep in mind for the future. As for the patch itself, thanks for finding this and sending a fix. Normally this is something I would send up to Linus at the end of the week during the -rcX phase, but since we are currently at -rc7 I'm going to simply add the -stable marking and merge it into audit/next to get pushed up to Linus early next week, assuming we see v5.14 released this Sunday. If for some reason we see a v5.14-rc8 next week I'll adjust things and send it to Linus as a -stable patch. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 RESEND] powerpc/audit: Convert powerpc to AUDIT_ARCH_COMPAT_GENERIC
On Tue, Aug 24, 2021 at 1:11 PM Christophe Leroy wrote: > Le 24/08/2021 à 16:47, Paul Moore a écrit : > > On Tue, Aug 24, 2021 at 9:36 AM Christophe Leroy > > wrote: > >> > >> Commit e65e1fc2d24b ("[PATCH] syscall class hookup for all normal > >> targets") added generic support for AUDIT but that didn't include > >> support for bi-arch like powerpc. > >> > >> Commit 4b58841149dc ("audit: Add generic compat syscall support") > >> added generic support for bi-arch. > >> > >> Convert powerpc to that bi-arch generic audit support. > >> > >> Cc: Paul Moore > >> Cc: Eric Paris > >> Signed-off-by: Christophe Leroy > >> --- > >> Resending v2 with Audit people in Cc > >> > >> v2: > >> - Missing 'git add' for arch/powerpc/include/asm/unistd32.h > >> - Finalised commit description > >> --- > >> arch/powerpc/Kconfig| 5 +- > >> arch/powerpc/include/asm/unistd32.h | 7 +++ > >> arch/powerpc/kernel/Makefile| 3 -- > >> arch/powerpc/kernel/audit.c | 84 - > >> arch/powerpc/kernel/compat_audit.c | 44 --- > >> 5 files changed, 8 insertions(+), 135 deletions(-) > >> create mode 100644 arch/powerpc/include/asm/unistd32.h > >> delete mode 100644 arch/powerpc/kernel/audit.c > >> delete mode 100644 arch/powerpc/kernel/compat_audit.c > > > > Can you explain, in detail please, the testing you have done to verify > > this patch? > > > > I built ppc64_defconfig and checked that the generated code is functionnaly > equivalent. > > ppc32_classify_syscall() is exactly the same as > audit_classify_compat_syscall() except that the > later takes the syscall as second argument (ie in r4) whereas the former > takes it as first argument > (ie in r3). > > audit_classify_arch() and powerpc audit_classify_syscall() are slightly > different between the > powerpc version and the generic version because the powerpc version checks > whether it is > AUDIT_ARCH_PPC or not (ie value 20), while the generic one checks whether it > has bit > __AUDIT_ARCH_64BIT set or not (__AUDIT_ARCH_64BIT is the sign bit of a word), > but taking into > account that the abi is either AUDIT_ARCH_PPC, AUDIT_ARCH_PPC64 or > AUDIT_ARCH_PPC64LE, the result is > the same. > > If you are asking I guess you saw something wrong ? I was asking because I didn't see any mention of testing, and when you are enabling something significant like this it is nice to see that it has been verified to work :) While binary dumps and comparisons are nice, it is always good to see verification from a test suite. I don't have access to the necessary hardware to test this, but could you verify that the audit-testsuite passes on your test system with your patches applied? * https://github.com/linux-audit/audit-testsuite -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/9] audit, io_uring, io-wq: add some basic audit support to io_uring
On 2021-06-02 13:46, Paul Moore wrote: > 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), >
Re: [RFC PATCH v2 0/9] Add LSM access controls and auditing to io_uring
On 2021-08-24 18:27, Paul Moore wrote: > On Tue, Aug 24, 2021 at 4:57 PM Richard Guy Briggs wrote: > > Thanks for the tests. I have a bunch of userspace patches to add to the > > last set I posted and these tests will help exercise them. I also have > > one more kernel patch to post... I'll dive back into that now. I had > > wanted to post them before now but got distracted with AUDIT_TRIM > > breakage. > > If it helps, last week I started working on a little test tool for the > audit-testsuite and selinux-testsuite (see attached). It may not be > final, but I don't expect too many changes to it before I post the > test suite patches; it is definitely usable now. It's inspired by the > previous tests, but it uses a much more test suite friendly fork/exec > model for testing the sharing of io_urings across process boundaries. > > Would you mind sharing your latest userspace patches, if not publicly > I would be okay with privately off-list; I'm putting together the test > suite patches this week and it would be good to make sure I'm using > your latest take on the userspace changes. I intend to publish them but they need squashing and some documentation first. And a run through with io_uring specific tests would be good to catch anything obvious... > Also, what is the kernel patch? Did you find a bug or is this some > new functionality you think might be useful? Both can be important, > but the bug is *really* important; even if you don't have a fix for > that, just a description of the problem would be good. It was a very small patch that I realize I had already talked about and you justified not including sessionid along with auid. That was addressed in a reply tacked on to your v1 patchset just now. > paul moore > /* > * io_uring test tool to exercise LSM/SELinux and audit kernel code paths > * Author: Paul Moore > * > * Copyright 2021 Microsoft Corporation > * > * At the time this code was written the best, and most current, source of > info > * on io_uring seemed to be the liburing sources themselves (link below). The > * code below is based on the lessons learned from looking at the liburing > * code. > * > * -> https://github.com/axboe/liburing > * > * The liburing LICENSE file contains the following: > * > * Copyright 2020 Jens Axboe > * > * Permission is hereby granted, free of charge, to any person obtaining a > copy > * of this software and associated documentation files (the "Software"), to > * deal in the Software without restriction, including without limitation the > * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or > * sell copies of the Software, and to permit persons to whom the Software is > * furnished to do so, subject to the following conditions: > * > * The above copyright notice and this permission notice shall be included in > * all copies or substantial portions of the Software. > * > * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > THE > * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > * DEALINGS IN THE SOFTWARE. > * > */ > > /* > * BUILDING: > * > * gcc -o -g -O0 -luring -lrt > * > * RUNNING: > * > * The program can be run using the following command lines: > * > * % prog sqpoll > * ... this invocation runs the io_uring SQPOLL test. > * > * % prog t1 > * ... this invocation runs the parent/child io_uring sharing test. > * > * % prog t1 > * ... this invocation runs the parent/child io_uring sharing test with the > * child process run in the specified SELinux domain. > * > */ > > #include > #include > #include > #include > #include > #include > #include > #include > #include > > #include > > struct urt_config { > struct io_uring ring; > struct io_uring_params ring_params; > int ring_creds; > }; > > #define URING_ENTRIES 8 > #define URING_SHM_NAME"/iouring_test_4" > > int selinux_state = -1; > #define SELINUX_CTX_MAX 512 > char selinux_ctx[SELINUX_CTX_MAX] = "\0"; > > /** > * Display an error message and exit > * @param msg the error message > * > * Output @msg to stderr and exit with errno as the exit value. > */ > void fatal(const char *msg) > { > const char *str = (msg ? msg : "unknown"); > > if (!errno) { > errno = 1; > fprintf(stderr, "%s: unknown error\n", msg); > } else > perror(str); > > if (errno < 0) > exit(-errno); > exit(errno); > } > > /** > * Determine if SELinux is enabled and set the int