| Message ID | 20220715115559.139691-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fd1894224407c484f652ad456e1ce423e89bb3eb |
| Delegated to: | BPF |
| Headers | show |
| Series | [v4,bpf-next] bpf: Don't redirect packets with invalid pkt_len | expand |
On Fri, Jul 15, 2022 at 4:51 AM Zhengchao Shao <shaozhengchao@huawei.com> wrote: > > Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any > skbs, that is, the flow->head is null. > The root cause, as the [2] says, is because that bpf_prog_test_run_skb() > run a bpf prog which redirects empty skbs. > So we should determine whether the length of the packet modified by bpf > prog or others like bpf_prog_test is valid before forwarding it directly. > > LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 > LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html > > Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> Reviewed-by: Stanislav Fomichev <sdf@google.com> Looks like it addresses everything? > --- > v3: modify debug print > v2: need move checking to convert___skb_to_skb and add debug info > v1: should not check len in fast path > > include/linux/skbuff.h | 8 ++++++++ > net/bpf/test_run.c | 3 +++ > net/core/dev.c | 1 + > 3 files changed, 12 insertions(+) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index f6a27ab19202..82e8368ba6e6 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) > > #endif /* NET_SKBUFF_DATA_USES_OFFSET */ > > +static inline void skb_assert_len(struct sk_buff *skb) > +{ > +#ifdef CONFIG_DEBUG_NET > + if (WARN_ONCE(!skb->len, "%s\n", __func__)) > + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); > +#endif /* CONFIG_DEBUG_NET */ > +} > + > /* > * Add data to an sk_buff > */ > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 2ca96acbc50a..dc9dc0bedca0 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -955,6 +955,9 @@ 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/dev.c b/net/core/dev.c > index d588fd0a54ce..716df64fcfa5 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > bool again = false; > > skb_reset_mac_header(skb); > + skb_assert_len(skb); > > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) > __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED); > -- > 2.17.1 >
Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Fri, 15 Jul 2022 19:55:59 +0800 you wrote: > Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any > skbs, that is, the flow->head is null. > The root cause, as the [2] says, is because that bpf_prog_test_run_skb() > run a bpf prog which redirects empty skbs. > So we should determine whether the length of the packet modified by bpf > prog or others like bpf_prog_test is valid before forwarding it directly. > > [...] Here is the summary with links: - [v4,bpf-next] bpf: Don't redirect packets with invalid pkt_len https://git.kernel.org/bpf/bpf-next/c/fd1894224407 You are awesome, thank you!
Hi, I think this patch is causing user-space breakage, see [0]. The gist is that we do BPF_PROG_RUN of a socket filter with 14 byte input to determine whether BPF_PROG_RUN is available or not. I'll fix this in cilium/ebpf, but I think this patch needs more work since users may be doing the same thing in their code. Thanks, Lorenz 0: https://github.com/cilium/ebpf/pull/788
On Wed, Sep 14, 2022 at 4:20 AM Lorenz Bauer <oss@lmb.io> wrote: > > Hi, > > I think this patch is causing user-space breakage, see [0]. > > The gist is that we do BPF_PROG_RUN of a socket filter with 14 byte input to determine whether > BPF_PROG_RUN is available or not. I'll fix this in cilium/ebpf, but I think this patch > needs more work since users may be doing the same thing in their code. Ooops, sorry about that. Instead of rejecting len=0 data, we might accept the packet but add some safe header? I think that should be more backwards compatible? Zhengchao, something you can look into? > Thanks, > Lorenz > > 0: https://github.com/cilium/ebpf/pull/788
On 2022/9/17 23:46, Stanislav Fomichev wrote: > On Wed, Sep 14, 2022 at 4:20 AM Lorenz Bauer <oss@lmb.io> wrote: >> >> Hi, >> >> I think this patch is causing user-space breakage, see [0]. >> >> The gist is that we do BPF_PROG_RUN of a socket filter with 14 byte input to determine whether >> BPF_PROG_RUN is available or not. I'll fix this in cilium/ebpf, but I think this patch >> needs more work since users may be doing the same thing in their code. > > Ooops, sorry about that. > > Instead of rejecting len=0 data, we might accept the packet but add > some safe header? I think that should be more backwards compatible? > Zhengchao, something you can look into? > > Sorry for the delay. I'm busy testing the TC module recently. I'm very sorry for the user-space breakage. The root cause of this problem is that eth_type_trans() is called when the protocol type of the SKB is parsed. The len value of the SKB is reduced to 0. If the user mode requires that the forwarding succeed, or if the MAC header is added again after the MAC header is subtracted, is this appropriate? Zhengchao Shao >> Thanks, >> Lorenz >> >> 0: https://github.com/cilium/ebpf/pull/788
On Mon, 19 Sep 2022, at 11:55, shaozhengchao wrote: > Sorry for the delay. I'm busy testing the TC module recently. I'm very > sorry for the user-space breakage. > > The root cause of this problem is that eth_type_trans() is called when > the protocol type of the SKB is parsed. The len value of the SKB is > reduced to 0. If the user mode requires that the forwarding succeed, or > if the MAC header is added again after the MAC header is subtracted, > is this appropriate? We don't require forwarding to succeed with a 14 byte input buffer. We also don't look at the MAC header. I think refusing to forward 0 length packets would be OK. Not 100% certain I understood you correctly, let me know if this helps. Best Lorenz
On 2022/9/20 22:42, Lorenz Bauer wrote: > On Mon, 19 Sep 2022, at 11:55, shaozhengchao wrote: >> Sorry for the delay. I'm busy testing the TC module recently. I'm very >> sorry for the user-space breakage. >> >> The root cause of this problem is that eth_type_trans() is called when >> the protocol type of the SKB is parsed. The len value of the SKB is >> reduced to 0. If the user mode requires that the forwarding succeed, or >> if the MAC header is added again after the MAC header is subtracted, >> is this appropriate? > > We don't require forwarding to succeed with a 14 byte input buffer. We also don't look at the MAC header. > > I think refusing to forward 0 length packets would be OK. Not 100% certain I understood you correctly, let me know if this helps. > > Best > Lorenz Hi Lorenz Sorry. But how does the rejection of the 0 length affect the test case? Is the return value abnormal, send packet failure or some others? Zhengchao Shao
On Wed, Sep 21, 2022 at 1:48 AM shaozhengchao <shaozhengchao@huawei.com> wrote: > > > > On 2022/9/20 22:42, Lorenz Bauer wrote: > > On Mon, 19 Sep 2022, at 11:55, shaozhengchao wrote: > >> Sorry for the delay. I'm busy testing the TC module recently. I'm very > >> sorry for the user-space breakage. > >> > >> The root cause of this problem is that eth_type_trans() is called when > >> the protocol type of the SKB is parsed. The len value of the SKB is > >> reduced to 0. If the user mode requires that the forwarding succeed, or > >> if the MAC header is added again after the MAC header is subtracted, > >> is this appropriate? > > > > We don't require forwarding to succeed with a 14 byte input buffer. We also don't look at the MAC header. > > > > I think refusing to forward 0 length packets would be OK. Not 100% certain I understood you correctly, let me know if this helps. > > > > Best > > Lorenz > Hi Lorenz > Sorry. But how does the rejection of the 0 length affect the > test case? Is the return value abnormal, send packet failure or some > others? Sorry for the late reply, I'm still traveling. What we want to avoid is creating invalid skbs via prog_test_run. We can reject <14 byte packets with ENIVAL, but that might break some of the existing apps (as Lorenz reported). So it seems like a safer alternative would be to accept those packets, but just append missing bytes in the kernel to create the valid packets? Padding with zeros seems fine? > Zhengchao Shao
On Wed, 21 Sep 2022, at 09:48, shaozhengchao wrote: > Hi Lorenz > Sorry. But how does the rejection of the 0 length affect the > test case? Is the return value abnormal, send packet failure or some > others? Hi Zhengchao, Did you get around to submitting another fix for this? Best Lorenz
On 2022/10/13 17:36, Lorenz Bauer wrote:
> Did you get around to submitting another fix for this?
Hi Bauer:
Sorry, I haven't fully understood your intentions yet.
Can you explain it more detail?
Zhengchao Shao
On Thu, 13 Oct 2022, at 11:44, shaozhengchao wrote: > Sorry, I haven't fully understood your intentions yet. > Can you explain it more detail? I'll try! Roughly, we do the following: 1. Create a BPF_PROG_TYPE_SOCKET_FILTER program that just returns 0 2. Load the program into the kernel 3. Call BPF_PROG_RUN with data_size_in == 14 After your bugfix, it seems like step 3 is rejected due to data_size_in == 14. We had to increase data_size_in to 15 to avoid this, see [0]. This breaks user space, so it would be great if you could fix this in a way that doesn't refuse BPF_PROG_RUN with data_size_in == 14. Since I don't understand the original problem very well I can't tell you what the best fix is however. 0: https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0 Thanks Lorenz
On 10/14, Lorenz Bauer wrote: > On Thu, 13 Oct 2022, at 11:44, shaozhengchao wrote: > > Sorry, I haven't fully understood your intentions yet. > > Can you explain it more detail? > I'll try! Roughly, we do the following: > 1. Create a BPF_PROG_TYPE_SOCKET_FILTER program that just returns 0 > 2. Load the program into the kernel > 3. Call BPF_PROG_RUN with data_size_in == 14 > After your bugfix, it seems like step 3 is rejected due to data_size_in > == 14. We had to increase data_size_in to 15 to > avoid this, see [0]. > This breaks user space, so it would be great if you could fix this in a > way that doesn't refuse BPF_PROG_RUN with [..] > data_size_in == 14. Since I don't understand the original problem very > well I can't tell you what the best fix is however. The problem was that we were able to generate skb with len=0 via BPF_PROG_RUN. Prohibiting those cases breaks backwards compatibility, so we either have to: a) (preferred?) accept inputs with <14, but maybe internally pad to 14 bytes to make the core stack happy b) revert the patch and instead have length checks at runtime; doesn't seem to be worth the penalty in the forwarding path because of some corner cases like these ? > 0: > https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0 > Thanks > Lorenz
On 2022/10/15 0:55, sdf@google.com wrote: > On 10/14, Lorenz Bauer wrote: >> On Thu, 13 Oct 2022, at 11:44, shaozhengchao wrote: >> > Sorry, I haven't fully understood your intentions yet. >> > Can you explain it more detail? > >> I'll try! Roughly, we do the following: > >> 1. Create a BPF_PROG_TYPE_SOCKET_FILTER program that just returns 0 >> 2. Load the program into the kernel >> 3. Call BPF_PROG_RUN with data_size_in == 14 > >> After your bugfix, it seems like step 3 is rejected due to >> data_size_in == 14. We had to increase data_size_in to 15 to >> avoid this, see [0]. > >> This breaks user space, so it would be great if you could fix this in >> a way that doesn't refuse BPF_PROG_RUN with > > [..] > >> data_size_in == 14. Since I don't understand the original problem very >> well I can't tell you what the best fix is however. > > The problem was that we were able to generate skb with len=0 via > BPF_PROG_RUN. Prohibiting those cases breaks backwards compatibility, so > we either have to: > > a) (preferred?) accept inputs with <14, but maybe internally pad to 14 > bytes to make the core stack happy > b) revert the patch and instead have length checks at runtime; doesn't > seem to > be worth the penalty in the forwarding path because of some corner cases > like these ? > Hi sdf: a） looks better and I'll put up a patch as soon as possible to fix it. Zhengchao Shao > >> 0: >> https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0 > >> Thanks >> Lorenz
On 7/15/22 4:55 AM, Zhengchao Shao wrote: > Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any > skbs, that is, the flow->head is null. > The root cause, as the [2] says, is because that bpf_prog_test_run_skb() > run a bpf prog which redirects empty skbs. > So we should determine whether the length of the packet modified by bpf > prog or others like bpf_prog_test is valid before forwarding it directly. > > LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 > LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html > > Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > v3: modify debug print > v2: need move checking to convert___skb_to_skb and add debug info > v1: should not check len in fast path > > include/linux/skbuff.h | 8 ++++++++ > net/bpf/test_run.c | 3 +++ > net/core/dev.c | 1 + > 3 files changed, 12 insertions(+) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index f6a27ab19202..82e8368ba6e6 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) > > #endif /* NET_SKBUFF_DATA_USES_OFFSET */ > > +static inline void skb_assert_len(struct sk_buff *skb) > +{ > +#ifdef CONFIG_DEBUG_NET > + if (WARN_ONCE(!skb->len, "%s\n", __func__)) > + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); > +#endif /* CONFIG_DEBUG_NET */ > +} > + > /* > * Add data to an sk_buff > */ > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 2ca96acbc50a..dc9dc0bedca0 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -955,6 +955,9 @@ 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; From another recent report [0], I don't think this change is fixing the report from syzbot. It probably makes sense to revert this patch. afaict, This '!skb->len' test is done after if (is_l2) __skb_push(skb, hh_len); Hence, skb->len is not zero in convert___skb_to_skb(). The proper place to test skb->len is before __skb_push() to ensure there is some network header after the mac or may as well ensure "data_size_in > ETH_HLEN" at the beginning. The fix in [0] is applied. If it turns out there are other cases caused by the skb generated by test_run that needs extra fixes in bpf_redirect_*, it needs to revisit an earlier !skb->len check mentioned above and the existing test cases outside of test_progs would have to adjust accordingly. [0]: https://lore.kernel.org/bpf/20221027225537.353077-1-sdf@google.com/ > + > if (!__skb) > return 0; > > diff --git a/net/core/dev.c b/net/core/dev.c > index d588fd0a54ce..716df64fcfa5 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > bool again = false; > > skb_reset_mac_header(skb); > + skb_assert_len(skb); > > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) > __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED);
On Thu, Nov 3, 2022 at 2:07 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 7/15/22 4:55 AM, Zhengchao Shao wrote: > > Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any > > skbs, that is, the flow->head is null. > > The root cause, as the [2] says, is because that bpf_prog_test_run_skb() > > run a bpf prog which redirects empty skbs. > > So we should determine whether the length of the packet modified by bpf > > prog or others like bpf_prog_test is valid before forwarding it directly. > > > > LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 > > LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html > > > > Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > > --- > > v3: modify debug print > > v2: need move checking to convert___skb_to_skb and add debug info > > v1: should not check len in fast path > > > > include/linux/skbuff.h | 8 ++++++++ > > net/bpf/test_run.c | 3 +++ > > net/core/dev.c | 1 + > > 3 files changed, 12 insertions(+) > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index f6a27ab19202..82e8368ba6e6 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) > > > > #endif /* NET_SKBUFF_DATA_USES_OFFSET */ > > > > +static inline void skb_assert_len(struct sk_buff *skb) > > +{ > > +#ifdef CONFIG_DEBUG_NET > > + if (WARN_ONCE(!skb->len, "%s\n", __func__)) > > + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); > > +#endif /* CONFIG_DEBUG_NET */ > > +} > > + > > /* > > * Add data to an sk_buff > > */ > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > index 2ca96acbc50a..dc9dc0bedca0 100644 > > --- a/net/bpf/test_run.c > > +++ b/net/bpf/test_run.c > > @@ -955,6 +955,9 @@ 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; > > From another recent report [0], I don't think this change is fixing the report > from syzbot. It probably makes sense to revert this patch. > > afaict, This '!skb->len' test is done after > if (is_l2) > __skb_push(skb, hh_len); > > Hence, skb->len is not zero in convert___skb_to_skb(). The proper place to test > skb->len is before __skb_push() to ensure there is some network header after the > mac or may as well ensure "data_size_in > ETH_HLEN" at the beginning. When is_l2==true, __skb_push will result in non-zero skb->len, so we should be good, right? The only issue is when we do bpf_redirect into a tunneling device and do __skb_pull, but that's now fixed by [0]. When is_l2==false, the existing check in convert___skb_to_skb will make sure there is something in the l3 headers. So it seems like this patch is still needed. Or am I missing something? > The fix in [0] is applied. If it turns out there are other cases caused by the > skb generated by test_run that needs extra fixes in bpf_redirect_*, it needs to > revisit an earlier !skb->len check mentioned above and the existing test cases > outside of test_progs would have to adjust accordingly. > > [0]: https://lore.kernel.org/bpf/20221027225537.353077-1-sdf@google.com/ > > > + > > if (!__skb) > > return 0; > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > index d588fd0a54ce..716df64fcfa5 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > bool again = false; > > > > skb_reset_mac_header(skb); > > + skb_assert_len(skb); > > > > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) > > __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED); >
On 11/3/22 2:36 PM, Stanislav Fomichev wrote: > On Thu, Nov 3, 2022 at 2:07 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 7/15/22 4:55 AM, Zhengchao Shao wrote: >>> Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any >>> skbs, that is, the flow->head is null. >>> The root cause, as the [2] says, is because that bpf_prog_test_run_skb() >>> run a bpf prog which redirects empty skbs. >>> So we should determine whether the length of the packet modified by bpf >>> prog or others like bpf_prog_test is valid before forwarding it directly. >>> >>> LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 >>> LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html >>> >>> Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com >>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >>> --- >>> v3: modify debug print >>> v2: need move checking to convert___skb_to_skb and add debug info >>> v1: should not check len in fast path >>> >>> include/linux/skbuff.h | 8 ++++++++ >>> net/bpf/test_run.c | 3 +++ >>> net/core/dev.c | 1 + >>> 3 files changed, 12 insertions(+) >>> >>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >>> index f6a27ab19202..82e8368ba6e6 100644 >>> --- a/include/linux/skbuff.h >>> +++ b/include/linux/skbuff.h >>> @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) >>> >>> #endif /* NET_SKBUFF_DATA_USES_OFFSET */ >>> >>> +static inline void skb_assert_len(struct sk_buff *skb) >>> +{ >>> +#ifdef CONFIG_DEBUG_NET >>> + if (WARN_ONCE(!skb->len, "%s\n", __func__)) >>> + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); >>> +#endif /* CONFIG_DEBUG_NET */ >>> +} >>> + >>> /* >>> * Add data to an sk_buff >>> */ >>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c >>> index 2ca96acbc50a..dc9dc0bedca0 100644 >>> --- a/net/bpf/test_run.c >>> +++ b/net/bpf/test_run.c >>> @@ -955,6 +955,9 @@ 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; >> >> From another recent report [0], I don't think this change is fixing the report >> from syzbot. It probably makes sense to revert this patch. >> >> afaict, This '!skb->len' test is done after >> if (is_l2) >> __skb_push(skb, hh_len); >> >> Hence, skb->len is not zero in convert___skb_to_skb(). The proper place to test >> skb->len is before __skb_push() to ensure there is some network header after the >> mac or may as well ensure "data_size_in > ETH_HLEN" at the beginning. > > When is_l2==true, __skb_push will result in non-zero skb->len, so we > should be good, right? > The only issue is when we do bpf_redirect into a tunneling device and > do __skb_pull, but that's now fixed by [0]. > > When is_l2==false, the existing check in convert___skb_to_skb will > make sure there is something in the l3 headers. > > So it seems like this patch is still needed. Or am I missing something? Replied in [0]. I think a small change in [0] will make this patch obsolete. My thinking is the !skb->len test in this patch does not address all cases, at least not the most common one (the sch_cls prog where is_l2 == true) and then it needs another change in __bpf_redirect_no_mac [0]. Then, instead of breaking the existing test cases, may as well solely depend on the change in __bpf_redirect_no_mac which seems to be the only redirect function that does not have the len check now. > >> The fix in [0] is applied. If it turns out there are other cases caused by the >> skb generated by test_run that needs extra fixes in bpf_redirect_*, it needs to >> revisit an earlier !skb->len check mentioned above and the existing test cases >> outside of test_progs would have to adjust accordingly. >> >> [0]: https://lore.kernel.org/bpf/20221027225537.353077-1-sdf@google.com/ >> >>> + >>> if (!__skb) >>> return 0; >>> >>> diff --git a/net/core/dev.c b/net/core/dev.c >>> index d588fd0a54ce..716df64fcfa5 100644 >>> --- a/net/core/dev.c >>> +++ b/net/core/dev.c >>> @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) >>> bool again = false; >>> >>> skb_reset_mac_header(skb); >>> + skb_assert_len(skb); >>> >>> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) >>> __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED); >>
On Thu, Nov 3, 2022 at 3:42 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 11/3/22 2:36 PM, Stanislav Fomichev wrote: > > On Thu, Nov 3, 2022 at 2:07 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >> > >> On 7/15/22 4:55 AM, Zhengchao Shao wrote: > >>> Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any > >>> skbs, that is, the flow->head is null. > >>> The root cause, as the [2] says, is because that bpf_prog_test_run_skb() > >>> run a bpf prog which redirects empty skbs. > >>> So we should determine whether the length of the packet modified by bpf > >>> prog or others like bpf_prog_test is valid before forwarding it directly. > >>> > >>> LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 > >>> LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html > >>> > >>> Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com > >>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > >>> --- > >>> v3: modify debug print > >>> v2: need move checking to convert___skb_to_skb and add debug info > >>> v1: should not check len in fast path > >>> > >>> include/linux/skbuff.h | 8 ++++++++ > >>> net/bpf/test_run.c | 3 +++ > >>> net/core/dev.c | 1 + > >>> 3 files changed, 12 insertions(+) > >>> > >>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > >>> index f6a27ab19202..82e8368ba6e6 100644 > >>> --- a/include/linux/skbuff.h > >>> +++ b/include/linux/skbuff.h > >>> @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) > >>> > >>> #endif /* NET_SKBUFF_DATA_USES_OFFSET */ > >>> > >>> +static inline void skb_assert_len(struct sk_buff *skb) > >>> +{ > >>> +#ifdef CONFIG_DEBUG_NET > >>> + if (WARN_ONCE(!skb->len, "%s\n", __func__)) > >>> + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); > >>> +#endif /* CONFIG_DEBUG_NET */ > >>> +} > >>> + > >>> /* > >>> * Add data to an sk_buff > >>> */ > >>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > >>> index 2ca96acbc50a..dc9dc0bedca0 100644 > >>> --- a/net/bpf/test_run.c > >>> +++ b/net/bpf/test_run.c > >>> @@ -955,6 +955,9 @@ 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; > >> > >> From another recent report [0], I don't think this change is fixing the report > >> from syzbot. It probably makes sense to revert this patch. > >> > >> afaict, This '!skb->len' test is done after > >> if (is_l2) > >> __skb_push(skb, hh_len); > >> > >> Hence, skb->len is not zero in convert___skb_to_skb(). The proper place to test > >> skb->len is before __skb_push() to ensure there is some network header after the > >> mac or may as well ensure "data_size_in > ETH_HLEN" at the beginning. > > > > When is_l2==true, __skb_push will result in non-zero skb->len, so we > > should be good, right? > > The only issue is when we do bpf_redirect into a tunneling device and > > do __skb_pull, but that's now fixed by [0]. > > > > When is_l2==false, the existing check in convert___skb_to_skb will > > make sure there is something in the l3 headers. > > > > So it seems like this patch is still needed. Or am I missing something? > > Replied in [0]. I think a small change in [0] will make this patch obsolete. > > My thinking is the !skb->len test in this patch does not address all cases, at > least not the most common one (the sch_cls prog where is_l2 == true) and then it > needs another change in __bpf_redirect_no_mac [0]. Then, instead of breaking > the existing test cases, may as well solely depend on the change in > __bpf_redirect_no_mac which seems to be the only redirect function that does not > have the len check now. Removing this check in convert___skb_to_skb and moving the new one in __bpf_redirect_no_mac out of (mlen) SGTM. Can follow up unless you or Zhengchao prefer to do it. There were some concerns about doing this len check at runtime per packet, but not sure whether it really affects anything.. > >> The fix in [0] is applied. If it turns out there are other cases caused by the > >> skb generated by test_run that needs extra fixes in bpf_redirect_*, it needs to > >> revisit an earlier !skb->len check mentioned above and the existing test cases > >> outside of test_progs would have to adjust accordingly. > >> > >> [0]: https://lore.kernel.org/bpf/20221027225537.353077-1-sdf@google.com/ > >> > >>> + > >>> if (!__skb) > >>> return 0; > >>> > >>> diff --git a/net/core/dev.c b/net/core/dev.c > >>> index d588fd0a54ce..716df64fcfa5 100644 > >>> --- a/net/core/dev.c > >>> +++ b/net/core/dev.c > >>> @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > >>> bool again = false; > >>> > >>> skb_reset_mac_header(skb); > >>> + skb_assert_len(skb); > >>> > >>> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) > >>> __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED); > >> >
" On Thu, Nov 3, 2022 at 3:58 PM Stanislav Fomichev <sdf@google.com> wrote: > > On Thu, Nov 3, 2022 at 3:42 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > > > On 11/3/22 2:36 PM, Stanislav Fomichev wrote: > > > On Thu, Nov 3, 2022 at 2:07 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > >> > > >> On 7/15/22 4:55 AM, Zhengchao Shao wrote: > > >>> Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any > > >>> skbs, that is, the flow->head is null. > > >>> The root cause, as the [2] says, is because that bpf_prog_test_run_skb() > > >>> run a bpf prog which redirects empty skbs. > > >>> So we should determine whether the length of the packet modified by bpf > > >>> prog or others like bpf_prog_test is valid before forwarding it directly. > > >>> > > >>> LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 > > >>> LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html > > >>> > > >>> Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com > > >>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > > >>> --- > > >>> v3: modify debug print > > >>> v2: need move checking to convert___skb_to_skb and add debug info > > >>> v1: should not check len in fast path > > >>> > > >>> include/linux/skbuff.h | 8 ++++++++ > > >>> net/bpf/test_run.c | 3 +++ > > >>> net/core/dev.c | 1 + > > >>> 3 files changed, 12 insertions(+) > > >>> > > >>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > >>> index f6a27ab19202..82e8368ba6e6 100644 > > >>> --- a/include/linux/skbuff.h > > >>> +++ b/include/linux/skbuff.h > > >>> @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) > > >>> > > >>> #endif /* NET_SKBUFF_DATA_USES_OFFSET */ > > >>> > > >>> +static inline void skb_assert_len(struct sk_buff *skb) > > >>> +{ > > >>> +#ifdef CONFIG_DEBUG_NET > > >>> + if (WARN_ONCE(!skb->len, "%s\n", __func__)) > > >>> + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); > > >>> +#endif /* CONFIG_DEBUG_NET */ > > >>> +} > > >>> + > > >>> /* > > >>> * Add data to an sk_buff > > >>> */ > > >>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > >>> index 2ca96acbc50a..dc9dc0bedca0 100644 > > >>> --- a/net/bpf/test_run.c > > >>> +++ b/net/bpf/test_run.c > > >>> @@ -955,6 +955,9 @@ 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; > > >> > > >> From another recent report [0], I don't think this change is fixing the report > > >> from syzbot. It probably makes sense to revert this patch. > > >> > > >> afaict, This '!skb->len' test is done after > > >> if (is_l2) > > >> __skb_push(skb, hh_len); > > >> > > >> Hence, skb->len is not zero in convert___skb_to_skb(). The proper place to test > > >> skb->len is before __skb_push() to ensure there is some network header after the > > >> mac or may as well ensure "data_size_in > ETH_HLEN" at the beginning. > > > > > > When is_l2==true, __skb_push will result in non-zero skb->len, so we > > > should be good, right? > > > The only issue is when we do bpf_redirect into a tunneling device and > > > do __skb_pull, but that's now fixed by [0]. > > > > > > When is_l2==false, the existing check in convert___skb_to_skb will > > > make sure there is something in the l3 headers. > > > > > > So it seems like this patch is still needed. Or am I missing something? > > > > Replied in [0]. I think a small change in [0] will make this patch obsolete. > > > > My thinking is the !skb->len test in this patch does not address all cases, at > > least not the most common one (the sch_cls prog where is_l2 == true) and then it > > needs another change in __bpf_redirect_no_mac [0]. Then, instead of breaking > > the existing test cases, may as well solely depend on the change in > > __bpf_redirect_no_mac which seems to be the only redirect function that does not > > have the len check now. > > Removing this check in convert___skb_to_skb and moving the new one in > __bpf_redirect_no_mac out of (mlen) SGTM. > Can follow up unless you or Zhengchao prefer to do it. > There were some concerns about doing this len check at runtime per > packet, but not sure whether it really affects anything.. I've implemented a simple selftest for both mac/no-mac cases, and I feel like this explicit len==0 has to happen in both cases. That "skb->mac_header >= skb->network_header" check is not enough :-( I'll send the series early next week after another round of xdp metadata.. > > >> The fix in [0] is applied. If it turns out there are other cases caused by the > > >> skb generated by test_run that needs extra fixes in bpf_redirect_*, it needs to > > >> revisit an earlier !skb->len check mentioned above and the existing test cases > > >> outside of test_progs would have to adjust accordingly. > > >> > > >> [0]: https://lore.kernel.org/bpf/20221027225537.353077-1-sdf@google.com/ > > >> > > >>> + > > >>> if (!__skb) > > >>> return 0; > > >>> > > >>> diff --git a/net/core/dev.c b/net/core/dev.c > > >>> index d588fd0a54ce..716df64fcfa5 100644 > > >>> --- a/net/core/dev.c > > >>> +++ b/net/core/dev.c > > >>> @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > >>> bool again = false; > > >>> > > >>> skb_reset_mac_header(skb); > > >>> + skb_assert_len(skb); > > >>> > > >>> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) > > >>> __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED); > > >> > >
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index f6a27ab19202..82e8368ba6e6 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2459,6 +2459,14 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset) #endif /* NET_SKBUFF_DATA_USES_OFFSET */ +static inline void skb_assert_len(struct sk_buff *skb) +{ +#ifdef CONFIG_DEBUG_NET + if (WARN_ONCE(!skb->len, "%s\n", __func__)) + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false); +#endif /* CONFIG_DEBUG_NET */ +} + /* * Add data to an sk_buff */ diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 2ca96acbc50a..dc9dc0bedca0 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -955,6 +955,9 @@ 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/dev.c b/net/core/dev.c index d588fd0a54ce..716df64fcfa5 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4168,6 +4168,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) bool again = false; skb_reset_mac_header(skb); + skb_assert_len(skb); if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP)) __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED);
Syzbot found an issue [1]: fq_codel_drop() try to drop a flow whitout any skbs, that is, the flow->head is null. The root cause, as the [2] says, is because that bpf_prog_test_run_skb() run a bpf prog which redirects empty skbs. So we should determine whether the length of the packet modified by bpf prog or others like bpf_prog_test is valid before forwarding it directly. LINK: [1] https://syzkaller.appspot.com/bug?id=0b84da80c2917757915afa89f7738a9d16ec96c5 LINK: [2] https://www.spinics.net/lists/netdev/msg777503.html Reported-by: syzbot+7a12909485b94426aceb@syzkaller.appspotmail.com Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- v3: modify debug print v2: need move checking to convert___skb_to_skb and add debug info v1: should not check len in fast path include/linux/skbuff.h | 8 ++++++++ net/bpf/test_run.c | 3 +++ net/core/dev.c | 1 + 3 files changed, 12 insertions(+)