Re: [PATCH v5 4/4] vduse: Add LSM hook to check Virtio device type
On Mon, Dec 18, 2023 at 12:21 PM Stephen Smalley wrote: > On Tue, Dec 12, 2023 at 8:17 AM Maxime Coquelin > wrote: > > This patch introduces a LSM hook for devices creation, > > destruction (ioctl()) and opening (open()) operations, > > checking the application is allowed to perform these > > operations for the Virtio device type. > > Can you explain why the existing LSM hooks and SELinux implementation > are not sufficient? We already control the ability to open device > nodes via selinux_inode_permission() and selinux_file_open(), and can > support fine-grained per-cmd ioctl checking via selinux_file_ioctl(). > And it should already be possible to label these nodes distinctly > through existing mechanisms (file_contexts if udev-created/labeled, > genfs_contexts if kernel-created). What exactly can't you do today > that this hook enables? I asked something similar back in the v4 patchset to see if there was some special labeling concerns that required the use of a dedicated hook and from what I can see there are none. > Other items to consider: > - If vduse devices are created using anonymous inodes, then SELinux > grew a general facility for labeling and controlling the creation of > those via selinux_inode_init_security_anon(). For the vduse folks, the associated LSM API function is security_inode_init_security_anon(); please don't call into SELinux directly. > - You can encode information about the device into its SELinux type > that then allows you to distinguish things like net vs block based on > the device's SELinux security context rather than encoding that in the > permission bits. > - If you truly need new LSM hooks (which you need to prove first), > then you should pass some usable information about the object in > question to allow SELinux to find a security context for it. Like an > inode associated with the device, for example. I agree with Stephen and I still remain skeptical that these hooks are needed. Until I see a compelling case as to why the existing LSM hooks are not sufficient I can't ACK these hooks. -- paul-moore.com
Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type
On Oct 20, 2023 "Michael S. Tsirkin" wrote:
>
> This patch introduces LSM hooks for devices creation,
> destruction and opening operations, checking the
> application is allowed to perform these operations for
> the Virtio device type.
>
> Signed-off-by: Maxime Coquelin
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 12 +++
> include/linux/lsm_hook_defs.h | 4 +++
> include/linux/security.h| 15
> security/security.c | 42 ++
> security/selinux/hooks.c| 55 +
> security/selinux/include/classmap.h | 2 ++
> 6 files changed, 130 insertions(+)
My apologies for the late reply, I've been trying to work my way through
the review backlog but it has been taking longer than expected; comments
below ...
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2aa0e219d721..65d9262a37f7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -21,6 +21,7 @@
> * Copyright (C) 2016 Mellanox Technologies
> */
>
> +#include "av_permissions.h"
> #include
> #include
> #include
> @@ -92,6 +93,7 @@
> #include
> #include
> #include
> +#include
>
> #include "avc.h"
> #include "objsec.h"
> @@ -6950,6 +6952,56 @@ static int selinux_uring_cmd(struct io_uring_cmd
> *ioucmd)
> }
> #endif /* CONFIG_IO_URING */
>
> +static int vduse_check_device_type(u32 sid, u32 device_id)
> +{
> + u32 requested;
> +
> + if (device_id == VIRTIO_ID_NET)
> + requested = VDUSE__NET;
> + else if (device_id == VIRTIO_ID_BLOCK)
> + requested = VDUSE__BLOCK;
> + else
> + return -EINVAL;
> +
> + return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested, NULL);
> +}
> +
> +static int selinux_vduse_dev_create(u32 device_id)
> +{
> + u32 sid = current_sid();
> + int ret;
> +
> + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVCREATE, NULL);
> + if (ret)
> + return ret;
> +
> + return vduse_check_device_type(sid, device_id);
> +}
I see there has been some discussion about the need for a dedicated
create hook as opposed to using the existing ioctl controls. I think
one important point that has been missing from the discussion is the
idea of labeling the newly created device. Unfortunately prior to a
few minutes ago I hadn't ever looked at VDUSE so please correct me if
I get some things wrong :)
>From what I can see userspace creates a new VDUSE device with
ioctl(VDUSE_CREATE_DEV), which trigger the creation of a new
/dev/vduse/XXX device which will be labeled according to the udev
and SELinux configuration, likely with a generic udev label. My
question is if we want to be able to uniquely label each VDUSE
device based on the process that initiates the device creation
with the call to ioctl()? If that is the case, we would need a
create hook not only to control the creation of the device, but to
record the triggering process' label in the new device; this label
would then be used in subsequent VDUSE open and destroy operations.
The normal device file I/O operations would still be subject to the
standard SELinux file I/O permissions using the device file label
assigned by systemd/udev when the device was created.
> +static int selinux_vduse_dev_destroy(u32 device_id)
> +{
> + u32 sid = current_sid();
> + int ret;
> +
> + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVDESTROY, NULL);
> + if (ret)
> + return ret;
> +
> + return vduse_check_device_type(sid, device_id);
> +}
> +
> +static int selinux_vduse_dev_open(u32 device_id)
> +{
> + u32 sid = current_sid();
> + int ret;
> +
> + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVOPEN, NULL);
> + if (ret)
> + return ret;
> +
> + return vduse_check_device_type(sid, device_id);
> +}
> +
> /*
> * IMPORTANT NOTE: When adding new hooks, please be careful to keep this
> order:
> * 1. any hooks that don't belong to (2.) or (3.) below,
> @@ -7243,6 +7295,9 @@ static struct security_hook_list selinux_hooks[]
> __ro_after_init = {
> #ifdef CONFIG_PERF_EVENTS
> LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
> #endif
> + LSM_HOOK_INIT(vduse_dev_create, selinux_vduse_dev_create),
> + LSM_HOOK_INIT(vduse_dev_destroy, selinux_vduse_dev_destroy),
> + LSM_HOOK_INIT(vduse_dev_open, selinux_vduse_dev_open),
> };
>
> static __init int selinux_init(void)
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index a3c380775d41..d3dc37fb03d4 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -256,6 +256,8 @@ const struct security_class_mapping secclass_map[] = {
> { "override_creds", "sqpoll", "cmd", NULL } },
> { "user_namespace",
> { "create", NULL } },
> + { "vduse",
> + { "devcreate", "devdestroy", "devopen", "net",
Re: [PATCH] selinux: Annotate struct sidtab_str_cache with __counted_by
On Aug 17, 2023 Kees Cook wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct sidtab_str_cache. > > [1] > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Paul Moore > Cc: Stephen Smalley > Cc: Eric Paris > Cc: Ondrej Mosnacek > Cc: seli...@vger.kernel.org > Signed-off-by: Kees Cook > --- > security/selinux/ss/sidtab.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged into selinux/next, thanks. -- paul-moore.com
Re: [PATCH] audit: Annotate struct audit_chunk with __counted_by
On Aug 17, 2023 Paul Moore wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct audit_chunk. > > [1] > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Paul Moore > Cc: Eric Paris > Cc: au...@vger.kernel.org > Signed-off-by: Kees Cook > --- > kernel/audit_tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged into audit/next, thanks. -- paul-moore.com
[GIT PULL] SELinux fixes for v5.12 (#2)
Hi Linus, I realize we are getting late in the v5.12-rcX release cycle, but we have three SELinux patches which I believe should be merged before the proper v5.12 release. The patches fix known problems relating to (re)loading SELinux policy or changing the policy booleans, and pass our test suite without problem. As of a few minutes ago, the tag below also merged cleanly into your tree. Please pull for the next v5.12-rcX release, thanks. -Paul -- The following changes since commit ee5de60a08b7d8d255722662da461ea159c15538: selinuxfs: unify policy load error reporting (2021-03-18 23:26:59 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20210409 for you to fetch changes up to 9ad6e9cb39c66366bf7b9aece114aca277981a1f: selinux: fix race between old and new sidtab (2021-04-07 20:42:56 -0400) selinux/stable-5.12 PR 20210409 Ondrej Mosnacek (3): selinux: make nslot handling in avtab more robust selinux: fix cond_list corruption when changing booleans selinux: fix race between old and new sidtab security/selinux/ss/avtab.c | 101 security/selinux/ss/avtab.h | 2 +- security/selinux/ss/conditional.c | 12 +-- security/selinux/ss/services.c| 157 +++--- security/selinux/ss/sidtab.c | 21 + security/selinux/ss/sidtab.h | 4 + 6 files changed, 185 insertions(+), 112 deletions(-) -- paul moore www.paul-moore.com
Re: [BUG] Oops in sidtab_context_to_sid
On Sat, Apr 3, 2021 at 11:21 AM Ondrej Mosnacek wrote: > On Sat, Apr 3, 2021 at 4:33 PM Paul Moore wrote: > > On Fri, Apr 2, 2021 at 6:35 PM Vijay Balakrishna > > wrote: > > > > > > Seeing oops in 5.4.83 sidtab_context_to_sid(). I checked with Tyler > > > (copied), he said it might be > > > > > > https://lore.kernel.org/selinux/CAFqZXNu8s5edDbSZuSutetTsy58i08vPuP2h-n9=kt34hcp...@mail.gmail.com/ > > > > > > Ondrej, can you confirm? Unfortunately, we don't have a on demand repro. > > > > I'm guessing this may be the problem that Tyler reported earlier and > > which appeared to be fixed by the patch below: > > > > https://lore.kernel.org/selinux/20210318215303.2578052-3-omosn...@redhat.com > > Nope, if that's really 5.4.83 with no extra backports, then it can't > be this issue as it has been introduced only in v5.10. Of course, good catch. -- paul moore www.paul-moore.com
Re: [BUG] Oops in sidtab_context_to_sid
On Fri, Apr 2, 2021 at 6:35 PM Vijay Balakrishna wrote: > > Seeing oops in 5.4.83 sidtab_context_to_sid(). I checked with Tyler > (copied), he said it might be > > https://lore.kernel.org/selinux/CAFqZXNu8s5edDbSZuSutetTsy58i08vPuP2h-n9=kt34hcp...@mail.gmail.com/ > > Ondrej, can you confirm? Unfortunately, we don't have a on demand repro. I'm guessing this may be the problem that Tyler reported earlier and which appeared to be fixed by the patch below: https://lore.kernel.org/selinux/20210318215303.2578052-3-omosn...@redhat.com ... which was merged into Linus' tree during the v5.12-rcX development phase, any chance you could try that patch to see if it resolves your issue? There are still some issues to be sorted out, but if you aren't reloading policy it shouldn't be a concern. Tyler, since both of you are at Microsoft, do you have a patched kernel that Vijay could try? -- paul moore www.paul-moore.com
Re: [PATCH] audit: drop /proc/PID/loginuid documentation Format field
On Thu, Apr 1, 2021 at 3:11 PM Richard Guy Briggs wrote: > > Drop the "Format:" field from the /proc/PID/loginuid documentation and > integrate the information into the Description field since it is not > recognized by the "./scripts/get_abi.pl validate" command which causes a > warning. Documentation/ABI/README describes the valid fields. > > Reported-by: Mauro Carvalho Chehab > Signed-off-by: Richard Guy Briggs > --- > .../ABI/stable/procfs-audit_loginuid | 22 +-- > 1 file changed, 11 insertions(+), 11 deletions(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com
Re: [PATCH v3 1/2] audit: document /proc/PID/loginuid
On Thu, Apr 1, 2021 at 9:48 AM Mauro Carvalho Chehab
wrote:
> Em Thu, 18 Mar 2021 15:19:10 -0400
> Richard Guy Briggs escreveu:
> > Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> > was added 2005-02-01 by commit 1e2d1492e178 ("[PATCH] audit: handle
> > loginuid through proc")
> >
> > Signed-off-by: Richard Guy Briggs
> > ---
> > Documentation/ABI/stable/procfs-audit_loginuid | 15 +++
> > 1 file changed, 15 insertions(+)
> > create mode 100644 Documentation/ABI/stable/procfs-audit_loginuid
> >
> > diff --git a/Documentation/ABI/stable/procfs-audit_loginuid
> > b/Documentation/ABI/stable/procfs-audit_loginuid
> > new file mode 100644
> > index ..e7c100b9ab18
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/procfs-audit_loginuid
> > @@ -0,0 +1,15 @@
> > +What:Audit Login UID
> > +Date:2005-02-01
> > +KernelVersion: 2.6.11-rc2 1e2d1492e178 ("[PATCH] audit: handle
> > loginuid through proc")
> > +Contact: linux-au...@redhat.com
> > +Format: %u
>
> The ABI definition doesn't include a "Format:" symbol. See:
>
> Documentation/ABI/README
>
> For the valid ones.
>
> This change causes a warning at the ABI parser:
>
>
> $ ./scripts/get_abi.pl validate
> Warning: file Documentation/ABI/stable/procfs-audit_loginuid#5:
> tag 'contact' is invalid. Line
> Format: %u
> Warning: file Documentation/ABI/stable/procfs-audit_loginuid#21:
> tag 'contact' is invalid. Line
> Format: %u
>
> You should either drop it or add it to the parser and to the README
> file, if the ABI maintainers are ok with such new field.
Thanks Mauro, I didn't realize there were tools that parsed these files.
Richard, please post a patch that drops the 'Format:' line from the
newly added audit files as soon as possible so I can merge it into
audit/next.
--
paul moore
www.paul-moore.com
Re: [PATCH v5] audit: log nftables configuration change events once per table
On Tue, Mar 30, 2021 at 6:53 PM Pablo Neira Ayuso wrote: > On Sun, Mar 28, 2021 at 08:50:45PM -0400, Paul Moore wrote: > [...] > > Netfilter folks, were you planning to pull this via your tree/netdev > > or would you like me to merge this via the audit tree? If the latter, > > I would appreciate it if I could get an ACK from one of you; if the > > former, my ACK is below. > > > > Acked-by: Paul Moore > > I'll merge this one into nf-next, this might simplify possible > conflict resolution later on. Yep, I think that's the best choice. Thanks. -- paul moore www.paul-moore.com
Re: [PATCH v5] audit: log nftables configuration change events once per table
On Fri, Mar 26, 2021 at 1:39 PM Richard Guy Briggs wrote:
>
> Reduce logging of nftables events to a level similar to iptables.
> Restore the table field to list the table, adding the generation.
>
> Indicate the op as the most significant operation in the event.
>
> A couple of sample events:
>
> type=PROCTITLE msg=audit(2021-03-18 09:30:49.801:143) :
> proctitle=/usr/bin/python3 -s /usr/sbin/firewalld --nofork --nopid
> type=SYSCALL msg=audit(2021-03-18 09:30:49.801:143) : arch=x86_64
> syscall=sendmsg success=yes exit=172 a0=0x6 a1=0x7ffdcfcbe650 a2=0x0
> a3=0x7ffdcfcbd52c items=0 ppid=1 pid=367 auid=unset uid=root gid=root
> euid=root suid=root fsuid=root egid=roo
> t sgid=root fsgid=root tty=(none) ses=unset comm=firewalld
> exe=/usr/bin/python3.9 subj=system_u:system_r:firewalld_t:s0 key=(null)
> type=NETFILTER_CFG msg=audit(2021-03-18 09:30:49.801:143) : table=firewalld:2
> family=ipv6 entries=1 op=nft_register_table pid=367
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> type=NETFILTER_CFG msg=audit(2021-03-18 09:30:49.801:143) : table=firewalld:2
> family=ipv4 entries=1 op=nft_register_table pid=367
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> type=NETFILTER_CFG msg=audit(2021-03-18 09:30:49.801:143) : table=firewalld:2
> family=inet entries=1 op=nft_register_table pid=367
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>
> type=PROCTITLE msg=audit(2021-03-18 09:30:49.839:144) :
> proctitle=/usr/bin/python3 -s /usr/sbin/firewalld --nofork --nopid
> type=SYSCALL msg=audit(2021-03-18 09:30:49.839:144) : arch=x86_64
> syscall=sendmsg success=yes exit=22792 a0=0x6 a1=0x7ffdcfcbe650 a2=0x0
> a3=0x7ffdcfcbd52c items=0 ppid=1 pid=367 auid=unset uid=root gid=root
> euid=root suid=root fsuid=root egid=r
> oot sgid=root fsgid=root tty=(none) ses=unset comm=firewalld
> exe=/usr/bin/python3.9 subj=system_u:system_r:firewalld_t:s0 key=(null)
> type=NETFILTER_CFG msg=audit(2021-03-18 09:30:49.839:144) : table=firewalld:3
> family=ipv6 entries=30 op=nft_register_chain pid=367
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> type=NETFILTER_CFG msg=audit(2021-03-18 09:30:49.839:144) : table=firewalld:3
> family=ipv4 entries=30 op=nft_register_chain pid=367
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> type=NETFILTER_CFG msg=audit(2021-03-18 09:30:49.839:144) : table=firewalld:3
> family=inet entries=165 op=nft_register_chain pid=367
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>
> The issue was originally documented in
> https://github.com/linux-audit/audit-kernel/issues/124
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v5:
> (sorry for all the noise...)
> - fix kbuild missing prototype warning in
> nf_tables_commit_audit_{alloc,collect,log}()
>
> v4:
> - move nf_tables_commit_audit_log() before nf_tables_commit_release() [fw]
> - move nft2audit_op[] from audit.h to nf_tables_api.c
>
> v3:
> - fix function braces, reduce parameter scope [pna]
> - pre-allocate nft_audit_data per table in step 1, bail on ENOMEM [pna]
>
> v2:
> - convert NFT ops to array indicies in nft2audit_op[] [ps]
> - use linux lists [pna]
> - use functions for each of collection and logging of audit data [pna]
> ---
> net/netfilter/nf_tables_api.c | 187 +++---
> 1 file changed, 104 insertions(+), 83 deletions(-)
Netfilter folks, were you planning to pull this via your tree/netdev
or would you like me to merge this via the audit tree? If the latter,
I would appreciate it if I could get an ACK from one of you; if the
former, my ACK is below.
Acked-by: Paul Moore
--
paul moore
www.paul-moore.com
Re: [PATCH v3] audit: log nftables configuration change events once per table
[NFT_MSG_DELOBJ]= AUDIT_NFT_OP_OBJ_UNREGISTER, > + [NFT_MSG_GETOBJ_RESET] = AUDIT_NFT_OP_OBJ_RESET, > + [NFT_MSG_NEWFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_REGISTER, > + [NFT_MSG_GETFLOWTABLE] = AUDIT_NFT_OP_INVALID, > + [NFT_MSG_DELFLOWTABLE] = AUDIT_NFT_OP_FLOWTABLE_UNREGISTER, > +}; The previously reported problem with this as a static still exists, correct? It does seem like this should live in nf_tables_api.c doesn't it? -- paul moore www.paul-moore.com
Re: [PATCH] [v2] audit: avoid -Wempty-body warning
On Mon, Mar 22, 2021 at 12:28 PM Arnd Bergmann wrote:
>
> From: Arnd Bergmann
>
> gcc warns about an empty statement when audit_remove_mark is defined to
> nothing:
>
> kernel/auditfilter.c: In function 'audit_data_to_entry':
> kernel/auditfilter.c:609:51: error: suggest braces around empty body in an
> 'if' statement [-Werror=empty-body]
> 609 | audit_remove_mark(entry->rule.exe); /* that's the
> template one */
> | ^
>
> Change the macros to use the usual "do { } while (0)" instead, and change a
> few more that were (void)0, for consistency.
>
> Signed-off-by: Arnd Bergmann
> ---
> v2: convert two more macros
> ---
> kernel/audit.h | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
Merged into audit/next, thanks.
--
paul moore
www.paul-moore.com
[GIT PULL] SELinux fixes for v5.12 (#1)
Hi Linus, Three SELinux patches to address problems in v5.12, and earlier, kernels: * Fix a problem where a local variable is used outside its associated function. Thankfully this can only be triggered by reloading the SELinux policy, which is a restricted operation for other obvious reasons. * Fix some incorrect, and inconsistent, audit and printk messages when loading the SELinux policy. All three patches are relatively minor and have been through our testing with no failures, please merge them for the next v5.12-rcX release. Thanks. -- The following changes since commit 365982aba1f264dba26f0908700d62bfa046918c: fs: anon_inodes: rephrase to appropriate kernel-doc (2021-01-15 12:17:25 -0500) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20210322 for you to fetch changes up to ee5de60a08b7d8d255722662da461ea159c15538: selinuxfs: unify policy load error reporting (2021-03-18 23:26:59 -0400) selinux/stable-5.12 PR 20210322 Ondrej Mosnacek (3): selinux: don't log MAC_POLICY_LOAD record on failed policy load selinux: fix variable scope issue in live sidtab conversion selinuxfs: unify policy load error reporting security/selinux/include/security.h | 15 ++--- security/selinux/selinuxfs.c| 22 ++--- security/selinux/ss/services.c | 63 + 3 files changed, 59 insertions(+), 41 deletions(-) -- paul moore www.paul-moore.com
Re: [PATCH v3 2/2] audit: document /proc/PID/sessionid
On Thu, Mar 18, 2021 at 3:19 PM Richard Guy Briggs wrote:
>
> Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> was added 2008-03-13 in commit 1e0bd7550ea9 ("[PATCH] export sessionid
> alongside the loginuid in procfs")
>
> Signed-off-by: Richard Guy Briggs
> ---
> Documentation/ABI/stable/procfs-audit_loginuid | 12
> 1 file changed, 12 insertions(+)
Merged into audit/next, thanks.
--
paul moore
www.paul-moore.com
Re: [PATCH v3 1/2] audit: document /proc/PID/loginuid
On Thu, Mar 18, 2021 at 3:19 PM Richard Guy Briggs wrote:
>
> Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> was added 2005-02-01 by commit 1e2d1492e178 ("[PATCH] audit: handle
> loginuid through proc")
>
> Signed-off-by: Richard Guy Briggs
> ---
> Documentation/ABI/stable/procfs-audit_loginuid | 15 +++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/ABI/stable/procfs-audit_loginuid
Merged into audit/next, thanks.
--
paul moore
www.paul-moore.com
Re: [PATCH v2 2/2] audit: document /proc/PID/sessionid
On Wed, Mar 17, 2021 at 9:51 PM Richard Guy Briggs wrote:
>
> Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> was added 2008-03-13 in commit 1e0bd7550ea9 ("[PATCH] export sessionid
> alongside the loginuid in procfs")
>
> Signed-off-by: Richard Guy Briggs
> ---
> Documentation/ABI/stable/procfs-audit_loginuid | 12
> 1 file changed, 12 insertions(+)
The same comments from patch 1/2 apply here.
> diff --git a/Documentation/ABI/stable/procfs-audit_loginuid
> b/Documentation/ABI/stable/procfs-audit_loginuid
> index 013bc1d74854..5d09637a4ae2 100644
> --- a/Documentation/ABI/stable/procfs-audit_loginuid
> +++ b/Documentation/ABI/stable/procfs-audit_loginuid
> @@ -13,3 +13,15 @@ Description:
> AUDIT_FEATURE_LOGINUID_IMMUTABLE is enabled. It cannot be
> unset if AUDIT_FEATURE_ONLY_UNSET_LOGINUID is enabled.
>
> +
> +What: Audit Login Session ID
> +Date: 2008-03-13
> +KernelVersion: 2.6.25-rc7 1e0bd7550ea9 ("[PATCH] export sessionid alongside
> the loginuid in procfs")
> +Contact: linux-au...@redhat.com
> +Format:%u (u32)
> +Users: auditd, libaudit, audit-testsuite, login
> +Description:
> + The /proc/$pid/sessionid pseudofile is read to get the
> + audit login session ID of process $pid. It is set
> + automatically, serially assigned with each new login.
> +
--
paul moore
www.paul-moore.com
Re: [PATCH v2 1/2] audit: document /proc/PID/loginuid
On Wed, Mar 17, 2021 at 9:51 PM Richard Guy Briggs wrote:
>
> Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> was added 2005-02-01 by commit 1e2d1492e178 ("[PATCH] audit: handle
> loginuid through proc")
>
> Signed-off-by: Richard Guy Briggs
> ---
> Documentation/ABI/stable/procfs-audit_loginuid | 15 +++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/ABI/stable/procfs-audit_loginuid
>
> diff --git a/Documentation/ABI/stable/procfs-audit_loginuid
> b/Documentation/ABI/stable/procfs-audit_loginuid
> new file mode 100644
> index ..013bc1d74854
> --- /dev/null
> +++ b/Documentation/ABI/stable/procfs-audit_loginuid
> @@ -0,0 +1,15 @@
> +What: Audit Login UID
> +Date: 2005-02-01
> +KernelVersion: 2.6.11-rc2 1e2d1492e178 ("[PATCH] audit: handle loginuid
> through proc")
> +Contact: linux-au...@redhat.com
> +Format:%u (u32)
Existing examples seem to just use the printf format specifier, e.g.
"%u", without the explicit type, e.g. "u32", which seems cleanest to
me. I would suggest changing this to just "Format: %u" to better fit
existing convention.
> +Users: auditd, libaudit, audit-testsuite, login
I didn't get an opportunity to reply to the previous thread before you
sent this, but I really don't like listing specific userspace
tools/libraries here. I recognize that you like the specificity, but
I do not, and I fear that it will become invalid over time either due
to deprecation of old packages or omission of new ones; the fact that
we are just now adding an entry from 2005 shows how this area of
Documentation can often be neglected.
Please replace this with something like "audit and login applications"
or something similar.
--
paul moore
www.paul-moore.com
Re: [PATCH v2] MAINTAINERS: update audit files
On Wed, Mar 17, 2021 at 9:49 PM Richard Guy Briggs wrote: > > Add files maintaned by the audit subsystem. > > Files from arch/*/*/*audit*.[ch] and arch/x86/include/asm/audit.h were not > added due to concern of the list not holding up over time. There exist > already exceptions that caused the need for this specificity. > > Signed-off-by: Richard Guy Briggs > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) Merged into audit/next, thanks. -- paul moore www.paul-moore.com
Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()
On Wed, Mar 17, 2021 at 11:44 AM David Brazdil wrote:
>
> For AF_VSOCK, accept() currently returns sockets that are unlabelled.
> Other socket families derive the child's SID from the SID of the parent
> and the SID of the incoming packet. This is typically done as the
> connected socket is placed in the queue that accept() removes from.
>
> Implement an LSM hook 'vsock_sk_clone' that takes the parent (server)
> and child (connection) struct socks, and assigns the parent SID to the
> child. There is no packet SID in this case.
>
> Signed-off-by: David Brazdil
> ---
> This is my first patch in this part of the kernel so please comment if I
> missed anything, specifically whether there is a packet SID that should
> be mixed into the child SID.
>
> Tested on Android.
>
> include/linux/lsm_hook_defs.h | 1 +
> include/linux/lsm_hooks.h | 7 +++
> include/linux/security.h | 5 +
> net/vmw_vsock/af_vsock.c | 1 +
> security/security.c | 5 +
> security/selinux/hooks.c | 10 ++
> 6 files changed, 29 insertions(+)
Additional comments below, but I think it would be a good idea for you
to test your patches on a more traditional Linux distribution as well
as Android.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5546710d8ac1..a9bf3b90cb2f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -755,6 +755,7 @@ static struct sock *__vsock_create(struct net *net,
> vsk->buffer_size = psk->buffer_size;
> vsk->buffer_min_size = psk->buffer_min_size;
> vsk->buffer_max_size = psk->buffer_max_size;
> + security_vsock_sk_clone(parent, sk);
Did you try calling the existing security_sk_clone() hook here? I
would be curious to hear why it doesn't work in this case.
Feel free to educate me on AF_VSOCK, it's entirely possible I'm
misunderstanding something here :)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ddd097790d47..7b92d6f2e0fd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5616,6 +5616,15 @@ static int selinux_tun_dev_open(void *security)
> return 0;
> }
>
> +static void selinux_socket_vsock_sk_clone(struct sock *sock, struct sock
> *newsk)
> +{
> + struct sk_security_struct *sksec_sock = sock->sk_security;
> + struct sk_security_struct *sksec_new = newsk->sk_security;
> +
> + /* Always returns 0 when packet SID is SECSID_NULL. */
> + WARN_ON_ONCE(selinux_conn_sid(sksec_sock->sid, SECSID_NULL,
> _new->sid));
> +}
If you are using selinux_conn_sid() with the second argument always
SECSID_NULL it probably isn't the best choice; it ends up doing a
simple "sksec_new->sid = sksec_sock->sid" ... which gets us back to
this function looking like a reimplementation of
selinux_sk_clone_security(), minus the peer_sid and sclass
initializations (which should be important things to have).
I strongly suggest you try making use of the existing
security_sk_clone() hook in the vsock code, it seems like a better way
to solve this problem.
--
paul moore
www.paul-moore.com
Re: [PATCH] perf/core: fix unconditional security_locked_down() call
On Tue, Mar 16, 2021 at 10:30 AM Peter Zijlstra wrote:
> On Tue, Mar 16, 2021 at 09:53:21AM -0400, Paul Moore wrote:
> > On Wed, Feb 24, 2021 at 4:59 PM Ondrej Mosnacek wrote:
> > >
> > > Currently, the lockdown state is queried unconditionally, even though
> > > its result is used only if the PERF_SAMPLE_REGS_INTR bit is set in
> > > attr.sample_type. While that doesn't matter in case of the Lockdown LSM,
> > > it causes trouble with the SELinux's lockdown hook implementation.
> > >
> > > SELinux implements the locked_down hook with a check whether the current
> > > task's type has the corresponding "lockdown" class permission
> > > ("integrity" or "confidentiality") allowed in the policy. This means
> > > that calling the hook when the access control decision would be ignored
> > > generates a bogus permission check and audit record.
> > >
> > > Fix this by checking sample_type first and only calling the hook when
> > > its result would be honored.
> > >
> > > Fixes: b0c8fdc7fdb7 ("lockdown: Lock down perf when in confidentiality
> > > mode")
> > > Signed-off-by: Ondrej Mosnacek
> > > ---
> > > kernel/events/core.c | 12 ++--
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > Perf/core folks, do you want to pull this in via your tree? If I
> > don't hear anything in the next day I'll pull this in via the
> > selinux/next tree.
> >
> > Reviewed-by: Paul Moore
>
> Ah, fell in the cracks... I've no idea what Changelog is trying to tell
> me. It is pure gibberish to me. But the patch seems harmless enough to me.
>
> Let me queue it then.
Great, thanks.
--
paul moore
www.paul-moore.com
Re: [PATCH] perf/core: fix unconditional security_locked_down() call
On Wed, Feb 24, 2021 at 4:59 PM Ondrej Mosnacek wrote:
>
> Currently, the lockdown state is queried unconditionally, even though
> its result is used only if the PERF_SAMPLE_REGS_INTR bit is set in
> attr.sample_type. While that doesn't matter in case of the Lockdown LSM,
> it causes trouble with the SELinux's lockdown hook implementation.
>
> SELinux implements the locked_down hook with a check whether the current
> task's type has the corresponding "lockdown" class permission
> ("integrity" or "confidentiality") allowed in the policy. This means
> that calling the hook when the access control decision would be ignored
> generates a bogus permission check and audit record.
>
> Fix this by checking sample_type first and only calling the hook when
> its result would be honored.
>
> Fixes: b0c8fdc7fdb7 ("lockdown: Lock down perf when in confidentiality mode")
> Signed-off-by: Ondrej Mosnacek
> ---
> kernel/events/core.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
Perf/core folks, do you want to pull this in via your tree? If I
don't hear anything in the next day I'll pull this in via the
selinux/next tree.
Reviewed-by: Paul Moore
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 129dee540a8b..0f857307e9bd 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11796,12 +11796,12 @@ SYSCALL_DEFINE5(perf_event_open,
> return err;
> }
>
> - err = security_locked_down(LOCKDOWN_PERF);
> - if (err && (attr.sample_type & PERF_SAMPLE_REGS_INTR))
> - /* REGS_INTR can leak data, lockdown must prevent this */
> - return err;
> -
> - err = 0;
> + /* REGS_INTR can leak data, lockdown must prevent this */
> + if (attr.sample_type & PERF_SAMPLE_REGS_INTR) {
> + err = security_locked_down(LOCKDOWN_PERF);
> + if (err)
> + return err;
> + }
>
> /*
> * In cgroup mode, the pid argument is used to pass the fd
> --
> 2.29.2
--
paul moore
www.paul-moore.com
Re: [PATCH] MAINTAINERS: update audit files
On Thu, Mar 11, 2021 at 11:41 AM Richard Guy Briggs wrote: > > Add files maintaned by the audit subsystem. > > Signed-off-by: Richard Guy Briggs > --- > MAINTAINERS | 4 > 1 file changed, 4 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 6eff4f720c72..a17532559665 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3015,9 +3015,13 @@ L: linux-au...@redhat.com (moderated for > non-subscribers) > S: Supported > W: https://github.com/linux-audit > T: git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git > +F: arch/*/*/*audit*.[ch] That looks like it has about two too many wildcards to hold up over time :) I understand what you're trying to do here, and while I don't disagree in principle, I worry that the arch specific paths vary enough that trying to handle it here is going to be a bit of a mess. > +F: arch/x86/include/asm/audit.h The fact that we need a special entry for the single header under x86 tends to reinforce that. The other additions make sense, but I think it may be best to leave the arch specific areas alone for now. > +F: include/asm-generic/audit_*.h > F: include/linux/audit.h > F: include/uapi/linux/audit.h > F: kernel/audit* > +F: lib/*audit.c > > AUXILIARY DISPLAY DRIVERS > M: Miguel Ojeda Sandonis > -- > 2.27.0 -- paul moore www.paul-moore.com
Re: [PATCH] audit: further cleanup of AUDIT_FILTER_ENTRY deprecation
On Thu, Mar 11, 2021 at 11:38 AM Richard Guy Briggs wrote:
>
> Remove the list parameter from the function call since the exit filter
> list is the only remaining list used by this function.
>
> This cleans up commit 5260ecc2e048
> ("audit: deprecate the AUDIT_FILTER_ENTRY filter")
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/auditsc.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
Looks good, merged. Thanks.
--
paul moore
www.paul-moore.com
Re: [PATCH 2/2] audit: document /proc/PID/sessionid
On Thu, Mar 11, 2021 at 11:41 AM Richard Guy Briggs wrote:
>
> Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> was added 2008-03-13 in commit 1e0bd7550ea9 ("[PATCH] export sessionid
> alongside the loginuid in procfs")
>
> Signed-off-by: Richard Guy Briggs
> ---
> Documentation/ABI/stable/procfs-audit_loginuid | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/ABI/stable/procfs-audit_loginuid
> b/Documentation/ABI/stable/procfs-audit_loginuid
> index fae63bef2970..175ee6ec3695 100644
> --- a/Documentation/ABI/stable/procfs-audit_loginuid
> +++ b/Documentation/ABI/stable/procfs-audit_loginuid
> @@ -13,3 +13,15 @@ Description:
> AUDIT_FEATURE_LOGINUID_IMMUTABLE is enabled. It cannot be
> unset if AUDIT_FEATURE_ONLY_UNSET_LOGINUID is enabled.
>
> +
> +What: Audit Login Session ID
> +Date: 2008-03-13
> +KernelVersion: 2.6.25-rc7 1e0bd7550ea9 ("[PATCH] export sessionid alongside
> the loginuid in procfs")
> +Contact: linux-au...@redhat.com
> +Format:u32
> +Users: auditd, libaudit, audit-testsuite, login
This should be obvious, but just to be safe - my comment from patch
1/2 also applies here.
> +Description:
> + The /proc/$pid/sessionid pseudofile is read to get the
> + audit login session ID of process $pid. It is set
> + automatically, serially assigned with each new login.
--
paul moore
www.paul-moore.com
Re: [PATCH 1/2] audit: document /proc/PID/loginuid
On Thu, Mar 11, 2021 at 11:41 AM Richard Guy Briggs wrote:
> Describe the /proc/PID/loginuid interface in Documentation/ABI/stable that
> was added 2005-02-01 by commit 1e2d1492e178 ("[PATCH] audit: handle
> loginuid through proc")
>
> Signed-off-by: Richard Guy Briggs
> ---
> Documentation/ABI/stable/procfs-audit_loginuid | 15 +++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/ABI/stable/procfs-audit_loginuid
After ~15 years, it might be time ;)
> diff --git a/Documentation/ABI/stable/procfs-audit_loginuid
> b/Documentation/ABI/stable/procfs-audit_loginuid
> new file mode 100664
> index ..fae63bef2970
> --- /dev/null
> +++ b/Documentation/ABI/stable/procfs-audit_loginuid
> @@ -0,0 +1,15 @@
> +What: Audit Login UID
> +Date: 2005-02-01
> +KernelVersion: 2.6.11-rc2 1e2d1492e178 ("[PATCH] audit: handle loginuid
> through proc")
> +Contact: linux-au...@redhat.com
> +Format:u32
I haven't applied the patch, but I'm going to assume that the "u32"
lines up correctly with the rest of the entries, right?
> +Users: auditd, libaudit, audit-testsuite, login
I think these entries are a bit too specific as I expect the kernel to
outlive most userspace libraries and applications. I would suggest
"audit and login applications" or something similar.
> +Description:
> + The /proc/$pid/loginuid pseudofile is written to set and
I'm really in no position to critique someone's English grammar, but
if we're talking about changes I might add a comma after "set", "...
is written to set, and read to get ...".
> + read to get the audit login UID of process $pid. If it is
> + unset, permissions are not needed to set it. The accessor
> must
> + have CAP_AUDIT_CONTROL in the initial user namespace to write
> + it if it has been set. It cannot be written again if
> + AUDIT_FEATURE_LOGINUID_IMMUTABLE is enabled. It cannot be
> + unset if AUDIT_FEATURE_ONLY_UNSET_LOGINUID is enabled.
--
paul moore
www.paul-moore.com
Re: [PATCH] security/selinux/include/: fix misspellings using codespell tool
On Mon, Mar 8, 2021 at 6:19 AM wrote: > > From: Xiong Zhenwu > > A typo is f out by codespell tool in 422th line of security.h: > > $ codespell ./security/selinux/include/ > ./security.h:422: thie ==> the, this > > Fix a typo found by codespell. > > Signed-off-by: Xiong Zhenwu > --- > security/selinux/include/security.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged, thanks. -- paul moore www.paul-moore.com
Re: [PATCH] security/selinux/ss: fix misspellings using codespell tool
On Mon, Mar 8, 2021 at 6:03 AM wrote: > > From: Xiong Zhenwu > > A typo is found out by codespell tool in 16th line of hashtab.c > > $ codespell ./security/selinux/ss/ > ./hashtab.c:16: rouding ==> rounding > > Fix a typo found by codespell. > > Signed-off-by: Xiong Zhenwu > --- > security/selinux/ss/hashtab.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged, thanks. -- paul moore www.paul-moore.com
Re: [PATCH v3] selinux: measure state and policy capabilities
On Fri, Mar 5, 2021 at 2:29 PM Lakshmi Ramasubramanian wrote: > On 3/5/21 11:22 AM, Paul Moore wrote: > > Hi Paul, > > > On Fri, Mar 5, 2021 at 12:57 PM James Bottomley > > wrote: > >> On Fri, 2021-03-05 at 12:52 -0500, Paul Moore wrote: > >> [...] > >>> This draft seems fine to me, but there is a small logistical blocker > >>> at the moment which means I can't merge this until -rc2 is released, > >>> which likely means this coming Monday. The problem is that this > >>> patch relies on code that went upstream via in the last merge window > >>> via the IMA tree, not the SELinux tree; normally that wouldn't be a > >>> problem as I typically rebase the selinux/next to Linus' -rc1 tag > >>> once the merge window is closed, but in this particular case the -rc1 > >>> tag is dangerously broken for some system configurations (the tag has > >>> since been renamed) so I'm not rebasing onto -rc1 this time around. > >>> > >>> Assuming that -rc2 fixes the swapfile/fs-corruption problem, early > >>> next week I'll rebase selinux/next to -rc2 and merge this patch. > >>> However, if the swapfile bug continues past -rc2 we can consider > >>> merging this via the IMA tree, but I'd assume not do that if possible > >>> due to merge conflict and testing reasons. > >> > >> If it helps, we rebased the SCSI tree on top of the merge for the > >> swapfile fix which is this one, without waiting for -rc2: > > > > Considering that -rc2 is only two days away I'm not going to lose a > > lot of sleep over it. > > > > Thanks for reviewing the patch. > > I can wait until the swapfile issue is resolved (in rc2 or later) and > you are able to merge this patch. Please take your time. Thanks for your patience Lakshmi, I just merged this into my local selinux/next branch and will be pushing it up to kernel.org later tonight - thank you! -- paul moore www.paul-moore.com
Re: [PATCH v3] selinux: measure state and policy capabilities
On Fri, Mar 5, 2021 at 12:57 PM James Bottomley wrote: > On Fri, 2021-03-05 at 12:52 -0500, Paul Moore wrote: > [...] > > This draft seems fine to me, but there is a small logistical blocker > > at the moment which means I can't merge this until -rc2 is released, > > which likely means this coming Monday. The problem is that this > > patch relies on code that went upstream via in the last merge window > > via the IMA tree, not the SELinux tree; normally that wouldn't be a > > problem as I typically rebase the selinux/next to Linus' -rc1 tag > > once the merge window is closed, but in this particular case the -rc1 > > tag is dangerously broken for some system configurations (the tag has > > since been renamed) so I'm not rebasing onto -rc1 this time around. > > > > Assuming that -rc2 fixes the swapfile/fs-corruption problem, early > > next week I'll rebase selinux/next to -rc2 and merge this patch. > > However, if the swapfile bug continues past -rc2 we can consider > > merging this via the IMA tree, but I'd assume not do that if possible > > due to merge conflict and testing reasons. > > If it helps, we rebased the SCSI tree on top of the merge for the > swapfile fix which is this one, without waiting for -rc2: Considering that -rc2 is only two days away I'm not going to lose a lot of sleep over it. -- paul moore www.paul-moore.com
Re: [PATCH v3] selinux: measure state and policy capabilities
On Fri, Feb 12, 2021 at 11:37 AM Lakshmi Ramasubramanian wrote: > > SELinux stores the configuration state and the policy capabilities > in kernel memory. Changes to this data at runtime would have an impact > on the security guarantees provided by SELinux. Measuring this data > through IMA subsystem provides a tamper-resistant way for > an attestation service to remotely validate it at runtime. > > Measure the configuration state and policy capabilities by calling > the IMA hook ima_measure_critical_data(). > > To enable SELinux data measurement, the following steps are required: > > 1, Add "ima_policy=critical_data" to the kernel command line arguments > to enable measuring SELinux data at boot time. > For example, > BOOT_IMAGE=/boot/vmlinuz-5.11.0-rc3+ > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux > ima_policy=critical_data > > 2, Add the following rule to /etc/ima/ima-policy >measure func=CRITICAL_DATA label=selinux > > Sample measurement of SELinux state and policy capabilities: > > 10 2122...65d8 ima-buf sha256:13c2...1292 selinux-state 696e...303b > > Execute the following command to extract the measured data > from the IMA's runtime measurements list: > > grep "selinux-state" > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut > -d' ' -f 6 | xxd -r -p > > The output should be a list of key-value pairs. For example, > > initialized=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0; > > To verify the measurement is consistent with the current SELinux state > reported on the system, compare the integer values in the following > files with those set in the IMA measurement (using the following commands): > > - cat /sys/fs/selinux/enforce > - cat /sys/fs/selinux/checkreqprot > - cat /sys/fs/selinux/policy_capabilities/[capability_file] > > Note that the actual verification would be against an expected state > and done on a separate system (likely an attestation server) requiring > "initialized=1;enforcing=1;checkreqprot=0;" > for a secure state and then whatever policy capabilities are actually > set in the expected policy (which can be extracted from the policy > itself via seinfo, for example). > > Signed-off-by: Lakshmi Ramasubramanian > Suggested-by: Stephen Smalley > Suggested-by: Paul Moore > --- > security/selinux/ima.c | 87 -- > security/selinux/include/ima.h | 6 +++ > security/selinux/selinuxfs.c | 6 +++ > security/selinux/ss/services.c | 2 +- > 4 files changed, 96 insertions(+), 5 deletions(-) This draft seems fine to me, but there is a small logistical blocker at the moment which means I can't merge this until -rc2 is released, which likely means this coming Monday. The problem is that this patch relies on code that went upstream via in the last merge window via the IMA tree, not the SELinux tree; normally that wouldn't be a problem as I typically rebase the selinux/next to Linus' -rc1 tag once the merge window is closed, but in this particular case the -rc1 tag is dangerously broken for some system configurations (the tag has since been renamed) so I'm not rebasing onto -rc1 this time around. Assuming that -rc2 fixes the swapfile/fs-corruption problem, early next week I'll rebase selinux/next to -rc2 and merge this patch. However, if the swapfile bug continues past -rc2 we can consider merging this via the IMA tree, but I'd assume not do that if possible due to merge conflict and testing reasons. -- paul moore www.paul-moore.com
Re: [PATCH] RTIC: selinux: ARM64: Move selinux_state to a separate page
On Tue, Feb 16, 2021 at 5:19 AM Preeti Nagar wrote: > > The changes introduce a new security feature, RunTime Integrity Check > (RTIC), designed to protect Linux Kernel at runtime. The motivation > behind these changes is: > 1. The system protection offered by Security Enhancements(SE) for > Android relies on the assumption of kernel integrity. If the kernel > itself is compromised (by a perhaps as yet unknown future vulnerability), > SE for Android security mechanisms could potentially be disabled and > rendered ineffective. > 2. Qualcomm Snapdragon devices use Secure Boot, which adds cryptographic > checks to each stage of the boot-up process, to assert the authenticity > of all secure software images that the device executes. However, due to > various vulnerabilities in SW modules, the integrity of the system can be > compromised at any time after device boot-up, leading to un-authorized > SW executing. > > The feature's idea is to move some sensitive kernel structures to a > separate page and monitor further any unauthorized changes to these, > from higher Exception Levels using stage 2 MMU. Moving these to a > different page will help avoid getting page faults from un-related data. > The mechanism we have been working on removes the write permissions for > HLOS in the stage 2 page tables for the regions to be monitored, such > that any modification attempts to these will lead to faults being > generated and handled by handlers. If the protected assets are moved to > a separate page, faults will be generated corresponding to change attempts > to these assets only. If not moved to a separate page, write attempts to > un-related data present on the monitored pages will also be generated. > > Using this feature, some sensitive variables of the kernel which are > initialized after init or are updated rarely can also be protected from > simple overwrites and attacks trying to modify these. > > Currently, the change moves selinux_state structure to a separate page. > The page is 2MB aligned not 4K to avoid TLB related performance impact as, > for some CPU core designs, the TLB does not cache 4K stage 2 (IPA to PA) > mappings if the IPA comes from a stage 1 mapping. In future, we plan to > move more security-related kernel assets to this page to enhance > protection. > > Signed-off-by: Preeti Nagar > --- > The RFC patch reviewed available at: > https://lore.kernel.org/linux-security-module/1610099389-28329-1-git-send-email-pna...@codeaurora.org/ > --- > include/asm-generic/vmlinux.lds.h | 10 ++ > include/linux/init.h | 6 ++ > security/Kconfig | 11 +++ > security/selinux/hooks.c | 2 +- > 4 files changed, 28 insertions(+), 1 deletion(-) As long as we are only talking about moving the selinux_state struct itself and none of the pointers inside I think we should be okay (the access decision cache pointed to by selinux_state->avc could change frequently). Have you done any performance measurements of this change? Assuming they are not terrible, I have no objections to this patch from a SELinux perspective. -- paul moore www.paul-moore.com
Re: [PATCH v3] selinux: measure state and policy capabilities
On Thu, Mar 4, 2021 at 2:20 PM Lakshmi Ramasubramanian wrote: > On 2/12/21 8:37 AM, Lakshmi Ramasubramanian wrote: > > Hi Paul, > > > SELinux stores the configuration state and the policy capabilities > > in kernel memory. Changes to this data at runtime would have an impact > > on the security guarantees provided by SELinux. Measuring this data > > through IMA subsystem provides a tamper-resistant way for > > an attestation service to remotely validate it at runtime. > > > > Measure the configuration state and policy capabilities by calling > > the IMA hook ima_measure_critical_data(). > > > > I have addressed your comments on the v2 patch for selinux measurement > using IMA. Could you please let me know if there are any other comments > that I need to address in this patch? The merge window just closed earlier this week, and there were a handful of bugs that needed to be addressed before I could look at this patch. If I don't get a chance to review this patch tonight, I will try to get to it this weekend or early next week. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Write in cipso_v4_doi_putdef
On Wed, Mar 3, 2021 at 11:20 AM Paul Moore wrote: > On Wed, Mar 3, 2021 at 10:53 AM syzbot > wrote: > > > > Hello, > > > > syzbot found the following issue on: > > > > HEAD commit:7a7fd0de Merge branch 'kmap-conversion-for-5.12' of git://.. > > git tree: upstream > > console output: https://syzkaller.appspot.com/x/log.txt?x=164a74dad0 > > kernel config: https://syzkaller.appspot.com/x/.config?x=779a2568b654c1c6 > > dashboard link: https://syzkaller.appspot.com/bug?extid=521772a90166b3fca21f > > compiler: Debian clang version 11.0.1-2 > > > > Unfortunately, I don't have any reproducer for this issue yet. > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > Reported-by: syzbot+521772a90166b3fca...@syzkaller.appspotmail.com > > > > == > > BUG: KASAN: use-after-free in instrument_atomic_read_write > > include/linux/instrumented.h:101 [inline] > > BUG: KASAN: use-after-free in atomic_fetch_sub_release > > include/asm-generic/atomic-instrumented.h:220 [inline] > > BUG: KASAN: use-after-free in __refcount_sub_and_test > > include/linux/refcount.h:272 [inline] > > BUG: KASAN: use-after-free in __refcount_dec_and_test > > include/linux/refcount.h:315 [inline] > > BUG: KASAN: use-after-free in refcount_dec_and_test > > include/linux/refcount.h:333 [inline] > > BUG: KASAN: use-after-free in cipso_v4_doi_putdef+0x2d/0x190 > > net/ipv4/cipso_ipv4.c:586 > > Write of size 4 at addr 8880179ecb18 by task syz-executor.5/20110 > > Almost surely the same problem as the others, I'm currently chasing > down a few remaining spots to make sure the fix I'm working on is > correct. I think I've now managed to convince myself that the patch I've got here is reasonable. I'm looping over a series of tests right now and plan to let it continue overnight; assuming everything still looks good in the morning I'll post it. Thanks for your help. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Write in cipso_v4_doi_putdef
On Wed, Mar 3, 2021 at 10:53 AM syzbot wrote: > > Hello, > > syzbot found the following issue on: > > HEAD commit:7a7fd0de Merge branch 'kmap-conversion-for-5.12' of git://.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=164a74dad0 > kernel config: https://syzkaller.appspot.com/x/.config?x=779a2568b654c1c6 > dashboard link: https://syzkaller.appspot.com/bug?extid=521772a90166b3fca21f > compiler: Debian clang version 11.0.1-2 > > Unfortunately, I don't have any reproducer for this issue yet. > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+521772a90166b3fca...@syzkaller.appspotmail.com > > == > BUG: KASAN: use-after-free in instrument_atomic_read_write > include/linux/instrumented.h:101 [inline] > BUG: KASAN: use-after-free in atomic_fetch_sub_release > include/asm-generic/atomic-instrumented.h:220 [inline] > BUG: KASAN: use-after-free in __refcount_sub_and_test > include/linux/refcount.h:272 [inline] > BUG: KASAN: use-after-free in __refcount_dec_and_test > include/linux/refcount.h:315 [inline] > BUG: KASAN: use-after-free in refcount_dec_and_test > include/linux/refcount.h:333 [inline] > BUG: KASAN: use-after-free in cipso_v4_doi_putdef+0x2d/0x190 > net/ipv4/cipso_ipv4.c:586 > Write of size 4 at addr 8880179ecb18 by task syz-executor.5/20110 Almost surely the same problem as the others, I'm currently chasing down a few remaining spots to make sure the fix I'm working on is correct. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Read in cipso_v4_genopt
On Tue, Mar 2, 2021 at 2:15 PM Dmitry Vyukov wrote: ... > Not sure if it's the root cause or not, but I am looking at this > reference drop in cipso_v4_doi_remove: > https://elixir.bootlin.com/linux/v5.12-rc1/source/net/ipv4/cipso_ipv4.c#L522 > The thing is that it does not remove from the list if reference is not > 0, right? So what if I send 1000 of netlink remove messages? Will it > drain refcount to 0? > I did not read all involved code, but the typical pattern is to drop > refcount and always remove from the list. Then the last use will > delete the object. > Does it make any sense? Looking at it quickly, the logic above seems sane. I wrote this code a *long* time ago, so let me get my head back into it and make sure that still holds. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Read in cipso_v4_genopt
On Tue, Mar 2, 2021 at 6:03 AM Dmitry Vyukov wrote: > ... > Besides these 2 crashes, we've also seen one on a 4.19 based kernel, see > below. > Based on the reports with mismatching stacks, it looks like > cipso_v4_genopt is doing some kind of wild pointer access (uninit > pointer?). Hmm, interesting. Looking quickly at the stack dump, it appears that the problem occurs (at least in the recent kernel) when accessing the cipso_v4_doi.tags[] array which is embedded in the cipso_v4_doi struct. Based on the code in cipso_v4_genopt() it doesn't appear that we are shooting past the end of the array/struct and the cipso_v4_doi struct appears to be refcounted correctly in cipso_v4_doi_getdef() and cipso_v4_doi_putdef(). I'll look at it some more today to see if something jumps out at me, but obviously a reproducer would be very helpful if you are able to find one. It's also worth adding that this code really hasn't changed much in a *long* time, not that this means it isn't broken, just that it might also be worth looking at other odd memory bugs to see if there is chance they are wandering around and stomping on memory ... -- paul moore www.paul-moore.com
Re: [BUG] Race between policy reload sidtab conversion and live conversion
On Mon, Mar 1, 2021 at 5:36 AM Ondrej Mosnacek wrote: > On Sun, Feb 28, 2021 at 8:21 PM Paul Moore wrote: > > On Fri, Feb 26, 2021 at 6:12 AM Ondrej Mosnacek wrote: > > > On Fri, Feb 26, 2021 at 2:07 AM Paul Moore wrote: > > > > On Wed, Feb 24, 2021 at 4:35 AM Ondrej Mosnacek > > > > wrote: ... > > Ah, yes, you're right. I was only thinking about the problem of > > adding an entry to the old sidtab, and not the (much more likely case) > > of an entry being added to the new sidtab. Bummer. > > > > Thinking aloud for a moment - what if we simply refused to add new > > sidtab entries if the task's sidtab pointer is "old"? Common sense > > would tell us that this scenario should be very rare at present, and I > > believe the testing mentioned in this thread adds some weight to that > > claim. After all, this only affects tasks which entered into their > > RCU protected session prior to the policy load RCU sync *AND* are > > attempting to add a new entry to the sidtab. That *has* to be a > > really low percentage, especially on a system that has been up and > > running for some time. My gut feeling is this should be safe as well; > > all of the calling code should have the necessary error handling in > > place as there are plenty of reasons why we could normally fail to add > > an entry to the sidtab; memory allocation failures being the most > > obvious failure point I would suspect. This obvious downside to such > > an approach is that those operations which do meet this criteria would > > fail - and we should likely emit an error in this case - but is this > > failure really worse than any other transient kernel failure, > > No, I don't like this approach at all. Before the sidtab refactor, it > had been done exactly this way ... I recognize I probably haven't made my feelings about reverts clear, or if I have, I haven't done so recently. Let me fix that now: I *hate* them. Further I hate reverts with a deep, passionate hatred that I reserve for very few things. Maybe we have to revert this change, even though I *hate* reverts they do remain an option; you just need to be 99% sure you've exhausted all the other options first. > Perhaps it wasn't clear from what I wrote, but I certainly don't want > to abandon it completely. Just to revert to a safe state until we > figure out how to do the RCU policy reload safely. The solution with > two-way conversion seems doable, it's just not a quick and easy fix. I suggest pursuing this before the revert to see what it looks like and we can discuss it further during review. -- paul moore www.paul-moore.com
Re: [linux-next:master 5983/6048] h8300-linux-ld: section .data VMA overlaps section __kcrctab VMA
On Mon, Mar 1, 2021 at 1:08 AM Feng Tang wrote: > > Hi Paul, > > On Wed, Feb 10, 2021 at 02:21:41AM +0800, Paul Moore wrote: > > On Tue, Feb 9, 2021 at 1:09 PM kernel test robot wrote: > > > tree: > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > > > head: 59fa6a163ffabc1bf25c5e0e33899e268a96d3cc > > > commit: 77d8143a5290b38e3331f61f55c0b682699884bc [5983/6048] Merge > > > remote-tracking branch 'selinux/next' > > > config: h8300-randconfig-r005-20210209 (attached as .config) > > > compiler: h8300-linux-gcc (GCC) 9.3.0 > > > reproduce (this is a W=1 build): > > > wget > > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross > > > -O ~/bin/make.cross > > > chmod +x ~/bin/make.cross > > > # > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=77d8143a5290b38e3331f61f55c0b682699884bc > > > git remote add linux-next > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git > > > git fetch --no-tags linux-next master > > > git checkout 77d8143a5290b38e3331f61f55c0b682699884bc > > > # save the attached .config to linux build tree > > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross > > > ARCH=h8300 > > > > > > If you fix the issue, kindly add following tag as appropriate > > > Reported-by: kernel test robot > > > > > > All errors (new ones prefixed by >>): > > > > > >h8300-linux-ld: section .init.text LMA > > > [00430360,004479a1] overlaps section .data LMA > > > [0041868c,004489eb] > > > >> h8300-linux-ld: section .data VMA [0040,0043035f] > > > >> overlaps section __kcrctab VMA [003fdd74,0040007b] > > > >> h8300-linux-ld: section __kcrctab_gpl VMA > > > >> [0040007c,004025a7] overlaps section .data VMA > > > >> [0040,0043035f] > > >h8300-linux-ld: arch/h8300/kernel/entry.o: in function `resume_kernel': > > >(.text+0x29a): undefined reference to `TI_PRE_COUNT' > > > > This really doesn't look like something caused by SELinux ... > > No, this is not related with SELinux, sorry for the false alarm. Thanks for confirming this and providing an explanation of the root cause. -- paul moore www.paul-moore.com
Re: [BUG] Race between policy reload sidtab conversion and live conversion
On Fri, Feb 26, 2021 at 6:12 AM Ondrej Mosnacek wrote: > On Fri, Feb 26, 2021 at 2:07 AM Paul Moore wrote: > > On Wed, Feb 24, 2021 at 4:35 AM Ondrej Mosnacek wrote: > > > After the switch to RCU, we now have: > > > 1. Start live conversion of new entries. > > > 2. Convert existing entries. > > > 3. RCU-assign the new policy pointer to selinux_state. > > > [!!! Now actually both old and new sidtab may be referenced by > > > readers, since there is no synchronization barrier previously provided > > > by the write lock.] > > > 4. Wait for synchronize_rcu() to return. > > > 5. Now only the new sidtab is visible to readers, so the old one can > > > be destroyed. > > > > > > So the race can happen between 3. and 5., if one thread already sees > > > the new sidtab and adds a new entry there, and a second thread still > > > has the reference to the old sidtab and also tires to add a new entry; > > > live-converting to the new sidtab, which it doesn't expect to change > > > by itself. Unfortunately I failed to realize this when reviewing the > > > patch :/ > > > > It is possible I'm not fully understanding the problem and/or missing > > an important detail - it is rather tricky code, and RCU can be very > > hard to reason at times - but I think we may be able to solve this > > with some lock fixes inside sidtab_context_to_sid(). Let me try to > > explain to see if we are on the same page here ... > > > > The problem is when we have two (or more) threads trying to > > add/convert the same context into a sid; the task with new_sidtab is > > looking to add a new sidtab entry, while the task with old_sidtab is > > looking to convert an entry in old_sidtab into a new entry in > > new_sidtab. Boom. > > > > Looking at the code in sidtab_context_to_sid(), when we have two > > sidtabs that are currently active (old_sidtab->convert pointer is > > valid) and a task with old_sidtab attempts to add a new entry to both > > sidtabs it first adds it to the old sidtab then it also adds it to the > > new sidtab. I believe the problem is that in this case while the task > > grabs the old_sidtab->lock, it never grabs the new_sidtab->lock which > > allows it to race with tasks that already see only new_sidtab. I > > think adding code to sidtab_context_to_sid() which grabs the > > new_sidtab->lock when adding entries to the new_sidtab *should* solve > > the problem. > > > > Did I miss something important? ;) > > Sadly, yes :) Consider this scenario (assuming we fix the locking at > sidtab level): > > If it happens that a new SID (x) is added via the new sidtab and then > another one (y) via the old sidtab, to avoid clash of SIDs, we would > need to leave a "hole" in the old sidtab for SID x. And this will > cause trouble if the thread that has just added SID y, then tries to > translate the context string corresponding to SID x (without re-taking > the RCU read lock and refreshing the policy pointer). Even if we > handle skipping the "holes" in the old sidtab safely, the translation > would then end up adding a duplicate SID entry for the context already > represented by SID x - which is not a state we want to end up in. Ah, yes, you're right. I was only thinking about the problem of adding an entry to the old sidtab, and not the (much more likely case) of an entry being added to the new sidtab. Bummer. Thinking aloud for a moment - what if we simply refused to add new sidtab entries if the task's sidtab pointer is "old"? Common sense would tell us that this scenario should be very rare at present, and I believe the testing mentioned in this thread adds some weight to that claim. After all, this only affects tasks which entered into their RCU protected session prior to the policy load RCU sync *AND* are attempting to add a new entry to the sidtab. That *has* to be a really low percentage, especially on a system that has been up and running for some time. My gut feeling is this should be safe as well; all of the calling code should have the necessary error handling in place as there are plenty of reasons why we could normally fail to add an entry to the sidtab; memory allocation failures being the most obvious failure point I would suspect. This obvious downside to such an approach is that those operations which do meet this criteria would fail - and we should likely emit an error in this case - but is this failure really worse than any other transient kernel failure, and is attempting to mitigate this failure worth abandoning the RCU approach for the sidtab? -- paul moore www.paul-moore.com
Re: [PATCH 09/11] pragma once: convert scripts/selinux/genheaders/genheaders.c
On Sun, Feb 28, 2021 at 12:04 PM Alexey Dobriyan wrote:
>
> From 097f2c8b2af7d9e88cff59376ea0ad51b95341cb Mon Sep 17 00:00:00 2001
> From: Alexey Dobriyan
> Date: Tue, 9 Feb 2021 00:39:23 +0300
> Subject: [PATCH 09/11] pragma once: convert
> scripts/selinux/genheaders/genheaders.c
>
> Generate security/selinux/flask.h and security/selinux/av_permissions.h
> without include guards.
>
> Signed-off-by: Alexey Dobriyan
> ---
> scripts/selinux/genheaders/genheaders.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
My LKML subscription must have died at some point due to mail bounces,
or maybe I dopped it (?), because I'm not seeing the rest of this
patchset for context.
However, unless the rest of the kernel transitions to this, or there
is some other big win that I'm missing, I don't see much of a reason
for this; can you provide some compelling reason for why we should
make this change? A quick search on "#pragma once" seems to indicate
it is non-standard, so why replace the simple #ifdef/#define solution
for this?
> diff --git a/scripts/selinux/genheaders/genheaders.c
> b/scripts/selinux/genheaders/genheaders.c
> index f355b3e0e968..e13ee4221993 100644
> --- a/scripts/selinux/genheaders/genheaders.c
> +++ b/scripts/selinux/genheaders/genheaders.c
> @@ -74,8 +74,8 @@ int main(int argc, char *argv[])
> initial_sid_to_string[i] = stoupperx(s);
> }
>
> + fprintf(fout, "#pragma once\n");
> fprintf(fout, "/* This file is automatically generated. Do not edit.
> */\n");
> - fprintf(fout, "#ifndef _SELINUX_FLASK_H_\n#define
> _SELINUX_FLASK_H_\n\n");
>
> for (i = 0; secclass_map[i].name; i++) {
> struct security_class_mapping *map = _map[i];
> @@ -109,7 +109,6 @@ int main(int argc, char *argv[])
> fprintf(fout, "\treturn sock;\n");
> fprintf(fout, "}\n");
>
> - fprintf(fout, "\n#endif\n");
> fclose(fout);
>
> fout = fopen(argv[2], "w");
> @@ -119,8 +118,8 @@ int main(int argc, char *argv[])
> exit(4);
> }
>
> + fprintf(fout, "#pragma once\n");
> fprintf(fout, "/* This file is automatically generated. Do not edit.
> */\n");
> - fprintf(fout, "#ifndef _SELINUX_AV_PERMISSIONS_H_\n#define
> _SELINUX_AV_PERMISSIONS_H_\n\n");
>
> for (i = 0; secclass_map[i].name; i++) {
> struct security_class_mapping *map = _map[i];
> @@ -136,7 +135,6 @@ int main(int argc, char *argv[])
> }
> }
>
> - fprintf(fout, "\n#endif\n");
> fclose(fout);
> exit(0);
> }
> --
> 2.29.2
--
paul moore
www.paul-moore.com
Re: [BUG] Race between policy reload sidtab conversion and live conversion
On Wed, Feb 24, 2021 at 4:35 AM Ondrej Mosnacek wrote: > After the switch to RCU, we now have: > 1. Start live conversion of new entries. > 2. Convert existing entries. > 3. RCU-assign the new policy pointer to selinux_state. > [!!! Now actually both old and new sidtab may be referenced by > readers, since there is no synchronization barrier previously provided > by the write lock.] > 4. Wait for synchronize_rcu() to return. > 5. Now only the new sidtab is visible to readers, so the old one can > be destroyed. > > So the race can happen between 3. and 5., if one thread already sees > the new sidtab and adds a new entry there, and a second thread still > has the reference to the old sidtab and also tires to add a new entry; > live-converting to the new sidtab, which it doesn't expect to change > by itself. Unfortunately I failed to realize this when reviewing the > patch :/ It is possible I'm not fully understanding the problem and/or missing an important detail - it is rather tricky code, and RCU can be very hard to reason at times - but I think we may be able to solve this with some lock fixes inside sidtab_context_to_sid(). Let me try to explain to see if we are on the same page here ... The problem is when we have two (or more) threads trying to add/convert the same context into a sid; the task with new_sidtab is looking to add a new sidtab entry, while the task with old_sidtab is looking to convert an entry in old_sidtab into a new entry in new_sidtab. Boom. Looking at the code in sidtab_context_to_sid(), when we have two sidtabs that are currently active (old_sidtab->convert pointer is valid) and a task with old_sidtab attempts to add a new entry to both sidtabs it first adds it to the old sidtab then it also adds it to the new sidtab. I believe the problem is that in this case while the task grabs the old_sidtab->lock, it never grabs the new_sidtab->lock which allows it to race with tasks that already see only new_sidtab. I think adding code to sidtab_context_to_sid() which grabs the new_sidtab->lock when adding entries to the new_sidtab *should* solve the problem. Did I miss something important? ;) -- paul moore www.paul-moore.com
Re: [BUG] Race between policy reload sidtab conversion and live conversion
On Thu, Feb 25, 2021 at 11:38 AM Ondrej Mosnacek wrote: > Unless someone objects, I'll start working on a patch to switch back > to read-write lock for now. If all goes well, I'll send it sometime > next week. Sorry, I was looking at other things and just got to this ... I'm not overly excited about switching back to the read-write lock so quickly, I'd rather we spend some additional time looking into resolving issues with the current RCU code. -- paul moore www.paul-moore.com
Re: [BUG] Race between policy reload sidtab conversion and live conversion
On Tue, Feb 23, 2021 at 5:36 PM Tyler Hicks wrote: > On 2021-02-23 15:50:56, Tyler Hicks wrote: > > On 2021-02-23 15:43:48, Tyler Hicks wrote: > > > I'm seeing a race during policy load while the "regular" sidtab > > > conversion is happening and a live conversion starts to take place in > > > sidtab_context_to_sid(). > > > > > > We have an initial policy that's loaded by systemd ~0.6s into boot and > > > then another policy gets loaded ~2-3s into boot. That second policy load > > > is what hits the race condition situation because the sidtab is only > > > partially populated and there's a decent amount of filesystem operations > > > happening, at the same time, which are triggering live conversions. > > Hmm, perhaps this is the same problem that's fixed by Ondrej's proposed > change here: > > https://lore.kernel.org/selinux/20210212185930.130477-3-omosn...@redhat.com/ > > I'll put these changes through a validation run (the only place that I > can seem to reproduce this crash) and see how it looks. Thanks, please let us know what you find out. -- paul moore www.paul-moore.com
Re: [GIT PULL] SELinux patches for v5.12
On Sun, Feb 21, 2021 at 8:07 PM Linus Torvalds wrote: > > On Mon, Feb 15, 2021 at 1:57 PM Paul Moore wrote: > > > > - Add support for labeling anonymous inodes, and extend this new > > support to userfaultfd. > > I've pulled this, but I just have to note how much I hate the function > names. "secure inode"? There's nothing particularly secure about the > resulting inode. > > It's gone through the security layer init, that doesn't make it > "secure". ALL normal inodes go through it, are all those inodes thus > "secure"? No. > > Naming matters, and I think these things are actively mis-named > implying things that they aren't. I don't disagree that naming is important, I would only add, non-sarcastically, that naming is hard (as a coworker likes to remind me on a regular basis). My personal take on the "secure" function variant is that it provides some indication that this is tied to a LSM hook. For better or worse, all of the LSM hooks start off with "security_" and most (all?) of the LSM blob void pointers in various structs throughout the kernel are named "security". While arguments can be made about the merits of that depending on how you define "security", the fact remains that they are named that way. If you, or anyone else reading this, has another suggestion for the function names I'm listening ... -- paul moore www.paul-moore.com
[GIT PULL] Audit patches for v5.12
Hi Linus, Three very trivial patches for audit this time. All pass the audit-testsuite and apply cleanly to your tree as of a few minutes ago; please merge these for v5.12. Thanks, -Paul -- The following changes since commit e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62: Linux 5.11-rc2 (2021-01-03 15:55:30 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git tags/audit-pr-20210215 for you to fetch changes up to 127c8c5f0589cea2208c329bff7dcb36e375f46c: audit: Make audit_filter_syscall() return void (2021-01-27 21:55:14 -0500) audit/stable-5.12 PR 20210215 Davidlohr Bueso (1): audit: Remove leftover reference to the audit_tasklet Yang Yang (1): audit: Make audit_filter_syscall() return void Zheng Yongjun (1): kernel/audit: convert comma to semicolon kernel/audit.c | 4 ++-- kernel/auditsc.c | 16 2 files changed, 10 insertions(+), 10 deletions(-) -- paul moore www.paul-moore.com
[GIT PULL] SELinux patches for v5.12
Hi Linus, We've got a good handful of patches for SELinux this time around; with everything passing the selinux-testsuite and applying cleanly to your tree as of a few minutes ago. The highlights are below: - Add support for labeling anonymous inodes, and extend this new support to userfaultfd. - Fallback to SELinux genfs file labeling if the filesystem does not have xattr support. This is useful for virtiofs which can vary in its xattr support depending on the backing filesystem. - Classify and handle MPTCP the same as TCP in SELinux. - Ensure consistent behavior between inode_getxattr and inode_listsecurity when the SELinux policy is not loaded. This fixes a known problem with overlayfs. - A couple of patches to prune some unused variables from the SELinux code, mark private variables as static, and mark other variables as __ro_after_init or __read_mostly. Thanks, -Paul -- The following changes since commit e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62: Linux 5.11-rc2 (2021-01-03 15:55:30 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20210215 for you to fetch changes up to 365982aba1f264dba26f0908700d62bfa046918c: fs: anon_inodes: rephrase to appropriate kernel-doc (2021-01-15 12:17:25 -0500) selinux/stable-5.12 PR 20210215 Amir Goldstein (1): selinux: fix inconsistency between inode_getxattr and inode_listsecurity Daniel Colascione (3): fs: add LSM-supporting anon-inode interface selinux: teach SELinux about anonymous inodes userfaultfd: use secure anon inodes for userfaultfd Lokesh Gidra (1): security: add inode_init_security_anon() LSM hook Lukas Bulwahn (1): fs: anon_inodes: rephrase to appropriate kernel-doc Ondrej Mosnacek (6): selinux: remove unused global variables selinux: drop the unnecessary aurule_callback variable selinux: make selinuxfs_mount static selinux: mark some global variables __ro_after_init selinux: mark selinux_xfrm_refcount as __read_mostly selinux: fall back to SECURITY_FS_USE_GENFS if no xattr support Paolo Abeni (1): selinux: handle MPTCP consistently with TCP fs/anon_inodes.c| 157 + fs/libfs.c | 5 -- fs/userfaultfd.c| 19 ++--- include/linux/anon_inodes.h | 5 ++ include/linux/lsm_hook_defs.h | 2 + include/linux/lsm_hooks.h | 9 +++ include/linux/security.h| 10 +++ security/security.c | 8 ++ security/selinux/avc.c | 10 +-- security/selinux/hooks.c| 141 - security/selinux/ibpkey.c | 1 - security/selinux/include/classmap.h | 2 + security/selinux/include/security.h | 1 - security/selinux/netif.c| 1 - security/selinux/netlink.c | 2 +- security/selinux/netnode.c | 1 - security/selinux/netport.c | 1 - security/selinux/selinuxfs.c| 4 +- security/selinux/ss/avtab.c | 4 +- security/selinux/ss/ebitmap.c | 2 +- security/selinux/ss/hashtab.c | 2 +- security/selinux/ss/services.c | 10 +-- security/selinux/xfrm.c | 2 +- 23 files changed, 294 insertions(+), 105 deletions(-) -- paul moore www.paul-moore.com
Re: [PATCH ghak124 v3] audit: log nftables configuration change events
On Thu, Feb 11, 2021 at 10:16 AM Phil Sutter wrote: > Hi, > > On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote: > > iptables, ip6tables, arptables and ebtables table registration, > > replacement and unregistration configuration events are logged for the > > native (legacy) iptables setsockopt api, but not for the > > nftables netlink api which is used by the nft-variant of iptables in > > addition to nftables itself. > > > > Add calls to log the configuration actions in the nftables netlink api. > > As discussed offline already, these audit notifications are pretty hefty > performance-wise. In an internal report, 300% restore time of a ruleset > containing 70k set elements is measured. If you're going to reference offline/off-list discussions in a post to a public list, perhaps the original discussion shouldn't have been off-list ;) If you don't involve us in the discussion, we have to waste a lot of time getting caught up. > If I'm not mistaken, iptables emits a single audit log per table, ipset > doesn't support audit at all. So I wonder how much audit logging is > required at all (for certification or whatever reason). How much > granularity is desired? That's a question for the people who track these certification requirements, which is thankfully not me at the moment. Unless somebody else wants to speak up, Steve Grubb is probably the only person who tracks that sort of stuff and comments here. I believe the netfilter auditing was mostly a nice-to-have bit of functionality to help add to the completeness of the audit logs, but I could very easily be mistaken. Richard put together those patches, he can probably provide the background/motivation for the effort. > I personally would notify once per transaction. This is easy and quick. > Once per table or chain should be acceptable, as well. At the very > least, we should not have to notify once per each element. This is the > last resort of fast ruleset adjustments. If we lose it, people are > better off with ipset IMHO. > > Unlike nft monitor, auditd is not designed to be disabled "at will". So > turning it off for performance-critical workloads is no option. Patches are always welcome, but it might be wise to get to the bottom of the certification requirements first. -- paul moore www.paul-moore.com
Re: [PATCH v2] selinux: measure state and policy capabilities
On Fri, Jan 29, 2021 at 11:49 AM Lakshmi Ramasubramanian
wrote:
>
> SELinux stores the configuration state and the policy capabilities
> in kernel memory. Changes to this data at runtime would have an impact
> on the security guarantees provided by SELinux. Measuring this data
> through IMA subsystem provides a tamper-resistant way for
> an attestation service to remotely validate it at runtime.
>
> Measure the configuration state and policy capabilities by calling
> the IMA hook ima_measure_critical_data().
>
> To enable SELinux data measurement, the following steps are required:
>
> 1, Add "ima_policy=critical_data" to the kernel command line arguments
> to enable measuring SELinux data at boot time.
> For example,
> BOOT_IMAGE=/boot/vmlinuz-5.11.0-rc3+
> root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux
> ima_policy=critical_data
>
> 2, Add the following rule to /etc/ima/ima-policy
>measure func=CRITICAL_DATA label=selinux
>
> Sample measurement of SELinux state and policy capabilities:
>
> 10 2122...65d8 ima-buf sha256:13c2...1292 selinux-state 696e...303b
>
> Execute the following command to extract the measured data
> from the IMA's runtime measurements list:
>
> grep "selinux-state"
> /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut
> -d' ' -f 6 | xxd -r -p
>
> The output should be a list of key-value pairs. For example,
>
> initialized=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>
> To verify the measurement is consistent with the current SELinux state
> reported on the system, compare the integer values in the following
> files with those set in the IMA measurement (using the following commands):
>
> - cat /sys/fs/selinux/enforce
> - cat /sys/fs/selinux/checkreqprot
> - cat /sys/fs/selinux/policy_capabilities/[capability_file]
>
> Note that the actual verification would be against an expected state
> and done on a separate system (likely an attestation server) requiring
> "initialized=1;enforcing=1;checkreqprot=0;"
> for a secure state and then whatever policy capabilities are actually
> set in the expected policy (which can be extracted from the policy
> itself via seinfo, for example).
>
> Signed-off-by: Lakshmi Ramasubramanian
> Suggested-by: Stephen Smalley
> Suggested-by: Paul Moore
> ---
> security/selinux/ima.c | 77 --
> security/selinux/include/ima.h | 6 +++
> security/selinux/selinuxfs.c | 6 +++
> security/selinux/ss/services.c | 2 +-
> 4 files changed, 86 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> index 03715893ff97..5c7f73cd1117 100644
> --- a/security/selinux/ima.c
> +++ b/security/selinux/ima.c
> @@ -13,18 +13,73 @@
> #include "ima.h"
>
> /*
> - * selinux_ima_measure_state - Measure hash of the SELinux policy
> + * selinux_ima_collect_state - Read selinux configuration settings
> *
> - * @state: selinux state struct
> + * @state: selinux_state
> *
> - * NOTE: This function must be called with policy_mutex held.
> + * On success returns the configuration settings string.
> + * On error, returns NULL.
> */
> -void selinux_ima_measure_state(struct selinux_state *state)
> +static char *selinux_ima_collect_state(struct selinux_state *state)
> +{
> + const char *on = "=1;", *off = "=0;";
> + char *buf;
> + int buf_len, i;
> +
> + /*
> +* Size of the following string including the terminating NULL char
> +*initialized=0;enforcing=0;checkreqprot=0;
> +*/
> + buf_len = 42;
It might be safer over the long term, and self-documenting, to do the
following instead:
buf_len = strlen("initialized=0;enforcing=0;checkreqprot=0;") + 1;
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++)
> + buf_len += strlen(selinux_policycap_names[i]) + 3;
's/3/strlen(on)/' or is that too much?
> +
> + buf = kzalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + return NULL;
> +
> + strscpy(buf, "initialized", buf_len);
I wonder if it might be a good idea to add a WARN_ON() to the various
copies, e.g.:
rc = strXXX(...);
WARN_ON(rc);
The strscpy/strlcat protections should ensure that nothing terrible
happens with respect to wandering off the end of the string, or
failing to NUL terminate, but they won't catch a logic error where the
string is not allocated correctly (resulting in a trunca
Re: [PATCH 1/2] audit: show user land backtrace as part of audit context messages
On Tue, Feb 2, 2021 at 4:44 PM Daniel Walker (danielwa) wrote: > On Tue, Feb 02, 2021 at 04:35:42PM -0500, Paul Moore wrote: > > On Tue, Feb 2, 2021 at 4:29 PM Daniel Walker wrote: > > > From: Victor Kamensky > > > > > > To efficiently find out where SELinux AVC denial is comming from > > > take backtrace of user land process and display it as type=UBACKTRACE > > > message that comes as audit context for SELinux AVC and other audit > > > messages ... > > > > Have you tried the new perf tracepoint for SELinux AVC decisions that > > trigger an audit event? It's a new feature for v5.10 and looks to > > accomplish most of what you are looking for with this patch. > > > > * https://www.paul-moore.com/blog/d/2020/12/linux_v510.html > > We haven't tried it, but I can look into it. We're not using v5.10 extensively > yet. Let us know if that works for you, and if it doesn't, let us know what might be missing. I hate seeing the kernel grow multiple features which do the same thing. -- paul moore www.paul-moore.com
Re: [PATCH 2/2] audit: show (grand)parents information of an audit context
On Tue, Feb 2, 2021 at 4:29 PM Daniel Walker wrote: > From: Phil Zhang > > To ease the root cause analysis of SELinux AVCs, this new feature > traverses task structs to iteratively find all parent processes > starting with the denied process and ending at the kernel. Meanwhile, > it prints out the command lines and subject contexts of those parents. > > This provides developers a clear view of how processes were spawned > and where transitions happened, without the need to reproduce the > issue and manually audit interesting events. > > Example on bash over ssh: > $ runcon -u system_u -r system_r -t polaris_hm_t ls > ... > type=PARENT msg=audit(1610548241.033:255): > subj=root:unconfined_r:unconfined_t:s0-s0:c0.c1023 cmdline="-bash" > type=PARENT msg=audit(1610548241.033:255): > subj=system_u:system_r:sshd_t:s0-s0:c0.c1023cmdline="sshd: root@pts/0" > type=PARENT msg=audit(1610548241.033:255): > subj=system_u:system_r:sshd_t:s0-s0:c0.c1023 > cmdline="/tmp/sw/rp/0/0/rp_security/mount/usr/sbin/sshd > type=PARENT msg=audit(1610548241.033:255): > subj=system_u:system_r:init_t:s0cmdline="/init" > type=PARENT msg=audit(1610548241.033:255): > subj=system_u:system_r:kernel_t:s0 > ... > > Cc: xe-linux-exter...@cisco.com > Signed-off-by: Phil Zhang > Signed-off-by: Daniel Walker > --- > include/uapi/linux/audit.h | 5 ++- > init/Kconfig | 7 + > kernel/audit.c | 3 +- > kernel/auditsc.c | 64 ++ > 4 files changed, 77 insertions(+), 2 deletions(-) This is just for development/testing of SELinux policy, right? It seems like this is better done in userspace to me through a combination of policy analysis and just understanding of how your system is put together. If you really need this information in the audit log for some production use, it seems like you could audit the various fork()/exec() syscalls to get an understanding of the various process (sub)trees on the system. It would require a bit of work to sift through the audit log and reconstruct the events that led to a process being started, and generating the AVC you are interested in debugging, but folks who live The Audit Life supposedly do this sort of thing a lot (this sort of thing being tracing a process/session). -- paul moore www.paul-moore.com
Re: [PATCH 1/2] audit: show user land backtrace as part of audit context messages
On Tue, Feb 2, 2021 at 4:29 PM Daniel Walker wrote: > From: Victor Kamensky > > To efficiently find out where SELinux AVC denial is comming from > take backtrace of user land process and display it as type=UBACKTRACE > message that comes as audit context for SELinux AVC and other audit > messages ... Have you tried the new perf tracepoint for SELinux AVC decisions that trigger an audit event? It's a new feature for v5.10 and looks to accomplish most of what you are looking for with this patch. * https://www.paul-moore.com/blog/d/2020/12/linux_v510.html -- paul moore www.paul-moore.com
Re: [PATCH] selinux: measure state and policy capabilities
On Sun, Jan 24, 2021 at 12:04 PM Lakshmi Ramasubramanian wrote: > On 1/22/21 1:21 PM, Paul Moore wrote: ... > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> index 644b17ec9e63..879a0d90615d 100644 > >> --- a/security/selinux/hooks.c > >> +++ b/security/selinux/hooks.c > >> @@ -7407,6 +7408,10 @@ int selinux_disable(struct selinux_state *state) > >> > >> selinux_mark_disabled(state); > >> > >> + mutex_lock(>policy_mutex); > >> + selinux_ima_measure_state(state); > >> + mutex_unlock(>policy_mutex); > > > > I'm not sure if this affects your decision to include this action in > > the measurements, but this function is hopefully going away in the not > > too distant future as we do away with support for disabling SELinux at > > runtime. > > > > FWIW, I'm not sure it's overly useful anyway; you only get here if you > > never had any SELinux policy/state configured and you decide to > > disable SELinux instead of loading a policy. However, I've got no > > objection to this code. > > If support for disabling SELinux at runtime will be removed, then I > don't see a reason to trigger a measurement here. I'll remove this > measurement. It's currently marked as deprecated, see Documentation/ABI/obsolete/sysfs-selinux-disable. -- paul moore www.paul-moore.com
Re: [PATCH v2] audit: Make audit_filter_syscall() return void
On Tue, Jan 26, 2021 at 9:52 PM wrote: > > From: Yang Yang > > No invoker uses the return value of audit_filter_syscall(). > So make it return void, and amend the comment of > audit_filter_syscall(). > > Changes since v1: > - amend the comment of audit_filter_syscall(). > > Signed-off-by: Yang Yang > Reviewed-by: Richard Guy Briggs > --- > kernel/auditsc.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) This is a simple enough patch so I think merging it during -rc5 should be okay; merged into audit/next - thanks! -- paul moore www.paul-moore.com
Re: [RFC,v3,1/1] audit: speed up syscall rule filtering
On Sun, Jan 24, 2021 at 8:04 AM wrote: > > From 85b3eccf7f12b091b78cc5ba8abfaf759cf0334e Mon Sep 17 00:00:00 2001 > From: Yang Yang > Date: Sun, 24 Jan 2021 20:40:50 +0800 > Subject: [PATCH] audit: speed up syscall rule filtering > audit_filter_syscall() traverses struct list_head audit_filter_list to find > out whether current syscall match one rule. This takes o(n), which is not > necessary, specially for user who add a very few syscall rules. On the other > hand, user may not much care about rule add/delete speed. So do o(n) > calculates when rule changes, and ease the burden of audit_filter_syscall(). > > Define audit_rule_syscall_mask[NR_syscalls], every element stands for > one syscall.audit_rule_syscall_mask[n] == 0 indicates no rule cares about > syscall n, so we can avoid unnecessary calling audit_filter_syscall(). > audit_rule_syscall_mask[n] > 0 indicates at least one rule cares about > syscall n, then calls audit_filter_syscall(). Update > audit_rule_syscall_mask[n] when syscall rule changes. > > Signed-off-by: Yang Yang > --- > include/linux/audit.h | 3 +++ > kernel/auditfilter.c | 4 > kernel/auditsc.c | 36 > 3 files changed, 39 insertions(+), 4 deletions(-) ... > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ce8c9e2..1b8ff4e 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -1627,8 +1653,9 @@ void __audit_free(struct task_struct *tsk) > context->return_valid = AUDITSC_INVALID; > context->return_code = 0; > > - audit_filter_syscall(tsk, context, > -_filter_list[AUDIT_FILTER_EXIT]); > + if (unlikely(audit_rule_syscall_mask[context->major])) > + audit_filter_syscall(tsk, context, > + > _filter_list[AUDIT_FILTER_EXIT]); > audit_filter_inodes(tsk, context); > if (context->current_state == AUDIT_RECORD_CONTEXT) > audit_log_exit(); > @@ -1735,8 +1762,9 @@ void __audit_syscall_exit(int success, long return_code) > else > context->return_code = return_code; > > - audit_filter_syscall(current, context, > -_filter_list[AUDIT_FILTER_EXIT]); > + if (unlikely(audit_rule_syscall_mask[context->major])) > + audit_filter_syscall(current, context, > + > _filter_list[AUDIT_FILTER_EXIT]); > audit_filter_inodes(current, context); > if (context->current_state == AUDIT_RECORD_CONTEXT) > audit_log_exit(); Looking at this revision I believe I may not have been as clear as I should have been with my last suggestion. Let me try to do better here. Thus far I'm not very happy with the audit_rule_syscall_mask[] additions; it looks both wasteful and inelegant to me at the moment. I would much rather see if we can improve the existing code by fixing inefficiencies in how we handle the rule filtering. This is why my previous comments suggested looking at the audit_filter_syscall() and audit_filter_inodes() calls in __audit_free() and __audit_syscall_exit(), the latter of course being more important due to its frequency. In both cases an audit_filter_inode() AUDIT_RECORD_CONTEXT decision takes precedence over any audit_filter_syscall() decision due to the code being structured as so: audit_filter_syscall(...); audit_filter_inodes(...); if (state == AUDIT_RECORD_CONTEXT) audit_log_exit(); ... my suggestion is to investigate what performance benefits might be had by leveraging this precedence, for example: audit_filter_inodes(...); if (state != AUDIT_RECORD_CONTEXT) audit_filter_syscall(...); if (state == AUDIT_RECORD_CONTEXT) audit_log_exit(); ... of course I would expect the performance to be roughly the same when there is no matching rule, but I think there would be a performance when in those cases where a watched inode triggers an audit rule. Beyond that, there is probably work we could do to combine some aspects of audit_filter_syscall() and audit_filter_inodes() to eliminate some redundancy, e.g. reduce the number of audit_in_mask() calls. Actually looking a bit closer there are a number of improvements that could likely be made, some might have some performance impacts. Let me know if you are going to pursue the suggestion above about reordering the audit_filter_*() functions as I'll hold off on the other changes. -- paul moore www.paul-moore.com
Re: [PATCH v24 21/25] audit: add support for non-syscall auxiliary records
On Tue, Jan 26, 2021 at 1:58 PM Casey Schaufler wrote: > > On 1/26/2021 10:42 AM, Richard Guy Briggs wrote: > > On 2021-01-26 08:41, Casey Schaufler wrote: > >> Standalone audit records have the timestamp and serial number generated > >> on the fly and as such are unique, making them standalone. This new > >> function audit_alloc_local() generates a local audit context that will > >> be used only for a standalone record and its auxiliary record(s). The > >> context is discarded immediately after the local associated records are > >> produced. > >> > >> Signed-off-by: Richard Guy Briggs > >> Signed-off-by: Casey Schaufler > >> Cc: linux-au...@redhat.com > >> To: Richard Guy Briggs > > This has been minorly bothering me for several revisions... Is there a > > way for the development/authorship to be accurately reflected > > if/when this patch is merged before the contid patch set? > > I don't know the right way to do that because I had to pull > some of what was in the original patch out. Any way you would > like it done is fine with me. I'm not sure if there is one perfect way. I typically see either a "From: " line if the author is different from the submitter, or in more complex cases such as this it seems like a simple note giving credit in the description might be the best option. -- paul moore www.paul-moore.com
Re: [PATCH] selinux: measure state and policy capabilities
ithin
a single file. The existing function is prefixed with "selinux_ima_"
perhaps we can do something similar here?
"selinux_ima_collect_state()" or something similar perhaps?
Perhaps instead of returning zero on success you could return the
length of the generated string? It's not a big deal, but it saves an
argument for whatever that is worth these days. I also might pass the
state as the first argument and the generated string pointer as the
second argument, but that is pretty nit-picky.
> +static int read_selinux_state(char **state_str, int *state_str_len,
> + struct selinux_state *state)
> +{
> + char *buf;
> + int i, buf_len, curr;
> + bool initialized = selinux_initialized(state);
> + bool enabled = !selinux_disabled(state);
> + bool enforcing = enforcing_enabled(state);
> + bool checkreqprot = checkreqprot_get(state);
> +
> + buf_len = snprintf(NULL, 0, "%s=%d;%s=%d;%s=%d;%s=%d;",
> + "initialized", initialized,
> + "enabled", enabled,
> + "enforcing", enforcing,
> + "checkreqprot", checkreqprot);
> +
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> + buf_len += snprintf(NULL, 0, "%s=%d;",
> + selinux_policycap_names[i],
> + state->policycap[i]);
> + }
> + ++buf_len;
With all of the variables you are measuring being booleans, it seems
like using snprintf() is a bit overkill, no? What about a series of
strlen() calls with additional constants for the booleans and extra
bits? For example:
buf_len = 1; // '\0';
buf_len += strlen("foo") + 3; // "foo=0;"
buf_len += strlen("bar") + 3; // "bar=0;"
Not that it matters a lot here, but the above must be more efficient
than calling snprintf().
> + buf = kzalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + curr = scnprintf(buf, buf_len, "%s=%d;%s=%d;%s=%d;%s=%d;",
> +"initialized", initialized,
> +"enabled", enabled,
> +"enforcing", enforcing,
> +"checkreqprot", checkreqprot);
> +
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> + curr += scnprintf((buf + curr), (buf_len - curr), "%s=%d;",
> + selinux_policycap_names[i],
> + state->policycap[i]);
> + }
Similarly, you could probably replace all of this with
strcat()/strlcat() calls since you don't have to render an integer
into a string.
> + *state_str = buf;
> + *state_str_len = curr;
> +
> + return 0;
> +}
> +
> /*
> * selinux_ima_measure_state - Measure hash of the SELinux policy
> *
> @@ -21,10 +75,24 @@
> */
> void selinux_ima_measure_state(struct selinux_state *state)
> {
> + char *state_str = NULL;
> + int state_str_len;
> void *policy = NULL;
> size_t policy_len;
> int rc = 0;
>
> + rc = read_selinux_state(_str, _str_len, state);
> + if (rc) {
> + pr_err("SELinux: %s: failed to read state %d.\n",
> + __func__, rc);
> + return;
> + }
> +
> + ima_measure_critical_data("selinux", "selinux-state",
> + state_str, state_str_len, false);
> +
> + kfree(state_str);
> +
> /*
> * Measure SELinux policy only after initialization is completed.
> */
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 4bde570d56a2..8b561e1c2caa 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -41,6 +41,7 @@
> #include "security.h"
> #include "objsec.h"
> #include "conditional.h"
> +#include "ima.h"
>
> enum sel_inos {
> SEL_ROOT_INO = 2,
> @@ -182,6 +183,10 @@ static ssize_t sel_write_enforce(struct file *file,
> const char __user *buf,
> selinux_status_update_setenforce(state, new_value);
> if (!new_value)
> call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> +
> + mutex_lock(>policy_mutex);
> + selinux_ima_measure_state(state);
> + mutex_unlock(>policy_mutex);
> }
> length = count;
> out:
> @@ -762,6 +767,11 @@ static ssize_t sel_write_checkreqprot(struct file *file,
> const char __user *buf,
>
> checkreqprot_set(fsi->state, (new_value ? 1 : 0));
> length = count;
> +
> + mutex_lock(>state->policy_mutex);
> + selinux_ima_measure_state(fsi->state);
> + mutex_unlock(>state->policy_mutex);
> +
The lock-measure-unlock pattern appears enough that I wonder if we
should move the lock/unlock into selinux_ima_measure_state() and
create a new function, selinux_ima_measure_state_unlocked(), to cover
the existing case in selinux_notify_policy_change(). It would have
the advantage of not requiring a pointless lock/unlock in the case
where CONFIG_IMA=n.
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: include a consumer of the new IMA critical data hook
On Thu, Jan 14, 2021 at 2:15 PM Lakshmi Ramasubramanian wrote: > > SELinux stores the active policy in memory, so the changes to this data > at runtime would have an impact on the security guarantees provided > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > provides a secure way for the attestation service to remotely validate > the policy contents at runtime. > > Measure the hash of the loaded policy by calling the IMA hook > ima_measure_critical_data(). Since the size of the loaded policy > can be large (several MB), measure the hash of the policy instead of > the entire policy to avoid bloating the IMA log entry. > > To enable SELinux data measurement, the following steps are required: > > 1, Add "ima_policy=critical_data" to the kernel command line arguments >to enable measuring SELinux data at boot time. > For example, > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux > ima_policy=critical_data > > 2, Add the following rule to /etc/ima/ima-policy >measure func=CRITICAL_DATA label=selinux > > Sample measurement of the hash of SELinux policy: > > To verify the measured data with the current SELinux policy run > the following commands and verify the output hash values match. > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > grep "selinux-policy-hash" > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut > -d' ' -f 6 > > Note that the actual verification of SELinux policy would require loading > the expected policy into an identical kernel on a pristine/known-safe > system and run the sha256sum /sys/kernel/selinux/policy there to get > the expected hash. > > Signed-off-by: Lakshmi Ramasubramanian > Suggested-by: Stephen Smalley > Acked-by: Paul Moore > Reviewed-by: Tyler Hicks > --- > Documentation/ABI/testing/ima_policy | 3 +- > security/selinux/Makefile| 2 + > security/selinux/ima.c | 44 +++ > security/selinux/include/ima.h | 24 +++ > security/selinux/include/security.h | 3 +- > security/selinux/ss/services.c | 64 > 6 files changed, 129 insertions(+), 11 deletions(-) > create mode 100644 security/selinux/ima.c > create mode 100644 security/selinux/include/ima.h Hi Mimi, Just checking as I didn't see a reply to this from you in my inbox, you merged this into the IMA linux-next branch, yes? > diff --git a/Documentation/ABI/testing/ima_policy > b/Documentation/ABI/testing/ima_policy > index 54fe1c15ed50..8365596cb42b 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -52,8 +52,9 @@ Description: > template:= name of a defined IMA template type > (eg, ima-ng). Only valid when action is "measure". > pcr:= decimal value > - label:= [data_label] > + label:= [selinux]|[data_label] > data_label:= a unique string used for grouping and > limiting critical data. > + For example, "selinux" to measure critical data for > SELinux. > > default policy: > # PROC_SUPER_MAGIC > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > index 4d8e0e8adf0b..776162444882 100644 > --- a/security/selinux/Makefile > +++ b/security/selinux/Makefile > @@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o > > selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o > > +selinux-$(CONFIG_IMA) += ima.o > + > ccflags-y := -I$(srctree)/security/selinux > -I$(srctree)/security/selinux/include > > $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h > diff --git a/security/selinux/ima.c b/security/selinux/ima.c > new file mode 100644 > index ..03715893ff97 > --- /dev/null > +++ b/security/selinux/ima.c > @@ -0,0 +1,44 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Microsoft Corporation > + * > + * Author: Lakshmi Ramasubramanian (nra...@linux.microsoft.com) > + * > + * Measure critical data structures maintainted by SELinux > + * using IMA subsystem. > + */ > +#include > +#include > +#include "security.h" > +#include "ima.h" > + > +/* > + * selinux_ima_measure_state - Measure hash of the SELinux policy > + * > + * @state: selinux state struct > + * > + * NOTE: This function must be called with policy_mutex held. > + */ > +void selinux_ima_measure_state(struct selinux_state *state) > +
Re: Fw:Re:Fw:Re:[RFC,v1,1/1] audit: speed up syscall rule match while exiting syscall
On Thu, Jan 21, 2021 at 9:25 AM wrote:
>
> Thanks for reply, I have sent a new patch with better performance.
> The v1 patch uses mutex() is not necessary.
>
> Performance measurements:
> 1.Environment
> CPU: Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz
> Linux kernel version: 5.11-rc3
> Audit version: 2.8.4
...
> 2.2 Syscall absolute time
> Test method:
> Use ktime_get_real_ts64() in do_syscall_64() to calculate time.
> Run command "chmod 777 /etc/fstab" with chown rules. Each test 10times and
> get average.
>
> do_syscall_64() time with 100 rules:
> before this patch: 7604ns
> after this patch: 5244ns, reduce 2360ns.
>
> do_syscall_64() time with CIS rules:
> before this patch: 6710ns
> after this patch: 7293ns, increase 583ns.
>
> do_syscall_64() time with 10 rules:
> before this patch: 5382ns
> after this patch: 5171ns, reduce 211ns.
>
> do_syscall_64() time with 1 rule:
> before this patch: 5361ns
> after this patch: 5375ns, increase 14ns.
>
> do_syscall_64() time with no rules:
> before this patch: 4735ns
> after this patch: 4804ns, increase 69ns.
>
> Analyse:
> With a few rules, performance is close.
> With 100 rules, performance is better, but with CIS rules performance
> regress. Maybe relevant to certain syscall.
These numbers aren't particularly good in my opinion, the negative
impact of the change to a small number of rules and to the CIS ruleset
is not a good thing. It also should be said that you are increasing
the memory footprint, even if it is relatively small.
However, if we take a step back and look at the motivation for this
work I wonder if there are some things we can do to improve the
per-syscall rule processing performance. On thing that jumped out
just now was this code in __audit_syscall_exit():
void __audit_syscall_exit(int success, long return_code)
{
/* ... */
/*
* we need to fix up the return code in the audit logs if the
* actual return codes are later going to be fixed up by the
* arch specific signal handlers ... */
if (unlikely(return_code <= -ERESTARTSYS) &&
(return_code >= -ERESTART_RESTARTBLOCK) &&
(return_code != -ENOIOCTLCMD))
context->return_code = -EINTR;
else
context->return_code = return_code;
audit_filter_syscall(current, context,
_filter_list[AUDIT_FILTER_EXIT]);
audit_filter_inodes(current, context);
if (context->current_state == AUDIT_RECORD_CONTEXT)
audit_log_exit();
}
... in the snippet above the audit_filter_inodes() function/rules are
given priority over the syscall rules in
audit_filter_syscall(AUDIT_FILTER_EXIT), so why not first execute
audit_filter_inodes() and only execute
audit_filter_syscall(AUDIT_FILTER_EXIT) if necessary? It may be that
I'm missing something on this quick look at the code, but I think it
is worth investigating. It's also possible there are other similar
improvements to made.
There is similar code in __audit_free() but that should be less
performance critical.
--
paul moore
www.paul-moore.com
Re: [RFC,v2,1/1] audit: speed up syscall rule match while exiting syscall
On Thu, Jan 21, 2021 at 8:54 AM wrote:
>
> From 72f3ecde58edb03d76cb359607fef98c1663d481 Mon Sep 17 00:00:00 2001
> From: Yang Yang
> Date: Thu, 21 Jan 2021 21:05:04 +0800
> Subject: [PATCH] [RFC,v2,1/1] speed up syscall rule match while exiting
> syscall
> audit_filter_syscall() traverses struct list_head audit_filter_list to find
> out whether current syscall match one rule. This takes o(n), which is not
> necessary, specially for user who add a very few syscall rules. On the other
> hand, user may not much care about rule add/delete speed. So do o(n)
> calculate at rule changing, and ease the burden of audit_filter_syscall().
>
> Define audit_syscall[NR_syscalls], every element stands for one syscall.
> audit_filter_syscall() checks audit_syscall[NR_syscalls].
> audit_syscall[n] == 0 indicates no rule audit syscall n, do a quick exit.
> audit_syscall[n] > 0 indicates at least one rule audit syscall n.
> audit_syscall[n] update when syscall rule changes.
>
> Signed-off-by: Yang Yang
> ---
> include/linux/audit.h | 2 ++
> kernel/audit.c| 4
> kernel/auditfilter.c | 30 ++
> kernel/auditsc.c | 5 -
> 4 files changed, 40 insertions(+), 1 deletion(-)
...
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 333b3bc..9d3e703 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -926,6 +926,28 @@ static struct audit_entry *audit_find_rule(struct
> audit_entry *entry,
> static u64 prio_low = ~0ULL/2;
> static u64 prio_high = ~0ULL/2 - 1;
>
> +#ifdef CONFIG_AUDITSYSCALL
> +static inline void update_auditing_syscall(struct audit_krule rule, bool add)
> +{
> +int i;
> +
> +/* syscall rule with type AUDIT_FILTER_EXIT */
> +if (rule.listnr == AUDIT_FILTER_EXIT && !rule.watch && !rule.tree) {
> +for (i = 0; i < NR_syscalls; i++) {
> +/* whether this rule include one syscall */
> +if (unlikely(audit_in_mask(, i))) {
> +if (add == true)
> +auditing_syscall[i]++;
> +else
> +auditing_syscall[i]--;
> +}
> +}
> +}
> +
> +return;
> +}
> +#endif
> +
> /* Add rule to given filterlist if not a duplicate. */
> static inline int audit_add_rule(struct audit_entry *entry)
> {
> @@ -957,6 +979,10 @@ static inline int audit_add_rule(struct audit_entry
> *entry)
> return err;
> }
>
> +#ifdef CONFIG_AUDITSYSCALL
> +update_auditing_syscall(entry->rule, true);
> +#endif
I'm going to reply to your other email where we are discussing the
performance of this patch, but I wanted to make one comment about the
approach you've taken with the update_auditing_syscall() here.
First, naming things is hard, but the chosen name is not a good one in
my opinion. Something like audit_rule_syscall_mask_update() would
probably be a better fit.
Second, in order to minimize preprocessor clutter, it is better to use
the following pattern:
#ifdef CONFIG_FOO
int func(int arg)
{
/* important stuff */
}
#else
int func(int arg)
{
return 0; /* appropriate return value */
}
#endif
There are probably a few other comments on this patch, but I want us
to discuss the performance impacts of this first as I'm not convinced
this is a solution we want upstream.
--
paul moore
www.paul-moore.com
Re: [PATCH] fs: anon_inodes: rephrase to appropriate kernel-doc
On Fri, Jan 15, 2021 at 7:03 AM Lukas Bulwahn wrote:
>
> Commit e7e832ce6fa7 ("fs: add LSM-supporting anon-inode interface") adds
> more kerneldoc description, but also a few new warnings on
> anon_inode_getfd_secure() due to missing parameter descriptions.
>
> Rephrase to appropriate kernel-doc for anon_inode_getfd_secure().
>
> Signed-off-by: Lukas Bulwahn
> ---
> fs/anon_inodes.c | 21 ++---
> 1 file changed, 14 insertions(+), 7 deletions(-)
Merged into selinux/next with the other related LSM/SELinux anon-inode
patches, thank you!
--
paul moore
www.paul-moore.com
Re: [PATCH] audit: Remove leftover reference to the audit_tasklet
On Thu, Jan 14, 2021 at 7:12 PM Davidlohr Bueso wrote: > > This was replaced with a kauditd_wait kthread long ago, > back in: > > b7d1125817c (AUDIT: Send netlink messages from a separate kernel thread) > > Update the stale comment. > > Signed-off-by: Davidlohr Bueso > --- > kernel/audit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Nice catch. Merged into audit/next, thanks! > diff --git a/kernel/audit.c b/kernel/audit.c > index 1ffc2e059027..8fd735190c12 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -2365,7 +2365,7 @@ int audit_signal_info(int sig, struct task_struct *t) > * > * We can not do a netlink send inside an irq context because it blocks (last > * arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed on > a > - * queue and a tasklet is scheduled to remove them from the queue outside the > + * queue and a kthread is scheduled to remove them from the queue outside the > * irq context. May be called in any context. > */ > void audit_log_end(struct audit_buffer *ab) > -- > 2.26.2 -- paul moore www.paul-moore.com
Re: [PATCH v15 0/4] SELinux support for anonymous inodes and UFFD
On Tue, Jan 12, 2021 at 12:15 PM Paul Moore wrote: > > On Fri, Jan 8, 2021 at 5:22 PM Lokesh Gidra wrote: > > > > Userfaultfd in unprivileged contexts could be potentially very > > useful. We'd like to harden userfaultfd to make such unprivileged use > > less risky. This patch series allows SELinux to manage userfaultfd > > file descriptors and in the future, other kinds of > > anonymous-inode-based file descriptor. > > ... > > > Daniel Colascione (3): > > fs: add LSM-supporting anon-inode interface > > selinux: teach SELinux about anonymous inodes > > userfaultfd: use secure anon inodes for userfaultfd > > > > Lokesh Gidra (1): > > security: add inode_init_security_anon() LSM hook > > > > fs/anon_inodes.c| 150 > > fs/libfs.c | 5 - > > fs/userfaultfd.c| 19 ++-- > > include/linux/anon_inodes.h | 5 + > > include/linux/lsm_hook_defs.h | 2 + > > include/linux/lsm_hooks.h | 9 ++ > > include/linux/security.h| 10 ++ > > security/security.c | 8 ++ > > security/selinux/hooks.c| 57 +++ > > security/selinux/include/classmap.h | 2 + > > 10 files changed, 213 insertions(+), 54 deletions(-) > > With several rounds of reviews done and the corresponding SELinux test > suite looking close to being ready I think it makes sense to merge > this via the SELinux tree. VFS folks, if you have any comments or > objections please let me know soon. If I don't hear anything within > the next day or two I'll go ahead and merge this for linux-next. With no comments over the last two days I merged the patchset into selinux/next. Thanks for all your work and patience on this Lokesh. Also, it looks like you are very close to getting the associated SELinux test suite additions merged, please continue to work with Ondrej to get those merged soon. -- paul moore www.paul-moore.com
Re: [PATCH] selinux: include a consumer of the new IMA critical data hook
On Thu, Jan 14, 2021 at 2:15 PM Lakshmi Ramasubramanian wrote: > > SELinux stores the active policy in memory, so the changes to this data > at runtime would have an impact on the security guarantees provided > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > provides a secure way for the attestation service to remotely validate > the policy contents at runtime. > > Measure the hash of the loaded policy by calling the IMA hook > ima_measure_critical_data(). Since the size of the loaded policy > can be large (several MB), measure the hash of the policy instead of > the entire policy to avoid bloating the IMA log entry. > > To enable SELinux data measurement, the following steps are required: > > 1, Add "ima_policy=critical_data" to the kernel command line arguments >to enable measuring SELinux data at boot time. > For example, > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux > ima_policy=critical_data > > 2, Add the following rule to /etc/ima/ima-policy >measure func=CRITICAL_DATA label=selinux > > Sample measurement of the hash of SELinux policy: > > To verify the measured data with the current SELinux policy run > the following commands and verify the output hash values match. > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > grep "selinux-policy-hash" > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut > -d' ' -f 6 > > Note that the actual verification of SELinux policy would require loading > the expected policy into an identical kernel on a pristine/known-safe > system and run the sha256sum /sys/kernel/selinux/policy there to get > the expected hash. > > Signed-off-by: Lakshmi Ramasubramanian > Suggested-by: Stephen Smalley > Acked-by: Paul Moore > Reviewed-by: Tyler Hicks > --- > Documentation/ABI/testing/ima_policy | 3 +- > security/selinux/Makefile| 2 + > security/selinux/ima.c | 44 +++ > security/selinux/include/ima.h | 24 +++ > security/selinux/include/security.h | 3 +- > security/selinux/ss/services.c | 64 > 6 files changed, 129 insertions(+), 11 deletions(-) > create mode 100644 security/selinux/ima.c > create mode 100644 security/selinux/include/ima.h I think this has changed enough that keeping the "Acked-by" and "Reviewed-by" tags is probably not a good choice. I took a quick look and this still looks okay from a SELinux perspective, I'll leave Mimi to comment on it from a IMA perspective. Unless Tyler has reviewed this version prior to your posting, it might be a good idea to remove his "Reviewed-by" unless he has a chance to look this over again before it is merged. > diff --git a/Documentation/ABI/testing/ima_policy > b/Documentation/ABI/testing/ima_policy > index 54fe1c15ed50..8365596cb42b 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -52,8 +52,9 @@ Description: > template:= name of a defined IMA template type > (eg, ima-ng). Only valid when action is "measure". > pcr:= decimal value > - label:= [data_label] > + label:= [selinux]|[data_label] > data_label:= a unique string used for grouping and > limiting critical data. > + For example, "selinux" to measure critical data for > SELinux. > > default policy: > # PROC_SUPER_MAGIC > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > index 4d8e0e8adf0b..776162444882 100644 > --- a/security/selinux/Makefile > +++ b/security/selinux/Makefile > @@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o > > selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o > > +selinux-$(CONFIG_IMA) += ima.o > + > ccflags-y := -I$(srctree)/security/selinux > -I$(srctree)/security/selinux/include > > $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h > diff --git a/security/selinux/ima.c b/security/selinux/ima.c > new file mode 100644 > index ..03715893ff97 > --- /dev/null > +++ b/security/selinux/ima.c > @@ -0,0 +1,44 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Microsoft Corporation > + * > + * Author: Lakshmi Ramasubramanian (nra...@linux.microsoft.com) > + * > + * Measure critical data structures maintainted by SELinux > + * using IMA subsystem. > + */ > +#include > +#include > +#inclu
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Thu, Jan 14, 2021 at 11:44 AM Mimi Zohar wrote: > > [Cc'ing Sasha] > > Hi Lakshmi, > > On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote: > > On 1/13/21 6:49 PM, Mimi Zohar wrote: > > > >>> Lakshmi is trying to address the situation where an event changes a > > >>> value, but then is restored to the original value. The original and > > >>> subsequent events are measured, but restoring to the original value > > >>> isn't re-measured. This isn't any different than when a file is > > >>> modified and then reverted. > > >>> > > >>> Instead of changing the name like this, which doesn't work for files, > > >>> allowing duplicate measurements should be generic, based on policy. > > >> > > >> Perhaps it is just the end of the day and I'm a bit tired, but I just > > >> read all of the above and I have no idea what your current thoughts > > >> are regarding this patch. > > > > > > Other than appending the timestamp, which is a hack, the patch is fine. > > > Support for re-measuring an event can be upstreamed independently. > > > > > > > Thanks for clarifying the details related to duplicate measurement > > detection and re-measuring. > > > > I will keep the timestamp for the time being, even though its a hack, as > > it helps with re-measuring state changes in SELinux. We will add support > > for "policy driven" re-measurement as a subsequent patch series. > > Once including the timestamp is upstreamed, removing it will be > difficult, especially if different userspace applications are dependent > on it. Unless everyone is on board that removing the timestamp > wouldn't be considered a regression, it cannot be upstreamed. I'm not a fan of merging things which are known to be broken only with the promise of fixing it later. That goes double when the proper fix will result in a user visible breaking change. -- paul moore www.paul-moore.com
Re: Fw:Re:[RFC,v1,1/1] audit: speed up syscall rule match while exiting syscall
On Thu, Jan 14, 2021 at 8:25 AM wrote: > > Performance measurements: > 1.Environment > CPU: Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz > Linux kernel version: 5.11-rc3 > Audit version: 2.8.4 > > 2.Result > 2.1 Syscall invocations > Test method: > Run command "top" with no-load. > Add rule likes "auditctl -a always,exit -F arch=b64 -S chmod -F > auid=[number]" which doesn't hit audit. > User command "perf record -Rg -t [top's pid] sleep 900" to get > audit_filter_syscall()'s execute time ratio. Thanks for providing some performance numbers so quickly, a few comments and thoughts below ... > audit_filter_syscall() ratio with 100 rules: > before this patch: 15.29%. > after this patch: 0.88%, reduce 14.41%. > audit_filter_syscall() ratio with CIS[1] rules: > before this patch: 2.25%. > after this patch: 1.93%%, reduce 0.32%. > audit_filter_syscall() ratio with 10 rules: > before this patch: 0.94%. > after this patch: 1.02%, increase 0.08%. > audit_filter_syscall() ratio with 1 rule: > before this patch: 0.20%. > after this patch: 0.88%, increase 0.68%. If we assume the CIS rules to be a reasonable common case (I'm not sure if that is correct or not, but we'll skip that discussion for now), we see an performance improvement of 0.32% correct, yes? We also see a performance regression with small number of syscall rules that equalizes above ten rules, yes? On your system can you provide some absolute numbers? For example, what does 0.32% equate to in terms of wall clock time for a given syscall invocation? > Analyse: > With 1 rule, after this patch performance is worse, because > mutex_lock()/mutex_unlock(). But user just add one rule seems unusual. > With more rule, after this patch performance is improved.Typical likes > CIS benchmark. > > 2.2 Rule change > Test method: > Use ktime_get_real_ts64() before and after > audit_add_rule()/audit_del_rule() to calculate time. > Add/delete rule by command "auditctl". Each test 10times and get average. In this case I'm less concerned about micro benchmarks, and more interested in the wall clock time difference when running auditctl to add/remove rules. The difference here in the micro benchmark is not trivial, but with a delta of 4~5us it is possible that it is a small(er) percentage when compared to the total time spent executing auditctl. > audit_add_rule() time: > before this patch: 3120ns. > after this patch: 7783ns, increase 149%. > audit_del_rule() time: > before this patch: 3510ns. > after this patch: 8519ns, increase 143%. > > Analyse: > After this patch, rule change time obviously increase. But rule change > may not happen very often. > > [1] CIS is a Linux Benchmarks for security purpose. > https://www.cisecurity.org/benchmark/distribution_independent_linux/ -- paul moore www.paul-moore.com
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Wed, Jan 13, 2021 at 6:11 PM Mimi Zohar wrote: > On Wed, 2021-01-13 at 17:10 -0500, Paul Moore wrote: > > On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar wrote: > > > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote: > > > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar wrote: > > > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote: > > > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi > > > > > > wrote: > > > > > > > From: Lakshmi Ramasubramanian > > > > > > > > > > > > > > SELinux stores the active policy in memory, so the changes to > > > > > > > this data > > > > > > > at runtime would have an impact on the security guarantees > > > > > > > provided > > > > > > > by SELinux. Measuring in-memory SELinux policy through IMA > > > > > > > subsystem > > > > > > > provides a secure way for the attestation service to remotely > > > > > > > validate > > > > > > > the policy contents at runtime. > > > > > > > > > > > > > > Measure the hash of the loaded policy by calling the IMA hook > > > > > > > ima_measure_critical_data(). Since the size of the loaded policy > > > > > > > can be large (several MB), measure the hash of the policy instead > > > > > > > of > > > > > > > the entire policy to avoid bloating the IMA log entry. > > > > > > > > > > > > > > To enable SELinux data measurement, the following steps are > > > > > > > required: > > > > > > > > > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line > > > > > > > arguments > > > > > > >to enable measuring SELinux data at boot time. > > > > > > > For example, > > > > > > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > > > > > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > > > > > > security=selinux ima_policy=critical_data > > > > > > > > > > > > > > 2, Add the following rule to /etc/ima/ima-policy > > > > > > >measure func=CRITICAL_DATA label=selinux > > > > > > > > > > > > > > Sample measurement of the hash of SELinux policy: > > > > > > > > > > > > > > To verify the measured data with the current SELinux policy run > > > > > > > the following commands and verify the output hash values match. > > > > > > > > > > > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > > > > > > > > > > > grep "selinux-policy-hash" > > > > > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | > > > > > > > tail -1 | cut -d' ' -f 6 > > > > > > > > > > > > > > Note that the actual verification of SELinux policy would require > > > > > > > loading > > > > > > > the expected policy into an identical kernel on a > > > > > > > pristine/known-safe > > > > > > > system and run the sha256sum /sys/kernel/selinux/policy there to > > > > > > > get > > > > > > > the expected hash. > > > > > > > > > > > > > > Signed-off-by: Lakshmi Ramasubramanian > > > > > > > > > > > > > > Suggested-by: Stephen Smalley > > > > > > > Reviewed-by: Tyler Hicks > > > > > > > --- > > > > > > > Documentation/ABI/testing/ima_policy | 3 +- > > > > > > > security/selinux/Makefile| 2 + > > > > > > > security/selinux/ima.c | 64 > > > > > > > > > > > > > > security/selinux/include/ima.h | 24 +++ > > > > > > > security/selinux/include/security.h | 3 +- > > > > > > > security/selinux/ss/services.c | 64 > > > > > > > > > > > > > > 6 files changed, 149 insertions(+), 11 deletions(-) > > > > > > > create mode 100644 security/selinu
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar wrote: > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote: > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar wrote: > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote: > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi > > > > wrote: > > > > > From: Lakshmi Ramasubramanian > > > > > > > > > > SELinux stores the active policy in memory, so the changes to this > > > > > data > > > > > at runtime would have an impact on the security guarantees provided > > > > > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > > > > > provides a secure way for the attestation service to remotely validate > > > > > the policy contents at runtime. > > > > > > > > > > Measure the hash of the loaded policy by calling the IMA hook > > > > > ima_measure_critical_data(). Since the size of the loaded policy > > > > > can be large (several MB), measure the hash of the policy instead of > > > > > the entire policy to avoid bloating the IMA log entry. > > > > > > > > > > To enable SELinux data measurement, the following steps are required: > > > > > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments > > > > >to enable measuring SELinux data at boot time. > > > > > For example, > > > > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > > > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > > > > security=selinux ima_policy=critical_data > > > > > > > > > > 2, Add the following rule to /etc/ima/ima-policy > > > > >measure func=CRITICAL_DATA label=selinux > > > > > > > > > > Sample measurement of the hash of SELinux policy: > > > > > > > > > > To verify the measured data with the current SELinux policy run > > > > > the following commands and verify the output hash values match. > > > > > > > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > > > > > > > grep "selinux-policy-hash" > > > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail > > > > > -1 | cut -d' ' -f 6 > > > > > > > > > > Note that the actual verification of SELinux policy would require > > > > > loading > > > > > the expected policy into an identical kernel on a pristine/known-safe > > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get > > > > > the expected hash. > > > > > > > > > > Signed-off-by: Lakshmi Ramasubramanian > > > > > Suggested-by: Stephen Smalley > > > > > Reviewed-by: Tyler Hicks > > > > > --- > > > > > Documentation/ABI/testing/ima_policy | 3 +- > > > > > security/selinux/Makefile| 2 + > > > > > security/selinux/ima.c | 64 > > > > > > > > > > security/selinux/include/ima.h | 24 +++ > > > > > security/selinux/include/security.h | 3 +- > > > > > security/selinux/ss/services.c | 64 > > > > > > > > > > 6 files changed, 149 insertions(+), 11 deletions(-) > > > > > create mode 100644 security/selinux/ima.c > > > > > create mode 100644 security/selinux/include/ima.h > > > > > > > > I remain concerned about the possibility of bypassing a measurement by > > > > tampering with the time, but I appear to be the only one who is > > > > worried about this so I'm not going to block this patch on those > > > > grounds. > > > > > > > > Acked-by: Paul Moore > > > > > > Thanks, Paul. > > > > > > Including any unique string would cause the buffer hash to change, > > > forcing a new measurement. Perhaps they were concerned with > > > overflowing a counter. > > > > My understanding is that Lakshmi wanted to force a new measurement > > each time and felt using a timestamp would be the best way to do that. > > A counter, even if it wraps, would have a different value each time > > whereas a timestamp is vulnerable to time adjustments. While a > > properly controlled and audited system could be configured and > > monitored to detect such an event (I *think*), why rely on that if it > > isn't necessary? > > Why are you saying that even if the counter wraps a new measurement is > guaranteed. I agree with the rest of what you said. I was assuming that the IMA code simply compares the passed "policy_event_name" value to the previous value, if they are different a new measurement is taken, if they are the same the measurement request is ignored. If this is the case the counter value is only important in as much as that it is different from the previous value, even simply toggling a single bit back and forth would suffice in this case. IMA doesn't keep a record of every previous "policy_event_name" value does it? Am I misunderstanding how ima_measure_critical_data(...) works? -- paul moore www.paul-moore.com
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar wrote: > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote: > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi > > wrote: > > > From: Lakshmi Ramasubramanian > > > > > > SELinux stores the active policy in memory, so the changes to this data > > > at runtime would have an impact on the security guarantees provided > > > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > > > provides a secure way for the attestation service to remotely validate > > > the policy contents at runtime. > > > > > > Measure the hash of the loaded policy by calling the IMA hook > > > ima_measure_critical_data(). Since the size of the loaded policy > > > can be large (several MB), measure the hash of the policy instead of > > > the entire policy to avoid bloating the IMA log entry. > > > > > > To enable SELinux data measurement, the following steps are required: > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments > > >to enable measuring SELinux data at boot time. > > > For example, > > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > > security=selinux ima_policy=critical_data > > > > > > 2, Add the following rule to /etc/ima/ima-policy > > >measure func=CRITICAL_DATA label=selinux > > > > > > Sample measurement of the hash of SELinux policy: > > > > > > To verify the measured data with the current SELinux policy run > > > the following commands and verify the output hash values match. > > > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > > > grep "selinux-policy-hash" > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | > > > cut -d' ' -f 6 > > > > > > Note that the actual verification of SELinux policy would require loading > > > the expected policy into an identical kernel on a pristine/known-safe > > > system and run the sha256sum /sys/kernel/selinux/policy there to get > > > the expected hash. > > > > > > Signed-off-by: Lakshmi Ramasubramanian > > > Suggested-by: Stephen Smalley > > > Reviewed-by: Tyler Hicks > > > --- > > > Documentation/ABI/testing/ima_policy | 3 +- > > > security/selinux/Makefile| 2 + > > > security/selinux/ima.c | 64 > > > security/selinux/include/ima.h | 24 +++ > > > security/selinux/include/security.h | 3 +- > > > security/selinux/ss/services.c | 64 > > > 6 files changed, 149 insertions(+), 11 deletions(-) > > > create mode 100644 security/selinux/ima.c > > > create mode 100644 security/selinux/include/ima.h > > > > I remain concerned about the possibility of bypassing a measurement by > > tampering with the time, but I appear to be the only one who is > > worried about this so I'm not going to block this patch on those > > grounds. > > > > Acked-by: Paul Moore > > Thanks, Paul. > > Including any unique string would cause the buffer hash to change, > forcing a new measurement. Perhaps they were concerned with > overflowing a counter. My understanding is that Lakshmi wanted to force a new measurement each time and felt using a timestamp would be the best way to do that. A counter, even if it wraps, would have a different value each time whereas a timestamp is vulnerable to time adjustments. While a properly controlled and audited system could be configured and monitored to detect such an event (I *think*), why rely on that if it isn't necessary? -- paul moore www.paul-moore.com
Re: [RFC,v1,1/1] audit: speed up syscall rule match while exiting syscall
On Wed, Jan 13, 2021 at 7:39 AM wrote: > From 82ebcf43481be21ee3e32ec1749b42f651737880 Mon Sep 17 00:00:00 2001 > From: Yang Yang > Date: Wed, 13 Jan 2021 20:18:04 +0800 > Subject: [PATCH] [RFC,v1,1/1] speed up syscall rule match while exiting > syscall > If user add any syscall rule, in all syscalls, audit_filter_syscall() > traverses struct list_head audit_filter_list to find out whether current > syscall match one rule. This takes o(n), which is not necessary, specially > for user who add a very few syscall rules. On the other hand, user may not > much care about rule add/delete speed. So do o(n) calculate at rule changing, > and ease the burden of audit_filter_syscall(). > > Define audit_syscall[NR_syscalls], every element stands for one syscall. > audit_filter_syscall() checks audit_syscall[NR_syscalls]. > audit_syscall[n] == 0 indicates no rule audit syscall n, do a quick exit. > audit_syscall[n] > 0 indicates at least one rule audit syscall n. > audit_syscall[n] update when syscall rule changes. > > Signed-off-by: Yang Yang > --- > include/linux/audit.h | 2 ++ > kernel/audit.c| 2 ++ > kernel/auditfilter.c | 16 > kernel/auditsc.c | 9 - > 4 files changed, 28 insertions(+), 1 deletion(-) Before we go too far into a review of this patch, please provide some performance measurements using a variety of rule counts, both common and extreme, so that we can better judge the benefits of this patch. The measurements should include both the rule add/delete time deltas as well as the impact on the syscall invocations. If non-obvious, please also include how you performed the measurements and captured the data. These are good things to include in the commit description when submitting patches focused on improving performance. -- paul moore www.paul-moore.com
Re: [PATCH v15 0/4] SELinux support for anonymous inodes and UFFD
On Fri, Jan 8, 2021 at 5:22 PM Lokesh Gidra wrote: > > Userfaultfd in unprivileged contexts could be potentially very > useful. We'd like to harden userfaultfd to make such unprivileged use > less risky. This patch series allows SELinux to manage userfaultfd > file descriptors and in the future, other kinds of > anonymous-inode-based file descriptor. ... > Daniel Colascione (3): > fs: add LSM-supporting anon-inode interface > selinux: teach SELinux about anonymous inodes > userfaultfd: use secure anon inodes for userfaultfd > > Lokesh Gidra (1): > security: add inode_init_security_anon() LSM hook > > fs/anon_inodes.c| 150 > fs/libfs.c | 5 - > fs/userfaultfd.c| 19 ++-- > include/linux/anon_inodes.h | 5 + > include/linux/lsm_hook_defs.h | 2 + > include/linux/lsm_hooks.h | 9 ++ > include/linux/security.h| 10 ++ > security/security.c | 8 ++ > security/selinux/hooks.c| 57 +++ > security/selinux/include/classmap.h | 2 + > 10 files changed, 213 insertions(+), 54 deletions(-) With several rounds of reviews done and the corresponding SELinux test suite looking close to being ready I think it makes sense to merge this via the SELinux tree. VFS folks, if you have any comments or objections please let me know soon. If I don't hear anything within the next day or two I'll go ahead and merge this for linux-next. Thanks. -- paul moore www.paul-moore.com
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi wrote: > From: Lakshmi Ramasubramanian > > SELinux stores the active policy in memory, so the changes to this data > at runtime would have an impact on the security guarantees provided > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > provides a secure way for the attestation service to remotely validate > the policy contents at runtime. > > Measure the hash of the loaded policy by calling the IMA hook > ima_measure_critical_data(). Since the size of the loaded policy > can be large (several MB), measure the hash of the policy instead of > the entire policy to avoid bloating the IMA log entry. > > To enable SELinux data measurement, the following steps are required: > > 1, Add "ima_policy=critical_data" to the kernel command line arguments >to enable measuring SELinux data at boot time. > For example, > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux > ima_policy=critical_data > > 2, Add the following rule to /etc/ima/ima-policy >measure func=CRITICAL_DATA label=selinux > > Sample measurement of the hash of SELinux policy: > > To verify the measured data with the current SELinux policy run > the following commands and verify the output hash values match. > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > grep "selinux-policy-hash" > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut > -d' ' -f 6 > > Note that the actual verification of SELinux policy would require loading > the expected policy into an identical kernel on a pristine/known-safe > system and run the sha256sum /sys/kernel/selinux/policy there to get > the expected hash. > > Signed-off-by: Lakshmi Ramasubramanian > Suggested-by: Stephen Smalley > Reviewed-by: Tyler Hicks > --- > Documentation/ABI/testing/ima_policy | 3 +- > security/selinux/Makefile| 2 + > security/selinux/ima.c | 64 > security/selinux/include/ima.h | 24 +++ > security/selinux/include/security.h | 3 +- > security/selinux/ss/services.c | 64 > 6 files changed, 149 insertions(+), 11 deletions(-) > create mode 100644 security/selinux/ima.c > create mode 100644 security/selinux/include/ima.h I remain concerned about the possibility of bypassing a measurement by tampering with the time, but I appear to be the only one who is worried about this so I'm not going to block this patch on those grounds. Acked-by: Paul Moore > diff --git a/Documentation/ABI/testing/ima_policy > b/Documentation/ABI/testing/ima_policy > index 54fe1c15ed50..8365596cb42b 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -52,8 +52,9 @@ Description: > template:= name of a defined IMA template type > (eg, ima-ng). Only valid when action is "measure". > pcr:= decimal value > - label:= [data_label] > + label:= [selinux]|[data_label] > data_label:= a unique string used for grouping and > limiting critical data. > + For example, "selinux" to measure critical data for > SELinux. > > default policy: > # PROC_SUPER_MAGIC > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > index 4d8e0e8adf0b..776162444882 100644 > --- a/security/selinux/Makefile > +++ b/security/selinux/Makefile > @@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o > > selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o > > +selinux-$(CONFIG_IMA) += ima.o > + > ccflags-y := -I$(srctree)/security/selinux > -I$(srctree)/security/selinux/include > > $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h > diff --git a/security/selinux/ima.c b/security/selinux/ima.c > new file mode 100644 > index ..0b835bdc3aa9 > --- /dev/null > +++ b/security/selinux/ima.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Microsoft Corporation > + * > + * Author: Lakshmi Ramasubramanian (nra...@linux.microsoft.com) > + * > + * Measure critical data structures maintainted by SELinux > + * using IMA subsystem. > + */ > +#include > +#include > +#include > +#include "security.h" > +#include "ima.h" > + > +/* > + * selinux_ima_measure_state - Measure hash of the SELinux policy > + * > + * @state: selinux state struct > +
Re: [PATCH v13 3/4] selinux: teach SELinux about anonymous inodes
On Fri, Jan 8, 2021 at 2:35 PM Stephen Smalley
wrote:
> On Wed, Jan 6, 2021 at 10:03 PM Paul Moore wrote:
> > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra wrote:
> > > From: Daniel Colascione
> > >
> > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > the previous patches to give SELinux the ability to control
> > > anonymous-inode files that are created using the new
> > > anon_inode_getfd_secure() function.
> > >
> > > A SELinux policy author detects and controls these anonymous inodes by
> > > adding a name-based type_transition rule that assigns a new security
> > > type to anonymous-inode files created in some domain. The name used
> > > for the name-based transition is the name associated with the
> > > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > > "[perf_event]".
> > >
> > > Example:
> > >
> > > type uffd_t;
> > > type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> > > allow sysadm_t uffd_t:anon_inode { create };
> > >
> > > (The next patch in this series is necessary for making userfaultfd
> > > support this new interface. The example above is just
> > > for exposition.)
> > >
> > > Signed-off-by: Daniel Colascione
> > > Signed-off-by: Lokesh Gidra
> > > ---
> > > security/selinux/hooks.c| 56 +
> > > security/selinux/include/classmap.h | 2 ++
> > > 2 files changed, 58 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6b1826fc3658..d092aa512868 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,61 @@ static int selinux_inode_init_security(struct
> > > inode *inode, struct inode *dir,
> > > return 0;
> > > }
> > >
> > > +static int selinux_inode_init_security_anon(struct inode *inode,
> > > + const struct qstr *name,
> > > + const struct inode
> > > *context_inode)
> > > +{
> > > + const struct task_security_struct *tsec =
> > > selinux_cred(current_cred());
> > > + struct common_audit_data ad;
> > > + struct inode_security_struct *isec;
> > > + int rc;
> > > +
> > > + if (unlikely(!selinux_initialized(_state)))
> > > + return 0;
> > > +
> > > + isec = selinux_inode(inode);
> > > +
> > > + /*
> > > +* We only get here once per ephemeral inode. The inode has
> > > +* been initialized via inode_alloc_security but is otherwise
> > > +* untouched.
> > > +*/
> > > +
> > > + if (context_inode) {
> > > + struct inode_security_struct *context_isec =
> > > + selinux_inode(context_inode);
> > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > + return -EACCES;
> > > +
> > > + isec->sclass = context_isec->sclass;
> >
> > Taking the object class directly from the context_inode is
> > interesting, and I suspect problematic. In the case below where no
> > context_inode is supplied the object class is set to
> > SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> > supplied there is no guarantee that the object class will be set to
> > SECCLASS_ANON_INODE. This could both pose a problem for policy
> > writers (how do you distinguish the anon inode from other normal file
> > inodes in this case?) as well as an outright fault later in this
> > function when we try to check the ANON_INODE__CREATE on an object
> > other than a SECCLASS_ANON_INODE object.
> >
> > It works in the userfaultfd case because the context_inode is
> > originally created with this function so the object class is correctly
> > set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
> > case? Do we ever need or want to support using a context_inode that
> > is not SECCLASS_ANON_INODE?
>
> Sorry, I haven't been following this. IIRC, the original reason for
> passing a context_inode was to support the /dev/kvm or similar use
> cases where the driver is creating anonymous inodes to represent
> specific objects/interfaces derived from
Re: [PATCH v13 3/4] selinux: teach SELinux about anonymous inodes
On Wed, Jan 6, 2021 at 10:55 PM Lokesh Gidra wrote:
> On Wed, Jan 6, 2021 at 7:03 PM Paul Moore wrote:
> > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra wrote:
> > > From: Daniel Colascione
> > >
> > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > the previous patches to give SELinux the ability to control
> > > anonymous-inode files that are created using the new
> > > anon_inode_getfd_secure() function.
> > >
> > > A SELinux policy author detects and controls these anonymous inodes by
> > > adding a name-based type_transition rule that assigns a new security
> > > type to anonymous-inode files created in some domain. The name used
> > > for the name-based transition is the name associated with the
> > > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > > "[perf_event]".
> > >
> > > Example:
> > >
> > > type uffd_t;
> > > type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> > > allow sysadm_t uffd_t:anon_inode { create };
> > >
> > > (The next patch in this series is necessary for making userfaultfd
> > > support this new interface. The example above is just
> > > for exposition.)
> > >
> > > Signed-off-by: Daniel Colascione
> > > Signed-off-by: Lokesh Gidra
> > > ---
> > > security/selinux/hooks.c| 56 +
> > > security/selinux/include/classmap.h | 2 ++
> > > 2 files changed, 58 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6b1826fc3658..d092aa512868 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,61 @@ static int selinux_inode_init_security(struct
> > > inode *inode, struct inode *dir,
> > > return 0;
> > > }
> > >
> > > +static int selinux_inode_init_security_anon(struct inode *inode,
> > > + const struct qstr *name,
> > > + const struct inode
> > > *context_inode)
> > > +{
> > > + const struct task_security_struct *tsec =
> > > selinux_cred(current_cred());
> > > + struct common_audit_data ad;
> > > + struct inode_security_struct *isec;
> > > + int rc;
> > > +
> > > + if (unlikely(!selinux_initialized(_state)))
> > > + return 0;
> > > +
> > > + isec = selinux_inode(inode);
> > > +
> > > + /*
> > > +* We only get here once per ephemeral inode. The inode has
> > > +* been initialized via inode_alloc_security but is otherwise
> > > +* untouched.
> > > +*/
> > > +
> > > + if (context_inode) {
> > > + struct inode_security_struct *context_isec =
> > > + selinux_inode(context_inode);
> > > + if (context_isec->initialized != LABEL_INITIALIZED)
> > > + return -EACCES;
> > > +
> > > + isec->sclass = context_isec->sclass;
> >
> > Taking the object class directly from the context_inode is
> > interesting, and I suspect problematic. In the case below where no
> > context_inode is supplied the object class is set to
> > SECCLASS_ANON_INODE, which is correct, but when a context_inode is
> > supplied there is no guarantee that the object class will be set to
> > SECCLASS_ANON_INODE. This could both pose a problem for policy
> > writers (how do you distinguish the anon inode from other normal file
> > inodes in this case?) as well as an outright fault later in this
> > function when we try to check the ANON_INODE__CREATE on an object
> > other than a SECCLASS_ANON_INODE object.
> >
> Thanks for catching this. I'll initialize 'sclass' unconditionally to
> SECCLASS_ANON_INODE in the next version. Also, do you think I should
> add a check that context_inode's sclass must be SECCLASS_ANON_INODE to
> confirm that we never receive a regular inode as context_inode?
This is one of the reasons why I was asking if you ever saw the need
to use a regular inode here. It seems much safer to me to add a check
to ensure that context_inode is SECCLASS_ANON_INODE and return an
error otherwise; I would also suggest emitting an error using pr_err()
with something along the lines of &quo
Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface
On Wed, Jan 6, 2021 at 9:44 PM Lokesh Gidra wrote: > On Wed, Jan 6, 2021 at 6:10 PM Paul Moore wrote: > > > > On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra wrote: > > > From: Daniel Colascione > > > > > > This change adds a new function, anon_inode_getfd_secure, that creates > > > anonymous-node file with individual non-S_PRIVATE inode to which security > > > modules can apply policy. Existing callers continue using the original > > > singleton-inode kind of anonymous-inode file. We can transition anonymous > > > inode users to the new kind of anonymous inode in individual patches for > > > the sake of bisection and review. > > > > > > The new function accepts an optional context_inode parameter that callers > > > can use to provide additional contextual information to security modules. > > > For example, in case of userfaultfd, the created inode is a 'logical > > > child' > > > of the context_inode (userfaultfd inode of the parent process) in the > > > sense > > > that it provides the security context required during creation of the > > > child > > > process' userfaultfd inode. > > > > > > Signed-off-by: Daniel Colascione > > > > > > [Delete obsolete comments to alloc_anon_inode()] > > > [Add context_inode description in comments to anon_inode_getfd_secure()] > > > [Remove definition of anon_inode_getfile_secure() as there are no callers] > > > [Make __anon_inode_getfile() static] > > > [Use correct error cast in __anon_inode_getfile()] > > > [Fix error handling in __anon_inode_getfile()] > > > > Lokesh, I'm assuming you made the changes in the brackets above? If > > so they should include your initials or some other means of > > attributing them to you, e.g. "[LG: Fix error ...]". > > Thanks for reviewing the patch. Sorry for missing this. If it's > critical then I can upload another version of the patches to fix this. > Kindly let me know. Normally that is something I could fix during a merge with your approval, but see my comments to patch 3/4; I think this patchset still needs some work. > > > Signed-off-by: Lokesh Gidra > > > Reviewed-by: Eric Biggers > > > --- > > > fs/anon_inodes.c| 150 ++-- > > > fs/libfs.c | 5 -- > > > include/linux/anon_inodes.h | 5 ++ > > > 3 files changed, 115 insertions(+), 45 deletions(-) ... > > > +static struct file *__anon_inode_getfile(const char *name, > > > +const struct file_operations > > > *fops, > > > +void *priv, int flags, > > > +const struct inode > > > *context_inode, > > > +bool secure) > > > > Is it necessary to pass both the context_inode pointer and the secure > > boolean? It seems like if context_inode is non-NULL then one could > > assume that a secure anonymous inode was requested; is there ever > > going to be a case where this is not true? > > Yes, it is necessary as there are scenarios where a secure anon-inode > is to be created but there is no context_inode available. For > instance, in patch 4/4 of this series you'll see that when a secure > anon-inode is created in the userfaultfd syscall, context_inode isn't > available. My mistake, I didn't realize this until I got further in the patchset. -- paul moore www.paul-moore.com
Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface
On Wed, Jan 6, 2021 at 9:42 PM dancol wrote: > > On 2021-01-06 21:09, Paul Moore wrote: > > Is it necessary to pass both the context_inode pointer and the secure > > boolean? It seems like if context_inode is non-NULL then one could > > assume that a secure anonymous inode was requested; is there ever > > going to be a case where this is not true? > > The converse isn't true though: it makes sense to ask for a secure inode > with a NULL context inode. Having looked at patch 3/4 and 4/4 I just realized that and was coming back to update my comments :) -- paul moore www.paul-moore.com
Re: [PATCH v13 3/4] selinux: teach SELinux about anonymous inodes
On Wed, Nov 11, 2020 at 8:54 PM Lokesh Gidra wrote:
> From: Daniel Colascione
>
> This change uses the anon_inodes and LSM infrastructure introduced in
> the previous patches to give SELinux the ability to control
> anonymous-inode files that are created using the new
> anon_inode_getfd_secure() function.
>
> A SELinux policy author detects and controls these anonymous inodes by
> adding a name-based type_transition rule that assigns a new security
> type to anonymous-inode files created in some domain. The name used
> for the name-based transition is the name associated with the
> anonymous inode for file listings --- e.g., "[userfaultfd]" or
> "[perf_event]".
>
> Example:
>
> type uffd_t;
> type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> allow sysadm_t uffd_t:anon_inode { create };
>
> (The next patch in this series is necessary for making userfaultfd
> support this new interface. The example above is just
> for exposition.)
>
> Signed-off-by: Daniel Colascione
> Signed-off-by: Lokesh Gidra
> ---
> security/selinux/hooks.c| 56 +
> security/selinux/include/classmap.h | 2 ++
> 2 files changed, 58 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6b1826fc3658..d092aa512868 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2927,6 +2927,61 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir,
> return 0;
> }
>
> +static int selinux_inode_init_security_anon(struct inode *inode,
> + const struct qstr *name,
> + const struct inode *context_inode)
> +{
> + const struct task_security_struct *tsec =
> selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + if (unlikely(!selinux_initialized(_state)))
> + return 0;
> +
> + isec = selinux_inode(inode);
> +
> + /*
> +* We only get here once per ephemeral inode. The inode has
> +* been initialized via inode_alloc_security but is otherwise
> +* untouched.
> +*/
> +
> + if (context_inode) {
> + struct inode_security_struct *context_isec =
> + selinux_inode(context_inode);
> + if (context_isec->initialized != LABEL_INITIALIZED)
> + return -EACCES;
> +
> + isec->sclass = context_isec->sclass;
Taking the object class directly from the context_inode is
interesting, and I suspect problematic. In the case below where no
context_inode is supplied the object class is set to
SECCLASS_ANON_INODE, which is correct, but when a context_inode is
supplied there is no guarantee that the object class will be set to
SECCLASS_ANON_INODE. This could both pose a problem for policy
writers (how do you distinguish the anon inode from other normal file
inodes in this case?) as well as an outright fault later in this
function when we try to check the ANON_INODE__CREATE on an object
other than a SECCLASS_ANON_INODE object.
It works in the userfaultfd case because the context_inode is
originally created with this function so the object class is correctly
set to SECCLASS_ANON_INODE, but can we always guarantee that to be the
case? Do we ever need or want to support using a context_inode that
is not SECCLASS_ANON_INODE?
> + isec->sid = context_isec->sid;
> + } else {
> + isec->sclass = SECCLASS_ANON_INODE;
> + rc = security_transition_sid(
> + _state, tsec->sid, tsec->sid,
> + isec->sclass, name, >sid);
> + if (rc)
> + return rc;
> + }
> +
> + isec->initialized = LABEL_INITIALIZED;
> +
> + /*
> +* Now that we've initialized security, check whether we're
> +* allowed to actually create this type of anonymous inode.
> +*/
> +
> + ad.type = LSM_AUDIT_DATA_INODE;
> + ad.u.inode = inode;
> +
> + return avc_has_perm(_state,
> + tsec->sid,
> + isec->sid,
> + isec->sclass,
> + ANON_INODE__CREATE,
> + );
> +}
--
paul moore
www.paul-moore.com
Re: [PATCH v13 2/4] fs: add LSM-supporting anon-inode interface
unt is always greater than zero,
> -* so ihold() is safe.
> -*/
> - ihold(anon_inode_inode);
> - file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
> + if (secure) {
> + inode = anon_inode_make_secure_inode(name, context_inode);
> + if (IS_ERR(inode)) {
> + file = ERR_CAST(inode);
> + goto err;
> + }
> + } else {
> + inode = anon_inode_inode;
> + if (IS_ERR(inode)) {
> + file = ERR_PTR(-ENODEV);
> + goto err;
> + }
> + /*
> +* We know the anon_inode inode count is always
> +* greater than zero, so ihold() is safe.
> +*/
> + ihold(inode);
> + }
> +
> + file = alloc_file_pseudo(inode, anon_inode_mnt, name,
> flags & (O_ACCMODE | O_NONBLOCK), fops);
> if (IS_ERR(file))
> - goto err;
> + goto err_iput;
>
> - file->f_mapping = anon_inode_inode->i_mapping;
> + file->f_mapping = inode->i_mapping;
>
> file->private_data = priv;
>
> return file;
>
> +err_iput:
> + iput(inode);
> err:
> - iput(anon_inode_inode);
> module_put(fops->owner);
> return file;
> }
> -EXPORT_SYMBOL_GPL(anon_inode_getfile);
--
paul moore
www.paul-moore.com
Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP_RCU
On Tue, Jan 5, 2021 at 7:38 PM Al Viro wrote: > On Tue, Jan 05, 2021 at 07:00:59PM -0500, Paul Moore wrote: ... > > I would expect the problem here to be the currently allocated audit > > buffer isn't large enough to hold the full audit record, in which case > > it will attempt to expand the buffer by a call to pskb_expand_head() - > > don't ask why audit buffers are skbs, it's awful - using a gfp flag > > that was established when the buffer was first created. In this > > particular case it is GFP_ATOMIC|__GFP_NOWARN, which I believe should > > be safe in that it will not sleep on an allocation miss. > > > > I need to go deal with dinner, so I can't trace the entire path at the > > moment, but I believe the potential audit buffer allocation is the > > main issue. > > Nope. dput() in dump_common_audit_data(), OTOH, is certainly not > safe. My mistake. My initial reaction is to always assume audit is the problem; I should have traced everything through before commenting. > OTTH, it's not really needed there - see vfs.git #work.audit > for (untested) turning that sucker non-blocking. I hadn't tried > a followup that would get rid of the entire AVC_NONBLOCKING thing yet, > but I suspect that it should simplify the things in there nicely... It would be nice to be able to get rid of the limitation on when we can update the AVC and do proper auditing. I doubt the impact is anything that anyone notices, but I agree that it should make things much cleaner. Thanks Al. -- paul moore www.paul-moore.com
Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP_RCU
On Tue, Jan 5, 2021 at 6:27 PM Stephen Brennan
wrote:
> Al Viro writes:
>
> > On Tue, Jan 05, 2021 at 04:50:05PM +, Al Viro wrote:
> >
> >> LSM_AUDIT_DATA_DENTRY is easy to handle - wrap
> >> audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
> >> into grabbing/dropping a->u.dentry->d_lock and we are done.
> >
> > Incidentally, LSM_AUDIT_DATA_DENTRY in mainline is *not* safe wrt
> > rename() - for long-named dentries it is possible to get preempted
> > in the middle of
> > audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
> > and have the bugger renamed, with old name ending up freed. The
> > same goes for LSM_AUDIT_DATA_INODE...
>
> In the case of proc_pid_permission(), this preemption doesn't seem
> possible. We have task_lock() (a spinlock) held by ptrace_may_access()
> during this call, so preemption should be disabled:
>
> proc_pid_permission()
> has_pid_permissions()
> ptrace_may_access()
> task_lock()
> __ptrace_may_access()
> | security_ptrace_access_check()
> | ptrace_access_check -> selinux_ptrace_access_check()
> | avc_has_perm()
> | avc_audit() // note that has_pid_permissions() didn't get a
> | // flags field to propagate, so flags will not
> | // contain MAY_NOT_BLOCK
> | slow_avc_audit()
> | common_lsm_audit()
> | dump_common_audit_data()
> task_unlock()
>
> I understand the issue of d_name.name being freed across a preemption is
> more general than proc_pid_permission() (as other callers may have
> preemption enabled). However, it seems like there's another issue here.
> avc_audit() seems to imply that slow_avc_audit() would sleep:
>
> static inline int avc_audit(struct selinux_state *state,
> u32 ssid, u32 tsid,
> u16 tclass, u32 requested,
> struct av_decision *avd,
> int result,
> struct common_audit_data *a,
> int flags)
> {
> u32 audited, denied;
> audited = avc_audit_required(requested, avd, result, 0, );
> if (likely(!audited))
> return 0;
> /* fall back to ref-walk if we have to generate audit */
> if (flags & MAY_NOT_BLOCK)
> return -ECHILD;
> return slow_avc_audit(state, ssid, tsid, tclass,
> requested, audited, denied, result,
> a);
> }
>
> If there are other cases in here where we might sleep, it would be a
> problem to sleep with the task lock held, correct?
I would expect the problem here to be the currently allocated audit
buffer isn't large enough to hold the full audit record, in which case
it will attempt to expand the buffer by a call to pskb_expand_head() -
don't ask why audit buffers are skbs, it's awful - using a gfp flag
that was established when the buffer was first created. In this
particular case it is GFP_ATOMIC|__GFP_NOWARN, which I believe should
be safe in that it will not sleep on an allocation miss.
I need to go deal with dinner, so I can't trace the entire path at the
moment, but I believe the potential audit buffer allocation is the
main issue.
--
paul moore
www.paul-moore.com
Re: [PATCH v9 8/8] selinux: include a consumer of the new IMA critical data hook
On Mon, Jan 4, 2021 at 6:30 PM Lakshmi Ramasubramanian
wrote:
> On 12/23/20 1:10 PM, Paul Moore wrote:
> Hi Paul,
Hello.
> >> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> >> new file mode 100644
> >> index ..b7e24358e11d
> >> --- /dev/null
> >> +++ b/security/selinux/measure.c
> >> @@ -0,0 +1,79 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Measure SELinux state using IMA subsystem.
> >> + */
> >> +#include
> >> +#include
> >> +#include
> >> +#include "security.h"
> >> +
> >> +/*
> >> + * This function creates a unique name by appending the timestamp to
> >> + * the given string. This string is passed as "event_name" to the IMA
> >> + * hook to measure the given SELinux data.
> >> + *
> >> + * The data provided by SELinux to the IMA subsystem for measuring may
> >> have
> >> + * already been measured (for instance the same state existed earlier).
> >> + * But for SELinux the current data represents a state change and hence
> >> + * needs to be measured again. To enable this, pass a unique "event_name"
> >> + * to the IMA hook so that IMA subsystem will always measure the given
> >> data.
> >> + *
> >> + * For example,
> >> + * At time T0 SELinux data to be measured is "foo". IMA measures it.
> >> + * At time T1 the data is changed to "bar". IMA measures it.
> >> + * At time T2 the data is changed to "foo" again. IMA will not measure it
> >> + * (since it was already measured) unless the event_name, for instance,
> >> + * is different in this call.
> >> + */
> >> +static char *selinux_event_name(const char *name_prefix)
> >> +{
> >> + struct timespec64 cur_time;
> >> +
> >> + ktime_get_real_ts64(_time);
> >> + return kasprintf(GFP_KERNEL, "%s-%lld:%09ld", name_prefix,
> >> +cur_time.tv_sec, cur_time.tv_nsec);
> >> +}
> >
> > Why is this a separate function? It's three lines long and only
> > called from selinux_measure_state(). Do you ever see the SELinux/IMA
> > code in this file expanding to the point where this function is nice
> > from a reuse standpoint?
>
> Earlier I had two measurements - one for SELinux configuration/state and
> another for SELinux policy. selinux_event_name() was used to generate
> event name for each of them.
>
> In this patch set I have included only one measurement - for SELinux
> policy. I plan to add "SELinux configuration/state" measurement in a
> separate patch - I can reuse selinux_event_name() in that patch.
I'm curious about this second measurement. My apologies if you posted
it previously, this patchset has gone through several iterations and
simply can't recall all the different versions without digging through
the list archives.
Is there a reason why the second measurement isn't included in this
patch? Or this patchset if it is too big to be a single patch?
> Also, I think the comment in the function header for
> selinux_event_name() is useful.
>
> I prefer to have a separate function, if that's fine by you.
Given just this patch I would prefer if you folded
selinux_event_name() into selinux_measure_state(). However, I agree
with you that the comments in the selinux_event_name() header block is
useful, I would suggest moving those into the body of
selinux_measure_state() directly above the calls to
ktime_get_real_ts64() and kasprintf().
> > Also, I assume you are not concerned about someone circumventing the
> > IMA measurements by manipulating the time? In most systems I would
> > expect the time to be a protected entity, but with many systems
> > getting their time from remote systems I thought it was worth
> > mentioning.
>
> I am using time function to generate a unique name for the IMA
> measurement event, such as, "selinux-policy-hash-1609790281:860232824".
> This is to ensure that state changes in SELinux data are always measured.
>
> If you think time manipulation can be an issue, please let me know a
> better way to generate unique event names.
Yes, I understand that you are using the time value as a way of
ensuring you always have a different event name and hence a new
measurement. However, I was wondering if you would be okay if the
time was adjusted such that an event name was duplicated and a
measurement missed? Is that a problem for you? It seems like it
might be an issue, but you and Mimi know IMA better than I do.
> >&
Re: [PATCH -next] kernel/audit: convert comma to semicolon
On Mon, Dec 14, 2020 at 9:34 PM Paul Moore wrote: > On Fri, Dec 11, 2020 at 10:33 AM Richard Guy Briggs wrote: > > On 2020-12-11 16:42, Zheng Yongjun wrote: > > > Replace a comma between expression statements by a semicolon. > > > > > > Signed-off-by: Zheng Yongjun > > > --- > > > kernel/audit.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index 68cee3bc8cfe..c8497115be35 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > @@ -2282,7 +2282,7 @@ static void audit_log_set_loginuid(kuid_t > > > koldloginuid, kuid_t kloginuid, > > > > > > uid = from_kuid(_user_ns, task_uid(current)); > > > oldloginuid = from_kuid(_user_ns, koldloginuid); > > > - loginuid = from_kuid(_user_ns, kloginuid), > > > + loginuid = from_kuid(_user_ns, kloginuid); > > > > Nice catch. That went unnoticed through 3 patches, the last two mine... > > Yes, thanks for catching this and submitting a patch. However, as it > came very late in the v5.10-rcX release cycle I'm going to wait until > after this merge window to merge it into audit/next. This should be in audit/next now, thanks! -- paul moore www.paul-moore.com
Re: [PATCH v9 8/8] selinux: include a consumer of the new IMA critical data hook
ruct policy_file fp;
> +
> + fp.data = data;
> + fp.len = *len;
> +
> + rc = policydb_write(>policydb, );
> + if (rc)
> + return rc;
> +
> + *len = (unsigned long)fp.data - (unsigned long)data;
> + return 0;
> +}
> +
> /**
> * security_read_policy - read the policy.
> + * @state: selinux_state
> * @data: binary policy data
> * @len: length of data in bytes
> *
> @@ -3885,8 +3911,6 @@ int security_read_policy(struct selinux_state *state,
> void **data, size_t *len)
> {
> struct selinux_policy *policy;
> - int rc;
> - struct policy_file fp;
>
> policy = rcu_dereference_protected(
> state->policy, lockdep_is_held(>policy_mutex));
> @@ -3898,14 +3922,43 @@ int security_read_policy(struct selinux_state *state,
> if (!*data)> --
> 2.17.1
>
> return -ENOMEM;
>
> - fp.data = *data;
> - fp.len = *len;
> + return security_read_selinux_policy(policy, *data, len);
> +}
>
> - rc = policydb_write(>policydb, );
> - if (rc)
> - return rc;
> +/**
> + * security_read_policy_kernel - read the policy.
> + * @state: selinux_state
> + * @data: binary policy data
> + * @len: length of data in bytes
> + *
> + * Allocates kernel memory for reading SELinux policy.
> + * This function is for internal use only and should not
> + * be used for returning data to user space.
> + *
> + * This function must be called with policy_mutex held.
> + */
> +int security_read_policy_kernel(struct selinux_state *state,
> + void **data, size_t *len)
Let's call this "security_read_state_kernel()".
> +{
> + struct selinux_policy *policy;
> + int rc = 0;
See below, the rc variable is not needed.
> - *len = (unsigned long)fp.data - (unsigned long)*data;
> - return 0;
> + policy = rcu_dereference_protected(
> + state->policy, lockdep_is_held(>policy_mutex));
> + if (!policy) {
> + rc = -EINVAL;
> + goto out;
Jumping to the out label is a little silly since it is just a return;
do a "return -EINVAL;" here instead.
> + }
> +
> + *len = policy->policydb.len;
> + *data = vmalloc(*len);
> + if (!*data) {
> + rc = -ENOMEM;
> + goto out;
Same as above, "return -ENOMEM;" please.
> + }
>
> + rc = security_read_selinux_policy(policy, *data, len);
You should be able to do "return security_read_selinux_policy(...);" here.
> +
> +out:
> + return rc;
> }
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 v10 01/11] audit: collect audit task parameters
On Mon, Dec 21, 2020 at 11:57 AM Richard Guy Briggs wrote: > > The audit-related parameters in struct task_struct should ideally be > collected together and accessed through a standard audit API and the audit > structures made opaque to other kernel subsystems. > > Collect the existing loginuid, sessionid and audit_context together in a > new opaque struct audit_task_info called "audit" in struct task_struct. > > Use kmem_cache to manage this pool of memory. > Un-inline audit_free() to be able to always recover that memory. > > Please see the upstream github issues > https://github.com/linux-audit/audit-kernel/issues/81 > https://github.com/linux-audit/audit-kernel/issues/90 > > Signed-off-by: Richard Guy Briggs > Acked-by: Neil Horman > Reviewed-by: Ondrej Mosnacek Did Neil and Ondrej really ACK/Review the changes that you made here in v10 or are you just carrying over the ACK/Review? I'm hopeful it is the former, because I'm going to be a little upset if it is the latter. > --- > fs/io-wq.c| 8 +-- > fs/io_uring.c | 16 ++--- > include/linux/audit.h | 49 +- > include/linux/sched.h | 7 +- > init/init_task.c | 3 +- > init/main.c | 2 + > kernel/audit.c| 154 +- > kernel/audit.h| 7 ++ > kernel/auditsc.c | 24 --- > kernel/fork.c | 1 - > 10 files changed, 205 insertions(+), 66 deletions(-) -- paul moore www.paul-moore.com
Re: [PATCH -next] kernel/audit: convert comma to semicolon
On Fri, Dec 11, 2020 at 10:33 AM Richard Guy Briggs wrote:
> On 2020-12-11 16:42, Zheng Yongjun wrote:
> > Replace a comma between expression statements by a semicolon.
> >
> > Signed-off-by: Zheng Yongjun
> > ---
> > kernel/audit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 68cee3bc8cfe..c8497115be35 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2282,7 +2282,7 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,
> >
> > uid = from_kuid(_user_ns, task_uid(current));
> > oldloginuid = from_kuid(_user_ns, koldloginuid);
> > - loginuid = from_kuid(_user_ns, kloginuid),
> > + loginuid = from_kuid(_user_ns, kloginuid);
>
> Nice catch. That went unnoticed through 3 patches, the last two mine...
Yes, thanks for catching this and submitting a patch. However, as it
came very late in the v5.10-rcX release cycle I'm going to wait until
after this merge window to merge it into audit/next.
> Not quite sure why no compiler complained about it...
Because it is legal; odd, but legal. :)
The comma operator allows multiple expressions to be executed with
only the last expression returned. Take the example below:
% cat test.c
#include
#include
int main(int argc, char *argv[])
{
int a, b, c;
a = (b = 1, c = 2);
printf("a = %d, b = %d, c = %d\n", a, b, c);
return 0;
}
% gcc -o test test.c
% ./test
a = 2, b = 1, c = 2
... we see both "b=1" and "c=2" are executed, and the last statement
in the comma separated list of expressions is used as the right-hand
value in the "a" assignment.
In the case of this patch, the existing code is actually okay: both
expressions are executed and we don't assign either expression's value
to a variable so it doesn't matter. However, it definitely looks odd
and is something we should fix.
--
paul moore
www.paul-moore.com
[GIT PULL] SELinux patches for v5.11
Hi Linus, While we have a small number of SELinux patches for v5.11, there are a few changes worth highlighting: - Change the LSM network hooks to pass flowi_common structs instead of the parent flowi struct as the LSMs do not currently need the full flowi struct and they do not have enough information to use it safely (missing information on the address family). This patch was discussed both with Herbert Xu (representing team netdev) and James Morris (representing team LSMs-other-than-SELinux). - Fix how we handle errors in inode_doinit_with_dentry() so that we attempt to properly label the inode on following lookups instead of continuing to treat it as unlabeled. - Tweak the kernel logic around allowx, auditallowx, and dontauditx SELinux policy statements such that the auditx/dontauditx are effective even without the allowx statement. Everything passes our test suite and as of an hour or two ago it applies cleanly to your tree; please merge for v5.11. Thanks, -Paul -- The following changes since commit 3650b228f83adda7e5ee532e2b90429c03f7b9ec: Linux 5.10-rc1 (2020-10-25 15:14:11 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20201214 for you to fetch changes up to 3df98d79215ace13d1e91ddfc5a67a0f5acbd83f: lsm,selinux: pass flowi_common instead of flowi to the LSM hooks (2020-11-23 18:36:21 -0500) selinux/stable-5.11 PR 20201214 Gustavo A. R. Silva (1): selinux: Fix fall-through warnings for Clang Ondrej Mosnacek (1): selinux: drop super_block backpointer from superblock_security_struct Paul Moore (2): selinux: fix inode_doinit_with_dentry() LABEL_INVALID error handling lsm,selinux: pass flowi_common instead of flowi to the LSM hooks Tianyue Ren (1): selinux: fix error initialization in inode_doinit_with_dentry() bauen1 (1): selinux: allow dontauditx and auditallowx rules to take effect without allowx .../chelsio/inline_crypto/chtls/chtls_cm.c | 2 +- drivers/net/wireguard/socket.c | 4 ++-- include/linux/lsm_hook_defs.h | 4 ++-- include/linux/lsm_hooks.h | 2 +- include/linux/security.h | 23 +--- include/net/flow.h | 10 + include/net/route.h| 6 ++--- net/dccp/ipv4.c| 2 +- net/dccp/ipv6.c| 6 ++--- net/ipv4/icmp.c| 4 ++-- net/ipv4/inet_connection_sock.c| 4 ++-- net/ipv4/ip_output.c | 2 +- net/ipv4/ping.c| 2 +- net/ipv4/raw.c | 2 +- net/ipv4/syncookies.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/af_inet6.c| 2 +- net/ipv6/datagram.c| 2 +- net/ipv6/icmp.c| 6 ++--- net/ipv6/inet6_connection_sock.c | 4 ++-- net/ipv6/netfilter/nf_reject_ipv6.c| 2 +- net/ipv6/ping.c| 2 +- net/ipv6/raw.c | 2 +- net/ipv6/syncookies.c | 2 +- net/ipv6/tcp_ipv6.c| 4 ++-- net/ipv6/udp.c | 2 +- net/l2tp/l2tp_ip6.c| 2 +- net/netfilter/nf_synproxy_core.c | 2 +- net/xfrm/xfrm_state.c | 6 +++-- security/security.c| 17 +++--- security/selinux/hooks.c | 26 -- security/selinux/include/objsec.h | 1 - security/selinux/include/xfrm.h| 2 +- security/selinux/ss/services.c | 4 +--- security/selinux/xfrm.c| 13 ++- 35 files changed, 101 insertions(+), 77 deletions(-) -- paul moore www.paul-moore.com
[GIT PULL] Audit patches for v5.11
Hi Linus, A small set of audit patches for v5.11 with four patches in total and only one of any real significance. Richard's patch to trigger accompanying records causes the kernel to emit additional related records when an audit event occurs; helping provide some much needed context to events in the audit log. It is also worth mentioning that this is a revised patch based on an earlier attempt that had to be reverted in the v5.8 time frame. Everything passes our test suite, and with no problems reported please merge this for v5.11. Thanks, -Paul -- The following changes since commit 3650b228f83adda7e5ee532e2b90429c03f7b9ec: Linux 5.10-rc1 (2020-10-25 15:14:11 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git tags/audit-pr-20201214 for you to fetch changes up to 6b3211842a115d697fbf78d09f3e83852200e413: audit: replace atomic_add_return() (2020-12-02 22:52:16 -0500) audit/stable-5.11 PR 20201214 Alex Shi (1): audit: fix macros warnings Mauro Carvalho Chehab (1): audit: fix a kernel-doc markup Richard Guy Briggs (1): audit: trigger accompanying records when no rules present Yejune Deng (1): audit: replace atomic_add_return() include/linux/audit.h | 8 kernel/audit.c| 9 ++--- kernel/auditsc.c | 38 -- security/lsm_audit.c | 5 - 4 files changed, 18 insertions(+), 42 deletions(-) -- paul moore www.paul-moore.com
Re: [PATCH v2] proc: Allow pid_revalidate() during LOOKUP_RCU
On Sun, Dec 13, 2020 at 11:30 AM Matthew Wilcox wrote:
> On Sun, Dec 13, 2020 at 08:22:32AM -0600, Eric W. Biederman wrote:
> > Matthew Wilcox writes:
> >
> > > On Thu, Dec 03, 2020 at 04:02:12PM -0800, Stephen Brennan wrote:
> > >> -void pid_update_inode(struct task_struct *task, struct inode *inode)
> > >> +static int do_pid_update_inode(struct task_struct *task, struct inode
> > >> *inode,
> > >> + unsigned int flags)
> > >
> > > I'm really nitpicking here, but this function only _updates_ the inode
> > > if flags says it should. So I was thinking something like this
> > > (compile tested only).
> > >
> > > I'd really appreocate feedback from someone like Casey or Stephen on
> > > what they need for their security modules.
> >
> > Just so we don't have security module questions confusing things
> > can we please make this a 2 patch series? With the first
> > patch removing security_task_to_inode?
> >
> > The justification for the removal is that all security_task_to_inode
> > appears to care about is the file type bits in inode->i_mode. Something
> > that never changes. Having this in a separate patch would make that
> > logical change easier to verify.
>
> I don't think that's right, which is why I keep asking Stephen & Casey
> for their thoughts.
The SELinux security_task_to_inode() implementation only cares about
inode->i_mode S_IFMT bits from the inode so that we can set the object
class correctly. The inode's SELinux label is taken from the
associated task.
Casey would need to comment on Smack's needs.
> For example,
>
> * Sets the smack pointer in the inode security blob
> */
> static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
> {
> struct inode_smack *isp = smack_inode(inode);
> struct smack_known *skp = smk_of_task_struct(p);
>
> isp->smk_inode = skp;
> isp->smk_flags |= SMK_INODE_INSTANT;
> }
>
> That seems to do rather more than checking the file type bits.
--
paul moore
www.paul-moore.com
Re: [PATCH] audit: remove unused macros
On Wed, Nov 11, 2020 at 7:38 PM Alex Shi wrote: > From 5fef5f1b7b745b52bedc9c7eec9fc163202ad421 Mon Sep 17 00:00:00 2001 > From: Alex Shi > Date: Fri, 6 Nov 2020 16:31:22 +0800 > Subject: [PATCH v3] audit: fix macros warnings > > Some unused macros could cause gcc warning: > kernel/audit.c:68:0: warning: macro "AUDIT_UNINITIALIZED" is not used > [-Wunused-macros] > kernel/auditsc.c:104:0: warning: macro "AUDIT_AUX_IPCPERM" is not used > [-Wunused-macros] > kernel/auditsc.c:82:0: warning: macro "AUDITSC_INVALID" is not used > [-Wunused-macros] > > AUDIT_UNINITIALIZED and AUDITSC_INVALID are still meaningful and should > be in incorporated. > > Just remove AUDIT_AUX_IPCPERM. > > Thanks comments from Richard Guy Briggs and Paul Moore. > > Signed-off-by: Alex Shi > Cc: Paul Moore > Cc: Richard Guy Briggs > Cc: Eric Paris > Cc: linux-au...@redhat.com > Cc: linux-kernel@vger.kernel.org > --- > kernel/audit.c | 2 +- > kernel/auditsc.c | 11 +-- > 2 files changed, 6 insertions(+), 7 deletions(-) Sorry for the delay, this was buried in my inbox but I've just merged it into audit/next. Thanks for your help and patience with this! -- paul moore www.paul-moore.com
Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes
On Tue, Nov 24, 2020 at 3:44 PM Lokesh Gidra wrote: > On Mon, Nov 23, 2020 at 2:43 PM Paul Moore wrote: > > On Mon, Nov 23, 2020 at 2:21 PM Lokesh Gidra wrote: > > > On Sun, Nov 22, 2020 at 3:14 PM Paul Moore wrote: > > > > On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra > > > > wrote: > > > > > I have created a cuttlefish build and have tested with the attached > > > > > userfaultfd program: > > > > > > > > Thanks, that's a good place to start, a few comments: > > > > > > > > - While we support Android as a distribution, it isn't a platform that > > > > we common use for development and testing. At the moment, Fedora is > > > > probably your best choice for that. > > > > > > > I tried setting up a debian/ubuntu system for testing using the > > > instructions on the selinux-testsuite page, but the system kept > > > freezing after 'setenforce 1'. I'll try with fedora now. > > > > I would expect you to have much better luck with Fedora. > > Yes. It worked! Excellent :) > > > > - Your test program should be written in vanilla C for the > > > > selinux-testsuite. Looking at the userfaultfdSimple.cc code that > > > > should be a trivial conversion. > > > > > > > > - I think you have a good start on a test for the selinux-testsuite, > > > > please take a look at the test suite and submit a patch against that > > > > repo. Ondrej (CC'd) currently maintains the test suite and he may > > > > have some additional thoughts. > > > > > > > > * https://github.com/SELinuxProject/selinux-testsuite > > > > > > Thanks a lot for the inputs. I'll start working on this. > > > > Great, let us know if you hit any problems. I think we would all like > > to see this upstream :) > > I have the patch ready. I couldn't find any instructions on the > testsuite site about patch submission. Can you please tell me how to > proceed. You can post it to the SELinux mailing list, much like you would do a SELinux kernel patch. I'll take a look and I'll make sure Ondrej looks at it too. Thanks! -- paul moore www.paul-moore.com
Re: [PATCH 053/141] selinux: Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 1:32 PM Gustavo A. R. Silva
wrote:
>
> In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning
> by explicitly adding a break statement instead of letting the code fall
> through to the next case.
>
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva
> ---
> security/selinux/hooks.c | 1 +
> 1 file changed, 1 insertion(+)
Merged into selinux/next, thanks.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6b1826fc3658..e57774367b3a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4029,6 +4029,7 @@ static int selinux_kernel_load_data(enum
> kernel_load_data_id id, bool contents)
> switch (id) {
> case LOADING_MODULE:
> rc = selinux_kernel_module_from_file(NULL);
> + break;
> default:
> break;
> }
> --
> 2.27.0
--
paul moore
www.paul-moore.com
Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes
On Mon, Nov 23, 2020 at 2:21 PM Lokesh Gidra wrote: > On Sun, Nov 22, 2020 at 3:14 PM Paul Moore wrote: > > On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra wrote: > > > I have created a cuttlefish build and have tested with the attached > > > userfaultfd program: > > > > Thanks, that's a good place to start, a few comments: > > > > - While we support Android as a distribution, it isn't a platform that > > we common use for development and testing. At the moment, Fedora is > > probably your best choice for that. > > > I tried setting up a debian/ubuntu system for testing using the > instructions on the selinux-testsuite page, but the system kept > freezing after 'setenforce 1'. I'll try with fedora now. I would expect you to have much better luck with Fedora. > > - Your test program should be written in vanilla C for the > > selinux-testsuite. Looking at the userfaultfdSimple.cc code that > > should be a trivial conversion. > > > > - I think you have a good start on a test for the selinux-testsuite, > > please take a look at the test suite and submit a patch against that > > repo. Ondrej (CC'd) currently maintains the test suite and he may > > have some additional thoughts. > > > > * https://github.com/SELinuxProject/selinux-testsuite > > Thanks a lot for the inputs. I'll start working on this. Great, let us know if you hit any problems. I think we would all like to see this upstream :) -- paul moore www.paul-moore.com
Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes
On Wed, Nov 18, 2020 at 5:39 PM Lokesh Gidra wrote:
> I have created a cuttlefish build and have tested with the attached
> userfaultfd program:
Thanks, that's a good place to start, a few comments:
- While we support Android as a distribution, it isn't a platform that
we common use for development and testing. At the moment, Fedora is
probably your best choice for that.
- Your test program should be written in vanilla C for the
selinux-testsuite. Looking at the userfaultfdSimple.cc code that
should be a trivial conversion.
- I think you have a good start on a test for the selinux-testsuite,
please take a look at the test suite and submit a patch against that
repo. Ondrej (CC'd) currently maintains the test suite and he may
have some additional thoughts.
* https://github.com/SELinuxProject/selinux-testsuite
> 1) Without these kernel patches the program executes without any restrictions
>
> vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
> api: 170
> features: 511
> ioctls: 9223372036854775811
>
> read: Try again
>
>
> 2) With these patches applied but without any policy the 'permission
> denied' is thrown
>
> vsoc_x86_64:/ $ ./system/bin/userfaultfdSimple
> syscall(userfaultfd): Permission denied
>
> with the following logcat message:
> 11-18 14:21:44.041 3130 3130 W userfaultfdSimp: type=1400
> audit(0.0:107): avc: denied { create } for dev="anon_inodefs"
> ino=45031 scontext=u:r:shell:s0 tcontext=u:object_r:shell:s0
> tclass=anon_inode permissive=0
>
>
> 3) With the attached .te policy file in place the following output is
> observed, confirming that the patch is working as intended.
> vsoc_x86_64:/ $ ./vendor/bin/userfaultfdSimple
> UFFDIO_API: Permission denied
>
> with the following logcat message:
> 11-18 14:33:29.142 2028 2028 W userfaultfdSimp: type=1400
> audit(0.0:104): avc: denied { ioctl } for
> path="anon_inode:[userfaultfd]" dev="anon_inodefs" ino=41169
> ioctlcmd=0xaa3f scontext=u:r:userfaultfdSimple:s0
> tcontext=u:object_r:uffd_t:s0 tclass=anon_inode permissive=0
--
paul moore
www.paul-moore.com
[GIT PULL] SELinux fixes for v5.10 (#1)
Hi Linus, One small SELinux patch for v5.10-rcX to make sure we return an error code when an allocation fails. It passes all of our tests, but given the nature of the patch that isn't surprising. Please merge during the v5.10-rcX cycle. Thanks, -Paul -- The following changes since commit 0d50f059c4cdc9e436f6f4db8779ac0795bfdadf: selinux: provide a "no sooner than" date for the checkreqprot removal (2020-09-29 16:50:57 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20201113 for you to fetch changes up to c350f8bea271782e2733419bd2ab9bf4ec2051ef: selinux: Fix error return code in sel_ib_pkey_sid_slow() (2020-11-12 20:16:09-0500) selinux/stable-5.10 PR 20201113 Chen Zhou (1): selinux: Fix error return code in sel_ib_pkey_sid_slow() security/selinux/ibpkey.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- paul moore www.paul-moore.com
Re: [PATCH v2] selinux: Fix error return code in sel_ib_pkey_sid_slow()
On Thu, Nov 12, 2020 at 8:48 AM Chen Zhou wrote:
>
> Fix to return a negative error code from the error handling case
> instead of 0 in function sel_ib_pkey_sid_slow(), as done elsewhere
> in this function.
>
> Fixes: 409dcf31538a ("selinux: Add a cache for quicker retreival of PKey
> SIDs")
> Reported-by: Hulk Robot
> Signed-off-by: Chen Zhou
> ---
> security/selinux/ibpkey.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Thanks for the fix! I've merged this into the selinux/stable-5.10
branch and I'll send this up to Linus tomorrow or early next week.
--
paul moore
www.paul-moore.com
Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes
On Tue, Nov 10, 2020 at 10:30 PM Lokesh Gidra wrote:
> On Tue, Nov 10, 2020 at 6:13 PM Paul Moore wrote:
> > On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra wrote:
> > > On Mon, Nov 9, 2020 at 7:12 PM Paul Moore wrote:
> > > > On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra
> > > > wrote:
> > > > >
> > > > > From: Daniel Colascione
> > > > >
> > > > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > > > the previous patches to give SELinux the ability to control
> > > > > anonymous-inode files that are created using the new
> > > > > anon_inode_getfd_secure() function.
> > > > >
> > > > > A SELinux policy author detects and controls these anonymous inodes by
> > > > > adding a name-based type_transition rule that assigns a new security
> > > > > type to anonymous-inode files created in some domain. The name used
> > > > > for the name-based transition is the name associated with the
> > > > > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > > > > "[perf_event]".
> > > > >
> > > > > Example:
> > > > >
> > > > > type uffd_t;
> > > > > type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> > > > > allow sysadm_t uffd_t:anon_inode { create };
> > > > >
> > > > > (The next patch in this series is necessary for making userfaultfd
> > > > > support this new interface. The example above is just
> > > > > for exposition.)
> > > > >
> > > > > Signed-off-by: Daniel Colascione
> > > > > Signed-off-by: Lokesh Gidra
> > > > > ---
> > > > > security/selinux/hooks.c| 53
> > > > > +
> > > > > security/selinux/include/classmap.h | 2 ++
> > > > > 2 files changed, 55 insertions(+)
> > > > >
> > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > index 6b1826fc3658..1c0adcdce7a8 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -2927,6 +2927,58 @@ static int selinux_inode_init_security(struct
> > > > > inode *inode, struct inode *dir,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int selinux_inode_init_security_anon(struct inode *inode,
> > > > > + const struct qstr *name,
> > > > > + const struct inode
> > > > > *context_inode)
> > > > > +{
> > > > > + const struct task_security_struct *tsec =
> > > > > selinux_cred(current_cred());
> > > > > + struct common_audit_data ad;
> > > > > + struct inode_security_struct *isec;
> > > > > + int rc;
> > > > > +
> > > > > + if (unlikely(!selinux_initialized(_state)))
> > > > > + return 0;
> > > > > +
> > > > > + isec = selinux_inode(inode);
> > > > > +
> > > > > + /*
> > > > > +* We only get here once per ephemeral inode. The inode has
> > > > > +* been initialized via inode_alloc_security but is otherwise
> > > > > +* untouched.
> > > > > +*/
> > > > > +
> > > > > + if (context_inode) {
> > > > > + struct inode_security_struct *context_isec =
> > > > > + selinux_inode(context_inode);
> > > > > + isec->sclass = context_isec->sclass;
> > > > > + isec->sid = context_isec->sid;
> > > >
> > > > I suppose this isn't a major concern given the limited usage at the
> > > > moment, but I wonder if it would be a good idea to make sure the
> > > > context_inode's SELinux label is valid before we assign it to the
> > > > anonymous inode? If it is invalid, what should we do? Do we attempt
> > > > to (re)validate it? Do we simply fallback to the transition approach?
> > >
> > > Frankly, I'm not too familiar with SELinux. Originally this patch
> > &g
Re: [PATCH] audit: remove unused macros
On Tue, Nov 10, 2020 at 10:23 AM Richard Guy Briggs wrote:
> On 2020-11-06 16:31, Alex Shi wrote:
> > Some unused macros could cause gcc warning:
> > kernel/audit.c:68:0: warning: macro "AUDIT_UNINITIALIZED" is not used
> > [-Wunused-macros]
> > kernel/auditsc.c:104:0: warning: macro "AUDIT_AUX_IPCPERM" is not used
> > [-Wunused-macros]
> > kernel/auditsc.c:82:0: warning: macro "AUDITSC_INVALID" is not used
> > [-Wunused-macros]
> >
> > remove them to tame gcc.
> >
> > Signed-off-by: Alex Shi
> > Cc: Paul Moore
> > Cc: Eric Paris
> > Cc: linux-au...@redhat.com
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > kernel/audit.c | 1 -
> > kernel/auditsc.c | 3 ---
> > 2 files changed, 4 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index ac0aeaa99937..dfac1e0ca887 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -65,7 +65,6 @@
> > /* No auditing will take place until audit_initialized ==
> > AUDIT_INITIALIZED.
> > * (Initialization happens after skb_init is called.) */
> > #define AUDIT_DISABLED -1
> > -#define AUDIT_UNINITIALIZED 0
> > #define AUDIT_INITIALIZED1
> > static int audit_initialized;
>
> This one is part of a set, so it feels like it should stay, but the code
> is structured in such a way that it is not necessary.
Yes, I'd like for us to find a way to keep this if possible. Let's
simply initialize "audit_initialized" to AUDIT_UNINITIALIZED in this
file. At some point someone will surely complain about not needing to
initialize to zero, but we can deal with that later.
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 183d79cc2e12..eeb4930d499f 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -80,7 +80,6 @@
> > #include "audit.h"
> >
> > /* flags stating the success for a syscall */
> > -#define AUDITSC_INVALID 0
> > #define AUDITSC_SUCCESS 1
> > #define AUDITSC_FAILURE 2
>
> Same here, but this one should really be fixed by using
> AUDITSC_INVALID as the value assigned to context->return_valid in
> __audit_free() to avoid using magic numbers.
Agreed.
We could probably explicitly set it in audit_alloc_context() as well
if we wanted to be complete.
> Similarly, the compared
> values in audit_filter_rules() under the AUDIT_EXIT and AUDIT_SUCCESS
> cases should be "ctx->return_valid != AUDITSC_INVALID" rather than just
> "ctx->return_valid". Same in audit_log_exit().
Agreed.
> > @@ -102,8 +101,6 @@ struct audit_aux_data {
> > int type;
> > };
> >
> > -#define AUDIT_AUX_IPCPERM 0
> > -
>
> Hmmm, this one looks like it was orphaned 15 years ago a couple of
> months after it was introduced due to this commit:
> c04049939f88 Steve Grubb 2005-05-13
> ("AUDIT: Add message types to audit records")
>
> Introduced here:
> 8e633c3fb2a2 David Woodhouse 2005-03-01
> ("Audit IPC object owner/permission changes.")
>
> I agree, remove it.
No arguments from me.
--
paul moore
www.paul-moore.com
Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes
On Tue, Nov 10, 2020 at 1:24 PM Lokesh Gidra wrote:
> On Mon, Nov 9, 2020 at 7:12 PM Paul Moore wrote:
> > On Fri, Nov 6, 2020 at 10:56 AM Lokesh Gidra wrote:
> > >
> > > From: Daniel Colascione
> > >
> > > This change uses the anon_inodes and LSM infrastructure introduced in
> > > the previous patches to give SELinux the ability to control
> > > anonymous-inode files that are created using the new
> > > anon_inode_getfd_secure() function.
> > >
> > > A SELinux policy author detects and controls these anonymous inodes by
> > > adding a name-based type_transition rule that assigns a new security
> > > type to anonymous-inode files created in some domain. The name used
> > > for the name-based transition is the name associated with the
> > > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > > "[perf_event]".
> > >
> > > Example:
> > >
> > > type uffd_t;
> > > type_transition sysadm_t sysadm_t : anon_inode uffd_t "[userfaultfd]";
> > > allow sysadm_t uffd_t:anon_inode { create };
> > >
> > > (The next patch in this series is necessary for making userfaultfd
> > > support this new interface. The example above is just
> > > for exposition.)
> > >
> > > Signed-off-by: Daniel Colascione
> > > Signed-off-by: Lokesh Gidra
> > > ---
> > > security/selinux/hooks.c| 53 +
> > > security/selinux/include/classmap.h | 2 ++
> > > 2 files changed, 55 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6b1826fc3658..1c0adcdce7a8 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2927,6 +2927,58 @@ static int selinux_inode_init_security(struct
> > > inode *inode, struct inode *dir,
> > > return 0;
> > > }
> > >
> > > +static int selinux_inode_init_security_anon(struct inode *inode,
> > > + const struct qstr *name,
> > > + const struct inode
> > > *context_inode)
> > > +{
> > > + const struct task_security_struct *tsec =
> > > selinux_cred(current_cred());
> > > + struct common_audit_data ad;
> > > + struct inode_security_struct *isec;
> > > + int rc;
> > > +
> > > + if (unlikely(!selinux_initialized(_state)))
> > > + return 0;
> > > +
> > > + isec = selinux_inode(inode);
> > > +
> > > + /*
> > > +* We only get here once per ephemeral inode. The inode has
> > > +* been initialized via inode_alloc_security but is otherwise
> > > +* untouched.
> > > +*/
> > > +
> > > + if (context_inode) {
> > > + struct inode_security_struct *context_isec =
> > > + selinux_inode(context_inode);
> > > + isec->sclass = context_isec->sclass;
> > > + isec->sid = context_isec->sid;
> >
> > I suppose this isn't a major concern given the limited usage at the
> > moment, but I wonder if it would be a good idea to make sure the
> > context_inode's SELinux label is valid before we assign it to the
> > anonymous inode? If it is invalid, what should we do? Do we attempt
> > to (re)validate it? Do we simply fallback to the transition approach?
>
> Frankly, I'm not too familiar with SELinux. Originally this patch
> series was developed by Daniel, in consultation with Stephen Smalley.
> In my (probably naive) opinion we should fallback to transition
> approach. But I'd request you to tell me if this needs to be addressed
> now, and if so then what's the right approach.
>
> If the decision is to address this now, then what's the best way to
> check the SELinux label validity?
You can check to see if an inode's label is valid by looking at the
isec->initialized field; if it is LABEL_INITIALIZED then it is all
set, if it is any other value then the label isn't entirely correct.
It may have not have ever been fully initialized (and has a default
value) or it may have live on a remote filesystem where the host has
signaled that the label has changed (and the label is now outdated).
This patchset includes support for userfaultfd, which means we don't
really have to worry about the remote fs problem, but the
never-f
Re: [PATCH v12 3/4] selinux: teach SELinux about anonymous inodes
rity, selinux_inode_free_security),
> LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
> + LSM_HOOK_INIT(inode_init_security_anon,
> selinux_inode_init_security_anon),
> LSM_HOOK_INIT(inode_create, selinux_inode_create),
> LSM_HOOK_INIT(inode_link, selinux_inode_link),
> LSM_HOOK_INIT(inode_unlink, selinux_inode_unlink),
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 40cebde62856..ba2e01a6955c 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -249,6 +249,8 @@ struct security_class_mapping secclass_map[] = {
> {"open", "cpu", "kernel", "tracepoint", "read", "write"} },
> { "lockdown",
> { "integrity", "confidentiality", NULL } },
> + { "anon_inode",
> + { COMMON_FILE_PERMS, NULL } },
> { NULL }
>};
>
--
paul moore
www.paul-moore.com
Re: [RFC PATCH v2] selinux: Fix kmemleak after disabling selinux runtime
On Fri, Oct 30, 2020 at 8:34 AM Casey Schaufler wrote: > On 10/30/2020 12:57 AM, Hou Tao wrote: > > Hi, > > > > On 2020/10/29 0:29, Casey Schaufler wrote: > >> On 10/27/2020 7:06 PM, Chen Jun wrote: > >>> From: Chen Jun > >>> > >>> Kmemleak will report a problem after using > >>> "echo 1 > /sys/fs/selinux/disable" to disable selinux on runtime. > >> Runtime disable of SELinux has been deprecated. It would be > >> wasteful to make these changes in support of a facility that > >> is going away. > >> > > But this sysfs file will still be present and workable on LTS kernel > > versions, so > > is the proposed fixe OK for these LTS kernel versions ? > > It's not my call to make. Paul Moore has the voice that matters here. > I think that the trivial memory leak here is inconsequential compared > to the overhead you're introducing by leaving the NO_DEL hooks enabled. Disabling SELinux at runtime is deprecated and will be removed in a future release, check the Documentation/ABI/obsolete/sysfs-selinux-disable in Linus' current tree for details. The recommended way to disable SELinux is at boot using the kernel command line, as described in the deprecation text: The preferred method of disabling SELinux is via the "selinux=0" boot parameter, but the selinuxfs "disable" node was created to make it easier for systems with primitive bootloaders that did not allow for easy modification of the kernel command line. Unfortunately, allowing for SELinux to be disabled at runtime makes it difficult to secure the kernel's LSM hooks using the "__ro_after_init" feature. Thankfully, the need for the SELinux runtime disable appears to be gone, the default Kconfig configuration disables this selinuxfs node, and only one of the major distributions, Fedora, supports disabling SELinux at runtime. Fedora is in the process of removing the selinuxfs "disable" node and once that is complete we will start the slow process of removing this code from the kernel. Because of the upcoming removal as well as the drawbacks and minimal gains provided by the patch in this thread, I would recommend against merging this patch. I would further recommend that distros and those building their own kernels leave CONFIG_SECURITY_SELINUX_DISABLE disabled and use the kernel command line instead. NACK. -- paul moore www.paul-moore.com
