| Message ID | 1365479891.3887.99.camel@edumazet-glaptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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>
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
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.
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>
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
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,