| Message ID | 20220414135902.100914-2-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Enlarge offset check value in bpf_skb_load_bytes | expand |
On 4/14/22 3:59 PM, Liu Jian wrote: > The data length of skb frags + frag_list may be greater than 0xffff, > and skb_header_pointer can not handle negative offset and negative len. > So here INT_MAX is used to check the validity of offset and len. > Add the same change to the related function skb_store_bytes. > > Fixes: 05c74e5e53f6 ("bpf: add bpf_skb_load_bytes helper") > Signed-off-by: Liu Jian <liujian56@huawei.com> > Acked-by: Song Liu <songliubraving@fb.com> > --- > v2->v3: change nothing > net/core/filter.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 64470a727ef7..1571b6bc51ea 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1687,7 +1687,7 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset, > > if (unlikely(flags & ~(BPF_F_RECOMPUTE_CSUM | BPF_F_INVALIDATE_HASH))) > return -EINVAL; > - if (unlikely(offset > 0xffff)) > + if (unlikely(offset > INT_MAX || len > INT_MAX)) One more follow-up question, len param is checked by the verifier for the provided buffer. Why is it needed here? Were you able to create e.g. map values of size larger than INT_MAX? Please provide details. (Other than that, rest looks good.) > return -EFAULT; > if (unlikely(bpf_try_make_writable(skb, offset + len))) > return -EFAULT; > @@ -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 > INT_MAX || len > INT_MAX)) > goto err_clear; > > ptr = skb_header_pointer(skb, offset, len, to); >
> -----Original Message----- > From: Daniel Borkmann [mailto:daniel@iogearbox.net] > Sent: Saturday, April 16, 2022 3:32 AM > To: liujian (CE) <liujian56@huawei.com>; ast@kernel.org; andrii@kernel.org; > kafai@fb.com; 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; pabeni@redhat.com > Subject: Re: [PATCH bpf-next v3 1/3] net: Enlarge offset check value from > 0xffff to INT_MAX in bpf_skb_load_bytes > > On 4/14/22 3:59 PM, Liu Jian wrote: > > The data length of skb frags + frag_list may be greater than 0xffff, > > and skb_header_pointer can not handle negative offset and negative len. > > So here INT_MAX is used to check the validity of offset and len. > > Add the same change to the related function skb_store_bytes. > > > > Fixes: 05c74e5e53f6 ("bpf: add bpf_skb_load_bytes helper") > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > Acked-by: Song Liu <songliubraving@fb.com> > > --- > > v2->v3: change nothing > > net/core/filter.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/core/filter.c b/net/core/filter.c index > > 64470a727ef7..1571b6bc51ea 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -1687,7 +1687,7 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff > > *, skb, u32, offset, > > > > if (unlikely(flags & ~(BPF_F_RECOMPUTE_CSUM | > BPF_F_INVALIDATE_HASH))) > > return -EINVAL; > > - if (unlikely(offset > 0xffff)) > > + if (unlikely(offset > INT_MAX || len > INT_MAX)) > > One more follow-up question, len param is checked by the verifier for the > provided buffer. > Why is it needed here? Were you able to create e.g. map values of size larger > than INT_MAX? > Please provide details. (Other than that, rest looks good.) > I only need change "offset > 0xffff". Delete " || len > INT_MAX " in v4. Thank you~ > > return -EFAULT; > > if (unlikely(bpf_try_make_writable(skb, offset + len))) > > return -EFAULT; > > @@ -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 > INT_MAX || len > INT_MAX)) > > goto err_clear; > > > > ptr = skb_header_pointer(skb, offset, len, to); > >
diff --git a/net/core/filter.c b/net/core/filter.c index 64470a727ef7..1571b6bc51ea 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1687,7 +1687,7 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset, if (unlikely(flags & ~(BPF_F_RECOMPUTE_CSUM | BPF_F_INVALIDATE_HASH))) return -EINVAL; - if (unlikely(offset > 0xffff)) + if (unlikely(offset > INT_MAX || len > INT_MAX)) return -EFAULT; if (unlikely(bpf_try_make_writable(skb, offset + len))) return -EFAULT; @@ -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 > INT_MAX || len > INT_MAX)) goto err_clear; ptr = skb_header_pointer(skb, offset, len, to);
The data length of skb frags + frag_list may be greater than 0xffff, and skb_header_pointer can not handle negative offset and negative len. So here INT_MAX is used to check the validity of offset and len. Add the same change to the related function skb_store_bytes. Fixes: 05c74e5e53f6 ("bpf: add bpf_skb_load_bytes helper") Signed-off-by: Liu Jian <liujian56@huawei.com> Acked-by: Song Liu <songliubraving@fb.com> --- v2->v3: change nothing net/core/filter.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)