From: "Dongseok Yi" <dseok.yi@samsung.com>
To: "'Daniel Borkmann'" <daniel@iogearbox.net>, <bpf@vger.kernel.org>
Cc: "'Alexei Starovoitov'" <ast@kernel.org>,
"'Andrii Nakryiko'" <andrii@kernel.org>,
"'Martin KaFai Lau'" <kafai@fb.com>,
"'Song Liu'" <songliubraving@fb.com>,
"'Yonghong Song'" <yhs@fb.com>,
"'John Fastabend'" <john.fastabend@gmail.com>,
"'KP Singh'" <kpsingh@kernel.org>,
"'David S. Miller'" <davem@davemloft.net>,
"'Jakub Kicinski'" <kuba@kernel.org>, <netdev@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <willemdebruijn.kernel@gmail.com>
Subject: RE: [PATCH bpf] bpf: check for data_len before upgrading mss when 6 to 4
Date: Thu, 6 May 2021 09:45:07 +0900 [thread overview]
Message-ID: <02bf01d74211$0ff4aed0$2fde0c70$@samsung.com> (raw)
In-Reply-To: <8c2ea41a-3fc5-d560-16e5-bf706949d857@iogearbox.net>
On Wed, May 05, 2021 at 10:55:10PM +0200, Daniel Borkmann wrote:
> On 4/29/21 12:08 PM, Dongseok Yi wrote:
> > tcp_gso_segment check for the size of GROed payload if it is bigger
> > than the mss. bpf_skb_proto_6_to_4 increases mss, but the mss can be
> > bigger than the size of GROed payload unexpectedly if data_len is not
> > big enough.
> >
> > Assume that skb gso_size = 1372 and data_len = 8. bpf_skb_proto_6_to_4
> > would increse the gso_size to 1392. tcp_gso_segment will get an error
> > with 1380 <= 1392.
> >
> > Check for the size of GROed payload if it is really bigger than target
> > mss when increase mss.
> >
> > Fixes: 6578171a7ff0 (bpf: add bpf_skb_change_proto helper)
> > Signed-off-by: Dongseok Yi <dseok.yi@samsung.com>
> > ---
> > net/core/filter.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 9323d34..3f79e3c 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3308,7 +3308,9 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb)
> > }
> >
> > /* Due to IPv4 header, MSS can be upgraded. */
> > - skb_increase_gso_size(shinfo, len_diff);
> > + if (skb->data_len > len_diff)
>
> Could you elaborate some more on what this has to do with data_len specifically
> here? I'm not sure I follow exactly your above commit description. Are you saying
> that you're hitting in tcp_gso_segment():
>
> [...]
> mss = skb_shinfo(skb)->gso_size;
> if (unlikely(skb->len <= mss))
> goto out;
> [...]
Yes, right
>
> Please provide more context on the bug, thanks!
tcp_gso_segment():
[...]
__skb_pull(skb, thlen);
mss = skb_shinfo(skb)->gso_size;
if (unlikely(skb->len <= mss))
[...]
skb->len will have total GROed TCP payload size after __skb_pull.
skb->len <= mss will not be happened in a normal GROed situation. But
bpf_skb_proto_6_to_4 would upgrade MSS by increasing gso_size, it can
hit an error condition.
We should ensure the following condition.
total GROed TCP payload > the original mss + (IPv6 size - IPv4 size)
Due to
total GROed TCP payload = the original mss + skb->data_len
IPv6 size - IPv4 size = len_diff
Finally, we can get the condition.
skb->data_len > len_diff
>
> > + skb_increase_gso_size(shinfo, len_diff);
> > +
> > /* Header must be checked, and gso_segs recomputed. */
> > shinfo->gso_type |= SKB_GSO_DODGY;
> > shinfo->gso_segs = 0;
> >
next prev parent reply other threads:[~2021-05-06 0:45 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20210429102143epcas2p4c8747c09a9de28f003c20389c050394a@epcas2p4.samsung.com>
2021-04-29 10:08 ` [PATCH bpf] bpf: check for data_len before upgrading mss when 6 to 4 Dongseok Yi
2021-05-05 20:55 ` Daniel Borkmann
2021-05-06 0:45 ` Dongseok Yi [this message]
2021-05-06 1:45 ` Willem de Bruijn
2021-05-06 2:27 ` Dongseok Yi
2021-05-06 18:21 ` Willem de Bruijn
2021-05-07 0:53 ` Dongseok Yi
2021-05-07 1:25 ` Willem de Bruijn
2021-05-07 1:45 ` Yunsheng Lin
2021-05-07 1:53 ` Willem de Bruijn
2021-05-07 8:25 ` Dongseok Yi
2021-05-07 9:11 ` Yunsheng Lin
2021-05-07 10:36 ` Dongseok Yi
2021-05-07 13:50 ` Willem de Bruijn
2021-05-10 2:22 ` Dongseok Yi
2021-05-10 13:19 ` Willem de Bruijn
2021-05-10 13:46 ` Willem de Bruijn
2021-05-11 1:11 ` Dongseok Yi
2021-05-11 17:38 ` Willem de Bruijn
2021-05-12 0:45 ` Dongseok Yi
[not found] ` <CGME20210511065056epcas2p1788505019deb274f5c57650a2f5d7ef0@epcas2p1.samsung.com>
2021-05-11 6:36 ` [PATCH bpf v2] bpf: check BPF_F_ADJ_ROOM_FIXED_GSO when upgrading mss in " Dongseok Yi
2021-05-11 17:42 ` Willem de Bruijn
2021-05-12 6:56 ` Dongseok Yi
[not found] ` <CGME20210512074058epcas2p35536c27bdfafaa6431e164c142007f96@epcas2p3.samsung.com>
2021-05-12 7:27 ` [PATCH bpf-next v3] bpf: check for BPF_F_ADJ_ROOM_FIXED_GSO when bpf_skb_change_proto Dongseok Yi
2021-05-12 14:13 ` Willem de Bruijn
2021-05-18 20:10 ` patchwork-bot+netdevbpf
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='02bf01d74211$0ff4aed0$2fde0c70$@samsung.com' \
--to=dseok.yi@samsung.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=willemdebruijn.kernel@gmail.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.