From 610b17b05c5c682fbb8fefedae1aacaab412eac3 Mon Sep 17 00:00:00 2001 From: Bo Liu Date: Tue, 25 Oct 2022 21:06:34 -0400 Subject: audit: Fix some kernel-doc warnings The current code provokes some kernel-doc warnings: security/lsm_audit.c:198: warning: Function parameter or member 'ab' not described in 'dump_common_audit_data' Signed-off-by: Bo Liu [PM: description line wrap] Signed-off-by: Paul Moore --- security/lsm_audit.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security') diff --git a/security/lsm_audit.c b/security/lsm_audit.c index 75cc3f8d2a42..a7355b4b9bb8 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -190,6 +190,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr, /** * dump_common_audit_data - helper to dump common audit data + * @ab : the audit buffer * @a : common audit data * */ -- cgit v1.2.3 From b10b9c342f7571f287fd422be5d5c0beb26ba974 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 10 Oct 2022 12:31:21 -0400 Subject: lsm: make security_socket_getpeersec_stream() sockptr_t safe Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the sockptr_t argument") made it possible to call sk_getsockopt() with both user and kernel address space buffers through the use of the sockptr_t type. Unfortunately at the time of conversion the security_socket_getpeersec_stream() LSM hook was written to only accept userspace buffers, and in a desire to avoid having to change the LSM hook the commit author simply passed the sockptr_t's userspace buffer pointer. Since the only sk_getsockopt() callers at the time of conversion which used kernel sockptr_t buffers did not allow SO_PEERSEC, and hence the security_socket_getpeersec_stream() hook, this was acceptable but also very fragile as future changes presented the possibility of silently passing kernel space pointers to the LSM hook. There are several ways to protect against this, including careful code review of future commits, but since relying on code review to catch bugs is a recipe for disaster and the upstream eBPF maintainer is "strongly against defensive programming", this patch updates the LSM hook, and all of the implementations to support sockptr_t and safely handle both user and kernel space buffers. Acked-by: Casey Schaufler Acked-by: John Johansen Signed-off-by: Paul Moore --- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 4 ++-- include/linux/security.h | 11 +++++++---- net/core/sock.c | 3 ++- security/apparmor/lsm.c | 29 +++++++++++++---------------- security/security.c | 6 +++--- security/selinux/hooks.c | 13 ++++++------- security/smack/smack_lsm.c | 19 ++++++++++--------- 8 files changed, 44 insertions(+), 43 deletions(-) (limited to 'security') diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index ec119da1d89b..6abde829b6e5 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -302,7 +302,7 @@ LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname) LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how) LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb) LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock, - char __user *optval, int __user *optlen, unsigned len) + sockptr_t optval, sockptr_t optlen, unsigned int len) LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock, struct sk_buff *skb, u32 *secid) LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 9a7e69aff3d1..2831efebde69 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -949,8 +949,8 @@ * SO_GETPEERSEC. For tcp sockets this can be meaningful if the * socket is associated with an ipsec SA. * @sock is the local socket. - * @optval userspace memory where the security state is to be copied. - * @optlen userspace int where the module should copy the actual length + * @optval memory where the security state is to be copied. + * @optlen memory where the module should copy the actual length * of the security state. * @len as input is the maximum length to copy to userspace provided * by the caller. diff --git a/include/linux/security.h b/include/linux/security.h index ca1b7109c0db..0e419c595cee 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -31,6 +31,7 @@ #include #include #include +#include struct linux_binprm; struct cred; @@ -1411,8 +1412,8 @@ int security_socket_getsockopt(struct socket *sock, int level, int optname); int security_socket_setsockopt(struct socket *sock, int level, int optname); int security_socket_shutdown(struct socket *sock, int how); int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb); -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len); +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval, + sockptr_t optlen, unsigned int len); int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid); int security_sk_alloc(struct sock *sk, int family, gfp_t priority); void security_sk_free(struct sock *sk); @@ -1548,8 +1549,10 @@ static inline int security_sock_rcv_skb(struct sock *sk, return 0; } -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +static inline int security_socket_getpeersec_stream(struct socket *sock, + sockptr_t optval, + sockptr_t optlen, + unsigned int len) { return -ENOPROTOOPT; } diff --git a/net/core/sock.c b/net/core/sock.c index a3ba0358c77c..c9c6af85d9c0 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1793,7 +1793,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname, break; case SO_PEERSEC: - return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len); + return security_socket_getpeersec_stream(sock, + optval, optlen, len); case SO_MARK: v.val = sk->sk_mark; diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index f56070270c69..89e84ef54e8e 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1103,11 +1103,10 @@ static struct aa_label *sk_peer_label(struct sock *sk) * Note: for tcp only valid if using ipsec or cipso on lan */ static int apparmor_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, + sockptr_t optval, sockptr_t optlen, unsigned int len) { - char *name; + char *name = NULL; int slen, error = 0; struct aa_label *label; struct aa_label *peer; @@ -1124,23 +1123,21 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock, /* don't include terminating \0 in slen, it breaks some apps */ if (slen < 0) { error = -ENOMEM; - } else { - if (slen > len) { - error = -ERANGE; - } else if (copy_to_user(optval, name, slen)) { - error = -EFAULT; - goto out; - } - if (put_user(slen, optlen)) - error = -EFAULT; -out: - kfree(name); - + goto done; + } + if (slen > len) { + error = -ERANGE; + goto done_len; } + if (copy_to_sockptr(optval, name, slen)) + error = -EFAULT; +done_len: + if (copy_to_sockptr(optlen, &slen, sizeof(slen))) + error = -EFAULT; done: end_current_label_crit_section(label); - + kfree(name); return error; } diff --git a/security/security.c b/security/security.c index 79d82cb6e469..f27c885ee98d 100644 --- a/security/security.c +++ b/security/security.c @@ -2267,11 +2267,11 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) } EXPORT_SYMBOL(security_sock_rcv_skb); -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval, + sockptr_t optlen, unsigned int len) { return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock, - optval, optlen, len); + optval, optlen, len); } int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f553c370397e..0bdddeba90a6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5119,11 +5119,12 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) return err; } -static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +static int selinux_socket_getpeersec_stream(struct socket *sock, + sockptr_t optval, sockptr_t optlen, + unsigned int len) { int err = 0; - char *scontext; + char *scontext = NULL; u32 scontext_len; struct sk_security_struct *sksec = sock->sk->sk_security; u32 peer_sid = SECSID_NULL; @@ -5139,17 +5140,15 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op &scontext_len); if (err) return err; - if (scontext_len > len) { err = -ERANGE; goto out_len; } - if (copy_to_user(optval, scontext, scontext_len)) + if (copy_to_sockptr(optval, scontext, scontext_len)) err = -EFAULT; - out_len: - if (put_user(scontext_len, optlen)) + if (copy_to_sockptr(optlen, &scontext_len, sizeof(scontext_len))) err = -EFAULT; kfree(scontext); return err; diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index b6306d71c908..2bd7fadf7fb4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4006,12 +4006,12 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) * returns zero on success, an error code otherwise */ static int smack_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, unsigned len) + sockptr_t optval, sockptr_t optlen, + unsigned int len) { struct socket_smack *ssp; char *rcp = ""; - int slen = 1; + u32 slen = 1; int rc = 0; ssp = sock->sk->sk_security; @@ -4019,15 +4019,16 @@ static int smack_socket_getpeersec_stream(struct socket *sock, rcp = ssp->smk_packet->smk_known; slen = strlen(rcp) + 1; } - - if (slen > len) + if (slen > len) { rc = -ERANGE; - else if (copy_to_user(optval, rcp, slen) != 0) - rc = -EFAULT; + goto out_len; + } - if (put_user(slen, optlen) != 0) + if (copy_to_sockptr(optval, rcp, slen)) + rc = -EFAULT; +out_len: + if (copy_to_sockptr(optlen, &slen, sizeof(slen))) rc = -EFAULT; - return rc; } -- cgit v1.2.3 From 86ef3c735ec81566cd38174ed5e90e9721473064 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 1 Nov 2022 17:05:29 -0700 Subject: LSM: Better reporting of actual LSMs at boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhance the details reported by "lsm.debug" in several ways: - report contents of "security=" - report contents of "CONFIG_LSM" - report contents of "lsm=" - report any early LSM details - whitespace-align the output of similar phases for easier visual parsing - change "disabled" to more accurate "skipped" - explain what "skipped" and "ignored" mean in a parenthetical Upgrade the "security= is ignored" warning from pr_info to pr_warn, and include full arguments list to make the cause even more clear. Replace static "Security Framework initializing" pr_info with specific list of the resulting order of enabled LSMs. For example, if the kernel is built with: CONFIG_SECURITY_SELINUX=y CONFIG_SECURITY_APPARMOR=y CONFIG_SECURITY_LOADPIN=y CONFIG_SECURITY_YAMA=y CONFIG_SECURITY_SAFESETID=y CONFIG_SECURITY_LOCKDOWN_LSM=y CONFIG_SECURITY_LANDLOCK=y CONFIG_INTEGRITY=y CONFIG_BPF_LSM=y CONFIG_DEFAULT_SECURITY_APPARMOR=y CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,integrity,selinux, smack,tomoyo,apparmor,bpf" Booting without options will show: LSM: initializing lsm=lockdown,capability,landlock,yama,loadpin, safesetid,integrity,selinux,bpf landlock: Up and running. Yama: becoming mindful. LoadPin: ready to pin (currently not enforcing) SELinux: Initializing. LSM support for eBPF active Boot with "lsm.debug" will show: LSM: legacy security= *unspecified* LSM: CONFIG_LSM=landlock,lockdown,yama,loadpin,safesetid,integrity, selinux,smack,tomoyo,apparmor,bpf LSM: boot arg lsm= *unspecified* LSM: early started: lockdown (enabled) LSM: first ordered: capability (enabled) LSM: builtin ordered: landlock (enabled) LSM: builtin ignored: lockdown (not built into kernel) LSM: builtin ordered: yama (enabled) LSM: builtin ordered: loadpin (enabled) LSM: builtin ordered: safesetid (enabled) LSM: builtin ordered: integrity (enabled) LSM: builtin ordered: selinux (enabled) LSM: builtin ignored: smack (not built into kernel) LSM: builtin ignored: tomoyo (not built into kernel) LSM: builtin ordered: apparmor (enabled) LSM: builtin ordered: bpf (enabled) LSM: exclusive chosen: selinux LSM: exclusive disabled: apparmor LSM: initializing lsm=lockdown,capability,landlock,yama,loadpin, safesetid,integrity,selinux,bpf LSM: cred blob size = 32 LSM: file blob size = 16 LSM: inode blob size = 72 LSM: ipc blob size = 8 LSM: msg_msg blob size = 4 LSM: superblock blob size = 80 LSM: task blob size = 8 LSM: initializing capability LSM: initializing landlock landlock: Up and running. LSM: initializing yama Yama: becoming mindful. LSM: initializing loadpin LoadPin: ready to pin (currently not enforcing) LSM: initializing safesetid LSM: initializing integrity LSM: initializing selinux SELinux: Initializing. LSM: initializing bpf LSM support for eBPF active And some examples of how the lsm.debug ordering report changes... With "lsm.debug security=selinux": LSM: legacy security=selinux LSM: CONFIG_LSM=landlock,lockdown,yama,loadpin,safesetid,integrity, selinux,smack,tomoyo,apparmor,bpf LSM: boot arg lsm= *unspecified* LSM: early started: lockdown (enabled) LSM: first ordered: capability (enabled) LSM: security=selinux disabled: apparmor (only one legacy major LSM) LSM: builtin ordered: landlock (enabled) LSM: builtin ignored: lockdown (not built into kernel) LSM: builtin ordered: yama (enabled) LSM: builtin ordered: loadpin (enabled) LSM: builtin ordered: safesetid (enabled) LSM: builtin ordered: integrity (enabled) LSM: builtin ordered: selinux (enabled) LSM: builtin ignored: smack (not built into kernel) LSM: builtin ignored: tomoyo (not built into kernel) LSM: builtin ordered: apparmor (disabled) LSM: builtin ordered: bpf (enabled) LSM: exclusive chosen: selinux LSM: initializing lsm=lockdown,capability,landlock,yama,loadpin, safesetid,integrity,selinux,bpf With "lsm.debug lsm=integrity,selinux,loadpin,crabability,bpf, loadpin,loadpin": LSM: legacy security= *unspecified* LSM: CONFIG_LSM=landlock,lockdown,yama,loadpin,safesetid,integrity, selinux,smack,tomoyo,apparmor,bpf LSM: boot arg lsm=integrity,selinux,loadpin,capability,bpf,loadpin, loadpin LSM: early started: lockdown (enabled) LSM: first ordered: capability (enabled) LSM: cmdline ordered: integrity (enabled) LSM: cmdline ordered: selinux (enabled) LSM: cmdline ordered: loadpin (enabled) LSM: cmdline ignored: crabability (not built into kernel) LSM: cmdline ordered: bpf (enabled) LSM: cmdline skipped: apparmor (not in requested order) LSM: cmdline skipped: yama (not in requested order) LSM: cmdline skipped: safesetid (not in requested order) LSM: cmdline skipped: landlock (not in requested order) LSM: exclusive chosen: selinux LSM: initializing lsm=lockdown,capability,integrity,selinux,loadpin,bpf Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook Acked-by: Casey Schaufler Acked-by: Mickaël Salaün [PM: line wrapped commit description] Signed-off-by: Paul Moore --- security/security.c | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-) (limited to 'security') diff --git a/security/security.c b/security/security.c index f27c885ee98d..d713d84d82d6 100644 --- a/security/security.c +++ b/security/security.c @@ -161,8 +161,8 @@ static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from) lsm->enabled = &lsm_enabled_true; ordered_lsms[last_lsm++] = lsm; - init_debug("%s ordering: %s (%sabled)\n", from, lsm->name, - is_enabled(lsm) ? "en" : "dis"); + init_debug("%s ordered: %s (%s)\n", from, lsm->name, + is_enabled(lsm) ? "enabled" : "disabled"); } /* Is an LSM allowed to be initialized? */ @@ -224,7 +224,7 @@ static void __init prepare_lsm(struct lsm_info *lsm) if (enabled) { if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && !exclusive) { exclusive = lsm; - init_debug("exclusive chosen: %s\n", lsm->name); + init_debug("exclusive chosen: %s\n", lsm->name); } lsm_set_blob_sizes(lsm->blobs); @@ -252,7 +252,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) /* LSM_ORDER_FIRST is always first. */ for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) { if (lsm->order == LSM_ORDER_FIRST) - append_ordered_lsm(lsm, "first"); + append_ordered_lsm(lsm, " first"); } /* Process "security=", if given. */ @@ -270,7 +270,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) if ((major->flags & LSM_FLAG_LEGACY_MAJOR) && strcmp(major->name, chosen_major_lsm) != 0) { set_enabled(major, false); - init_debug("security=%s disabled: %s\n", + init_debug("security=%s disabled: %s (only one legacy major LSM)\n", chosen_major_lsm, major->name); } } @@ -291,7 +291,8 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) } if (!found) - init_debug("%s ignored: %s\n", origin, name); + init_debug("%s ignored: %s (not built into kernel)\n", + origin, name); } /* Process "security=", if given. */ @@ -309,7 +310,8 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) if (exists_ordered_lsm(lsm)) continue; set_enabled(lsm, false); - init_debug("%s disabled: %s\n", origin, lsm->name); + init_debug("%s skipped: %s (not in requested order)\n", + origin, lsm->name); } kfree(sep); @@ -320,6 +322,24 @@ static void __init lsm_early_task(struct task_struct *task); static int lsm_append(const char *new, char **result); +static void __init report_lsm_order(void) +{ + struct lsm_info **lsm, *early; + int first = 0; + + pr_info("initializing lsm="); + + /* Report each enabled LSM name, comma separated. */ + for (early = __start_early_lsm_info; early < __end_early_lsm_info; early++) + if (is_enabled(early)) + pr_cont("%s%s", first++ == 0 ? "" : ",", early->name); + for (lsm = ordered_lsms; *lsm; lsm++) + if (is_enabled(*lsm)) + pr_cont("%s%s", first++ == 0 ? "" : ",", (*lsm)->name); + + pr_cont("\n"); +} + static void __init ordered_lsm_init(void) { struct lsm_info **lsm; @@ -329,7 +349,8 @@ static void __init ordered_lsm_init(void) if (chosen_lsm_order) { if (chosen_major_lsm) { - pr_info("security= is ignored because it is superseded by lsm=\n"); + pr_warn("security=%s is ignored because it is superseded by lsm=%s\n", + chosen_major_lsm, chosen_lsm_order); chosen_major_lsm = NULL; } ordered_lsm_parse(chosen_lsm_order, "cmdline"); @@ -339,6 +360,8 @@ static void __init ordered_lsm_init(void) for (lsm = ordered_lsms; *lsm; lsm++) prepare_lsm(*lsm); + report_lsm_order(); + init_debug("cred blob size = %d\n", blob_sizes.lbs_cred); init_debug("file blob size = %d\n", blob_sizes.lbs_file); init_debug("inode blob size = %d\n", blob_sizes.lbs_inode); @@ -395,13 +418,17 @@ int __init security_init(void) { struct lsm_info *lsm; - pr_info("Security Framework initializing\n"); + init_debug("legacy security=%s\n", chosen_major_lsm ?: " *unspecified*"); + init_debug(" CONFIG_LSM=%s\n", builtin_lsm_order); + init_debug("boot arg lsm=%s\n", chosen_lsm_order ?: " *unspecified*"); /* * Append the names of the early LSM modules now that kmalloc() is * available */ for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { + init_debug(" early started: %s (%s)\n", lsm->name, + is_enabled(lsm) ? "enabled" : "disabled"); if (lsm->enabled) lsm_append(lsm->name, &lsm_names); } -- cgit v1.2.3 From e68bfbd3b3c3a0ec3cf8c230996ad8cabe90322f Mon Sep 17 00:00:00 2001 From: Wang Weiyang Date: Tue, 25 Oct 2022 19:31:01 +0800 Subject: device_cgroup: Roll back to original exceptions after copy failure When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's exceptions will be cleaned and A's behavior is changed to DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's whitelist. If copy failure occurs, just return leaving A to grant permissions to all devices. And A may grant more permissions than parent. Backup A's whitelist and recover original exceptions after copy failure. Cc: stable@vger.kernel.org Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior") Signed-off-by: Wang Weiyang Reviewed-by: Aristeu Rozanski Signed-off-by: Paul Moore --- security/device_cgroup.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) (limited to 'security') diff --git a/security/device_cgroup.c b/security/device_cgroup.c index a9f8c63a96d1..bef2b9285fb3 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -82,6 +82,17 @@ free_and_exit: return -ENOMEM; } +static void dev_exceptions_move(struct list_head *dest, struct list_head *orig) +{ + struct dev_exception_item *ex, *tmp; + + lockdep_assert_held(&devcgroup_mutex); + + list_for_each_entry_safe(ex, tmp, orig, list) { + list_move_tail(&ex->list, dest); + } +} + /* * called under devcgroup_mutex */ @@ -604,11 +615,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, int count, rc = 0; struct dev_exception_item ex; struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent); + struct dev_cgroup tmp_devcgrp; if (!capable(CAP_SYS_ADMIN)) return -EPERM; memset(&ex, 0, sizeof(ex)); + memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp)); b = buffer; switch (*b) { @@ -620,15 +633,27 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, if (!may_allow_all(parent)) return -EPERM; - dev_exception_clean(devcgroup); - devcgroup->behavior = DEVCG_DEFAULT_ALLOW; - if (!parent) + if (!parent) { + devcgroup->behavior = DEVCG_DEFAULT_ALLOW; + dev_exception_clean(devcgroup); break; + } + INIT_LIST_HEAD(&tmp_devcgrp.exceptions); + rc = dev_exceptions_copy(&tmp_devcgrp.exceptions, + &devcgroup->exceptions); + if (rc) + return rc; + dev_exception_clean(devcgroup); rc = dev_exceptions_copy(&devcgroup->exceptions, &parent->exceptions); - if (rc) + if (rc) { + dev_exceptions_move(&devcgroup->exceptions, + &tmp_devcgrp.exceptions); return rc; + } + devcgroup->behavior = DEVCG_DEFAULT_ALLOW; + dev_exception_clean(&tmp_devcgrp); break; case DEVCG_DENY: if (css_has_online_children(&devcgroup->css)) -- cgit v1.2.3 From f6fbd8cbf3ed1915a7b957f2801f7c306a686c08 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 9 Nov 2022 14:14:35 -0500 Subject: lsm,fs: fix vfs_getxattr_alloc() return type and caller error paths The vfs_getxattr_alloc() function currently returns a ssize_t value despite the fact that it only uses int values internally for return values. Fix this by converting vfs_getxattr_alloc() to return an int type and adjust the callers as necessary. As part of these caller modifications, some of the callers are fixed to properly free the xattr value buffer on both success and failure to ensure that memory is not leaked in the failure case. Reviewed-by: Serge Hallyn Reviewed-by: Mimi Zohar Signed-off-by: Paul Moore --- fs/xattr.c | 5 +++-- include/linux/xattr.h | 6 +++--- security/apparmor/domain.c | 3 +-- security/commoncap.c | 22 ++++++++++------------ security/integrity/evm/evm_crypto.c | 5 +++-- security/integrity/evm/evm_main.c | 7 +++++-- security/integrity/ima/ima.h | 5 +++-- security/integrity/ima/ima_appraise.c | 6 +++--- security/integrity/ima/ima_main.c | 6 ++++-- security/integrity/ima/ima_template_lib.c | 11 +++++------ 10 files changed, 40 insertions(+), 36 deletions(-) (limited to 'security') diff --git a/fs/xattr.c b/fs/xattr.c index 61107b6bbed2..f38fd969b5fc 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -354,11 +354,12 @@ out_noalloc: * vfs_getxattr_alloc - allocate memory, if necessary, before calling getxattr * * Allocate memory, if not already allocated, or re-allocate correct size, - * before retrieving the extended attribute. + * before retrieving the extended attribute. The xattr value buffer should + * always be freed by the caller, even on error. * * Returns the result of alloc, if failed, or the getxattr operation. */ -ssize_t +int vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry, const char *name, char **xattr_value, size_t xattr_size, gfp_t flags) diff --git a/include/linux/xattr.h b/include/linux/xattr.h index 4c379d23ec6e..2218a9645b89 100644 --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -68,9 +68,9 @@ int __vfs_removexattr_locked(struct user_namespace *, struct dentry *, int vfs_removexattr(struct user_namespace *, struct dentry *, const char *); ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size); -ssize_t vfs_getxattr_alloc(struct user_namespace *mnt_userns, - struct dentry *dentry, const char *name, - char **xattr_value, size_t size, gfp_t flags); +int vfs_getxattr_alloc(struct user_namespace *mnt_userns, + struct dentry *dentry, const char *name, + char **xattr_value, size_t size, gfp_t flags); int xattr_supported_namespace(struct inode *inode, const char *prefix); diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 91689d34d281..04a818d51604 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -311,10 +311,9 @@ static int aa_xattrs_match(const struct linux_binprm *bprm, struct aa_profile *profile, unsigned int state) { int i; - ssize_t size; struct dentry *d; char *value = NULL; - int value_size = 0, ret = profile->xattr_count; + int size, value_size = 0, ret = profile->xattr_count; if (!bprm || !profile->xattr_count) return 0; diff --git a/security/commoncap.c b/security/commoncap.c index 5fc8986c3c77..d4fc89095513 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -350,14 +350,14 @@ static __u32 sansflags(__u32 m) return m & ~VFS_CAP_FLAGS_EFFECTIVE; } -static bool is_v2header(size_t size, const struct vfs_cap_data *cap) +static bool is_v2header(int size, const struct vfs_cap_data *cap) { if (size != XATTR_CAPS_SZ_2) return false; return sansflags(le32_to_cpu(cap->magic_etc)) == VFS_CAP_REVISION_2; } -static bool is_v3header(size_t size, const struct vfs_cap_data *cap) +static bool is_v3header(int size, const struct vfs_cap_data *cap) { if (size != XATTR_CAPS_SZ_3) return false; @@ -379,7 +379,7 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns, struct inode *inode, const char *name, void **buffer, bool alloc) { - int size, ret; + int size; kuid_t kroot; u32 nsmagic, magic; uid_t root, mappedroot; @@ -395,20 +395,18 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns, dentry = d_find_any_alias(inode); if (!dentry) return -EINVAL; - - size = sizeof(struct vfs_ns_cap_data); - ret = (int)vfs_getxattr_alloc(mnt_userns, dentry, XATTR_NAME_CAPS, - &tmpbuf, size, GFP_NOFS); + size = vfs_getxattr_alloc(mnt_userns, dentry, XATTR_NAME_CAPS, &tmpbuf, + sizeof(struct vfs_ns_cap_data), GFP_NOFS); dput(dentry); - - if (ret < 0 || !tmpbuf) - return ret; + /* gcc11 complains if we don't check for !tmpbuf */ + if (size < 0 || !tmpbuf) + goto out_free; fs_ns = inode->i_sb->s_user_ns; cap = (struct vfs_cap_data *) tmpbuf; - if (is_v2header((size_t) ret, cap)) { + if (is_v2header(size, cap)) { root = 0; - } else if (is_v3header((size_t) ret, cap)) { + } else if (is_v3header(size, cap)) { nscap = (struct vfs_ns_cap_data *) tmpbuf; root = le32_to_cpu(nscap->rootid); } else { diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 708de9656bbd..fa5ff13fa8c9 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -335,14 +335,15 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode) (char **)&xattr_data, 0, GFP_NOFS); if (rc <= 0) { if (rc == -ENODATA) - return 0; - return rc; + rc = 0; + goto out; } if (xattr_data->type == EVM_XATTR_PORTABLE_DIGSIG) rc = 1; else rc = 0; +out: kfree(xattr_data); return rc; } diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 23d484e05e6f..bce72e80fd12 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -519,14 +519,17 @@ static int evm_xattr_change(struct user_namespace *mnt_userns, rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name, &xattr_data, 0, GFP_NOFS); - if (rc < 0) - return 1; + if (rc < 0) { + rc = 1; + goto out; + } if (rc == xattr_value_len) rc = !!memcmp(xattr_value, xattr_data, rc); else rc = 1; +out: kfree(xattr_data); return rc; } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index be965a8715e4..03b440921e61 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -326,7 +326,7 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint, enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, int xattr_len); int ima_read_xattr(struct dentry *dentry, - struct evm_ima_xattr_data **xattr_value); + struct evm_ima_xattr_data **xattr_value, int xattr_len); #else static inline int ima_check_blacklist(struct integrity_iint_cache *iint, @@ -372,7 +372,8 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len) } static inline int ima_read_xattr(struct dentry *dentry, - struct evm_ima_xattr_data **xattr_value) + struct evm_ima_xattr_data **xattr_value, + int xattr_len) { return 0; } diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 3e0fbbd99534..88ffb15ca0e2 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -221,12 +221,12 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, } int ima_read_xattr(struct dentry *dentry, - struct evm_ima_xattr_data **xattr_value) + struct evm_ima_xattr_data **xattr_value, int xattr_len) { - ssize_t ret; + int ret; ret = vfs_getxattr_alloc(&init_user_ns, dentry, XATTR_NAME_IMA, - (char **)xattr_value, 0, GFP_NOFS); + (char **)xattr_value, xattr_len, GFP_NOFS); if (ret == -EOPNOTSUPP) ret = 0; return ret; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 040b03ddc1c7..0226899947d6 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -293,7 +293,8 @@ static int process_measurement(struct file *file, const struct cred *cred, /* HASH sets the digital signature and update flags, nothing else */ if ((action & IMA_HASH) && !(test_bit(IMA_DIGSIG, &iint->atomic_flags))) { - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value, xattr_len); if ((xattr_value && xattr_len > 2) && (xattr_value->type == EVM_IMA_XATTR_DIGSIG)) set_bit(IMA_DIGSIG, &iint->atomic_flags); @@ -316,7 +317,8 @@ static int process_measurement(struct file *file, const struct cred *cred, if ((action & IMA_APPRAISE_SUBMASK) || strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { /* read 'security.ima' */ - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value, xattr_len); /* * Read the appended modsig if allowed by the policy, and allow diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 7bf9b1507220..4564faae7d67 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -601,16 +601,15 @@ int ima_eventevmsig_init(struct ima_event_data *event_data, rc = vfs_getxattr_alloc(&init_user_ns, file_dentry(event_data->file), XATTR_NAME_EVM, (char **)&xattr_data, 0, GFP_NOFS); - if (rc <= 0) - return 0; - - if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) { - kfree(xattr_data); - return 0; + if (rc <= 0 || xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) { + rc = 0; + goto out; } rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX, field_data); + +out: kfree(xattr_data); return rc; } -- cgit v1.2.3