diff mbox

tcp: assign the sock correctly to an outgoing SYNACK packet

Message ID 1365445825.3887.35.camel@edumazet-glaptop
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet April 8, 2013, 6:30 p.m. UTC 
On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
> On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> 
> > 
> > It seems a bit fragile to me, perhaps even hacky, but in some ways I guess it 
> > isn't anymore fragile than relying on skb->sk - as this problem demonstrates.  
> > My other concern is that adding this hook *correctly* is likely to touch a lot 
> > of files and may be a bit much so late in the 3.9 cycle, Dave, what say you?
> 
> I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> 3.9 ?
> 
> I am preparing a fix right now. Not a revert, thank you.
> 

Is the following patch not good enough ?

 include/linux/security.h |    7 +++++++
 net/ipv4/tcp_output.c    |    1 +
 security/security.c      |    6 ++++++
 3 files changed, 14 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

Comments

Paul Moore April 8, 2013, 8:37 p.m. UTC | #1 
On Monday, April 08, 2013 11:30:25 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
> > On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> > > It seems a bit fragile to me, perhaps even hacky, but in some ways I
> > > guess it isn't anymore fragile than relying on skb->sk - as this
> > > problem demonstrates. My other concern is that adding this hook
> > > *correctly* is likely to touch a lot of files and may be a bit much so
> > > late in the 3.9 cycle, Dave, what say you?> 
> > I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> > 3.9 ?
> > 
> > I am preparing a fix right now. Not a revert, thank you.
> 
> Is the following patch not good enough ?

I think it is somewhat telling that the hook you're proposing doesn't ever 
make any calls into any of the individual LSMs, it only calls back into the 
networking stack.  In my mind, this makes it an abuse of the LSM mechanism.

On Monday, April 08, 2013 11:34:11 AM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 14:26 -0400, Paul Moore wrote:
> > I guess we'll have to wait and see then; the more I think about the new
> > hook you proposed the less enthused I am about it.
> > 
> > I'm still curious to hear what Dave has to say on this.
> 
> 90ba9b1986b5ac4b2 is 10 months old, and nobody complained until today ?

The people who use this functionality almost never use upstream kernels, they 
need to protection/certification/warm-fuzzies/etc. that come from a 
distribution kernel and a support infrastructure.  I didn't catch it because I 
use a slightly different configuration that didn't expose this bug; while I 
would like to run a full regression test every release I simply don't have the 
time to do that myself.

> This sounds like a very small issue to me, a revert is simply overkill.

It all depends on your use case.  To you, whom I assume doesn't use SELinux, 
it is indeed a trivial issue.  To someone who relies on SELinux for its 
network access controls this is a pretty significant issue.
David Miller April 8, 2013, 8:44 p.m. UTC | #2 
From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 16:37:22 -0400

> On Monday, April 08, 2013 11:30:25 AM Eric Dumazet wrote:
>> On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
>> > On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
>> > > It seems a bit fragile to me, perhaps even hacky, but in some ways I
>> > > guess it isn't anymore fragile than relying on skb->sk - as this
>> > > problem demonstrates. My other concern is that adding this hook
>> > > *correctly* is likely to touch a lot of files and may be a bit much so
>> > > late in the 3.9 cycle, Dave, what say you?> 
>> > I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
>> > 3.9 ?
>> > 
>> > I am preparing a fix right now. Not a revert, thank you.
>> 
>> Is the following patch not good enough ?
> 
> I think it is somewhat telling that the hook you're proposing doesn't ever 
> make any calls into any of the individual LSMs, it only calls back into the 
> networking stack.  In my mind, this makes it an abuse of the LSM mechanism.

Without LSMs the socket reference is spurious and pointless overhead,
therefore the only acceptable fix one which only takes the socket
reference when there are LSMs with a need.
--
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
Paul Moore April 8, 2013, 8:53 p.m. UTC | #3 
On Monday, April 08, 2013 04:44:39 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Mon, 08 Apr 2013 16:37:22 -0400
> 
> > On Monday, April 08, 2013 11:30:25 AM Eric Dumazet wrote:
> >> On Mon, 2013-04-08 at 11:21 -0700, Eric Dumazet wrote:
> >> > On Mon, 2013-04-08 at 14:12 -0400, Paul Moore wrote:
> >> > > It seems a bit fragile to me, perhaps even hacky, but in some ways I
> >> > > guess it isn't anymore fragile than relying on skb->sk - as this
> >> > > problem demonstrates. My other concern is that adding this hook
> >> > > *correctly* is likely to touch a lot of files and may be a bit much
> >> > > so late in the 3.9 cycle, Dave, what say you?>
> >> > 
> >> > I don't get it, 90ba9b1986b5ac4b2d18 was in 3.6, why do you care of
> >> > 3.9 ?
> >> > 
> >> > I am preparing a fix right now. Not a revert, thank you.
> >> 
> >> Is the following patch not good enough ?
> > 
> > I think it is somewhat telling that the hook you're proposing doesn't ever
> > make any calls into any of the individual LSMs, it only calls back into
> > the networking stack.  In my mind, this makes it an abuse of the LSM
> > mechanism.
> 
> Without LSMs the socket reference is spurious and pointless overhead,
> therefore the only acceptable fix one which only takes the socket
> reference when there are LSMs with a need.

Well that's frustrating, but if that is the game so be it.

Eric, your patch didn't cross-post to the LSM list, do you want to post it 
there for review/merging?  I would recommend also adding a pointer to this 
thread for reference.
Eric Dumazet April 8, 2013, 8:55 p.m. UTC | #4 
On Mon, 2013-04-08 at 16:37 -0400, Paul Moore wrote:

> The people who use this functionality almost never use upstream kernels, they 
> need to protection/certification/warm-fuzzies/etc. that come from a 
> distribution kernel and a support infrastructure.  I didn't catch it because I 
> use a slightly different configuration that didn't expose this bug; while I 
> would like to run a full regression test every release I simply don't have the 
> time to do that myself.
> 
> > This sounds like a very small issue to me, a revert is simply overkill.
> 
> It all depends on your use case.  To you, whom I assume doesn't use SELinux, 
> it is indeed a trivial issue.  To someone who relies on SELinux for its 
> network access controls this is a pretty significant issue.
> 

Is the patch I sent addressing the problem or not ?

Note that I do have : CONFIG_SECURITY=y

So this patch basically adds the overhead back, and I'll have to use
real hook later in net-next.

At least my patch clearly _shows_ the security requirement, instead of
relying on a side effect of a previous sock_wmalloc()

Again, it would be nice you understand the plan.


--
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
Paul Moore April 8, 2013, 9:09 p.m. UTC | #5 
On Monday, April 08, 2013 01:55:01 PM Eric Dumazet wrote:
> On Mon, 2013-04-08 at 16:37 -0400, Paul Moore wrote:
> > The people who use this functionality almost never use upstream kernels,
> > they need to protection/certification/warm-fuzzies/etc. that come from a
> > distribution kernel and a support infrastructure.  I didn't catch it
> > because I use a slightly different configuration that didn't expose this
> > bug; while I would like to run a full regression test every release I
> > simply don't have the time to do that myself.
> > 
> > > This sounds like a very small issue to me, a revert is simply overkill.
> > 
> > It all depends on your use case.  To you, whom I assume doesn't use
> > SELinux, it is indeed a trivial issue.  To someone who relies on SELinux
> > for its network access controls this is a pretty significant issue.
> 
> Is the patch I sent addressing the problem or not ?
> 
> Note that I do have : CONFIG_SECURITY=y
> 
> So this patch basically adds the overhead back, and I'll have to use
> real hook later in net-next.

Please repost the patch to the LSM list, it needs to be discussed there.

> At least my patch clearly _shows_ the security requirement, instead of
> relying on a side effect of a previous sock_wmalloc()

I don't see it as a side effect, and as far as demonstration, I think the 
SELinux network access controls in their entirety shows the security 
requirement.  If we want to make the security requirements even more explicit 
in the networking stack, let's add a security blob to the sk_buff and allow 
some proper LSM hooks.

> Again, it would be nice you understand the plan.

I have no idea what the above sentence is trying to say.
David Miller April 8, 2013, 9:14 p.m. UTC | #6 
From: Paul Moore <pmoore@redhat.com>
Date: Mon, 08 Apr 2013 17:09:32 -0400

> On Monday, April 08, 2013 01:55:01 PM Eric Dumazet wrote:
>> At least my patch clearly _shows_ the security requirement, instead of
>> relying on a side effect of a previous sock_wmalloc()
> 
> I don't see it as a side effect, and as far as demonstration, I think the 
> SELinux network access controls in their entirety shows the security 
> requirement.  If we want to make the security requirements even more explicit 
> in the networking stack, let's add a security blob to the sk_buff and allow 
> some proper LSM hooks.

You don't get it.

Without LSM there is no need to use sock_wmalloc() or to have a socket
context attacked to the SYN/ACK packet at all.

Therefore, the need should be explicit (Eric's approach), rather than
implicit (what you seem to be after).
--
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 8, 2013, 9:17 p.m. UTC | #7 
On Mon, 2013-04-08 at 17:09 -0400, Paul Moore wrote:

> I have no idea what the above sentence is trying to say.

I guess the amount of mails exchanged today on this subject is kind of
astonishing. I am truly sorry I had more urgent issues to deal with
today.

If the patch is not officially sent by you today, I'll take care of it
after my day work.

Thank you.





--
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 mbox

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index eee7478..1bde8bb 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2588,6 +2588,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 +2781,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/security.c b/security/security.c
index 7b88c6a..cdabd4d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -25,6 +25,7 @@ 
 #include <linux/personality.h>
 #include <linux/backing-dev.h>
 #include <net/flow.h>
+#include <net/sock.h>
 
 #define MAX_LSM_EVM_XATTR	2
 
@@ -1290,6 +1291,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)
+{
+	skb_set_owner_w(skb, sk);
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM