Message ID | 4f13798ae41986f8fe8a6f8698c7cbeaefba93b0.1591315176.git.zhuyifei@google.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | Fix bpf_skb_load_bytes_relative for cgroup_skb/egress | expand |
On 6/5/20 2:07 AM, YiFei Zhu wrote: > Added a check in the switch case on start_header that checks for > the existence of the header, and in the case that MAC is not set > and the caller requests for MAC, -EFAULT. If the caller requests > for NET then MAC's existence is completely ignored. > > There is no function to check NET header's existence and as far > as cgroup_skb/egress is concerned it should always be set. > > Removed for ptr >= the start of header, considering offset is > bounded unsigned and should always be true. ptr + len <= end is > overflow-unsafe and replaced with len <= end - ptr, and > len <= end - mac is redundant to this condition. > > Fixes: 3eee1f75f2b9 ("bpf: fix bpf_skb_load_bytes_relative pkt length check") > Reviewed-by: Stanislav Fomichev <sdf@google.com> > Signed-off-by: YiFei Zhu <zhuyifei@google.com> > --- > net/core/filter.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index d01a244b5087..d3e8445b5494 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1755,25 +1755,27 @@ BPF_CALL_5(bpf_skb_load_bytes_relative, const struct sk_buff *, skb, > u32, offset, void *, to, u32, len, u32, start_header) > { > u8 *end = skb_tail_pointer(skb); > - u8 *net = skb_network_header(skb); > - u8 *mac = skb_mac_header(skb); > - u8 *ptr; > + u8 *start, *ptr; > > - if (unlikely(offset > 0xffff || len > (end - mac))) > + if (unlikely(offset > 0xffff)) > goto err_clear; > > switch (start_header) { > case BPF_HDR_START_MAC: > - ptr = mac + offset; > + if (unlikely(!skb_mac_header_was_set(skb))) > + goto err_clear; > + start = skb_mac_header(skb); > break; > case BPF_HDR_START_NET: > - ptr = net + offset; > + start = skb_network_header(skb); > break; > default: > goto err_clear; > } > > - if (likely(ptr >= mac && ptr + len <= end)) { > + ptr = start + offset; > + > + if (likely(len <= end - ptr)) { Couldn't you run into the case above where the passed offset is large enough that start + offset goes beyond end pointer [and then above comparison is performed as unsigned ..]? (At least on x86-64, the 'ptr + len <= end' should never have an issue [0].) Either way, maybe lets add a test in 2/2 to assert correct behavior there. [0] https://www.kernel.org/doc/Documentation/x86/x86_64/mm.txt > memcpy(to, ptr, len); > return 0; > } >
On Mon, Jun 8, 2020 at 8:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > Couldn't you run into the case above where the passed offset is large enough > that start + offset goes beyond end pointer [and then above comparison is > performed as unsigned ..]? You are right. I missed that offset would be large and make start + offset > end, when I was trying to reason the offsets and overflows. I just checked that on x86_64 it emits a 'jg' instruction on x86_64, and the test I tried with offset = 0xffff does return -EFAULT. However, I searched around and saw that this is due to integer promotion of len and the test would fail (i.e. not returning -EFAULT) on x86_32 (I have not tested this). > (At least on x86-64, the 'ptr + len <= end' should > never have an issue [0].) Alright, I see that len is an ARG_CONST_SIZE, which would be checked by check_helper_mem_access, so it is bound by the stack size. So the argument against ptr >= start also applies here, correct? YiFei Zhu
diff --git a/net/core/filter.c b/net/core/filter.c index d01a244b5087..d3e8445b5494 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1755,25 +1755,27 @@ BPF_CALL_5(bpf_skb_load_bytes_relative, const struct sk_buff *, skb, u32, offset, void *, to, u32, len, u32, start_header) { u8 *end = skb_tail_pointer(skb); - u8 *net = skb_network_header(skb); - u8 *mac = skb_mac_header(skb); - u8 *ptr; + u8 *start, *ptr; - if (unlikely(offset > 0xffff || len > (end - mac))) + if (unlikely(offset > 0xffff)) goto err_clear; switch (start_header) { case BPF_HDR_START_MAC: - ptr = mac + offset; + if (unlikely(!skb_mac_header_was_set(skb))) + goto err_clear; + start = skb_mac_header(skb); break; case BPF_HDR_START_NET: - ptr = net + offset; + start = skb_network_header(skb); break; default: goto err_clear; } - if (likely(ptr >= mac && ptr + len <= end)) { + ptr = start + offset; + + if (likely(len <= end - ptr)) { memcpy(to, ptr, len); return 0; }