[nf-next,v4,5/5] af_packet: Introduce egress hook

Commit Message

Lukas Wunner Jan. 22, 2021, 8:47 a.m. UTC

From: Pablo Neira Ayuso <[e-mail]>

Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS
socket option set to on, which allows packets to escape without being
filtered in the egress path.

This patch only updates the AF_PACKET path, it does not update
dev_direct_xmit() so the XDP infrastructure has a chance to bypass
Netfilter.

Signed-off-by: Pablo Neira Ayuso <[e-mail]>
[lukas: acquire rcu_read_lock, fix typos, rebase]
Signed-off-by: Lukas Wunner <[e-mail]>
---
 net/packet/af_packet.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Willem de Bruijn Jan. 22, 2021, 4:13 p.m. UTC | #1

On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <[e-mail]> wrote:
>
> From: Pablo Neira Ayuso <[e-mail]>
>
> Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS
> socket option set to on, which allows packets to escape without being
> filtered in the egress path.
>
> This patch only updates the AF_PACKET path, it does not update
> dev_direct_xmit() so the XDP infrastructure has a chance to bypass
> Netfilter.
>
> Signed-off-by: Pablo Neira Ayuso <[e-mail]>
> [lukas: acquire rcu_read_lock, fix typos, rebase]
> Signed-off-by: Lukas Wunner <[e-mail]>

Isn't the point of PACKET_QDISC_BYPASS to skip steps like this?

Lukas Wunner Jan. 24, 2021, 11:14 a.m. UTC | #2

On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote:
> On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <[e-mail]> wrote:
> > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS
> > socket option set to on, which allows packets to escape without being
> > filtered in the egress path.
> >
> > This patch only updates the AF_PACKET path, it does not update
> > dev_direct_xmit() so the XDP infrastructure has a chance to bypass
> > Netfilter.
> 
> Isn't the point of PACKET_QDISC_BYPASS to skip steps like this?

I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc,
not to bypass everything."

(The quote is taken from this message by Eric Dumazet:
https://lore.kernel.org/netfilter-devel/[e-mail]/
)

Thanks,

Lukas

Willem de Bruijn Jan. 24, 2021, 4:18 p.m. UTC | #3

On Sun, Jan 24, 2021 at 6:14 AM Lukas Wunner <[e-mail]> wrote:
>
> On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote:
> > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <[e-mail]> wrote:
> > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS
> > > socket option set to on, which allows packets to escape without being
> > > filtered in the egress path.
> > >
> > > This patch only updates the AF_PACKET path, it does not update
> > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass
> > > Netfilter.
> >
> > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this?
>
> I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc,
> not to bypass everything."
>
> (The quote is taken from this message by Eric Dumazet:
> https://lore.kernel.org/netfilter-devel/[e-mail]/
> )

I see. I don't understand the value of a short-cut fast path if we
start chipping away at its characteristic feature.

It also bypasses sch_handle_egress and packet sockets. This new
feature seems broadly similar to the first?

Lukas Wunner Jan. 30, 2021, 4:26 p.m. UTC | #4

On Sun, Jan 24, 2021 at 11:18:00AM -0500, Willem de Bruijn wrote:
> On Sun, Jan 24, 2021 at 6:14 AM Lukas Wunner <[e-mail]> wrote:
> > On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote:
> > > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <[e-mail]> wrote:
> > > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS
> > > > socket option set to on, which allows packets to escape without being
> > > > filtered in the egress path.
> > > >
> > > > This patch only updates the AF_PACKET path, it does not update
> > > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass
> > > > Netfilter.
> > >
> > > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this?
> >
> > I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc,
> > not to bypass everything."
> >
> > (The quote is taken from this message by Eric Dumazet:
> > https://lore.kernel.org/netfilter-devel/[e-mail]/
> > )
> 
> I see. I don't understand the value of a short-cut fast path if we
> start chipping away at its characteristic feature.

The point is to filter traffic coming in through af_packet.
Exempting PACKET_QDISC_BYPASS from filtering would open up a
trivial security hole.

Thanks,

Lukas

Willem de Bruijn Jan. 30, 2021, 4:58 p.m. UTC | #5

On Sat, Jan 30, 2021 at 11:26 AM Lukas Wunner <[e-mail]> wrote:
>
> On Sun, Jan 24, 2021 at 11:18:00AM -0500, Willem de Bruijn wrote:
> > On Sun, Jan 24, 2021 at 6:14 AM Lukas Wunner <[e-mail]> wrote:
> > > On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote:
> > > > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <[e-mail]> wrote:
> > > > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS
> > > > > socket option set to on, which allows packets to escape without being
> > > > > filtered in the egress path.
> > > > >
> > > > > This patch only updates the AF_PACKET path, it does not update
> > > > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass
> > > > > Netfilter.
> > > >
> > > > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this?
> > >
> > > I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc,
> > > not to bypass everything."
> > >
> > > (The quote is taken from this message by Eric Dumazet:
> > > https://lore.kernel.org/netfilter-devel/[e-mail]/
> > > )
> >
> > I see. I don't understand the value of a short-cut fast path if we
> > start chipping away at its characteristic feature.
>
> The point is to filter traffic coming in through af_packet.
> Exempting PACKET_QDISC_BYPASS from filtering would open up a
> trivial security hole.

Sure. But that argument is no different for TC_EGRESS.

That's why packet sockets require CAP_NET_RAW. It is perhaps
unfortunately that it is ns_capable instead of capable. But there is
nothing netfilter specific about this.

Patch

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 6bbc7a448593..6dca6ead1162 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -89,6 +89,7 @@ 
 #endif
 #include <linux/bpf.h>
 #include <net/compat.h>
+#include <linux/netfilter_netdev.h>
 
 #include "internal.h"
 
@@ -239,8 +240,42 @@  struct packet_skb_cb {
 static void __fanout_unlink(struct sock *sk, struct packet_sock *po);
 static void __fanout_link(struct sock *sk, struct packet_sock *po);
 
+#ifdef CONFIG_NETFILTER_EGRESS
+static noinline struct sk_buff *nf_hook_direct_egress(struct sk_buff *skb)
+{
+	struct sk_buff *next, *head = NULL, *tail;
+	int rc;
+
+	rcu_read_lock();
+	for (; skb != NULL; skb = next) {
+		next = skb->next;
+		skb_mark_not_on_list(skb);
+
+		if (!nf_hook_egress(skb, &rc, skb->dev))
+			continue;
+
+		if (!head)
+			head = skb;
+		else
+			tail->next = skb;
+
+		tail = skb;
+	}
+	rcu_read_unlock();
+
+	return head;
+}
+#endif
+
 static int packet_direct_xmit(struct sk_buff *skb)
 {
+#ifdef CONFIG_NETFILTER_EGRESS
+	if (nf_hook_egress_active()) {
+		skb = nf_hook_direct_egress(skb);
+		if (!skb)
+			return NET_XMIT_DROP;
+	}
+#endif
 	return dev_direct_xmit(skb, packet_pick_tx_queue(skb));
 }