Patchwork selinux: add a skb_owned_by() hook

login
register
mail settings
Submitter Eric Dumazet
Date April 9, 2013, 3:58 a.m.
Message ID <1365479891.3887.99.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/234952/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - April 9, 2013, 3:58 a.m.
From: Eric Dumazet <edumazet@google.com>

Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
broke certain SELinux/NetLabel configurations by no longer correctly
assigning the sock to the outgoing SYNACK packet.

Cost of atomic operations on the LISTEN socket is quite big,
and we would like it to happen only if really needed.

This patch introduces a new security_ops->skb_owned_by() method,
that is a void operation unless selinux is active.

Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
Diagnosed-by: Paul Moore <pmoore@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-security-module@vger.kernel.org
---
 include/linux/security.h |    8 ++++++++
 net/ipv4/tcp_output.c    |    1 +
 security/capability.c    |    6 ++++++
 security/security.c      |    5 +++++
 security/selinux/hooks.c |    7 +++++++
 5 files changed, 27 insertions(+)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler - April 9, 2013, 4:29 a.m.
On 4/8/2013 8:58 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
>
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
>
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.

I don't understand what this hook does.
Does it affect Smack (which uses NetLabel) as well?
How can I find out?

>
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org
> ---
>  include/linux/security.h |    8 ++++++++
>  net/ipv4/tcp_output.c    |    1 +
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  security/selinux/hooks.c |    7 +++++++
>  5 files changed, 27 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index eee7478..6c3a78a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1638,6 +1638,7 @@ struct security_operations {
>  	int (*tun_dev_attach_queue) (void *security);
>  	int (*tun_dev_attach) (struct sock *sk, void *security);
>  	int (*tun_dev_open) (void *security);
> +	void (*skb_owned_by) (struct sk_buff *skb, struct sock *sk);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> @@ -2588,6 +2589,8 @@ int security_tun_dev_attach_queue(void *security);
>  int security_tun_dev_attach(struct sock *sk, void *security);
>  int security_tun_dev_open(void *security);
>  
> +void security_skb_owned_by(struct sk_buff *skb, struct sock *sk);
> +
>  #else	/* CONFIG_SECURITY_NETWORK */
>  static inline int security_unix_stream_connect(struct sock *sock,
>  					       struct sock *other,
> @@ -2779,6 +2782,11 @@ static inline int security_tun_dev_open(void *security)
>  {
>  	return 0;
>  }
> +
> +static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
> +{
> +}
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 5d0b438..b44cf81 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2709,6 +2709,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
>  	skb_reserve(skb, MAX_TCP_HEADER);
>  
>  	skb_dst_set(skb, dst);
> +	security_skb_owned_by(skb, sk);
>  
>  	mss = dst_metric_advmss(dst);
>  	if (tp->rx_opt.user_mss && tp->rx_opt.user_mss < mss)
> diff --git a/security/capability.c b/security/capability.c
> index 5797750..c36cca6 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -737,6 +737,11 @@ static int cap_tun_dev_open(void *security)
>  {
>  	return 0;
>  }
> +
> +static void cap_skb_owned_by(struct sk_buff *skb, struct sock *sk) 
> +{
> +}
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> @@ -1071,6 +1076,7 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, tun_dev_open);
>  	set_to_cap_if_null(ops, tun_dev_attach_queue);
>  	set_to_cap_if_null(ops, tun_dev_attach);
> +	set_to_cap_if_null(ops, skb_owned_by);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  	set_to_cap_if_null(ops, xfrm_policy_alloc_security);
> diff --git a/security/security.c b/security/security.c
> index 7b88c6a..03f248b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1290,6 +1290,11 @@ int security_tun_dev_open(void *security)
>  }
>  EXPORT_SYMBOL(security_tun_dev_open);
>  
> +void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
> +{
> +	security_ops->skb_owned_by(skb, sk);
> +}
> +
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2fa28c8..7171a95 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -51,6 +51,7 @@
>  #include <linux/tty.h>
>  #include <net/icmp.h>
>  #include <net/ip.h>		/* for local_port_range[] */
> +#include <net/sock.h>
>  #include <net/tcp.h>		/* struct or_callable used in sock_rcv_skb */
>  #include <net/net_namespace.h>
>  #include <net/netlabel.h>
> @@ -4363,6 +4364,11 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
>  	selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
>  }
>  
> +static void selinux_skb_owned_by(struct sk_buff *skb, struct sock *sk)
> +{
> +	skb_set_owner_w(skb, sk);
> +}
> +
>  static int selinux_secmark_relabel_packet(u32 sid)
>  {
>  	const struct task_security_struct *__tsec;
> @@ -5664,6 +5670,7 @@ static struct security_operations selinux_ops = {
>  	.tun_dev_attach_queue =		selinux_tun_dev_attach_queue,
>  	.tun_dev_attach =		selinux_tun_dev_attach,
>  	.tun_dev_open =			selinux_tun_dev_open,
> +	.skb_owned_by =			selinux_skb_owned_by,
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 9, 2013, 4:41 a.m.
It makes sure SYN/ACKs have a socket context attached to the
packet, which only LSMs actually need.

You participated in the thread where this stuff was discussed and the
initial version of this patch was posted, so this patch, or any aspect
of it, should not be a mystery.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler - April 9, 2013, 5:14 a.m.
On 4/8/2013 9:41 PM, David Miller wrote:
> It makes sure SYN/ACKs have a socket context attached to the
> packet, which only LSMs actually need.
>
> You participated in the thread where this stuff was discussed and the
> initial version of this patch was posted, so this patch, or any aspect
> of it, should not be a mystery.

I am flattered by your assumption that I can recall
on a moment's notice every thread I've been involved in since
1978. A reference to that thread would be mighty helpful.

The reality is that someone new to the thread is going to
have a bunch of trouble figuring out what you're talking
about. Every patch posting should have sufficient context
that someone who did not happen to see the background
threads will understand why it is being proposed.

I believe you when you say that I participated in the discussion.
I'll be dipped in caramel and rolled in nuts if I can find the
threads. Help me out by providing enough context that I can find
what I said, what you said, and if I still agree with myself.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - April 9, 2013, 6:24 a.m.
On Mon, 2013-04-08 at 21:29 -0700, Casey Schaufler wrote:

> I don't understand what this hook does.

It documents that security modules might need to get an sk pointer from
an skb, especially for TCP SYNACK messages.

> Does it affect Smack (which uses NetLabel) as well?
> How can I find out?

If you ask the question, thats is probably because Smack is not
affected.

selinux uses netfilter hooks, not Smack. selinux could probably refine
the need to set skb->sk based on CONFIG_NETFILTER, but I leave that
for a future change.

Just try the patch, and add your 'Tested-by', that will be fine.

If you believe Smack has an issue, tell us why, and we'll add the
follow-up patch.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Morris - April 9, 2013, 7:38 a.m.
On Mon, 8 Apr 2013, Eric Dumazet wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
> 
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
> 
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org

Acked-by: James Morris <james.l.morris@oracle.com>
Paul Moore - April 9, 2013, 11:39 a.m.
On Tuesday, April 09, 2013 12:41:44 AM David Miller wrote:
> It makes sure SYN/ACKs have a socket context attached to the
> packet, which only LSMs actually need.
> 
> You participated in the thread where this stuff was discussed and the
> initial version of this patch was posted, so this patch, or any aspect
> of it, should not be a mystery.

Casey, and the LSM list as a whole, was not included in the entire thread as 
when I first posted my original patch I believed the "mergeable" fix was going 
to be self contained within the network stack.

For Casey, and others on the LSM list, here is a pointer to the original 
thread which started on netdev.  Enjoy.

 * http://marc.info/?t=136543607500006&r=1&w=2
Paul Moore - April 9, 2013, 11:45 a.m.
On Monday, April 08, 2013 09:29:35 PM Casey Schaufler wrote:
> Does it affect Smack (which uses NetLabel) as well?

The short answer is "no".  Smack approaches the network access controls 
differently and as a result has hooks in different places that do slightly 
different things; the regression in the network stack only affects SELinux.
Paul Moore - April 9, 2013, 12:06 p.m.
On Monday, April 08, 2013 08:58:11 PM Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
> 
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
> 
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org
> ---
>  include/linux/security.h |    8 ++++++++
>  net/ipv4/tcp_output.c    |    1 +
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  security/selinux/hooks.c |    7 +++++++
>  5 files changed, 27 insertions(+)

I've already voiced my objections to this approach, but I've just tested it 
and it does resolve the regression in the network stack.

Tested-by: Paul Moore <pmoore@redhat.com>
Acked-by: Paul Moore <pmoore@redhat.com>
David Miller - April 9, 2013, 5:23 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 08 Apr 2013 20:58:11 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
> 
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
> 
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
> 
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Since this fixes a regression that got added by the networking
tree, I'll push this to Linus, applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index eee7478..6c3a78a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1638,6 +1638,7 @@  struct security_operations {
 	int (*tun_dev_attach_queue) (void *security);
 	int (*tun_dev_attach) (struct sock *sk, void *security);
 	int (*tun_dev_open) (void *security);
+	void (*skb_owned_by) (struct sk_buff *skb, struct sock *sk);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -2588,6 +2589,8 @@  int security_tun_dev_attach_queue(void *security);
 int security_tun_dev_attach(struct sock *sk, void *security);
 int security_tun_dev_open(void *security);
 
+void security_skb_owned_by(struct sk_buff *skb, struct sock *sk);
+
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
 					       struct sock *other,
@@ -2779,6 +2782,11 @@  static inline int security_tun_dev_open(void *security)
 {
 	return 0;
 }
+
+static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5d0b438..b44cf81 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2709,6 +2709,7 @@  struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 	skb_reserve(skb, MAX_TCP_HEADER);
 
 	skb_dst_set(skb, dst);
+	security_skb_owned_by(skb, sk);
 
 	mss = dst_metric_advmss(dst);
 	if (tp->rx_opt.user_mss && tp->rx_opt.user_mss < mss)
diff --git a/security/capability.c b/security/capability.c
index 5797750..c36cca6 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -737,6 +737,11 @@  static int cap_tun_dev_open(void *security)
 {
 	return 0;
 }
+
+static void cap_skb_owned_by(struct sk_buff *skb, struct sock *sk) 
+{
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -1071,6 +1076,7 @@  void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, tun_dev_open);
 	set_to_cap_if_null(ops, tun_dev_attach_queue);
 	set_to_cap_if_null(ops, tun_dev_attach);
+	set_to_cap_if_null(ops, skb_owned_by);
 #endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	set_to_cap_if_null(ops, xfrm_policy_alloc_security);
diff --git a/security/security.c b/security/security.c
index 7b88c6a..03f248b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1290,6 +1290,11 @@  int security_tun_dev_open(void *security)
 }
 EXPORT_SYMBOL(security_tun_dev_open);
 
+void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+	security_ops->skb_owned_by(skb, sk);
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2fa28c8..7171a95 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -51,6 +51,7 @@ 
 #include <linux/tty.h>
 #include <net/icmp.h>
 #include <net/ip.h>		/* for local_port_range[] */
+#include <net/sock.h>
 #include <net/tcp.h>		/* struct or_callable used in sock_rcv_skb */
 #include <net/net_namespace.h>
 #include <net/netlabel.h>
@@ -4363,6 +4364,11 @@  static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
 	selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
 }
 
+static void selinux_skb_owned_by(struct sk_buff *skb, struct sock *sk)
+{
+	skb_set_owner_w(skb, sk);
+}
+
 static int selinux_secmark_relabel_packet(u32 sid)
 {
 	const struct task_security_struct *__tsec;
@@ -5664,6 +5670,7 @@  static struct security_operations selinux_ops = {
 	.tun_dev_attach_queue =		selinux_tun_dev_attach_queue,
 	.tun_dev_attach =		selinux_tun_dev_attach,
 	.tun_dev_open =			selinux_tun_dev_open,
+	.skb_owned_by =			selinux_skb_owned_by,
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,