| Message ID | 20220315123916.110409-1-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] net: Use skb->len to check the validity of the parameters in bpf_skb_load_bytes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 25 this patch: 25 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 30 this patch: 30 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 28 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next | fail | VM_Test |
On Tue, Mar 15, 2022 at 08:39:16PM +0800, Liu Jian wrote: > The data length of skb frags + frag_list may be greater than 0xffff, > so here use skb->len to check the validity of the parameters. What is the use case that needs to look beyond 0xffff ?
> -----Original Message----- > From: Martin KaFai Lau [mailto:kafai@fb.com] > Sent: Wednesday, March 16, 2022 3:58 AM > To: liujian (CE) <liujian56@huawei.com> > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > Subject: Re: [PATCH bpf-next] net: Use skb->len to check the validity of the > parameters in bpf_skb_load_bytes > > On Tue, Mar 15, 2022 at 08:39:16PM +0800, Liu Jian wrote: > > The data length of skb frags + frag_list may be greater than 0xffff, > > so here use skb->len to check the validity of the parameters. > What is the use case that needs to look beyond 0xffff ? I use sockmap with strparser, the stm->strp.offset (the begin of one application layer protocol message) maybe beyond 0xffff, but i need load the message head to do something.
liujian (CE) wrote: > > > > -----Original Message----- > > From: Martin KaFai Lau [mailto:kafai@fb.com] > > Sent: Wednesday, March 16, 2022 3:58 AM > > To: liujian (CE) <liujian56@huawei.com> > > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > > Subject: Re: [PATCH bpf-next] net: Use skb->len to check the validity of the > > parameters in bpf_skb_load_bytes > > > > On Tue, Mar 15, 2022 at 08:39:16PM +0800, Liu Jian wrote: > > > The data length of skb frags + frag_list may be greater than 0xffff, > > > so here use skb->len to check the validity of the parameters. > > What is the use case that needs to look beyond 0xffff ? > I use sockmap with strparser, the stm->strp.offset (the begin of one > application layer protocol message) maybe beyond 0xffff, but i need > load the message head to do something. This would explain skb_load_bytes but not the other two right? Also if we are doing this why not just remove those two checks in flow_dissector_load() I think skb_header_pointer() does duplicate checks. Please check.
> -----Original Message----- > From: John Fastabend [mailto:john.fastabend@gmail.com] > Sent: Wednesday, March 16, 2022 12:00 PM > To: liujian (CE) <liujian56@huawei.com>; Martin KaFai Lau <kafai@fb.com> > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > Subject: RE: [PATCH bpf-next] net: Use skb->len to check the validity of the > parameters in bpf_skb_load_bytes > > liujian (CE) wrote: > > > > > > > -----Original Message----- > > > From: Martin KaFai Lau [mailto:kafai@fb.com] > > > Sent: Wednesday, March 16, 2022 3:58 AM > > > To: liujian (CE) <liujian56@huawei.com> > > > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > > > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > > > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > > > Subject: Re: [PATCH bpf-next] net: Use skb->len to check the > > > validity of the parameters in bpf_skb_load_bytes > > > > > > On Tue, Mar 15, 2022 at 08:39:16PM +0800, Liu Jian wrote: > > > > The data length of skb frags + frag_list may be greater than > > > > 0xffff, so here use skb->len to check the validity of the parameters. > > > What is the use case that needs to look beyond 0xffff ? > > > I use sockmap with strparser, the stm->strp.offset (the begin of one > > application layer protocol message) maybe beyond 0xffff, but i need > > load the message head to do something. > > This would explain skb_load_bytes but not the other two right? Also if we Yes, I just see that these two functions have the same judgment. > are doing this why not just remove those two checks in > flow_dissector_load() I think skb_header_pointer() does duplicate checks. > Please check. Yes, skb_header_pointer() have checked as below, and I will send v2 to remove 0xffff check. ----skb_header_pointer -------- __skb_header_pointer ------------skb_copy_bits ---------------- if (offset > (int)skb->len - len) --------------------goto fault; Thank you~
On Wed, Mar 16, 2022 at 6:08 AM liujian (CE) <liujian56@huawei.com> wrote: > > > > > -----Original Message----- > > From: John Fastabend [mailto:john.fastabend@gmail.com] > > Sent: Wednesday, March 16, 2022 12:00 PM > > To: liujian (CE) <liujian56@huawei.com>; Martin KaFai Lau <kafai@fb.com> > > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > > Subject: RE: [PATCH bpf-next] net: Use skb->len to check the validity of the > > parameters in bpf_skb_load_bytes > > > > liujian (CE) wrote: > > > > > > > > > > -----Original Message----- > > > > From: Martin KaFai Lau [mailto:kafai@fb.com] > > > > Sent: Wednesday, March 16, 2022 3:58 AM > > > > To: liujian (CE) <liujian56@huawei.com> > > > > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > > > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > > > > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > > > > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > > > > Subject: Re: [PATCH bpf-next] net: Use skb->len to check the > > > > validity of the parameters in bpf_skb_load_bytes > > > > > > > > On Tue, Mar 15, 2022 at 08:39:16PM +0800, Liu Jian wrote: > > > > > The data length of skb frags + frag_list may be greater than > > > > > 0xffff, so here use skb->len to check the validity of the parameters. > > > > What is the use case that needs to look beyond 0xffff ? > > > > > I use sockmap with strparser, the stm->strp.offset (the begin of one > > > application layer protocol message) maybe beyond 0xffff, but i need > > > load the message head to do something. > > > > This would explain skb_load_bytes but not the other two right? Also if we > Yes, I just see that these two functions have the same judgment. > > are doing this why not just remove those two checks in > > flow_dissector_load() I think skb_header_pointer() does duplicate checks. > > Please check. > Yes, skb_header_pointer() have checked as below, and I will send v2 to remove 0xffff check. > ----skb_header_pointer > -------- __skb_header_pointer > ------------skb_copy_bits > ---------------- if (offset > (int)skb->len - len) > --------------------goto fault; > > Thank you~ Do we need to have at least "offset <= 0x7fffffff" check? IOW, do we need to enforce the unsignedness of the offset? Or does skb_header_pointer et all properly work with the negative offsets?
> -----Original Message----- > From: Stanislav Fomichev [mailto:sdf@google.com] > Sent: Wednesday, March 16, 2022 11:09 PM > To: liujian (CE) <liujian56@huawei.com> > Cc: John Fastabend <john.fastabend@gmail.com>; Martin KaFai Lau > <kafai@fb.com>; ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > songliubraving@fb.com; yhs@fb.com; kpsingh@kernel.org; > davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org; > bpf@vger.kernel.org > Subject: Re: [PATCH bpf-next] net: Use skb->len to check the validity of the > parameters in bpf_skb_load_bytes > > On Wed, Mar 16, 2022 at 6:08 AM liujian (CE) <liujian56@huawei.com> wrote: > > > > > > > > > -----Original Message----- > > > From: John Fastabend [mailto:john.fastabend@gmail.com] > > > Sent: Wednesday, March 16, 2022 12:00 PM > > > To: liujian (CE) <liujian56@huawei.com>; Martin KaFai Lau > > > <kafai@fb.com> > > > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > > > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > > > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > > > Subject: RE: [PATCH bpf-next] net: Use skb->len to check the > > > validity of the parameters in bpf_skb_load_bytes > > > > > > liujian (CE) wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Martin KaFai Lau [mailto:kafai@fb.com] > > > > > Sent: Wednesday, March 16, 2022 3:58 AM > > > > > To: liujian (CE) <liujian56@huawei.com> > > > > > Cc: ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > > > > songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com; > > > > > kpsingh@kernel.org; davem@davemloft.net; kuba@kernel.org; > > > > > sdf@google.com; netdev@vger.kernel.org; bpf@vger.kernel.org > > > > > Subject: Re: [PATCH bpf-next] net: Use skb->len to check the > > > > > validity of the parameters in bpf_skb_load_bytes > > > > > > > > > > On Tue, Mar 15, 2022 at 08:39:16PM +0800, Liu Jian wrote: > > > > > > The data length of skb frags + frag_list may be greater than > > > > > > 0xffff, so here use skb->len to check the validity of the parameters. > > > > > What is the use case that needs to look beyond 0xffff ? > > > > > > > I use sockmap with strparser, the stm->strp.offset (the begin of > > > > one application layer protocol message) maybe beyond 0xffff, but i > > > > need load the message head to do something. > > > > > > This would explain skb_load_bytes but not the other two right? Also > > > if we > > Yes, I just see that these two functions have the same judgment. > > > are doing this why not just remove those two checks in > > > flow_dissector_load() I think skb_header_pointer() does duplicate > checks. > > > Please check. > > Yes, skb_header_pointer() have checked as below, and I will send v2 to > remove 0xffff check. > > ----skb_header_pointer > > -------- __skb_header_pointer > > ------------skb_copy_bits > > ---------------- if (offset > (int)skb->len - len) > > --------------------goto fault; > > > > Thank you~ > > Do we need to have at least "offset <= 0x7fffffff" check? IOW, do we need > to enforce the unsignedness of the offset? Or does skb_header_pointer et > all properly work with the negative offsets? Yes, skb_header_pointer can not handle the negative offset. I sent a new patch. Please help review it again. Thank you. https://patchwork.kernel.org/project/netdevbpf/patch/20220317135940.358774-1-liujian56@huawei.com/
diff --git a/net/core/filter.c b/net/core/filter.c index 9eb785842258..61c353caf141 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1722,7 +1722,7 @@ BPF_CALL_4(bpf_skb_load_bytes, const struct sk_buff *, skb, u32, offset, { void *ptr; - if (unlikely(offset > 0xffff)) + if (unlikely(offset >= skb->len)) goto err_clear; ptr = skb_header_pointer(skb, offset, len, to); @@ -1753,10 +1753,10 @@ BPF_CALL_4(bpf_flow_dissector_load_bytes, { void *ptr; - if (unlikely(offset > 0xffff)) + if (unlikely(!ctx->skb)) goto err_clear; - if (unlikely(!ctx->skb)) + if (unlikely(offset >= ctx->skb->len)) goto err_clear; ptr = skb_header_pointer(ctx->skb, offset, len, to); @@ -1787,7 +1787,7 @@ BPF_CALL_5(bpf_skb_load_bytes_relative, const struct sk_buff *, skb, u8 *end = skb_tail_pointer(skb); u8 *start, *ptr; - if (unlikely(offset > 0xffff)) + if (unlikely(offset >= skb->len)) goto err_clear; switch (start_header) {
The data length of skb frags + frag_list may be greater than 0xffff, so here use skb->len to check the validity of the parameters. And modify bpf_flow_dissector_load_bytes and bpf_skb_load_bytes_relative in the same way. Fixes: 05c74e5e53f6 ("bpf: add bpf_skb_load_bytes helper") Fixes: 4e1ec56cdc59 ("bpf: add skb_load_bytes_relative helper") Fixes: 089b19a9204f ("flow_dissector: switch kernel context to struct bpf_flow_dissector") Signed-off-by: Liu Jian <liujian56@huawei.com> --- net/core/filter.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)