From b21507e272627c434e8dd74e8d51fd8245281b59 Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Mon, 9 Jan 2017 10:07:31 -0500 Subject: proc,security: move restriction on writing /proc/pid/attr nodes to proc Processes can only alter their own security attributes via /proc/pid/attr nodes. This is presently enforced by each individual security module and is also imposed by the Linux credentials implementation, which only allows a task to alter its own credentials. Move the check enforcing this restriction from the individual security modules to proc_pid_attr_write() before calling the security hook, and drop the unnecessary task argument to the security hook since it can only ever be the current task. Signed-off-by: Stephen Smalley Acked-by: Casey Schaufler Acked-by: John Johansen Signed-off-by: Paul Moore --- security/smack/smack_lsm.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 94dc9d406ce3..8da4a6b9ca4d 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) /** * smack_setprocattr - Smack process attribute setting - * @p: the object task * @name: the name of the attribute in /proc/.../attr * @value: the value to set * @size: the size of the value @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) * * Returns the length of the smack label or an error code */ -static int smack_setprocattr(struct task_struct *p, char *name, - void *value, size_t size) +static int smack_setprocattr(const char *name, void *value, size_t size) { struct task_smack *tsp = current_security(); struct cred *new; @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name, struct smack_known_list_elem *sklep; int rc; - /* - * Changing another process' Smack value is too dangerous - * and supports no sane use case. - */ - if (p != current) - return -EPERM; - if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) return -EPERM; -- cgit v1.2.3 From 3c7ce3427106f0fa2d8593f9fd8f82c494215782 Mon Sep 17 00:00:00 2001 From: Vishal Goel Date: Wed, 23 Nov 2016 10:31:08 +0530 Subject: SMACK: Add the rcu synchronization mechanism in ipv6 hooks Add the rcu synchronization mechanism for accessing smk_ipv6_port_list in smack IPv6 hooks. Access to the port list is vulnerable to a race condition issue,it does not apply proper synchronization methods while working on critical section. It is possible that when one thread is reading the list, at the same time another thread is modifying the same port list, which can cause the major problems. To ensure proper synchronization between two threads, rcu mechanism has been applied while accessing and modifying the port list. RCU will also not affect the performance, as there are more accesses than modification where RCU is most effective synchronization mechanism. Signed-off-by: Vishal Goel Signed-off-by: Himanshu Shukla Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 94dc9d406ce3..b76696b84e5c 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -52,6 +52,7 @@ #define SMK_SENDING 2 #ifdef SMACK_IPV6_PORT_LABELING +DEFINE_MUTEX(smack_ipv6_lock); static LIST_HEAD(smk_ipv6_port_list); #endif static struct kmem_cache *smack_inode_cache; @@ -2603,17 +2604,20 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) * on the bound socket. Take the changes to the port * as well. */ - list_for_each_entry(spp, &smk_ipv6_port_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { if (sk != spp->smk_sock) continue; spp->smk_in = ssp->smk_in; spp->smk_out = ssp->smk_out; + rcu_read_unlock(); return; } /* * A NULL address is only used for updating existing * bound entries. If there isn't one, it's OK. */ + rcu_read_unlock(); return; } @@ -2629,16 +2633,18 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) * Look for an existing port list entry. * This is an indication that a port is getting reused. */ - list_for_each_entry(spp, &smk_ipv6_port_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { if (spp->smk_port != port) continue; spp->smk_port = port; spp->smk_sock = sk; spp->smk_in = ssp->smk_in; spp->smk_out = ssp->smk_out; + rcu_read_unlock(); return; } - + rcu_read_unlock(); /* * A new port entry is required. */ @@ -2651,7 +2657,9 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) spp->smk_in = ssp->smk_in; spp->smk_out = ssp->smk_out; - list_add(&spp->list, &smk_ipv6_port_list); + mutex_lock(&smack_ipv6_lock); + list_add_rcu(&spp->list, &smk_ipv6_port_list); + mutex_unlock(&smack_ipv6_lock); return; } @@ -2702,7 +2710,8 @@ static int smk_ipv6_port_check(struct sock *sk, struct sockaddr_in6 *address, return 0; port = ntohs(address->sin6_port); - list_for_each_entry(spp, &smk_ipv6_port_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { if (spp->smk_port != port) continue; object = spp->smk_in; @@ -2710,6 +2719,7 @@ static int smk_ipv6_port_check(struct sock *sk, struct sockaddr_in6 *address, ssp->smk_packet = spp->smk_out; break; } + rcu_read_unlock(); return smk_ipv6_check(skp, object, address, act); } -- cgit v1.2.3 From 9d44c97384fdc04585c5d5c985fe88ba7285b5ac Mon Sep 17 00:00:00 2001 From: Vishal Goel Date: Wed, 23 Nov 2016 10:31:59 +0530 Subject: Smack: Fix the issue of permission denied error in ipv6 hook Permission denied error comes when 2 IPv6 servers are running and client tries to connect one of them. Scenario is that both servers are using same IP and port but different protocols(Udp and tcp). They are using different SMACK64IPIN labels.Tcp server is using "test" and udp server is using "test-in". When we try to run tcp client with SMACK64IPOUT label as "test", then connection denied error comes. It should not happen since both tcp server and client labels are same.This happens because there is no check for protocol in smk_ipv6_port_label() function while searching for the earlier port entry. It checks whether there is an existing port entry on the basis of port only. So it updates the earlier port entry in the list. Due to which smack label gets changed for earlier entry in the "smk_ipv6_port_list" list and permission denied error comes. Now a check is added for socket type also.Now if 2 processes use same port but different protocols (tcp or udp), then 2 different port entries will be added in the list. Similarly while checking smack access in smk_ipv6_port_check() function, port entry is searched on the basis of both port and protocol. Signed-off-by: Vishal Goel Signed-off-by: Himanshu Shukla Signed-off-by: Casey Schaufler --- security/smack/smack.h | 1 + security/smack/smack_lsm.c | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack.h b/security/smack/smack.h index 77abe2efacae..73480ee07478 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -173,6 +173,7 @@ struct smk_port_label { unsigned short smk_port; /* the port number */ struct smack_known *smk_in; /* inbound label */ struct smack_known *smk_out; /* outgoing label */ + short smk_sock_type; /* Socket type */ }; #endif /* SMACK_IPV6_PORT_LABELING */ diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index b76696b84e5c..5e4d2bdb38cb 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -2635,7 +2635,7 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) */ rcu_read_lock(); list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { - if (spp->smk_port != port) + if (spp->smk_port != port || spp->smk_sock_type != sock->type) continue; spp->smk_port = port; spp->smk_sock = sk; @@ -2656,6 +2656,7 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) spp->smk_sock = sk; spp->smk_in = ssp->smk_in; spp->smk_out = ssp->smk_out; + spp->smk_sock_type = sock->type; mutex_lock(&smack_ipv6_lock); list_add_rcu(&spp->list, &smk_ipv6_port_list); @@ -2712,7 +2713,7 @@ static int smk_ipv6_port_check(struct sock *sk, struct sockaddr_in6 *address, port = ntohs(address->sin6_port); rcu_read_lock(); list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { - if (spp->smk_port != port) + if (spp->smk_port != port || spp->smk_sock_type != sk->sk_type) continue; object = spp->smk_in; if (act == SMK_CONNECTING) -- cgit v1.2.3 From 0c96d1f5328e834048480e4696e6867992115c33 Mon Sep 17 00:00:00 2001 From: Vishal Goel Date: Wed, 23 Nov 2016 10:32:54 +0530 Subject: Smack: Fix the issue of wrong SMACK label update in socket bind fail case Fix the issue of wrong SMACK label (SMACK64IPIN) update when a second bind call is made to same IP address & port, but with different SMACK label (SMACK64IPIN) by second instance of server. In this case server returns with "Bind:Address already in use" error but before returning, SMACK label is updated in SMACK port-label mapping list inside smack_socket_bind() hook To fix this issue a new check has been added in smk_ipv6_port_label() function before updating the existing port entry. It checks whether the socket for matching port entry is closed or not. If it is closed then it means port is not bound and it is safe to update the existing port entry else return if port is still getting used. For checking whether socket is closed or not, one more field "smk_can_reuse" has been added in the "smk_port_label" structure. This field will be set to '1' in "smack_sk_free_security()" function which is called to free the socket security blob when the socket is being closed. In this function, port entry is searched in the SMACK port-label mapping list for the closing socket. If entry is found then "smk_can_reuse" field is set to '1'.Initially "smk_can_reuse" field is set to '0' in smk_ipv6_port_label() function after creating a new entry in the list which indicates that socket is in use. Signed-off-by: Vishal Goel Signed-off-by: Himanshu Shukla Signed-off-by: Casey Schaufler --- security/smack/smack.h | 1 + security/smack/smack_lsm.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack.h b/security/smack/smack.h index 73480ee07478..2fac3b5bf44a 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -174,6 +174,7 @@ struct smk_port_label { struct smack_known *smk_in; /* inbound label */ struct smack_known *smk_out; /* outgoing label */ short smk_sock_type; /* Socket type */ + short smk_can_reuse; }; #endif /* SMACK_IPV6_PORT_LABELING */ diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 5e4d2bdb38cb..ed6885bccec4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -2354,6 +2354,20 @@ static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags) */ static void smack_sk_free_security(struct sock *sk) { +#ifdef SMACK_IPV6_PORT_LABELING + struct smk_port_label *spp; + + if (sk->sk_family == PF_INET6) { + rcu_read_lock(); + list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { + if (spp->smk_sock != sk) + continue; + spp->smk_can_reuse = 1; + break; + } + rcu_read_unlock(); + } +#endif kfree(sk->sk_security); } @@ -2637,10 +2651,15 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) list_for_each_entry_rcu(spp, &smk_ipv6_port_list, list) { if (spp->smk_port != port || spp->smk_sock_type != sock->type) continue; + if (spp->smk_can_reuse != 1) { + rcu_read_unlock(); + return; + } spp->smk_port = port; spp->smk_sock = sk; spp->smk_in = ssp->smk_in; spp->smk_out = ssp->smk_out; + spp->smk_can_reuse = 0; rcu_read_unlock(); return; } @@ -2657,6 +2676,7 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address) spp->smk_in = ssp->smk_in; spp->smk_out = ssp->smk_out; spp->smk_sock_type = sock->type; + spp->smk_can_reuse = 0; mutex_lock(&smack_ipv6_lock); list_add_rcu(&spp->list, &smk_ipv6_port_list); -- cgit v1.2.3 From d54a197964e7eb636a0c64fb0dbdd67759eb71f2 Mon Sep 17 00:00:00 2001 From: Himanshu Shukla Date: Wed, 23 Nov 2016 11:58:48 +0530 Subject: SMACK: Delete list_head repeated initialization smk_copy_rules() and smk_copy_relabel() are initializing list_head though they have been initialized already in new_task_smack() function. Delete repeated initialization. Signed-off-by: Himanshu Shukla Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 4 ---- 1 file changed, 4 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index ed6885bccec4..1368f8925343 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -348,8 +348,6 @@ static int smk_copy_rules(struct list_head *nhead, struct list_head *ohead, struct smack_rule *orp; int rc = 0; - INIT_LIST_HEAD(nhead); - list_for_each_entry_rcu(orp, ohead, list) { nrp = kzalloc(sizeof(struct smack_rule), gfp); if (nrp == NULL) { @@ -376,8 +374,6 @@ static int smk_copy_relabel(struct list_head *nhead, struct list_head *ohead, struct smack_known_list_elem *nklep; struct smack_known_list_elem *oklep; - INIT_LIST_HEAD(nhead); - list_for_each_entry(oklep, ohead, list) { nklep = kzalloc(sizeof(struct smack_known_list_elem), gfp); if (nklep == NULL) { -- cgit v1.2.3 From 3d4f673a6988f57e6f6ccd1a3b79eee171545e08 Mon Sep 17 00:00:00 2001 From: Himanshu Shukla Date: Wed, 23 Nov 2016 11:59:19 +0530 Subject: SMACK: Free the i_security blob in inode using RCU There is race condition issue while freeing the i_security blob in SMACK module. There is existing condition where i_security can be freed while inode_permission is called from path lookup on second CPU. There has been observed the page fault with such condition. VFS code and Selinux module takes care of this condition by freeing the inode and i_security field using RCU via call_rcu(). But in SMACK directly the i_secuirty blob is being freed. Use call_rcu() to fix this race condition issue. Signed-off-by: Himanshu Shukla Signed-off-by: Vishal Goel Signed-off-by: Casey Schaufler --- security/smack/smack.h | 1 + security/smack/smack_lsm.c | 32 ++++++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack.h b/security/smack/smack.h index 2fac3b5bf44a..612b810fbbc6 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -114,6 +114,7 @@ struct inode_smack { struct smack_known *smk_mmap; /* label of the mmap domain */ struct mutex smk_lock; /* initialization lock */ int smk_flags; /* smack inode flags */ + struct rcu_head smk_rcu; /* for freeing inode_smack */ }; struct task_smack { diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 1368f8925343..5deda8e0fe96 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1006,15 +1006,39 @@ static int smack_inode_alloc_security(struct inode *inode) } /** - * smack_inode_free_security - free an inode blob + * smack_inode_free_rcu - Free inode_smack blob from cache + * @head: the rcu_head for getting inode_smack pointer + * + * Call back function called from call_rcu() to free + * the i_security blob pointer in inode + */ +static void smack_inode_free_rcu(struct rcu_head *head) +{ + struct inode_smack *issp; + + issp = container_of(head, struct inode_smack, smk_rcu); + kmem_cache_free(smack_inode_cache, issp); +} + +/** + * smack_inode_free_security - free an inode blob using call_rcu() * @inode: the inode with a blob * - * Clears the blob pointer in inode + * Clears the blob pointer in inode using RCU */ static void smack_inode_free_security(struct inode *inode) { - kmem_cache_free(smack_inode_cache, inode->i_security); - inode->i_security = NULL; + struct inode_smack *issp = inode->i_security; + + /* + * The inode may still be referenced in a path walk and + * a call to smack_inode_permission() can be made + * after smack_inode_free_security() is called. + * To avoid race condition free the i_security via RCU + * and leave the current inode->i_security pointer intact. + * The inode will be freed after the RCU grace period too. + */ + call_rcu(&issp->smk_rcu, smack_inode_free_rcu); } /** -- cgit v1.2.3 From 348dc288d4bf4c0272a46a80f97748f36916601b Mon Sep 17 00:00:00 2001 From: Vishal Goel Date: Wed, 23 Nov 2016 10:45:31 +0530 Subject: Smack: Traverse the smack_known_list using list_for_each_entry_rcu macro In smack_from_secattr function,"smack_known_list" is being traversed using list_for_each_entry macro, although it is a rcu protected structure. So it should be traversed using "list_for_each_entry_rcu" macro to fetch the rcu protected entry. Signed-off-by: Vishal Goel Signed-off-by: Himanshu Shukla Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 5deda8e0fe96..4dd458a2b1e8 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3900,7 +3900,7 @@ static struct smack_known *smack_from_secattr(struct netlbl_lsm_secattr *sap, * ambient value. */ rcu_read_lock(); - list_for_each_entry(skp, &smack_known_list, list) { + list_for_each_entry_rcu(skp, &smack_known_list, list) { if (sap->attr.mls.lvl != skp->smk_netlabel.attr.mls.lvl) continue; /* -- cgit v1.2.3 From c9d238a18baa92600ba015d6d6c2cde53f55c572 Mon Sep 17 00:00:00 2001 From: Himanshu Shukla Date: Wed, 23 Nov 2016 11:59:45 +0530 Subject: SMACK: Use smk_tskacc() instead of smk_access() for proper logging smack_file_open() is first checking the capability of calling subject, this check will skip the SMACK logging for success case. Use smk_tskacc() for proper logging and SMACK access check. Signed-off-by: Himanshu Shukla Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 4dd458a2b1e8..681583d66c0e 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1955,12 +1955,9 @@ static int smack_file_open(struct file *file, const struct cred *cred) struct smk_audit_info ad; int rc; - if (smack_privileged(CAP_MAC_OVERRIDE)) - return 0; - smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); smk_ad_setfield_u_fs_path(&ad, file->f_path); - rc = smk_access(tsp->smk_task, smk_of_inode(inode), MAY_READ, &ad); + rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad); rc = smk_bu_credfile(cred, file, MAY_READ, rc); return rc; -- cgit v1.2.3 From 805b65a80bed029572c6462cc4be0a260e1482e9 Mon Sep 17 00:00:00 2001 From: Rafal Krypa Date: Fri, 9 Dec 2016 14:03:04 +0100 Subject: Smack: fix d_instantiate logic for sockfs and pipefs Since 4b936885a (v2.6.32) all inodes on sockfs and pipefs are disconnected. It caused filesystem specific code in smack_d_instantiate to be skipped, because all inodes on those pseudo filesystems were treated as root inodes. As a result all sockfs inodes had the Smack label set to floor. In most cases access checks for sockets use socket_smack data so the inode label is not important. But there are special cases that were broken. One example would be calling fcntl with F_SETOWN command on a socket fd. Now smack_d_instantiate expects all pipefs and sockfs inodes to be disconnected and has the logic in appropriate place. Signed-off-by: Rafal Krypa Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 681583d66c0e..225c4ad56444 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3486,6 +3486,13 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) case PIPEFS_MAGIC: isp->smk_inode = smk_of_current(); break; + case SOCKFS_MAGIC: + /* + * Socket access is controlled by the socket + * structures associated with the task involved. + */ + isp->smk_inode = &smack_known_star; + break; default: isp->smk_inode = sbsp->smk_root; break; @@ -3502,19 +3509,12 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) */ switch (sbp->s_magic) { case SMACK_MAGIC: - case PIPEFS_MAGIC: - case SOCKFS_MAGIC: case CGROUP_SUPER_MAGIC: /* * Casey says that it's a little embarrassing * that the smack file system doesn't do * extended attributes. * - * Casey says pipes are easy (?) - * - * Socket access is controlled by the socket - * structures associated with the task involved. - * * Cgroupfs is special */ final = &smack_known_star; -- cgit v1.2.3 From 83a1e53f392075e291a90746241dce45c6f9429a Mon Sep 17 00:00:00 2001 From: Seung-Woo Kim Date: Mon, 12 Dec 2016 17:35:26 +0900 Subject: Smack: ignore private inode for file functions The access to fd from anon_inode is always failed because there is no set xattr operations. So this patch fixes to ignore private inode including anon_inode for file functions. It was only ignored for smack_file_receive() to share dma-buf fd, but dma-buf has other functions like ioctl and mmap. Reference: https://lkml.org/lkml/2015/4/17/16 Signed-off-by: Seung-Woo Kim Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'security/smack/smack_lsm.c') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 225c4ad56444..679455350faf 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1647,6 +1647,9 @@ static int smack_file_ioctl(struct file *file, unsigned int cmd, struct smk_audit_info ad; struct inode *inode = file_inode(file); + if (unlikely(IS_PRIVATE(inode))) + return 0; + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); smk_ad_setfield_u_fs_path(&ad, file->f_path); @@ -1676,6 +1679,9 @@ static int smack_file_lock(struct file *file, unsigned int cmd) int rc; struct inode *inode = file_inode(file); + if (unlikely(IS_PRIVATE(inode))) + return 0; + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH); smk_ad_setfield_u_fs_path(&ad, file->f_path); rc = smk_curacc(smk_of_inode(inode), MAY_LOCK, &ad); @@ -1702,6 +1708,9 @@ static int smack_file_fcntl(struct file *file, unsigned int cmd, int rc = 0; struct inode *inode = file_inode(file); + if (unlikely(IS_PRIVATE(inode))) + return 0; + switch (cmd) { case F_GETLK: break; @@ -1755,6 +1764,9 @@ static int smack_mmap_file(struct file *file, if (file == NULL) return 0; + if (unlikely(IS_PRIVATE(file_inode(file)))) + return 0; + isp = file_inode(file)->i_security; if (isp->smk_mmap == NULL) return 0; -- cgit v1.2.3 From 3a2f5a59a695a73e0cde9a61e0feae5fa730e936 Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Tue, 10 Jan 2017 12:28:32 -0500 Subject: security,selinux,smack: kill security_task_wait hook As reported by yangshukui, a permission denial from security_task_wait() can lead to a soft lockup in zap_pid_ns_processes() since it only expects sys_wait4() to return 0 or -ECHILD. Further, security_task_wait() can in general lead to zombies; in the absence of some way to automatically reparent a child process upon a denial, the hook is not useful. Remove the security hook and its implementations in SELinux and Smack. Smack already removed its check from its hook. Reported-by: yangshukui Signed-off-by: Stephen Smalley Acked-by: Casey Schaufler Acked-by: Oleg Nesterov Signed-off-by: Paul Moore --- include/linux/lsm_hooks.h | 7 ------- include/linux/security.h | 6 ------ kernel/exit.c | 19 ++----------------- security/security.c | 6 ------ security/selinux/hooks.c | 7 ------- security/smack/smack_lsm.c | 20 -------------------- 6 files changed, 2 insertions(+), 63 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 0dde95900196..6fe7a5cb0be1 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -666,11 +666,6 @@ * @sig contains the signal value. * @secid contains the sid of the process where the signal originated * Return 0 if permission is granted. - * @task_wait: - * Check permission before allowing a process to reap a child process @p - * and collect its status information. - * @p contains the task_struct for process. - * Return 0 if permission is granted. * @task_prctl: * Check permission before performing a process control operation on the * current process. @@ -1507,7 +1502,6 @@ union security_list_options { int (*task_movememory)(struct task_struct *p); int (*task_kill)(struct task_struct *p, struct siginfo *info, int sig, u32 secid); - int (*task_wait)(struct task_struct *p); int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5); void (*task_to_inode)(struct task_struct *p, struct inode *inode); @@ -1767,7 +1761,6 @@ struct security_hook_heads { struct list_head task_getscheduler; struct list_head task_movememory; struct list_head task_kill; - struct list_head task_wait; struct list_head task_prctl; struct list_head task_to_inode; struct list_head ipc_permission; diff --git a/include/linux/security.h b/include/linux/security.h index f4ebac117fa6..d3868f2ebada 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -332,7 +332,6 @@ int security_task_getscheduler(struct task_struct *p); int security_task_movememory(struct task_struct *p); int security_task_kill(struct task_struct *p, struct siginfo *info, int sig, u32 secid); -int security_task_wait(struct task_struct *p); int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5); void security_task_to_inode(struct task_struct *p, struct inode *inode); @@ -980,11 +979,6 @@ static inline int security_task_kill(struct task_struct *p, return 0; } -static inline int security_task_wait(struct task_struct *p) -{ - return 0; -} - static inline int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, diff --git a/kernel/exit.c b/kernel/exit.c index 8f14b866f9f6..60f245190571 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -14,7 +14,6 @@ #include #include #include -#include #include #include #include @@ -1360,7 +1359,7 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p) * Returns nonzero for a final return, when we have unlocked tasklist_lock. * Returns zero if the search for a child should continue; * then ->notask_error is 0 if @p is an eligible child, - * or another error from security_task_wait(), or still -ECHILD. + * or still -ECHILD. */ static int wait_consider_task(struct wait_opts *wo, int ptrace, struct task_struct *p) @@ -1380,20 +1379,6 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace, if (!ret) return ret; - ret = security_task_wait(p); - if (unlikely(ret < 0)) { - /* - * If we have not yet seen any eligible child, - * then let this error code replace -ECHILD. - * A permission error will give the user a clue - * to look for security policy problems, rather - * than for mysterious wait bugs. - */ - if (wo->notask_error) - wo->notask_error = ret; - return 0; - } - if (unlikely(exit_state == EXIT_TRACE)) { /* * ptrace == 0 means we are the natural parent. In this case @@ -1486,7 +1471,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace, * Returns nonzero for a final return, when we have unlocked tasklist_lock. * Returns zero if the search for a child should continue; then * ->notask_error is 0 if there were any eligible children, - * or another error from security_task_wait(), or still -ECHILD. + * or still -ECHILD. */ static int do_wait_thread(struct wait_opts *wo, struct task_struct *tsk) { diff --git a/security/security.c b/security/security.c index 32052f5c76b2..8c9fee59e60a 100644 --- a/security/security.c +++ b/security/security.c @@ -1025,11 +1025,6 @@ int security_task_kill(struct task_struct *p, struct siginfo *info, return call_int_hook(task_kill, 0, p, info, sig, secid); } -int security_task_wait(struct task_struct *p) -{ - return call_int_hook(task_wait, 0, p); -} - int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5) { @@ -1769,7 +1764,6 @@ struct security_hook_heads security_hook_heads = { .task_movememory = LIST_HEAD_INIT(security_hook_heads.task_movememory), .task_kill = LIST_HEAD_INIT(security_hook_heads.task_kill), - .task_wait = LIST_HEAD_INIT(security_hook_heads.task_wait), .task_prctl = LIST_HEAD_INIT(security_hook_heads.task_prctl), .task_to_inode = LIST_HEAD_INIT(security_hook_heads.task_to_inode), diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 55ad878f1146..a5398fea0966 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3963,12 +3963,6 @@ static int selinux_task_kill(struct task_struct *p, struct siginfo *info, return avc_has_perm(secid, task_sid(p), SECCLASS_PROCESS, perm, NULL); } -static int selinux_task_wait(struct task_struct *p) -{ - return avc_has_perm(task_sid(p), current_sid(), SECCLASS_PROCESS, - PROCESS__SIGCHLD, NULL); -} - static void selinux_task_to_inode(struct task_struct *p, struct inode *inode) { @@ -6211,7 +6205,6 @@ static struct security_hook_list selinux_hooks[] = { LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler), LSM_HOOK_INIT(task_movememory, selinux_task_movememory), LSM_HOOK_INIT(task_kill, selinux_task_kill), - LSM_HOOK_INIT(task_wait, selinux_task_wait), LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode), LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission), diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 8da4a6b9ca4d..2166373ea5a4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -2271,25 +2271,6 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info, return rc; } -/** - * smack_task_wait - Smack access check for waiting - * @p: task to wait for - * - * Returns 0 - */ -static int smack_task_wait(struct task_struct *p) -{ - /* - * Allow the operation to succeed. - * Zombies are bad. - * In userless environments (e.g. phones) programs - * get marked with SMACK64EXEC and even if the parent - * and child shouldn't be talking the parent still - * may expect to know when the child exits. - */ - return 0; -} - /** * smack_task_to_inode - copy task smack into the inode blob * @p: task to copy from @@ -4658,7 +4639,6 @@ static struct security_hook_list smack_hooks[] = { LSM_HOOK_INIT(task_getscheduler, smack_task_getscheduler), LSM_HOOK_INIT(task_movememory, smack_task_movememory), LSM_HOOK_INIT(task_kill, smack_task_kill), - LSM_HOOK_INIT(task_wait, smack_task_wait), LSM_HOOK_INIT(task_to_inode, smack_task_to_inode), LSM_HOOK_INIT(ipc_permission, smack_ipc_permission), -- cgit v1.2.3 From d69dece5f5b6bc7a5e39d2b6136ddc69469331fe Mon Sep 17 00:00:00 2001 From: Casey Schaufler Date: Wed, 18 Jan 2017 17:09:05 -0800 Subject: LSM: Add /sys/kernel/security/lsm I am still tired of having to find indirect ways to determine what security modules are active on a system. I have added /sys/kernel/security/lsm, which contains a comma separated list of the active security modules. No more groping around in /proc/filesystems or other clever hacks. Unchanged from previous versions except for being updated to the latest security next branch. Signed-off-by: Casey Schaufler Acked-by: John Johansen Acked-by: Paul Moore Acked-by: Kees Cook Signed-off-by: James Morris --- Documentation/security/LSM.txt | 7 +++++++ include/linux/lsm_hooks.h | 12 ++++-------- security/apparmor/lsm.c | 3 ++- security/commoncap.c | 3 ++- security/inode.c | 26 ++++++++++++++++++++++++-- security/loadpin/loadpin.c | 2 +- security/security.c | 38 ++++++++++++++++++++++++++++++++++++++ security/selinux/hooks.c | 2 +- security/smack/smack_lsm.c | 2 +- security/tomoyo/tomoyo.c | 2 +- security/yama/yama_lsm.c | 2 +- 11 files changed, 82 insertions(+), 17 deletions(-) (limited to 'security/smack/smack_lsm.c') diff --git a/Documentation/security/LSM.txt b/Documentation/security/LSM.txt index 3db7e671c440..c2683f28ed36 100644 --- a/Documentation/security/LSM.txt +++ b/Documentation/security/LSM.txt @@ -22,6 +22,13 @@ system, building their checks on top of the defined capability hooks. For more details on capabilities, see capabilities(7) in the Linux man-pages project. +A list of the active security modules can be found by reading +/sys/kernel/security/lsm. This is a comma separated list, and +will always include the capability module. The list reflects the +order in which checks are made. The capability module will always +be first, followed by any "minor" modules (e.g. Yama) and then +the one "major" module (e.g. SELinux) if there is one configured. + Based on https://lkml.org/lkml/2007/10/26/215, a new LSM is accepted into the kernel when its intent (a description of what it tries to protect against and in what cases one would expect to diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 9cf50ad2fe20..0f3c309065d7 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1875,6 +1875,7 @@ struct security_hook_list { struct list_head list; struct list_head *head; union security_list_options hook; + char *lsm; }; /* @@ -1887,15 +1888,10 @@ struct security_hook_list { { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } } extern struct security_hook_heads security_hook_heads; +extern char *lsm_names; -static inline void security_add_hooks(struct security_hook_list *hooks, - int count) -{ - int i; - - for (i = 0; i < count; i++) - list_add_tail_rcu(&hooks[i].list, hooks[i].head); -} +extern void security_add_hooks(struct security_hook_list *hooks, int count, + char *lsm); #ifdef CONFIG_SECURITY_SELINUX_DISABLE /* diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 5217a0a54047..b63d39ca6278 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -999,7 +999,8 @@ static int __init apparmor_init(void) aa_free_root_ns(); goto buffers_out; } - security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks)); + security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks), + "apparmor"); /* Report that AppArmor successfully initialized */ apparmor_initialized = 1; diff --git a/security/commoncap.c b/security/commoncap.c index 8df676fbd393..6d4d586b9356 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -1093,7 +1093,8 @@ struct security_hook_list capability_hooks[] = { void __init capability_add_hooks(void) { - security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks)); + security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks), + "capability"); } #endif /* CONFIG_SECURITY */ diff --git a/security/inode.c b/security/inode.c index c83db05c15ab..2cb14162ff8d 100644 --- a/security/inode.c +++ b/security/inode.c @@ -20,6 +20,7 @@ #include #include #include +#include #include static struct vfsmount *mount; @@ -204,6 +205,21 @@ void securityfs_remove(struct dentry *dentry) } EXPORT_SYMBOL_GPL(securityfs_remove); +#ifdef CONFIG_SECURITY +static struct dentry *lsm_dentry; +static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count, + loff_t *ppos) +{ + return simple_read_from_buffer(buf, count, ppos, lsm_names, + strlen(lsm_names)); +} + +static const struct file_operations lsm_ops = { + .read = lsm_read, + .llseek = generic_file_llseek, +}; +#endif + static int __init securityfs_init(void) { int retval; @@ -213,9 +229,15 @@ static int __init securityfs_init(void) return retval; retval = register_filesystem(&fs_type); - if (retval) + if (retval) { sysfs_remove_mount_point(kernel_kobj, "security"); - return retval; + return retval; + } +#ifdef CONFIG_SECURITY + lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL, + &lsm_ops); +#endif + return 0; } core_initcall(securityfs_init); diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index 89a46f10b8a7..1d82eae3a5b8 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -182,7 +182,7 @@ static struct security_hook_list loadpin_hooks[] = { void __init loadpin_add_hooks(void) { pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis"); - security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks)); + security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin"); } /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */ diff --git a/security/security.c b/security/security.c index f825304f04a7..f0a802ee29b6 100644 --- a/security/security.c +++ b/security/security.c @@ -32,6 +32,7 @@ /* Maximum number of letters for an LSM name string */ #define SECURITY_NAME_MAX 10 +char *lsm_names; /* Boot-time LSM user choice */ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] = CONFIG_DEFAULT_SECURITY; @@ -78,6 +79,22 @@ static int __init choose_lsm(char *str) } __setup("security=", choose_lsm); +static int lsm_append(char *new, char **result) +{ + char *cp; + + if (*result == NULL) { + *result = kstrdup(new, GFP_KERNEL); + } else { + cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new); + if (cp == NULL) + return -ENOMEM; + kfree(*result); + *result = cp; + } + return 0; +} + /** * security_module_enable - Load given security module on boot ? * @module: the name of the module @@ -97,6 +114,27 @@ int __init security_module_enable(const char *module) return !strcmp(module, chosen_lsm); } +/** + * security_add_hooks - Add a modules hooks to the hook lists. + * @hooks: the hooks to add + * @count: the number of hooks to add + * @lsm: the name of the security module + * + * Each LSM has to register its hooks with the infrastructure. + */ +void __init security_add_hooks(struct security_hook_list *hooks, int count, + char *lsm) +{ + int i; + + for (i = 0; i < count; i++) { + hooks[i].lsm = lsm; + list_add_tail_rcu(&hooks[i].list, hooks[i].head); + } + if (lsm_append(lsm, &lsm_names) < 0) + panic("%s - Cannot get early memory.\n", __func__); +} + /* * Hook list operation macros. * diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index c7c6619431d5..fdc24ea65fd0 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6349,7 +6349,7 @@ static __init int selinux_init(void) 0, SLAB_PANIC, NULL); avc_init(); - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks)); + security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux"); if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET)) panic("SELinux: Unable to register AVC netcache callback\n"); diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 94dc9d406ce3..839150e40eb1 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4819,7 +4819,7 @@ static __init int smack_init(void) /* * Register with LSM */ - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks)); + security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack"); return 0; } diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 75c998700190..edc52d620f29 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -542,7 +542,7 @@ static int __init tomoyo_init(void) if (!security_module_enable("tomoyo")) return 0; /* register ourselves with the security framework */ - security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks)); + security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo"); printk(KERN_INFO "TOMOYO Linux initialized\n"); cred->security = &tomoyo_kernel_domain; tomoyo_mm_init(); diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c index 968e5e0a3f81..88271a3bf37f 100644 --- a/security/yama/yama_lsm.c +++ b/security/yama/yama_lsm.c @@ -485,6 +485,6 @@ static inline void yama_init_sysctl(void) { } void __init yama_add_hooks(void) { pr_info("Yama: becoming mindful.\n"); - security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks)); + security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama"); yama_init_sysctl(); } -- cgit v1.2.3