[PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Stanislav Fomichev]

If we have a flow dissector BPF program attached to the namespace,
FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.

Handle FLOW_DISSECTOR_KEY_ETH_ADDRS before BPF and only if we have
an skb (used by tc-flower only).

Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/core/flow_dissector.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 9ca784c592ac..ba76d9168c8b 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -825,6 +825,18 @@ bool __skb_flow_dissect(const struct net *net,
 			else if (skb->sk)
 				net = sock_net(skb->sk);
 		}
+
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+			struct ethhdr *eth = eth_hdr(skb);
+			struct flow_dissector_key_eth_addrs *key_eth_addrs;
+
+			key_eth_addrs = skb_flow_dissector_target(flow_dissector,
+								  FLOW_DISSECTOR_KEY_ETH_ADDRS,
+								  target_container);
+			memcpy(key_eth_addrs, &eth->h_dest,
+			       sizeof(*key_eth_addrs));
+		}
 	}
 
 	WARN_ON_ONCE(!net);
@@ -860,17 +872,6 @@ bool __skb_flow_dissect(const struct net *net,
 		rcu_read_unlock();
 	}
 
-	if (dissector_uses_key(flow_dissector,
-			       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
-		struct ethhdr *eth = eth_hdr(skb);
-		struct flow_dissector_key_eth_addrs *key_eth_addrs;
-
-		key_eth_addrs = skb_flow_dissector_target(flow_dissector,
-							  FLOW_DISSECTOR_KEY_ETH_ADDRS,
-							  target_container);
-		memcpy(key_eth_addrs, &eth->h_dest, sizeof(*key_eth_addrs));
-	}
-
 proto_again:
 	fdret = FLOW_DISSECT_RET_CONTINUE;
 
-- 
2.21.0.1020.gf2820cf01a-goog

[PATCH bpf 2/2] selftests/bpf: test L2 dissection in flow dissector

[Stanislav Fomichev]

Make sure that everything that's coming from a pre-defined mac address
can be dropped.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../selftests/bpf/test_flow_dissector.sh      | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_flow_dissector.sh b/tools/testing/selftests/bpf/test_flow_dissector.sh
index d23d4da66b83..1505d0a5fb32 100755
--- a/tools/testing/selftests/bpf/test_flow_dissector.sh
+++ b/tools/testing/selftests/bpf/test_flow_dissector.sh
@@ -112,4 +112,27 @@ tc filter add dev lo parent ffff: protocol ipv6 pref 1337 flower ip_proto \
 # Send 10 IPv6/UDP packets from port 10. Filter should not drop any.
 ./test_flow_dissector -i 6 -f 10
 
+tc filter del dev lo ingress pref 1337
+
+echo "Testing L2..."
+ip link set lo address 02:01:03:04:05:06
+
+# Drops all packets coming from forged localhost mac
+tc filter add dev lo parent ffff: protocol ip pref 1337 flower \
+	src_mac 02:01:03:04:05:06 action drop
+
+# Send packets from any port. Filter should drop all.
+./test_flow_dissector -i 4 -f 8 -F
+
+tc filter del dev lo ingress pref 1337
+
+# Drops all packets coming from "random" non-localhost mac
+tc filter add dev lo parent ffff: protocol ip pref 1337 flower \
+	src_mac 02:01:03:04:05:07 action drop
+
+# Send packets from any port. Filter should not drop any.
+./test_flow_dissector -i 4 -f 8
+
+tc filter del dev lo ingress pref 1337
+
 exit 0
-- 
2.21.0.1020.gf2820cf01a-goog

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Willem de Bruijn]

On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> If we have a flow dissector BPF program attached to the namespace,
> FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.

I suppose that this is true for a variety of keys? For instance, also
FLOW_DISSECTOR_KEY_IPV4_ADDRS.

We originally intended BPF flow dissection for all paths except
tc_flower. As that catches all the vulnerable cases on the ingress
path on the one hand and it is infeasible to support all the
flower features, now and future. I think that is the real fix.

>
> Handle FLOW_DISSECTOR_KEY_ETH_ADDRS before BPF and only if we have
> an skb (used by tc-flower only).
>
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  net/core/flow_dissector.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 9ca784c592ac..ba76d9168c8b 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -825,6 +825,18 @@ bool __skb_flow_dissect(const struct net *net,
>                         else if (skb->sk)
>                                 net = sock_net(skb->sk);
>                 }
> +
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> +                       struct ethhdr *eth = eth_hdr(skb);

Here as well as in the original patch: is it safe to just cast to
eth_hdr? In the same file, __skb_flow_dissect_gre does test for
(encapsulated) protocol first.

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Stanislav Fomichev]

On 05/13, Willem de Bruijn wrote:
> On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > If we have a flow dissector BPF program attached to the namespace,
> > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> 
> I suppose that this is true for a variety of keys? For instance, also
> FLOW_DISSECTOR_KEY_IPV4_ADDRS.
I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
without any esoteric protocols. Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).

> We originally intended BPF flow dissection for all paths except
> tc_flower. As that catches all the vulnerable cases on the ingress
> path on the one hand and it is infeasible to support all the
> flower features, now and future. I think that is the real fix.
Sorry, didn't get what you meant by the real fix.
Don't care about tc_flower? Just support a minimal set of features
needed by selftests?

> >
> > Handle FLOW_DISSECTOR_KEY_ETH_ADDRS before BPF and only if we have
> > an skb (used by tc-flower only).
> >
> > Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  net/core/flow_dissector.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 9ca784c592ac..ba76d9168c8b 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -825,6 +825,18 @@ bool __skb_flow_dissect(const struct net *net,
> >                         else if (skb->sk)
> >                                 net = sock_net(skb->sk);
> >                 }
> > +
> > +               if (dissector_uses_key(flow_dissector,
> > +                                      FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> > +                       struct ethhdr *eth = eth_hdr(skb);
> 
> Here as well as in the original patch: is it safe to just cast to
> eth_hdr? In the same file, __skb_flow_dissect_gre does test for
> (encapsulated) protocol first.
Good question, I guess the assumption here is that
FLOW_DISSECTOR_KEY_ETH_ADDRS is only used by tc_flower and the appropriate
checks should be there as well.
It's probably better to check skb->proto here though.

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Willem de Bruijn]

On Mon, May 13, 2019 at 5:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 05/13, Willem de Bruijn wrote:
> > On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > If we have a flow dissector BPF program attached to the namespace,
> > > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> >
> > I suppose that this is true for a variety of keys? For instance, also
> > FLOW_DISSECTOR_KEY_IPV4_ADDRS.

> I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
> without any esoteric protocols.

Indeed. But this applies both to protocols and the feature set. Both
are more limited.

> Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).

Ah, I chose a bad example then.

> > We originally intended BPF flow dissection for all paths except
> > tc_flower. As that catches all the vulnerable cases on the ingress
> > path on the one hand and it is infeasible to support all the
> > flower features, now and future. I think that is the real fix.

> Sorry, didn't get what you meant by the real fix.
> Don't care about tc_flower? Just support a minimal set of features
> needed by selftests?

I do mean exclude BPF flow dissector (only) for tc_flower, as we
cannot guarantee that the BPF program can fully implement the
requested feature.

>
> > >
> > > Handle FLOW_DISSECTOR_KEY_ETH_ADDRS before BPF and only if we have
> > > an skb (used by tc-flower only).
> > >
> > > Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > >  net/core/flow_dissector.c | 23 ++++++++++++-----------
> > >  1 file changed, 12 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > > index 9ca784c592ac..ba76d9168c8b 100644
> > > --- a/net/core/flow_dissector.c
> > > +++ b/net/core/flow_dissector.c
> > > @@ -825,6 +825,18 @@ bool __skb_flow_dissect(const struct net *net,
> > >                         else if (skb->sk)
> > >                                 net = sock_net(skb->sk);
> > >                 }
> > > +
> > > +               if (dissector_uses_key(flow_dissector,
> > > +                                      FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> > > +                       struct ethhdr *eth = eth_hdr(skb);
> >
> > Here as well as in the original patch: is it safe to just cast to
> > eth_hdr? In the same file, __skb_flow_dissect_gre does test for
> > (encapsulated) protocol first.

> Good question, I guess the assumption here is that
> FLOW_DISSECTOR_KEY_ETH_ADDRS is only used by tc_flower and the appropriate
> checks should be there as well.
> It's probably better to check skb->proto here though.

Right, as a mistaken or malicious admin can request it on a non
Ethernet device and read garbage.

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Willem de Bruijn]

On Mon, May 13, 2019 at 5:21 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, May 13, 2019 at 5:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 05/13, Willem de Bruijn wrote:
> > > On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> > > >
> > > > If we have a flow dissector BPF program attached to the namespace,
> > > > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> > >
> > > I suppose that this is true for a variety of keys? For instance, also
> > > FLOW_DISSECTOR_KEY_IPV4_ADDRS.
>
> > I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
> > without any esoteric protocols.
>
> Indeed. But this applies both to protocols and the feature set. Both
> are more limited.
>
> > Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> > looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).
>
> Ah, I chose a bad example then.
>
> > > We originally intended BPF flow dissection for all paths except
> > > tc_flower. As that catches all the vulnerable cases on the ingress
> > > path on the one hand and it is infeasible to support all the
> > > flower features, now and future. I think that is the real fix.
>
> > Sorry, didn't get what you meant by the real fix.
> > Don't care about tc_flower? Just support a minimal set of features
> > needed by selftests?
>
> I do mean exclude BPF flow dissector (only) for tc_flower, as we
> cannot guarantee that the BPF program can fully implement the
> requested feature.

Though, the user inserting the BPF flow dissector is the same as the
user inserting the flower program, the (per netns) admin. So arguably
is aware of the constraints incurred by BPF flow dissection. And else
can still detect when a feature is not supported from the (lack of)
output, as in this case of Ethernet address. I don't think we want to
mix BPF and non-BPF flow dissection though. That subverts the safety
argument of switching to BPF for flow dissection.

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Stanislav Fomichev]

On 05/13, Willem de Bruijn wrote:
> On Mon, May 13, 2019 at 5:21 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Mon, May 13, 2019 at 5:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 05/13, Willem de Bruijn wrote:
> > > > On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> > > > >
> > > > > If we have a flow dissector BPF program attached to the namespace,
> > > > > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> > > >
> > > > I suppose that this is true for a variety of keys? For instance, also
> > > > FLOW_DISSECTOR_KEY_IPV4_ADDRS.
> >
> > > I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
> > > without any esoteric protocols.
> >
> > Indeed. But this applies both to protocols and the feature set. Both
> > are more limited.
> >
> > > Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> > > looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).
> >
> > Ah, I chose a bad example then.
> >
> > > > We originally intended BPF flow dissection for all paths except
> > > > tc_flower. As that catches all the vulnerable cases on the ingress
> > > > path on the one hand and it is infeasible to support all the
> > > > flower features, now and future. I think that is the real fix.
> >
> > > Sorry, didn't get what you meant by the real fix.
> > > Don't care about tc_flower? Just support a minimal set of features
> > > needed by selftests?
> >
> > I do mean exclude BPF flow dissector (only) for tc_flower, as we
> > cannot guarantee that the BPF program can fully implement the
> > requested feature.
> 
> Though, the user inserting the BPF flow dissector is the same as the
> user inserting the flower program, the (per netns) admin. So arguably
> is aware of the constraints incurred by BPF flow dissection. And else
> can still detect when a feature is not supported from the (lack of)
> output, as in this case of Ethernet address. I don't think we want to
> mix BPF and non-BPF flow dissection though. That subverts the safety
> argument of switching to BPF for flow dissection.
Yes, we cannot completely avoid tc_flower because we use it to do
the end-to-end testing. That's why I was trying to make sure "basic"
stuff works (it might feel confusing that tc_flower {src,dst}_mac
stop working with a bpf program installed).

TBH, I'd not call this particular piece of code that exports src/dst
addresses a dissection. At this point, it's a well-formed skb with
a proper l2 header and we just copy the addresses out. It's probably
part of the reason the original patch didn't include any skb->protocol
checks.

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Stanislav Fomichev]

> On 05/13, Willem de Bruijn wrote:
> > On Mon, May 13, 2019 at 5:21 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Mon, May 13, 2019 at 5:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > On 05/13, Willem de Bruijn wrote:
> > > > > On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> > > > > >
> > > > > > If we have a flow dissector BPF program attached to the namespace,
> > > > > > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> > > > >
> > > > > I suppose that this is true for a variety of keys? For instance, also
> > > > > FLOW_DISSECTOR_KEY_IPV4_ADDRS.
> > >
> > > > I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
> > > > without any esoteric protocols.
> > >
> > > Indeed. But this applies both to protocols and the feature set. Both
> > > are more limited.
> > >
> > > > Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> > > > looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).
> > >
> > > Ah, I chose a bad example then.
> > >
> > > > > We originally intended BPF flow dissection for all paths except
> > > > > tc_flower. As that catches all the vulnerable cases on the ingress
> > > > > path on the one hand and it is infeasible to support all the
> > > > > flower features, now and future. I think that is the real fix.
> > >
> > > > Sorry, didn't get what you meant by the real fix.
> > > > Don't care about tc_flower? Just support a minimal set of features
> > > > needed by selftests?
> > >
> > > I do mean exclude BPF flow dissector (only) for tc_flower, as we
> > > cannot guarantee that the BPF program can fully implement the
> > > requested feature.
> >
> > Though, the user inserting the BPF flow dissector is the same as the
> > user inserting the flower program, the (per netns) admin. So arguably
> > is aware of the constraints incurred by BPF flow dissection. And else
> > can still detect when a feature is not supported from the (lack of)
> > output, as in this case of Ethernet address. I don't think we want to
> > mix BPF and non-BPF flow dissection though. That subverts the safety
> > argument of switching to BPF for flow dissection.
> Yes, we cannot completely avoid tc_flower because we use it to do
> the end-to-end testing. That's why I was trying to make sure "basic"
> stuff works (it might feel confusing that tc_flower {src,dst}_mac
> stop working with a bpf program installed).
>
> TBH, I'd not call this particular piece of code that exports src/dst
> addresses a dissection. At this point, it's a well-formed skb with
> a proper l2 header and we just copy the addresses out. It's probably
> part of the reason the original patch didn't include any skb->protocol
> checks.
On the other hand, we can probably follow a simple rule:
if it's not exported via bpf_flow_keys (and src/dsc mac is not),
tc_flower is not supported as well.

Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

[Willem de Bruijn]

From: Stanislav Fomichev <sdf@google.com>
Date: Mon, May 13, 2019 at 7:21 PM
To: Stanislav Fomichev
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>, Network
Development, bpf, David Miller, Alexei Starovoitov, Daniel Borkmann,
Willem de Bruijn <willemb@google.com>, Petar Penkov

> > On 05/13, Willem de Bruijn wrote:
> > > On Mon, May 13, 2019 at 5:21 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > On Mon, May 13, 2019 at 5:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > > >
> > > > > On 05/13, Willem de Bruijn wrote:
> > > > > > On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> > > > > > >
> > > > > > > If we have a flow dissector BPF program attached to the namespace,
> > > > > > > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> > > > > >
> > > > > > I suppose that this is true for a variety of keys? For instance, also
> > > > > > FLOW_DISSECTOR_KEY_IPV4_ADDRS.
> > > >
> > > > > I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
> > > > > without any esoteric protocols.
> > > >
> > > > Indeed. But this applies both to protocols and the feature set. Both
> > > > are more limited.
> > > >
> > > > > Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> > > > > looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).
> > > >
> > > > Ah, I chose a bad example then.
> > > >
> > > > > > We originally intended BPF flow dissection for all paths except
> > > > > > tc_flower. As that catches all the vulnerable cases on the ingress
> > > > > > path on the one hand and it is infeasible to support all the
> > > > > > flower features, now and future. I think that is the real fix.
> > > >
> > > > > Sorry, didn't get what you meant by the real fix.
> > > > > Don't care about tc_flower? Just support a minimal set of features
> > > > > needed by selftests?
> > > >
> > > > I do mean exclude BPF flow dissector (only) for tc_flower, as we
> > > > cannot guarantee that the BPF program can fully implement the
> > > > requested feature.
> > >
> > > Though, the user inserting the BPF flow dissector is the same as the
> > > user inserting the flower program, the (per netns) admin. So arguably
> > > is aware of the constraints incurred by BPF flow dissection. And else
> > > can still detect when a feature is not supported from the (lack of)
> > > output, as in this case of Ethernet address. I don't think we want to
> > > mix BPF and non-BPF flow dissection though. That subverts the safety
> > > argument of switching to BPF for flow dissection.
> > Yes, we cannot completely avoid tc_flower because we use it to do
> > the end-to-end testing. That's why I was trying to make sure "basic"
> > stuff works (it might feel confusing that tc_flower {src,dst}_mac
> > stop working with a bpf program installed).
> >
> > TBH, I'd not call this particular piece of code that exports src/dst
> > addresses a dissection. At this point, it's a well-formed skb with
> > a proper l2 header and we just copy the addresses out. It's probably
> > part of the reason the original patch didn't include any skb->protocol
> > checks.

But it is not guaranteed to be an Ethernet link layer device. Making
this a good example of why when moving to BPF for safety we should not
keep any C dissection code in the path at all.

> On the other hand, we can probably follow a simple rule:
> if it's not exported via bpf_flow_keys (and src/dsc mac is not),
> tc_flower is not supported as well.

Agreed. I was using that as point of reference just now, too.