[PATCH bpf-next 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect

[PATCH bpf-next 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect

[Stanislav Fomichev]

To avoid potentially breaking existing users.

Both mac/no-mac cases have to be amended; mac_header >= network_header
is not enough (verified with a new test, see next patch).

Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/bpf/test_run.c |  3 ---
 net/core/filter.c  | 11 ++++++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 13d578ce2a09..6fba440efc40 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -979,9 +979,6 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
 {
 	struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb;
 
-	if (!skb->len)
-		return -EINVAL;
-
 	if (!__skb)
 		return 0;
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 6dd2baf5eeb2..910ccd416465 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2124,12 +2124,13 @@ static int __bpf_redirect_no_mac(struct sk_buff *skb, struct net_device *dev,
 {
 	unsigned int mlen = skb_network_offset(skb);
 
+	if (unlikely(skb->len <= mlen)) {
+		kfree_skb(skb);
+		return -ERANGE;
+	}
+
 	if (mlen) {
 		__skb_pull(skb, mlen);
-		if (unlikely(!skb->len)) {
-			kfree_skb(skb);
-			return -ERANGE;
-		}
 
 		/* At ingress, the mac header has already been pulled once.
 		 * At egress, skb_pospull_rcsum has to be done in case that
@@ -2149,7 +2150,7 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
 				 u32 flags)
 {
 	/* Verify that a link layer header is carried */
-	if (unlikely(skb->mac_header >= skb->network_header)) {
+	if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
 		kfree_skb(skb);
 		return -ERANGE;
 	}
-- 
2.38.1.431.g37b22c650d-goog

[PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Stanislav Fomichev]

LWT_XMIT to test L3 case, TC to test L2 case.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../selftests/bpf/prog_tests/empty_skb.c      | 140 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/empty_skb.c |  37 +++++
 2 files changed, 177 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/empty_skb.c
 create mode 100644 tools/testing/selftests/bpf/progs/empty_skb.c

diff --git a/tools/testing/selftests/bpf/prog_tests/empty_skb.c b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
new file mode 100644
index 000000000000..6e35739babed
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <net/if.h>
+#include "empty_skb.skel.h"
+
+#define SYS(cmd) ({ \
+	if (!ASSERT_OK(system(cmd), (cmd))) \
+		goto out; \
+})
+
+void test_empty_skb(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, tattr);
+	struct empty_skb *bpf_obj = NULL;
+	struct nstoken *tok = NULL;
+	struct bpf_program *prog;
+	char eth_hlen_pp[15];
+	char eth_hlen[14];
+	int veth_ifindex;
+	int ipip_ifindex;
+	int err;
+	int i;
+
+	struct {
+		const char *msg;
+		const void *data_in;
+		__u32 data_size_in;
+		int *ifindex;
+		int err;
+		int ret;
+		bool success_on_tc;
+	} tests[] = {
+		/* Empty packets are always rejected. */
+
+		{
+			.msg = "veth empty ingress packet",
+			.data_in = NULL,
+			.data_size_in = 0,
+			.ifindex = &veth_ifindex,
+			.err = -EINVAL,
+		},
+		{
+			.msg = "ipip empty ingress packet",
+			.data_in = NULL,
+			.data_size_in = 0,
+			.ifindex = &ipip_ifindex,
+			.err = -EINVAL,
+		},
+
+		/* ETH_HLEN-sized packets:
+		 * - can not be redirected at LWT_XMIT
+		 * - can be redirected at TC
+		 */
+
+		{
+			.msg = "veth ETH_HLEN packet ingress",
+			.data_in = eth_hlen,
+			.data_size_in = sizeof(eth_hlen),
+			.ifindex = &veth_ifindex,
+			.ret = -ERANGE,
+			.success_on_tc = true,
+		},
+		{
+			.msg = "ipip ETH_HLEN packet ingress",
+			.data_in = eth_hlen,
+			.data_size_in = sizeof(eth_hlen),
+			.ifindex = &veth_ifindex,
+			.ret = -ERANGE,
+			.success_on_tc = true,
+		},
+
+		/* ETH_HLEN+1-sized packet should be redirected. */
+
+		{
+			.msg = "veth ETH_HLEN+1 packet ingress",
+			.data_in = eth_hlen_pp,
+			.data_size_in = sizeof(eth_hlen_pp),
+			.ifindex = &veth_ifindex,
+		},
+		{
+			.msg = "ipip ETH_HLEN+1 packet ingress",
+			.data_in = eth_hlen_pp,
+			.data_size_in = sizeof(eth_hlen_pp),
+			.ifindex = &veth_ifindex,
+		},
+
+	};
+
+	SYS("ip netns add empty_skb");
+	tok = open_netns("empty_skb");
+	SYS("ip link add veth0 type veth peer veth1");
+	SYS("ip link set dev veth0 up");
+	SYS("ip link set dev veth1 up");
+	SYS("ip addr add 10.0.0.1/8 dev veth0");
+	SYS("ip addr add 10.0.0.2/8 dev veth1");
+	veth_ifindex = if_nametoindex("veth0");
+
+	SYS("ip link add ipip0 type ipip local 10.0.0.1 remote 10.0.0.2");
+	SYS("ip link set ipip0 up");
+	SYS("ip addr add 192.168.1.1/16 dev ipip0");
+	ipip_ifindex = if_nametoindex("ipip0");
+
+	bpf_obj = empty_skb__open_and_load();
+	if (!ASSERT_OK_PTR(bpf_obj, "open skeleton"))
+		goto out;
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		bpf_object__for_each_program(prog, bpf_obj->obj) {
+			char buf[128];
+			bool at_tc = !strncmp(bpf_program__section_name(prog), "tc", 2);
+
+			tattr.data_in = tests[i].data_in;
+			tattr.data_size_in = tests[i].data_size_in;
+
+			tattr.data_size_out = 0;
+			bpf_obj->bss->ifindex = *tests[i].ifindex;
+			bpf_obj->bss->ret = 0;
+			err = bpf_prog_test_run_opts(bpf_program__fd(prog), &tattr);
+			sprintf(buf, "err: %s [%s]", tests[i].msg, bpf_program__name(prog));
+
+			if (at_tc && tests[i].success_on_tc)
+				ASSERT_GE(err, 0, buf);
+			else
+				ASSERT_EQ(err, tests[i].err, buf);
+			sprintf(buf, "ret: %s [%s]", tests[i].msg, bpf_program__name(prog));
+			if (at_tc && tests[i].success_on_tc)
+				ASSERT_GE(bpf_obj->bss->ret, 0, buf);
+			else
+				ASSERT_EQ(bpf_obj->bss->ret, tests[i].ret, buf);
+		}
+	}
+
+out:
+	if (bpf_obj)
+		empty_skb__destroy(bpf_obj);
+	if (tok)
+		close_netns(tok);
+	system("ip netns del empty_skb");
+}
diff --git a/tools/testing/selftests/bpf/progs/empty_skb.c b/tools/testing/selftests/bpf/progs/empty_skb.c
new file mode 100644
index 000000000000..4b0cd6753251
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/empty_skb.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+char _license[] SEC("license") = "GPL";
+
+int ifindex;
+int ret;
+
+SEC("lwt_xmit")
+int redirect_ingress(struct __sk_buff *skb)
+{
+	ret = bpf_clone_redirect(skb, ifindex, BPF_F_INGRESS);
+	return 0;
+}
+
+SEC("lwt_xmit")
+int redirect_egress(struct __sk_buff *skb)
+{
+	ret = bpf_clone_redirect(skb, ifindex, 0);
+	return 0;
+}
+
+SEC("tc")
+int tc_redirect_ingress(struct __sk_buff *skb)
+{
+	ret = bpf_clone_redirect(skb, ifindex, BPF_F_INGRESS);
+	return 0;
+}
+
+SEC("tc")
+int tc_redirect_egress(struct __sk_buff *skb)
+{
+	ret = bpf_clone_redirect(skb, ifindex, 0);
+	return 0;
+}
-- 
2.38.1.431.g37b22c650d-goog

Re: [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Martin KaFai Lau]

On 11/14/22 7:10 PM, Stanislav Fomichev wrote:
> LWT_XMIT to test L3 case, TC to test L2 case.

It will be useful to add more details here to explain which test is testing the 
skb->len check in __bpf_redirect_no_mac() and __bpf_redirect_common() in patch 1.

> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>   .../selftests/bpf/prog_tests/empty_skb.c      | 140 ++++++++++++++++++
>   tools/testing/selftests/bpf/progs/empty_skb.c |  37 +++++
>   2 files changed, 177 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/empty_skb.c
>   create mode 100644 tools/testing/selftests/bpf/progs/empty_skb.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/empty_skb.c b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> new file mode 100644
> index 000000000000..6e35739babed
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include <net/if.h>
> +#include "empty_skb.skel.h"
> +
> +#define SYS(cmd) ({ \
> +	if (!ASSERT_OK(system(cmd), (cmd))) \
> +		goto out; \
> +})
> +
> +void test_empty_skb(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, tattr);
> +	struct empty_skb *bpf_obj = NULL;
> +	struct nstoken *tok = NULL;
> +	struct bpf_program *prog;
> +	char eth_hlen_pp[15];
> +	char eth_hlen[14];
> +	int veth_ifindex;
> +	int ipip_ifindex;
> +	int err;
> +	int i;
> +
> +	struct {
> +		const char *msg;
> +		const void *data_in;
> +		__u32 data_size_in;
> +		int *ifindex;
> +		int err;
> +		int ret;
> +		bool success_on_tc;
> +	} tests[] = {
> +		/* Empty packets are always rejected. */
> +
> +		{
> +			.msg = "veth empty ingress packet",
> +			.data_in = NULL,
> +			.data_size_in = 0,
> +			.ifindex = &veth_ifindex,
> +			.err = -EINVAL,
> +		},
> +		{
> +			.msg = "ipip empty ingress packet",
> +			.data_in = NULL,
> +			.data_size_in = 0,
> +			.ifindex = &ipip_ifindex,
> +			.err = -EINVAL,
> +		},


> +
> +		/* ETH_HLEN-sized packets:
> +		 * - can not be redirected at LWT_XMIT
> +		 * - can be redirected at TC
> +		 */
> +
> +		{
> +			.msg = "veth ETH_HLEN packet ingress",
> +			.data_in = eth_hlen,
> +			.data_size_in = sizeof(eth_hlen),
> +			.ifindex = &veth_ifindex,
> +			.ret = -ERANGE,
> +			.success_on_tc = true,
> +		},
> +		{
> +			.msg = "ipip ETH_HLEN packet ingress",
> +			.data_in = eth_hlen,
> +			.data_size_in = sizeof(eth_hlen),
> +			.ifindex = &veth_ifindex,
> +			.ret = -ERANGE,
> +			.success_on_tc = true,
> +		},


hmm... these two tests don't look right.  They are the same except the ".msg" 
part.  The latter one should use &ipip_ifindex?

> +
> +		/* ETH_HLEN+1-sized packet should be redirected. */
> +
> +		{
> +			.msg = "veth ETH_HLEN+1 packet ingress",
> +			.data_in = eth_hlen_pp,
> +			.data_size_in = sizeof(eth_hlen_pp),
> +			.ifindex = &veth_ifindex,
> +		},
> +		{
> +			.msg = "ipip ETH_HLEN+1 packet ingress",
> +			.data_in = eth_hlen_pp,
> +			.data_size_in = sizeof(eth_hlen_pp),
> +			.ifindex = &veth_ifindex,
> +		},

Same here.

Re: [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable

[Stanislav Fomichev]

On Wed, Nov 16, 2022 at 12:38 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 11/14/22 7:10 PM, Stanislav Fomichev wrote:
> > LWT_XMIT to test L3 case, TC to test L2 case.
>
> It will be useful to add more details here to explain which test is testing the
> skb->len check in __bpf_redirect_no_mac() and __bpf_redirect_common() in patch 1.

SG, will try to annotate.

> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >   .../selftests/bpf/prog_tests/empty_skb.c      | 140 ++++++++++++++++++
> >   tools/testing/selftests/bpf/progs/empty_skb.c |  37 +++++
> >   2 files changed, 177 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/empty_skb.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/empty_skb.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/empty_skb.c b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> > new file mode 100644
> > index 000000000000..6e35739babed
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/empty_skb.c
> > @@ -0,0 +1,140 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <test_progs.h>
> > +#include <network_helpers.h>
> > +#include <net/if.h>
> > +#include "empty_skb.skel.h"
> > +
> > +#define SYS(cmd) ({ \
> > +     if (!ASSERT_OK(system(cmd), (cmd))) \
> > +             goto out; \
> > +})
> > +
> > +void test_empty_skb(void)
> > +{
> > +     LIBBPF_OPTS(bpf_test_run_opts, tattr);
> > +     struct empty_skb *bpf_obj = NULL;
> > +     struct nstoken *tok = NULL;
> > +     struct bpf_program *prog;
> > +     char eth_hlen_pp[15];
> > +     char eth_hlen[14];
> > +     int veth_ifindex;
> > +     int ipip_ifindex;
> > +     int err;
> > +     int i;
> > +
> > +     struct {
> > +             const char *msg;
> > +             const void *data_in;
> > +             __u32 data_size_in;
> > +             int *ifindex;
> > +             int err;
> > +             int ret;
> > +             bool success_on_tc;
> > +     } tests[] = {
> > +             /* Empty packets are always rejected. */
> > +
> > +             {
> > +                     .msg = "veth empty ingress packet",
> > +                     .data_in = NULL,
> > +                     .data_size_in = 0,
> > +                     .ifindex = &veth_ifindex,
> > +                     .err = -EINVAL,
> > +             },
> > +             {
> > +                     .msg = "ipip empty ingress packet",
> > +                     .data_in = NULL,
> > +                     .data_size_in = 0,
> > +                     .ifindex = &ipip_ifindex,
> > +                     .err = -EINVAL,
> > +             },
>
>
> > +
> > +             /* ETH_HLEN-sized packets:
> > +              * - can not be redirected at LWT_XMIT
> > +              * - can be redirected at TC
> > +              */
> > +
> > +             {
> > +                     .msg = "veth ETH_HLEN packet ingress",
> > +                     .data_in = eth_hlen,
> > +                     .data_size_in = sizeof(eth_hlen),
> > +                     .ifindex = &veth_ifindex,
> > +                     .ret = -ERANGE,
> > +                     .success_on_tc = true,
> > +             },
> > +             {
> > +                     .msg = "ipip ETH_HLEN packet ingress",
> > +                     .data_in = eth_hlen,
> > +                     .data_size_in = sizeof(eth_hlen),
> > +                     .ifindex = &veth_ifindex,
> > +                     .ret = -ERANGE,
> > +                     .success_on_tc = true,
> > +             },
>
>
> hmm... these two tests don't look right.  They are the same except the ".msg"
> part.  The latter one should use &ipip_ifindex?

Oh, good catch!

> > +
> > +             /* ETH_HLEN+1-sized packet should be redirected. */
> > +
> > +             {
> > +                     .msg = "veth ETH_HLEN+1 packet ingress",
> > +                     .data_in = eth_hlen_pp,
> > +                     .data_size_in = sizeof(eth_hlen_pp),
> > +                     .ifindex = &veth_ifindex,
> > +             },
> > +             {
> > +                     .msg = "ipip ETH_HLEN+1 packet ingress",
> > +                     .data_in = eth_hlen_pp,
> > +                     .data_size_in = sizeof(eth_hlen_pp),
> > +                     .ifindex = &veth_ifindex,
> > +             },
>
> Same here.
>