From 0bbf87d852d243680ed7074110ccc1dea003b61a Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sat, 28 Sep 2013 14:10:59 -0700 Subject: net ipv4: Convert ipv4.ip_local_port_range to be per netns v3 - Move sysctl_local_ports from a global variable into struct netns_ipv4. - Modify inet_get_local_port_range to take a struct net, and update all of the callers. - Move the initialization of sysctl_local_ports into sysctl_net_ipv4.c:ipv4_sysctl_init_net from inet_connection_sock.c v2: - Ensure indentation used tabs - Fixed ip.h so it applies cleanly to todays net-next v3: - Compile fixes of strange callers of inet_get_local_port_range. This patch now successfully passes an allmodconfig build. Removed manual inlining of inet_get_local_port_range in ipv4_local_port_range Originally-by: Samya Acked-by: Nicolas Dichtel Signed-off-by: "Eric W. Biederman" Signed-off-by: David S. Miller --- security/selinux/hooks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a5091ec..568c769 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3929,7 +3929,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in if (snum) { int low, high; - inet_get_local_port_range(&low, &high); + inet_get_local_port_range(sock_net(sk), &low, &high); if (snum < max(PROT_SOCK, low) || snum > high) { err = sel_netport_sid(sk->sk_protocol, -- cgit v1.1 From 795aa6ef6a1aba99050735eadd0c2341b789b53b Mon Sep 17 00:00:00 2001 From: Patrick McHardy Date: Thu, 10 Oct 2013 09:21:55 +0200 Subject: netfilter: pass hook ops to hookfn Pass the hook ops to the hookfn to allow for generic hook functions. This change is required by nf_tables. Signed-off-by: Patrick McHardy Signed-off-by: Pablo Neira Ayuso --- security/selinux/hooks.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 568c769..3f224d7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4668,7 +4668,7 @@ static unsigned int selinux_ip_forward(struct sk_buff *skb, int ifindex, return NF_ACCEPT; } -static unsigned int selinux_ipv4_forward(unsigned int hooknum, +static unsigned int selinux_ipv4_forward(const struct nf_hook_ops *ops, struct sk_buff *skb, const struct net_device *in, const struct net_device *out, @@ -4678,7 +4678,7 @@ static unsigned int selinux_ipv4_forward(unsigned int hooknum, } #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) -static unsigned int selinux_ipv6_forward(unsigned int hooknum, +static unsigned int selinux_ipv6_forward(const struct nf_hook_ops *ops, struct sk_buff *skb, const struct net_device *in, const struct net_device *out, @@ -4710,7 +4710,7 @@ static unsigned int selinux_ip_output(struct sk_buff *skb, return NF_ACCEPT; } -static unsigned int selinux_ipv4_output(unsigned int hooknum, +static unsigned int selinux_ipv4_output(const struct nf_hook_ops *ops, struct sk_buff *skb, const struct net_device *in, const struct net_device *out, @@ -4837,7 +4837,7 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, return NF_ACCEPT; } -static unsigned int selinux_ipv4_postroute(unsigned int hooknum, +static unsigned int selinux_ipv4_postroute(const struct nf_hook_ops *ops, struct sk_buff *skb, const struct net_device *in, const struct net_device *out, @@ -4847,7 +4847,7 @@ static unsigned int selinux_ipv4_postroute(unsigned int hooknum, } #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) -static unsigned int selinux_ipv6_postroute(unsigned int hooknum, +static unsigned int selinux_ipv6_postroute(const struct nf_hook_ops *ops, struct sk_buff *skb, const struct net_device *in, const struct net_device *out, -- cgit v1.1 From b805b198dc74b73aabb6969a3db734c71c05c88c Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Fri, 24 May 2013 12:09:50 -0400 Subject: selinux: apply selinux checks on new audit message types We use the read check to get the feature set (like AUDIT_GET) and the write check to set the features (like AUDIT_SET). Signed-off-by: Eric Paris Signed-off-by: Richard Guy Briggs Signed-off-by: Eric Paris --- security/selinux/nlmsgtab.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'security/selinux') diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index 855e464..332ac8a 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -116,6 +116,8 @@ static struct nlmsg_perm nlmsg_audit_perms[] = { AUDIT_MAKE_EQUIV, NETLINK_AUDIT_SOCKET__NLMSG_WRITE }, { AUDIT_TTY_GET, NETLINK_AUDIT_SOCKET__NLMSG_READ }, { AUDIT_TTY_SET, NETLINK_AUDIT_SOCKET__NLMSG_TTY_AUDIT }, + { AUDIT_GET_FEATURE, NETLINK_AUDIT_SOCKET__NLMSG_READ }, + { AUDIT_SET_FEATURE, NETLINK_AUDIT_SOCKET__NLMSG_WRITE }, }; -- cgit v1.1 From 0af901643fe3f1f8d44e41115d36609ee4bda2bf Mon Sep 17 00:00:00 2001 From: "Geyslan G. Bem" Date: Wed, 4 Dec 2013 16:10:24 -0500 Subject: selinux: fix possible memory leak Free 'ctx_str' when necessary. Signed-off-by: Geyslan G. Bem Signed-off-by: Paul Moore --- security/selinux/xfrm.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index a91d205..cf79a45 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -327,19 +327,22 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x, return rc; ctx = kmalloc(sizeof(*ctx) + str_len, GFP_ATOMIC); - if (!ctx) - return -ENOMEM; + if (!ctx) { + rc = -ENOMEM; + goto out; + } ctx->ctx_doi = XFRM_SC_DOI_LSM; ctx->ctx_alg = XFRM_SC_ALG_SELINUX; ctx->ctx_sid = secid; ctx->ctx_len = str_len; memcpy(ctx->ctx_str, ctx_str, str_len); - kfree(ctx_str); x->security = ctx; atomic_inc(&selinux_xfrm_refcount); - return 0; +out: + kfree(ctx_str); + return rc; } /* -- cgit v1.1 From 47180068276a04ed31d24fe04c673138208b07a9 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 4 Dec 2013 16:10:45 -0500 Subject: selinux: handle TCP SYN-ACK packets correctly in selinux_ip_output() In selinux_ip_output() we always label packets based on the parent socket. While this approach works in almost all cases, it doesn't work in the case of TCP SYN-ACK packets when the correct label is not the label of the parent socket, but rather the label of the larval socket represented by the request_sock struct. Unfortunately, since the request_sock isn't queued on the parent socket until *after* the SYN-ACK packet is sent, we can't lookup the request_sock to determine the correct label for the packet; at this point in time the best we can do is simply pass/NF_ACCEPT the packet. It must be said that simply passing the packet without any explicit labeling action, while far from ideal, is not terrible as the SYN-ACK packet will inherit any IP option based labeling from the initial connection request so the label *should* be correct and all our access controls remain in place so we shouldn't have to worry about information leaks. Reported-by: Janak Desai Tested-by: Janak Desai Cc: stable@vger.kernel.org Signed-off-by: Paul Moore --- security/selinux/hooks.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 777ee98..877bab7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -53,6 +53,7 @@ #include /* for local_port_range[] */ #include #include /* struct or_callable used in sock_rcv_skb */ +#include #include #include #include @@ -4731,6 +4732,7 @@ static unsigned int selinux_ipv6_forward(unsigned int hooknum, static unsigned int selinux_ip_output(struct sk_buff *skb, u16 family) { + struct sock *sk; u32 sid; if (!netlbl_enabled()) @@ -4739,8 +4741,27 @@ static unsigned int selinux_ip_output(struct sk_buff *skb, /* we do this in the LOCAL_OUT path and not the POST_ROUTING path * because we want to make sure we apply the necessary labeling * before IPsec is applied so we can leverage AH protection */ - if (skb->sk) { - struct sk_security_struct *sksec = skb->sk->sk_security; + sk = skb->sk; + if (sk) { + struct sk_security_struct *sksec; + + if (sk->sk_state == TCP_LISTEN) + /* if the socket is the listening state then this + * packet is a SYN-ACK packet which means it needs to + * be labeled based on the connection/request_sock and + * not the parent socket. unfortunately, we can't + * lookup the request_sock yet as it isn't queued on + * the parent socket until after the SYN-ACK is sent. + * the "solution" is to simply pass the packet as-is + * as any IP option based labeling should be copied + * from the initial connection request (in the IP + * layer). it is far from ideal, but until we get a + * security label in the packet itself this is the + * best we can do. */ + return NF_ACCEPT; + + /* standard practice, label using the parent socket */ + sksec = sk->sk_security; sid = sksec->sid; } else sid = SECINITSID_KERNEL; -- cgit v1.1 From 446b802437f285de68ffb8d6fac3c44c3cab5b04 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 4 Dec 2013 16:10:51 -0500 Subject: selinux: handle TCP SYN-ACK packets correctly in selinux_ip_postroute() In selinux_ip_postroute() we perform access checks based on the packet's security label. For locally generated traffic we get the packet's security label from the associated socket; this works in all cases except for TCP SYN-ACK packets. In the case of SYN-ACK packet's the correct security label is stored in the connection's request_sock, not the server's socket. Unfortunately, at the point in time when selinux_ip_postroute() is called we can't query the request_sock directly, we need to recreate the label using the same logic that originally labeled the associated request_sock. See the inline comments for more explanation. Reported-by: Janak Desai Tested-by: Janak Desai Cc: stable@vger.kernel.org Signed-off-by: Paul Moore --- security/selinux/hooks.c | 68 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 15 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 877bab7..cc076a9 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3847,6 +3847,30 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb, u16 family, u32 *sid) return 0; } +/** + * selinux_conn_sid - Determine the child socket label for a connection + * @sk_sid: the parent socket's SID + * @skb_sid: the packet's SID + * @conn_sid: the resulting connection SID + * + * If @skb_sid is valid then the user:role:type information from @sk_sid is + * combined with the MLS information from @skb_sid in order to create + * @conn_sid. If @skb_sid is not valid then then @conn_sid is simply a copy + * of @sk_sid. Returns zero on success, negative values on failure. + * + */ +static int selinux_conn_sid(u32 sk_sid, u32 skb_sid, u32 *conn_sid) +{ + int err = 0; + + if (skb_sid != SECSID_NULL) + err = security_sid_mls_copy(sk_sid, skb_sid, conn_sid); + else + *conn_sid = sk_sid; + + return err; +} + /* socket security operations */ static int socket_sockcreate_sid(const struct task_security_struct *tsec, @@ -4453,7 +4477,7 @@ static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb, struct sk_security_struct *sksec = sk->sk_security; int err; u16 family = sk->sk_family; - u32 newsid; + u32 connsid; u32 peersid; /* handle mapped IPv4 packets arriving via IPv6 sockets */ @@ -4463,16 +4487,11 @@ static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb, err = selinux_skb_peerlbl_sid(skb, family, &peersid); if (err) return err; - if (peersid == SECSID_NULL) { - req->secid = sksec->sid; - req->peer_secid = SECSID_NULL; - } else { - err = security_sid_mls_copy(sksec->sid, peersid, &newsid); - if (err) - return err; - req->secid = newsid; - req->peer_secid = peersid; - } + err = selinux_conn_sid(sksec->sid, peersid, &connsid); + if (err) + return err; + req->secid = connsid; + req->peer_secid = peersid; return selinux_netlbl_inet_conn_request(req, family); } @@ -4846,12 +4865,12 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, if (!secmark_active && !peerlbl_active) return NF_ACCEPT; - /* if the packet is being forwarded then get the peer label from the - * packet itself; otherwise check to see if it is from a local - * application or the kernel, if from an application get the peer label - * from the sending socket, otherwise use the kernel's sid */ sk = skb->sk; if (sk == NULL) { + /* Without an associated socket the packet is either coming + * from the kernel or it is being forwarded; check the packet + * to determine which and if the packet is being forwarded + * query the packet directly to determine the security label. */ if (skb->skb_iif) { secmark_perm = PACKET__FORWARD_OUT; if (selinux_skb_peerlbl_sid(skb, family, &peer_sid)) @@ -4860,7 +4879,26 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, secmark_perm = PACKET__SEND; peer_sid = SECINITSID_KERNEL; } + } else if (sk->sk_state == TCP_LISTEN) { + /* Locally generated packet but the associated socket is in the + * listening state which means this is a SYN-ACK packet. In + * this particular case the correct security label is assigned + * to the connection/request_sock but unfortunately we can't + * query the request_sock as it isn't queued on the parent + * socket until after the SYN-ACK packet is sent; the only + * viable choice is to regenerate the label like we do in + * selinux_inet_conn_request(). See also selinux_ip_output() + * for similar problems. */ + u32 skb_sid; + struct sk_security_struct *sksec = sk->sk_security; + if (selinux_skb_peerlbl_sid(skb, family, &skb_sid)) + return NF_DROP; + if (selinux_conn_sid(sksec->sid, skb_sid, &peer_sid)) + return NF_DROP; + secmark_perm = PACKET__SEND; } else { + /* Locally generated packet, fetch the security label from the + * associated socket. */ struct sk_security_struct *sksec = sk->sk_security; peer_sid = sksec->sid; secmark_perm = PACKET__SEND; -- cgit v1.1 From 817eff718dca4e54d5721211ddde0914428fbb7c Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 10 Dec 2013 14:57:54 -0500 Subject: selinux: look for IPsec labels on both inbound and outbound packets Previously selinux_skb_peerlbl_sid() would only check for labeled IPsec security labels on inbound packets, this patch enables it to check both inbound and outbound traffic for labeled IPsec security labels. Reported-by: Janak Desai Cc: stable@vger.kernel.org Signed-off-by: Paul Moore --- security/selinux/hooks.c | 2 +- security/selinux/include/xfrm.h | 8 ++++--- security/selinux/xfrm.c | 51 +++++++++++++++++++++++++++++++++-------- 3 files changed, 47 insertions(+), 14 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index cc076a9..8b28123 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3829,7 +3829,7 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb, u16 family, u32 *sid) u32 nlbl_sid; u32 nlbl_type; - err = selinux_skb_xfrm_sid(skb, &xfrm_sid); + err = selinux_xfrm_skb_sid(skb, &xfrm_sid); if (unlikely(err)) return -EACCES; err = selinux_netlbl_skbuff_getsid(skb, family, &nlbl_type, &nlbl_sid); diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h index 0dec76c..48c3cc9 100644 --- a/security/selinux/include/xfrm.h +++ b/security/selinux/include/xfrm.h @@ -39,6 +39,7 @@ int selinux_xfrm_sock_rcv_skb(u32 sk_sid, struct sk_buff *skb, int selinux_xfrm_postroute_last(u32 sk_sid, struct sk_buff *skb, struct common_audit_data *ad, u8 proto); int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall); +int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid); static inline void selinux_xfrm_notify_policyload(void) { @@ -79,11 +80,12 @@ static inline int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, static inline void selinux_xfrm_notify_policyload(void) { } -#endif -static inline int selinux_skb_xfrm_sid(struct sk_buff *skb, u32 *sid) +static inline int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid) { - return selinux_xfrm_decode_session(skb, sid, 0); + *sid = SECSID_NULL; + return 0; } +#endif #endif /* _SELINUX_XFRM_H_ */ diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index cf79a45..0462cb3 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -209,19 +209,26 @@ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, NULL) ? 0 : 1); } -/* - * LSM hook implementation that checks and/or returns the xfrm sid for the - * incoming packet. - */ -int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall) +static u32 selinux_xfrm_skb_sid_egress(struct sk_buff *skb) { - u32 sid_session = SECSID_NULL; - struct sec_path *sp; + struct dst_entry *dst = skb_dst(skb); + struct xfrm_state *x; - if (skb == NULL) - goto out; + if (dst == NULL) + return SECSID_NULL; + x = dst->xfrm; + if (x == NULL || !selinux_authorizable_xfrm(x)) + return SECSID_NULL; + + return x->security->ctx_sid; +} + +static int selinux_xfrm_skb_sid_ingress(struct sk_buff *skb, + u32 *sid, int ckall) +{ + u32 sid_session = SECSID_NULL; + struct sec_path *sp = skb->sp; - sp = skb->sp; if (sp) { int i; @@ -248,6 +255,30 @@ out: } /* + * LSM hook implementation that checks and/or returns the xfrm sid for the + * incoming packet. + */ +int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall) +{ + if (skb == NULL) { + *sid = SECSID_NULL; + return 0; + } + return selinux_xfrm_skb_sid_ingress(skb, sid, ckall); +} + +int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid) +{ + int rc; + + rc = selinux_xfrm_skb_sid_ingress(skb, sid, 0); + if (rc == 0 && *sid == SECSID_NULL) + *sid = selinux_xfrm_skb_sid_egress(skb); + + return rc; +} + +/* * LSM hook implementation that allocs and transfers uctx spec to xfrm_policy. */ int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, -- cgit v1.1 From c0828e50485932b7e019df377a6b0a8d1ebd3080 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 10 Dec 2013 14:58:01 -0500 Subject: selinux: process labeled IPsec TCP SYN-ACK packets properly in selinux_ip_postroute() Due to difficulty in arriving at the proper security label for TCP SYN-ACK packets in selinux_ip_postroute(), we need to check packets while/before they are undergoing XFRM transforms instead of waiting until afterwards so that we can determine the correct security label. Reported-by: Janak Desai Cc: stable@vger.kernel.org Signed-off-by: Paul Moore --- security/selinux/hooks.c | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 8b28123..6db2e58 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4850,22 +4850,31 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, * as fast and as clean as possible. */ if (!selinux_policycap_netpeer) return selinux_ip_postroute_compat(skb, ifindex, family); + + secmark_active = selinux_secmark_enabled(); + peerlbl_active = selinux_peerlbl_enabled(); + if (!secmark_active && !peerlbl_active) + return NF_ACCEPT; + + sk = skb->sk; + #ifdef CONFIG_XFRM /* If skb->dst->xfrm is non-NULL then the packet is undergoing an IPsec * packet transformation so allow the packet to pass without any checks * since we'll have another chance to perform access control checks * when the packet is on it's final way out. * NOTE: there appear to be some IPv6 multicast cases where skb->dst - * is NULL, in this case go ahead and apply access control. */ - if (skb_dst(skb) != NULL && skb_dst(skb)->xfrm != NULL) + * is NULL, in this case go ahead and apply access control. + * NOTE: if this is a local socket (skb->sk != NULL) that is in the + * TCP listening state we cannot wait until the XFRM processing + * is done as we will miss out on the SA label if we do; + * unfortunately, this means more work, but it is only once per + * connection. */ + if (skb_dst(skb) != NULL && skb_dst(skb)->xfrm != NULL && + !(sk != NULL && sk->sk_state == TCP_LISTEN)) return NF_ACCEPT; #endif - secmark_active = selinux_secmark_enabled(); - peerlbl_active = selinux_peerlbl_enabled(); - if (!secmark_active && !peerlbl_active) - return NF_ACCEPT; - sk = skb->sk; if (sk == NULL) { /* Without an associated socket the packet is either coming * from the kernel or it is being forwarded; check the packet @@ -4893,6 +4902,25 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, struct sk_security_struct *sksec = sk->sk_security; if (selinux_skb_peerlbl_sid(skb, family, &skb_sid)) return NF_DROP; + /* At this point, if the returned skb peerlbl is SECSID_NULL + * and the packet has been through at least one XFRM + * transformation then we must be dealing with the "final" + * form of labeled IPsec packet; since we've already applied + * all of our access controls on this packet we can safely + * pass the packet. */ + if (skb_sid == SECSID_NULL) { + switch (family) { + case PF_INET: + if (IPCB(skb)->flags & IPSKB_XFRM_TRANSFORMED) + return NF_ACCEPT; + break; + case PF_INET6: + if (IP6CB(skb)->flags & IP6SKB_XFRM_TRANSFORMED) + return NF_ACCEPT; + default: + return NF_DROP_ERR(-ECONNREFUSED); + } + } if (selinux_conn_sid(sksec->sid, skb_sid, &peer_sid)) return NF_DROP; secmark_perm = PACKET__SEND; -- cgit v1.1 From 29b1deb2a48a9dd02b93597aa4c055a24c0e989f Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sun, 15 Dec 2013 11:17:45 -0800 Subject: Revert "selinux: consider filesystem subtype in policies" This reverts commit 102aefdda4d8275ce7d7100bc16c88c74272b260. Tom London reports that it causes sync() to hang on Fedora rawhide: https://bugzilla.redhat.com/show_bug.cgi?id=1033965 and Josh Boyer bisected it down to this commit. Reverting the commit in the rawhide kernel fixes the problem. Eric Paris root-caused it to incorrect subtype matching in that commit breaking fuse, and has a tentative patch, but by now we're better off retrying this in 3.14 rather than playing with it any more. Reported-by: Tom London Bisected-by: Josh Boyer Acked-by: Eric Paris Cc: James Morris Cc: Anand Avati Cc: Paul Moore Signed-off-by: Linus Torvalds --- security/selinux/hooks.c | 40 ++++++++++++++++++---------------------- security/selinux/ss/services.c | 42 ++++-------------------------------------- 2 files changed, 22 insertions(+), 60 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 794c3ca..98b1caa 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -95,10 +95,6 @@ #include "audit.h" #include "avc_ss.h" -#define SB_TYPE_FMT "%s%s%s" -#define SB_SUBTYPE(sb) (sb->s_subtype && sb->s_subtype[0]) -#define SB_TYPE_ARGS(sb) sb->s_type->name, SB_SUBTYPE(sb) ? "." : "", SB_SUBTYPE(sb) ? sb->s_subtype : "" - extern struct security_operations *security_ops; /* SECMARK reference count */ @@ -413,8 +409,8 @@ static int sb_finish_set_opts(struct super_block *sb) the first boot of the SELinux kernel before we have assigned xattr values to the filesystem. */ if (!root_inode->i_op->getxattr) { - printk(KERN_WARNING "SELinux: (dev %s, type "SB_TYPE_FMT") has no " - "xattr support\n", sb->s_id, SB_TYPE_ARGS(sb)); + printk(KERN_WARNING "SELinux: (dev %s, type %s) has no " + "xattr support\n", sb->s_id, sb->s_type->name); rc = -EOPNOTSUPP; goto out; } @@ -422,22 +418,22 @@ static int sb_finish_set_opts(struct super_block *sb) if (rc < 0 && rc != -ENODATA) { if (rc == -EOPNOTSUPP) printk(KERN_WARNING "SELinux: (dev %s, type " - SB_TYPE_FMT") has no security xattr handler\n", - sb->s_id, SB_TYPE_ARGS(sb)); + "%s) has no security xattr handler\n", + sb->s_id, sb->s_type->name); else printk(KERN_WARNING "SELinux: (dev %s, type " - SB_TYPE_FMT") getxattr errno %d\n", sb->s_id, - SB_TYPE_ARGS(sb), -rc); + "%s) getxattr errno %d\n", sb->s_id, + sb->s_type->name, -rc); goto out; } } if (sbsec->behavior > ARRAY_SIZE(labeling_behaviors)) - printk(KERN_ERR "SELinux: initialized (dev %s, type "SB_TYPE_FMT"), unknown behavior\n", - sb->s_id, SB_TYPE_ARGS(sb)); + printk(KERN_ERR "SELinux: initialized (dev %s, type %s), unknown behavior\n", + sb->s_id, sb->s_type->name); else - printk(KERN_DEBUG "SELinux: initialized (dev %s, type "SB_TYPE_FMT"), %s\n", - sb->s_id, SB_TYPE_ARGS(sb), + printk(KERN_DEBUG "SELinux: initialized (dev %s, type %s), %s\n", + sb->s_id, sb->s_type->name, labeling_behaviors[sbsec->behavior-1]); sbsec->flags |= SE_SBINITIALIZED; @@ -600,6 +596,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, const struct cred *cred = current_cred(); int rc = 0, i; struct superblock_security_struct *sbsec = sb->s_security; + const char *name = sb->s_type->name; struct inode *inode = sbsec->sb->s_root->d_inode; struct inode_security_struct *root_isec = inode->i_security; u32 fscontext_sid = 0, context_sid = 0, rootcontext_sid = 0; @@ -658,8 +655,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, strlen(mount_options[i]), &sid); if (rc) { printk(KERN_WARNING "SELinux: security_context_to_sid" - "(%s) failed for (dev %s, type "SB_TYPE_FMT") errno=%d\n", - mount_options[i], sb->s_id, SB_TYPE_ARGS(sb), rc); + "(%s) failed for (dev %s, type %s) errno=%d\n", + mount_options[i], sb->s_id, name, rc); goto out; } switch (flags[i]) { @@ -806,8 +803,7 @@ out: out_double_mount: rc = -EINVAL; printk(KERN_WARNING "SELinux: mount invalid. Same superblock, different " - "security settings for (dev %s, type "SB_TYPE_FMT")\n", sb->s_id, - SB_TYPE_ARGS(sb)); + "security settings for (dev %s, type %s)\n", sb->s_id, name); goto out; } @@ -2480,8 +2476,8 @@ static int selinux_sb_remount(struct super_block *sb, void *data) rc = security_context_to_sid(mount_options[i], len, &sid); if (rc) { printk(KERN_WARNING "SELinux: security_context_to_sid" - "(%s) failed for (dev %s, type "SB_TYPE_FMT") errno=%d\n", - mount_options[i], sb->s_id, SB_TYPE_ARGS(sb), rc); + "(%s) failed for (dev %s, type %s) errno=%d\n", + mount_options[i], sb->s_id, sb->s_type->name, rc); goto out_free_opts; } rc = -EINVAL; @@ -2519,8 +2515,8 @@ out_free_secdata: return rc; out_bad_option: printk(KERN_WARNING "SELinux: unable to change security options " - "during remount (dev %s, type "SB_TYPE_FMT")\n", sb->s_id, - SB_TYPE_ARGS(sb)); + "during remount (dev %s, type=%s)\n", sb->s_id, + sb->s_type->name); goto out_free_opts; } diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index ee470a0..d106733 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2334,50 +2334,16 @@ int security_fs_use(struct super_block *sb) struct ocontext *c; struct superblock_security_struct *sbsec = sb->s_security; const char *fstype = sb->s_type->name; - const char *subtype = (sb->s_subtype && sb->s_subtype[0]) ? sb->s_subtype : NULL; - struct ocontext *base = NULL; read_lock(&policy_rwlock); - for (c = policydb.ocontexts[OCON_FSUSE]; c; c = c->next) { - char *sub; - int baselen; - - baselen = strlen(fstype); - - /* if base does not match, this is not the one */ - if (strncmp(fstype, c->u.name, baselen)) - continue; - - /* if there is no subtype, this is the one! */ - if (!subtype) - break; - - /* skip past the base in this entry */ - sub = c->u.name + baselen; - - /* entry is only a base. save it. keep looking for subtype */ - if (sub[0] == '\0') { - base = c; - continue; - } - - /* entry is not followed by a subtype, so it is not a match */ - if (sub[0] != '.') - continue; - - /* whew, we found a subtype of this fstype */ - sub++; /* move past '.' */ - - /* exact match of fstype AND subtype */ - if (!strcmp(subtype, sub)) + c = policydb.ocontexts[OCON_FSUSE]; + while (c) { + if (strcmp(fstype, c->u.name) == 0) break; + c = c->next; } - /* in case we had found an fstype match but no subtype match */ - if (!c) - c = base; - if (c) { sbsec->behavior = c->v.behavior; if (!c->sid[0]) { -- cgit v1.1 From 46d01d63221c3508421dd72ff9c879f61053cffc Mon Sep 17 00:00:00 2001 From: Chad Hanson Date: Mon, 23 Dec 2013 17:45:01 -0500 Subject: selinux: fix broken peer recv check Fix a broken networking check. Return an error if peer recv fails. If secmark is active and the packet recv succeeds the peer recv error is ignored. Signed-off-by: Chad Hanson Cc: stable@vger.kernel.org Signed-off-by: Paul Moore --- security/selinux/hooks.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 419491d..5db2646 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4334,8 +4334,10 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) } err = avc_has_perm(sk_sid, peer_sid, SECCLASS_PEER, PEER__RECV, &ad); - if (err) + if (err) { selinux_netlbl_err(skb, err, 0); + return err; + } } if (secmark_active) { -- cgit v1.1 From c0c1439541f5305b57a83d599af32b74182933fe Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 23 Dec 2013 17:45:01 -0500 Subject: selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock() selinux_setprocattr() does ptrace_parent(p) under task_lock(p), but task_struct->alloc_lock doesn't pin ->parent or ->ptrace, this looks confusing and triggers the "suspicious RCU usage" warning because ptrace_parent() does rcu_dereference_check(). And in theory this is wrong, spin_lock()->preempt_disable() doesn't necessarily imply rcu_read_lock() we need to access the ->parent. Reported-by: Evan McNabb Signed-off-by: Oleg Nesterov Cc: stable@vger.kernel.org Signed-off-by: Paul Moore --- security/selinux/hooks.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 5db2646..6625699 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5588,11 +5588,11 @@ static int selinux_setprocattr(struct task_struct *p, /* Check for ptracing, and update the task SID if ok. Otherwise, leave SID unchanged and fail. */ ptsid = 0; - task_lock(p); + rcu_read_lock(); tracer = ptrace_parent(p); if (tracer) ptsid = task_sid(tracer); - task_unlock(p); + rcu_read_unlock(); if (tracer) { error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS, -- cgit v1.1 From 3dc91d4338d698ce77832985f9cb183d8eeaf6be Mon Sep 17 00:00:00 2001 From: Steven Rostedt Date: Thu, 9 Jan 2014 21:46:34 -0500 Subject: SELinux: Fix possible NULL pointer dereference in selinux_inode_permission() While running stress tests on adding and deleting ftrace instances I hit this bug: BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 IP: selinux_inode_permission+0x85/0x160 PGD 63681067 PUD 7ddbe067 PMD 0 Oops: 0000 [#1] PREEMPT CPU: 0 PID: 5634 Comm: ftrace-test-mki Not tainted 3.13.0-rc4-test-00033-gd2a6dde-dirty #20 Hardware name: /DG965MQ, BIOS MQ96510J.86A.0372.2006.0605.1717 06/05/2006 task: ffff880078375800 ti: ffff88007ddb0000 task.ti: ffff88007ddb0000 RIP: 0010:[] [] selinux_inode_permission+0x85/0x160 RSP: 0018:ffff88007ddb1c48 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000800000 RCX: ffff88006dd43840 RDX: 0000000000000001 RSI: 0000000000000081 RDI: ffff88006ee46000 RBP: ffff88007ddb1c88 R08: 0000000000000000 R09: ffff88007ddb1c54 R10: 6e6576652f6f6f66 R11: 0000000000000003 R12: 0000000000000000 R13: 0000000000000081 R14: ffff88006ee46000 R15: 0000000000000000 FS: 00007f217b5b6700(0000) GS:ffffffff81e21000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033^M CR2: 0000000000000020 CR3: 000000006a0fe000 CR4: 00000000000007f0 Call Trace: security_inode_permission+0x1c/0x30 __inode_permission+0x41/0xa0 inode_permission+0x18/0x50 link_path_walk+0x66/0x920 path_openat+0xa6/0x6c0 do_filp_open+0x43/0xa0 do_sys_open+0x146/0x240 SyS_open+0x1e/0x20 system_call_fastpath+0x16/0x1b Code: 84 a1 00 00 00 81 e3 00 20 00 00 89 d8 83 c8 02 40 f6 c6 04 0f 45 d8 40 f6 c6 08 74 71 80 cf 02 49 8b 46 38 4c 8d 4d cc 45 31 c0 <0f> b7 50 20 8b 70 1c 48 8b 41 70 89 d9 8b 78 04 e8 36 cf ff ff RIP selinux_inode_permission+0x85/0x160 CR2: 0000000000000020 Investigating, I found that the inode->i_security was NULL, and the dereference of it caused the oops. in selinux_inode_permission(): isec = inode->i_security; rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd); Note, the crash came from stressing the deletion and reading of debugfs files. I was not able to recreate this via normal files. But I'm not sure they are safe. It may just be that the race window is much harder to hit. What seems to have happened (and what I have traced), is the file is being opened at the same time the file or directory is being deleted. As the dentry and inode locks are not held during the path walk, nor is the inodes ref counts being incremented, there is nothing saving these structures from being discarded except for an rcu_read_lock(). The rcu_read_lock() protects against freeing of the inode, but it does not protect freeing of the inode_security_struct. Now if the freeing of the i_security happens with a call_rcu(), and the i_security field of the inode is not changed (it gets freed as the inode gets freed) then there will be no issue here. (Linus Torvalds suggested not setting the field to NULL such that we do not need to check if it is NULL in the permission check). Note, this is a hack, but it fixes the problem at hand. A real fix is to restructure the destroy_inode() to call all the destructor handlers from the RCU callback. But that is a major job to do, and requires a lot of work. For now, we just band-aid this bug with this fix (it works), and work on a more maintainable solution in the future. Link: http://lkml.kernel.org/r/20140109101932.0508dec7@gandalf.local.home Link: http://lkml.kernel.org/r/20140109182756.17abaaa8@gandalf.local.home Cc: stable@vger.kernel.org Signed-off-by: Steven Rostedt Signed-off-by: Linus Torvalds --- security/selinux/hooks.c | 20 ++++++++++++++++++-- security/selinux/include/objsec.h | 5 ++++- 2 files changed, 22 insertions(+), 3 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 6625699..57b0b49 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -234,6 +234,14 @@ static int inode_alloc_security(struct inode *inode) return 0; } +static void inode_free_rcu(struct rcu_head *head) +{ + struct inode_security_struct *isec; + + isec = container_of(head, struct inode_security_struct, rcu); + kmem_cache_free(sel_inode_cache, isec); +} + static void inode_free_security(struct inode *inode) { struct inode_security_struct *isec = inode->i_security; @@ -244,8 +252,16 @@ static void inode_free_security(struct inode *inode) list_del_init(&isec->list); spin_unlock(&sbsec->isec_lock); - inode->i_security = NULL; - kmem_cache_free(sel_inode_cache, isec); + /* + * The inode may still be referenced in a path walk and + * a call to selinux_inode_permission() can be made + * after inode_free_security() is called. Ideally, the VFS + * wouldn't do this, but fixing that is a much harder + * job. For now, simply 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(&isec->rcu, inode_free_rcu); } static int file_alloc_security(struct file *file) diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h index b1dfe10..078e553 100644 --- a/security/selinux/include/objsec.h +++ b/security/selinux/include/objsec.h @@ -38,7 +38,10 @@ struct task_security_struct { struct inode_security_struct { struct inode *inode; /* back pointer to inode object */ - struct list_head list; /* list of inode_security_struct */ + union { + struct list_head list; /* list of inode_security_struct */ + struct rcu_head rcu; /* for freeing the inode_security_struct */ + }; u32 task_sid; /* SID of creating task */ u32 sid; /* SID of this object */ u16 sclass; /* security class of this object */ -- cgit v1.1