| Message ID | 1378396127-8342-1-git-send-email-dborkman@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Daniel Borkmann <dborkman@redhat.com> Date: Thu, 5 Sep 2013 17:48:47 +0200 > From: Daniel Borkmann <dborkmann@redhat.com> > > Fix finer-grained control and let only a whitelist of allowed netlink > protocols pass, in our case related to networking. If later on, other > subsystems decide they want to add their protocol as well to the list > of allowed protocols they shall simply add it. While at it, we also > need to tell what protocol is in use otherwise BPF_S_ANC_PROTOCOL can > not pick it up (as it's not filled out). > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> This takes away functionality that I'd be more interesting in using, namely being able to listen to all netlink protocols using one tap. Seriously, when I first saw this feature, that was the first way I'd imagine myself using it, as a tcpdump for netlink traffic, all of it. If I just want to hear all netlink traffic, don't make me be forced to know every single NETLINK_* protocol value and have to open that many sockets just to do so. It also makes it so that I can't listen to userlevel custom netlink protocols, another minus of filtering. At the very least, allow an sk_protocol of zero or similar to have this meaning of "everything". I'm not applying this patch, sorry. -- 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 Thu, 05 Sep 2013 14:44:42 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Thu, 5 Sep 2013 17:48:47 +0200 > > > From: Daniel Borkmann <dborkmann@redhat.com> > > > > Fix finer-grained control and let only a whitelist of allowed netlink > > protocols pass, in our case related to networking. If later on, other > > subsystems decide they want to add their protocol as well to the list > > of allowed protocols they shall simply add it. While at it, we also > > need to tell what protocol is in use otherwise BPF_S_ANC_PROTOCOL can > > not pick it up (as it's not filled out). > > > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > > This takes away functionality that I'd be more interesting in using, > namely being able to listen to all netlink protocols using one tap. > > Seriously, when I first saw this feature, that was the first way I'd > imagine myself using it, as a tcpdump for netlink traffic, all of > it. > > If I just want to hear all netlink traffic, don't make me be forced to > know every single NETLINK_* protocol value and have to open that many > sockets just to do so. > > It also makes it so that I can't listen to userlevel custom netlink > protocols, another minus of filtering. > > At the very least, allow an sk_protocol of zero or similar to have this > meaning of "everything". > > I'm not applying this patch, sorry. > -- > 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 If you want filtering, why not add BPF (sk_filter) support to this? -- 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
From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 5 Sep 2013 11:57:55 -0700 > If you want filtering, why not add BPF (sk_filter) support to this? That's one idea. But using sk_protocol is at least consistent with how AF_PACKET does things. -- 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 Thu, 05 Sep 2013 15:03:41 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Thu, 5 Sep 2013 11:57:55 -0700 > > > If you want filtering, why not add BPF (sk_filter) support to this? > > That's one idea. > > But using sk_protocol is at least consistent with how AF_PACKET does > things. Why not both. -- 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 09/05/2013 08:44 PM, David Miller wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Thu, 5 Sep 2013 17:48:47 +0200 > >> From: Daniel Borkmann <dborkmann@redhat.com> >> >> Fix finer-grained control and let only a whitelist of allowed netlink >> protocols pass, in our case related to networking. If later on, other >> subsystems decide they want to add their protocol as well to the list >> of allowed protocols they shall simply add it. While at it, we also >> need to tell what protocol is in use otherwise BPF_S_ANC_PROTOCOL can >> not pick it up (as it's not filled out). >> >> Signed-off-by: Daniel Borkmann <dborkman@redhat.com> To answer Stephen and Dave in one message ... ;-) > This takes away functionality that I'd be more interesting in using, > namely being able to listen to all netlink protocols using one tap. > > Seriously, when I first saw this feature, that was the first way I'd > imagine myself using it, as a tcpdump for netlink traffic, all of > it. > > If I just want to hear all netlink traffic, don't make me be forced to > know every single NETLINK_* protocol value and have to open that many > sockets just to do so. > > It also makes it so that I can't listen to userlevel custom netlink > protocols, another minus of filtering. > > At the very least, allow an sk_protocol of zero or similar to have this > meaning of "everything". I agree with you with all the above and I think with having this in tcpdump would be great to debug netlink traffic of course ... With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get all users from the suggested white-list of the patch, which is the majority of netlink users I believe. Hence, you do not need to have one socket per protocol. skbs from there should get dragged into pf_packet via dev_queue_xmit_nit() which works on ptype_all list. Plus, with the nskb->protocol addition in this patch, they can then either take all netlink traffic from that, or filter with BPF e.g. for particular protocols via BPF_S_ANC_PROTOCOL or more advanced stuff, for example. Also, they can select particular dissectors with that information that comes in via sll_protocol which is useful, of course. I think for out-of-tree modules, we never really cared much, and they could use generic netlink probably anyway. The thing why I wanted to do this is to keep security related messages (audit, selinux) generally under the radar, which is "more or less" what's remaining, so we still get main users. This patch would accomplish those things, that's why I think it's useful. -- 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
From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 5 Sep 2013 12:44:37 -0700 > On Thu, 05 Sep 2013 15:03:41 -0400 (EDT) > David Miller <davem@davemloft.net> wrote: > >> From: Stephen Hemminger <stephen@networkplumber.org> >> Date: Thu, 5 Sep 2013 11:57:55 -0700 >> >> > If you want filtering, why not add BPF (sk_filter) support to this? >> >> That's one idea. >> >> But using sk_protocol is at least consistent with how AF_PACKET does >> things. > > Why not both. That's fine too. -- 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
From: Daniel Borkmann <dborkman@redhat.com> Date: Thu, 05 Sep 2013 21:48:00 +0200 > With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get > all users from the suggested white-list of the patch, which is the > majority of netlink users I believe. Hence, you do not need to have > one socket per protocol. skbs from there should get dragged into > pf_packet via dev_queue_xmit_nit() which works on ptype_all list. What about user level netlink protocols? -- 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 09/05/2013 09:54 PM, David Miller wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Thu, 05 Sep 2013 21:48:00 +0200 > >> With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get >> all users from the suggested white-list of the patch, which is the >> majority of netlink users I believe. Hence, you do not need to have >> one socket per protocol. skbs from there should get dragged into >> pf_packet via dev_queue_xmit_nit() which works on ptype_all list. > > What about user level netlink protocols? If you are referring to NETLINK_USERSOCK, then we let this pass here, so nothing changes. -- 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
From: Daniel Borkmann <dborkman@redhat.com> Date: Thu, 05 Sep 2013 21:59:28 +0200 > On 09/05/2013 09:54 PM, David Miller wrote: >> From: Daniel Borkmann <dborkman@redhat.com> >> Date: Thu, 05 Sep 2013 21:48:00 +0200 >> >>> With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get >>> all users from the suggested white-list of the patch, which is the >>> majority of netlink users I believe. Hence, you do not need to have >>> one socket per protocol. skbs from there should get dragged into >>> pf_packet via dev_queue_xmit_nit() which works on ptype_all list. >> >> What about user level netlink protocols? > > If you are referring to NETLINK_USERSOCK, then we let this pass here, > so nothing changes. Ok I need to think about this some more, I moved your patch back into under review state. -- 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
From: Daniel Borkmann <dborkman@redhat.com> Date: Thu, 05 Sep 2013 21:59:28 +0200 > On 09/05/2013 09:54 PM, David Miller wrote: >> From: Daniel Borkmann <dborkman@redhat.com> >> Date: Thu, 05 Sep 2013 21:48:00 +0200 >> >>> With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get >>> all users from the suggested white-list of the patch, which is the >>> majority of netlink users I believe. Hence, you do not need to have >>> one socket per protocol. skbs from there should get dragged into >>> pf_packet via dev_queue_xmit_nit() which works on ptype_all list. >> >> What about user level netlink protocols? > > If you are referring to NETLINK_USERSOCK, then we let this pass here, > so nothing changes. Ok, I've applied this, thanks Daniel. -- 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/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 0c61b59..350187e 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -168,16 +168,43 @@ int netlink_remove_tap(struct netlink_tap *nt) } EXPORT_SYMBOL_GPL(netlink_remove_tap); +static bool netlink_filter_tap(const struct sk_buff *skb) +{ + struct sock *sk = skb->sk; + bool pass = false; + + /* We take the more conservative approach and + * whitelist socket protocols that may pass. + */ + switch (sk->sk_protocol) { + case NETLINK_ROUTE: + case NETLINK_USERSOCK: + case NETLINK_SOCK_DIAG: + case NETLINK_NFLOG: + case NETLINK_XFRM: + case NETLINK_FIB_LOOKUP: + case NETLINK_NETFILTER: + case NETLINK_GENERIC: + pass = true; + break; + } + + return pass; +} + static int __netlink_deliver_tap_skb(struct sk_buff *skb, struct net_device *dev) { struct sk_buff *nskb; + struct sock *sk = skb->sk; int ret = -ENOMEM; dev_hold(dev); nskb = skb_clone(skb, GFP_ATOMIC); if (nskb) { nskb->dev = dev; + nskb->protocol = htons((u16) sk->sk_protocol); + ret = dev_queue_xmit(nskb); if (unlikely(ret > 0)) ret = net_xmit_errno(ret); @@ -192,6 +219,9 @@ static void __netlink_deliver_tap(struct sk_buff *skb) int ret; struct netlink_tap *tmp; + if (!netlink_filter_tap(skb)) + return; + list_for_each_entry_rcu(tmp, &netlink_tap_all, list) { ret = __netlink_deliver_tap_skb(skb, tmp->dev); if (unlikely(ret))