From: sdf@google•com
To: shaozhengchao <shaozhengchao@huawei•com>
Cc: Daniel Borkmann <daniel@iogearbox•net>,
"bpf@vger•kernel.org" <bpf@vger•kernel.org>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"linux-kernel@vger•kernel.org" <linux-kernel@vger•kernel.org>,
"davem@davemloft•net" <davem@davemloft•net>,
"edumazet@google•com" <edumazet@google•com>,
"kuba@kernel•org" <kuba@kernel•org>,
"pabeni@redhat•com" <pabeni@redhat•com>,
"hawk@kernel•org" <hawk@kernel•org>,
"ast@kernel•org" <ast@kernel•org>,
"andrii@kernel•org" <andrii@kernel•org>,
"martin.lau@linux•dev" <martin.lau@linux•dev>,
"song@kernel•org" <song@kernel•org>, "yhs@fb•com" <yhs@fb•com>,
"john.fastabend@gmail•com" <john.fastabend@gmail•com>,
"kpsingh@kernel•org" <kpsingh@kernel•org>,
"weiyongjun (A)" <weiyongjun1@huawei•com>,
yuehaibing <yuehaibing@huawei•com>
Subject: Re: 答复: [PATCH bpf-next] bpf: Don't redirect packets with invalid pkt_len
Date: Wed, 13 Jul 2022 09:02:14 -0700 [thread overview]
Message-ID: <Ys7shru6xUa/9XwY@google.com> (raw)
In-Reply-To: <d7f22715be8d477cbc3e6e545c219048@huawei.com>
On 07/13, shaozhengchao wrote:
> -----邮件原件-----
> 发件人: Daniel Borkmann [mailto:daniel@iogearbox•net]
> 发送时间: 2022年7月13日 4:12
> 收件人: sdf@google•com; shaozhengchao <shaozhengchao@huawei•com>
> 抄送: bpf@vger•kernel.org; netdev@vger•kernel.org;
> linux-kernel@vger•kernel.org; davem@davemloft•net; edumazet@google•com;
> kuba@kernel•org; pabeni@redhat•com; hawk@kernel•org; ast@kernel•org;
> andrii@kernel•org; martin.lau@linux•dev; song@kernel•org; yhs@fb•com;
> john.fastabend@gmail•com; kpsingh@kernel•org; weiyongjun (A)
> <weiyongjun1@huawei•com>; yuehaibing <yuehaibing@huawei•com>
> 主题: Re: [PATCH bpf-next] bpf: Don't redirect packets with invalid
> pkt_len
> On 7/12/22 6:58 PM, sdf@google•com wrote:
> > On 07/12, 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=0b84da80c2917757915afa89f7738a9d
> >> 16ec96c5
> >> 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>
> >> ---
> >> net/core/filter.c | 9 ++++++++-
> >> 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> >> diff --git a/net/core/filter.c b/net/core/filter.c index
> >> 4ef77ec5255e..27801b314960 100644
> >> --- a/net/core/filter.c
> >> +++ b/net/core/filter.c
> >> @@ -2122,6 +2122,11 @@ 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 == 0)) {
> >> + kfree_skb(skb);
> >> + return -EINVAL;
> >> + }
> >> +
> >> if (mlen) {
> >> __skb_pull(skb, mlen);
> >
> >> @@ -2143,7 +2148,9 @@ 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) ||
> >> + (min_t(u32, skb_mac_header_len(skb), skb->len) <
> >> + (u32)dev->min_header_len)) {
> >
> > Why check skb->len != 0 above but skb->len < dev->min_header_len here?
> > I guess it doesn't make sense in __bpf_redirect_no_mac because we know
> > that mac is empty, but why do we care in __bpf_redirect_common?
> > Why not put this check in the common __bpf_redirect?
> >
> > Also, it's still not clear to me whether we should bake it into the
> > core stack vs having some special checks from test_prog_run only. I'm
> > assuming the issue is that we can construct illegal skbs with that
> > test_prog_run interface, so maybe start by fixing that?
> Agree, ideally we can prevent it right at the source rather than adding
> more tests into the fast-path.
> > Did you have a chance to look at the reproducer more closely? What
> > exactly is it doing?
> >
> >> kfree_skb(skb);
> >> return -ERANGE;
> >> }
> >> --
> >> 2.17.1
> >
> Hi Daniel and sdf:
> Thank you for your reply. I read the poc code carefully, and I think the
> current call stack is like:
> sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr)) ->
> bpf_prog_test_run->bpf_prog_test_run_skb.
> In function bpf_prog_test_run_skb, procedure will use build_skb to
> generate a new skb. Poc code pass
> a 14Byte packet for direct. First ,skb->len = 14, but after trans eth
> type, the len = 0; but is_l2 is false,
> so len=0 when run bpf_test_run. Is it possible to add check in
> convert___skb_to_skb? When skb->len=0,
> we drop the packet.
Not sure it belongs in convert___skb_to_skb, but checking somewhere before
convert___skb_to_skb seems like a good way to go?
> But, if some other paths call bpf redirect with skb->len=0, this is not
> effective, such as some driver call redirect fuction.
> I don't know if I'm thinking right.
I think the consensus so far that it's only bpf_prog_test_run that
generates these types of packets, so let's start with fixing that.
prev parent reply other threads:[~2022-07-13 16:02 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-12 12:01 [PATCH bpf-next] bpf: Don't redirect packets with invalid pkt_len Zhengchao Shao
2022-07-12 16:58 ` sdf
2022-07-12 20:12 ` Daniel Borkmann
2022-07-13 12:53 ` 答复: " shaozhengchao
2022-07-13 16:02 ` sdf [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ys7shru6xUa/9XwY@google.com \
--to=sdf@google$(echo .)com \
--cc=andrii@kernel$(echo .)org \
--cc=ast@kernel$(echo .)org \
--cc=bpf@vger$(echo .)kernel.org \
--cc=daniel@iogearbox$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=hawk@kernel$(echo .)org \
--cc=john.fastabend@gmail$(echo .)com \
--cc=kpsingh@kernel$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=martin.lau@linux$(echo .)dev \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=shaozhengchao@huawei$(echo .)com \
--cc=song@kernel$(echo .)org \
--cc=weiyongjun1@huawei$(echo .)com \
--cc=yhs@fb$(echo .)com \
--cc=yuehaibing@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox