From 494688efdc5912b858d0a05563c9cf258c75d29a Mon Sep 17 00:00:00 2001 From: "GONG, Ruiqi" Date: Wed, 18 May 2022 09:21:37 +0000 Subject: [PATCH 1/8] selinux: add __randomize_layout to selinux_audit_data Randomize the layout of struct selinux_audit_data as suggested in [1], since it contains a pointer to struct selinux_state, an already randomized strucure. [1]: https://github.com/KSPP/linux/issues/188 Signed-off-by: GONG, Ruiqi Signed-off-by: Paul Moore --- security/selinux/include/avc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h index 2b372f98f2d7..5525b94fd266 100644 --- a/security/selinux/include/avc.h +++ b/security/selinux/include/avc.h @@ -53,7 +53,7 @@ struct selinux_audit_data { u32 denied; int result; struct selinux_state *state; -}; +} __randomize_layout; /* * AVC operations From 4d3d0ed60ee0d2da4c541e525c132dc374464624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 17 Feb 2022 15:21:28 +0100 Subject: [PATCH 2/8] selinux: drop unnecessary NULL check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()") introduced a NULL check on the context after a successful call to security_sid_to_context(). This is on the one hand redundant after checking for success and on the other hand insufficient on an actual NULL pointer, since the context is passed to seq_escape() leading to a call of strlen() on it. Reported by Clang analyzer: In file included from security/selinux/hooks.c:28: In file included from ./include/linux/tracehook.h:50: In file included from ./include/linux/memcontrol.h:13: In file included from ./include/linux/cgroup.h:18: ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg] seq_escape_mem(m, src, strlen(src), flags, esc); ^~~~~~~~~~~ Signed-off-by: Christian Göttsche Signed-off-by: Paul Moore --- security/selinux/hooks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index beceb89f68d9..4af4986d3893 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1019,7 +1019,7 @@ static int show_sid(struct seq_file *m, u32 sid) rc = security_sid_to_context(&selinux_state, sid, &context, &len); if (!rc) { - bool has_comma = context && strchr(context, ','); + bool has_comma = strchr(context, ','); seq_putc(m, '='); if (has_comma) From 9691e4f9ba6c7dc6af07b8a4feba6279d76f0003 Mon Sep 17 00:00:00 2001 From: Jonas Lindner Date: Thu, 9 Jun 2022 00:36:16 +0200 Subject: [PATCH 3/8] selinux: fix typos in comments Signed-off-by: Jonas Lindner [PM: fixed duplicated subject line] Signed-off-by: Paul Moore --- security/selinux/hooks.c | 4 ++-- security/selinux/include/audit.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4af4986d3893..4d20a139a86d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -640,7 +640,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, * we need to skip the double mount verification. * * This does open a hole in which we will not notice if the first - * mount using this sb set explict options and a second mount using + * mount using this sb set explicit options and a second mount using * this sb does not set any security options. (The first options * will be used for both mounts) */ @@ -6795,7 +6795,7 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode) } /* This function will check the file pass through unix socket or binder to see - * if it is a bpf related object. And apply correspinding checks on the bpf + * if it is a bpf related object. And apply corresponding checks on the bpf * object based on the type. The bpf maps and programs, not like other files and * socket, are using a shared anonymous inode inside the kernel as their inode. * So checking that inode cannot identify if the process have privilege to diff --git a/security/selinux/include/audit.h b/security/selinux/include/audit.h index 1cba83d17f41..406bceb90c6c 100644 --- a/security/selinux/include/audit.h +++ b/security/selinux/include/audit.h @@ -18,7 +18,7 @@ /** * selinux_audit_rule_init - alloc/init an selinux audit rule structure. * @field: the field this rule refers to - * @op: the operater the rule uses + * @op: the operator the rule uses * @rulestr: the text "target" of the rule * @rule: pointer to the new rule structure returned via this * From 2bfe15c5261212130f1a71f32a300bcf426443d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 25 Jan 2022 15:33:04 +0100 Subject: [PATCH 4/8] mm: create security context for memfd_secret inodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create a security context for the inodes created by memfd_secret(2) via the LSM hook inode_init_security_anon to allow a fine grained control. As secret memory areas can affect hibernation and have a global shared limit access control might be desirable. Signed-off-by: Christian Göttsche Signed-off-by: Paul Moore --- mm/secretmem.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/mm/secretmem.c b/mm/secretmem.c index 206ed6b40c1d..f544ec66ebaf 100644 --- a/mm/secretmem.c +++ b/mm/secretmem.c @@ -180,11 +180,20 @@ static struct file *secretmem_file_create(unsigned long flags) { struct file *file = ERR_PTR(-ENOMEM); struct inode *inode; + const char *anon_name = "[secretmem]"; + const struct qstr qname = QSTR_INIT(anon_name, strlen(anon_name)); + int err; inode = alloc_anon_inode(secretmem_mnt->mnt_sb); if (IS_ERR(inode)) return ERR_CAST(inode); + err = security_inode_init_security_anon(inode, &qname, NULL); + if (err) { + file = ERR_PTR(err); + goto err_free_inode; + } + file = alloc_file_pseudo(inode, secretmem_mnt, "secretmem", O_RDWR, &secretmem_fops); if (IS_ERR(file)) From 8d6d51edcb79b0906288170df165c1b86e278218 Mon Sep 17 00:00:00 2001 From: Randy Dunlap Date: Mon, 28 Feb 2022 20:14:54 -0800 Subject: [PATCH 5/8] docs: selinux: add '=' signs to kernel boot options Provide the full kernel boot option string (with ending '=' sign). They won't work without that and that is how other boot options are listed. If used without an '=' sign (as listed here), they cause an "Unknown parameters" message and are added to init's argument strings, polluting them. Unknown kernel command line parameters "enforcing checkreqprot BOOT_IMAGE=/boot/bzImage-517rc6", will be passed to user space. Run /sbin/init as init process with arguments: /sbin/init enforcing checkreqprot with environment: HOME=/ TERM=linux BOOT_IMAGE=/boot/bzImage-517rc6 Signed-off-by: Randy Dunlap Cc: Paul Moore Cc: Stephen Smalley Cc: Eric Paris Cc: selinux@vger.kernel.org Cc: Jonathan Corbet [PM: removed bogus 'Fixes' line] Signed-off-by: Paul Moore --- Documentation/admin-guide/kernel-parameters.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 8090130b544b..815af6d7a199 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -550,7 +550,7 @@ nosocket -- Disable socket memory accounting. nokmem -- Disable kernel memory accounting. - checkreqprot [SELINUX] Set initial checkreqprot flag value. + checkreqprot= [SELINUX] Set initial checkreqprot flag value. Format: { "0" | "1" } See security/selinux/Kconfig help text. 0 -- check protection applied by kernel (includes @@ -1439,7 +1439,7 @@ (in particular on some ATI chipsets). The kernel tries to set a reasonable default. - enforcing [SELINUX] Set initial enforcing status. + enforcing= [SELINUX] Set initial enforcing status. Format: {"0" | "1"} See security/selinux/Kconfig help text. 0 -- permissive (log only, no denials). From 73de1befcc53a7c68b0c5e76b9b5ac41c517760f Mon Sep 17 00:00:00 2001 From: Xiu Jianfeng Date: Mon, 13 Jun 2022 21:59:53 +0800 Subject: [PATCH 6/8] selinux: fix memleak in security_read_state_kernel() In this function, it directly returns the result of __security_read_policy without freeing the allocated memory in *data, cause memory leak issue, so free the memory if __security_read_policy failed. Signed-off-by: Xiu Jianfeng [PM: subject line tweak] Signed-off-by: Paul Moore --- security/selinux/ss/services.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 69b2734311a6..fe5fcf571c56 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -4048,6 +4048,7 @@ int security_read_policy(struct selinux_state *state, int security_read_state_kernel(struct selinux_state *state, void **data, size_t *len) { + int err; struct selinux_policy *policy; policy = rcu_dereference_protected( @@ -4060,5 +4061,11 @@ int security_read_state_kernel(struct selinux_state *state, if (!*data) return -ENOMEM; - return __security_read_policy(policy, *data, len); + err = __security_read_policy(policy, *data, len); + if (err) { + vfree(*data); + *data = NULL; + *len = 0; + } + return err; } From 15ec76fb29be31df2bccb30fc09875274cba2776 Mon Sep 17 00:00:00 2001 From: Xiu Jianfeng Date: Tue, 14 Jun 2022 10:14:49 +0800 Subject: [PATCH 7/8] selinux: Add boundary check in put_entry() Just like next_entry(), boundary check is necessary to prevent memory out-of-bound access. Signed-off-by: Xiu Jianfeng Signed-off-by: Paul Moore --- security/selinux/ss/policydb.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index c24d4e1063ea..ffc4e7bad205 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -370,6 +370,8 @@ static inline int put_entry(const void *buf, size_t bytes, int num, struct polic { size_t len = bytes * num; + if (len > fp->len) + return -EINVAL; memcpy(fp->data, buf, len); fp->data += len; fp->len -= len; From ef54ccb61616d8293bc68220d88a8e74271141b5 Mon Sep 17 00:00:00 2001 From: Xiu Jianfeng Date: Fri, 17 Jun 2022 17:44:12 +0800 Subject: [PATCH 8/8] selinux: selinux_add_opt() callers free memory The selinux_add_opt() function may need to allocate memory for the mount options if none has already been allocated, but there is no need to free that memory on error as the callers handle that. Drop the existing kfree() on error to help increase consistency in the selinux_add_opt() error handling. This patch also changes selinux_add_opt() to return -EINVAL when the mount option value, @s, is NULL. It currently return -ENOMEM. Link: https://lore.kernel.org/lkml/20220611090550.135674-1-xiujianfeng@huawei.com/T/ Suggested-by: Paul Moore Signed-off-by: Xiu Jianfeng [PM: fix subject, rework commit description language] Signed-off-by: Paul Moore --- security/selinux/hooks.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4d20a139a86d..9d08b91e05a2 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -944,10 +944,12 @@ out: return rc; } +/* + * NOTE: the caller is resposible for freeing the memory even if on error. + */ static int selinux_add_opt(int token, const char *s, void **mnt_opts) { struct selinux_mnt_opts *opts = *mnt_opts; - bool is_alloc_opts = false; u32 *dst_sid; int rc; @@ -955,7 +957,7 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) /* eaten and completely ignored */ return 0; if (!s) - return -ENOMEM; + return -EINVAL; if (!selinux_initialized(&selinux_state)) { pr_warn("SELinux: Unable to set superblock options before the security server is initialized\n"); @@ -967,7 +969,6 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) if (!opts) return -ENOMEM; *mnt_opts = opts; - is_alloc_opts = true; } switch (token) { @@ -1002,10 +1003,6 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) return rc; err: - if (is_alloc_opts) { - kfree(opts); - *mnt_opts = NULL; - } pr_warn(SEL_MOUNT_FAIL_MSG); return -EINVAL; }