[bpf,1/2] net/filter: Permit reading NET in load_bytes_relative when MAC not set

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(-)

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