* [PATCH bpf-next 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect
@ 2022-11-15 3:10 Stanislav Fomichev
2022-11-15 3:10 ` [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable Stanislav Fomichev
0 siblings, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2022-11-15 3:10 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable
2022-11-15 3:10 [PATCH bpf-next 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect Stanislav Fomichev
@ 2022-11-15 3:10 ` Stanislav Fomichev
2022-11-16 20:38 ` Martin KaFai Lau
0 siblings, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2022-11-15 3:10 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable
2022-11-15 3:10 ` [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable Stanislav Fomichev
@ 2022-11-16 20:38 ` Martin KaFai Lau
2022-11-16 21:02 ` Stanislav Fomichev
0 siblings, 1 reply; 4+ messages in thread
From: Martin KaFai Lau @ 2022-11-16 20:38 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, song, yhs, john.fastabend, kpsingh,
haoluo, jolsa
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable
2022-11-16 20:38 ` Martin KaFai Lau
@ 2022-11-16 21:02 ` Stanislav Fomichev
0 siblings, 0 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2022-11-16 21:02 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, ast, daniel, andrii, song, yhs, john.fastabend, kpsingh,
haoluo, jolsa
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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-16 21:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-15 3:10 [PATCH bpf-next 1/2] bpf: Move skb->len == 0 checks into __bpf_redirect Stanislav Fomichev
2022-11-15 3:10 ` [PATCH bpf-next 2/2] selftests/bpf: Make sure zero-len skbs aren't redirectable Stanislav Fomichev
2022-11-16 20:38 ` Martin KaFai Lau
2022-11-16 21:02 ` Stanislav Fomichev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.