netdev - Re: [PATCH bpf-next v4 07/11] bpf: Add support for reading sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 6 Jan 2018 02:46:10 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Lawrence Brakmo <brakmo@...com>, netdev <netdev@...r.kernel.org>
Cc:     Kernel Team <kernel-team@...com>, Blake Matheny <bmatheny@...com>,
        Alexei Starovoitov <ast@...com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH bpf-next v4 07/11] bpf: Add support for reading sk_state
 and more

On 01/05/2018 12:55 AM, Lawrence Brakmo wrote:
> Add support for reading many more tcp_sock fields
> 
>   state,	same as sk->sk_state
>   rtt_min	same as sk->rtt_min.s[0].v (current rtt_min)
>   snd_ssthresh
>   rcv_nxt
>   snd_nxt
>   snd_una
>   mss_cache
>   ecn_flags
>   rate_delivered
>   rate_interval_us
>   packets_out
>   retrans_out
>   total_retrans
>   segs_in
>   data_segs_in
>   segs_out
>   data_segs_out
>   bytes_received (__u64)
>   bytes_acked    (__u64)
> 
> Signed-off-by: Lawrence Brakmo <brakmo@...com>
> ---
>  include/uapi/linux/bpf.h |  19 +++++++++
>  net/core/filter.c        | 101 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a1316c7..a8f4cf0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -963,6 +963,25 @@ struct bpf_sock_ops {
>  	__u32 snd_cwnd;
>  	__u32 srtt_us;		/* Averaged RTT << 3 in usecs */
>  	__u32 bpf_sock_ops_flags; /* flags defined in uapi/linux/tcp.h */
> +	__u32 state;
> +	__u32 rtt_min;
> +	__u32 snd_ssthresh;
> +	__u32 rcv_nxt;
> +	__u32 snd_nxt;
> +	__u32 snd_una;
> +	__u32 mss_cache;
> +	__u32 ecn_flags;
> +	__u32 rate_delivered;
> +	__u32 rate_interval_us;
> +	__u32 packets_out;
> +	__u32 retrans_out;
> +	__u32 total_retrans;
> +	__u32 segs_in;
> +	__u32 data_segs_in;
> +	__u32 segs_out;
> +	__u32 data_segs_out;
> +	__u64 bytes_received;
> +	__u64 bytes_acked;
>  };
>  
>  /* List of known BPF sock_ops operators.
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 76fd6e9..d4c5c1a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3829,7 +3829,7 @@ static bool __is_valid_sock_ops_access(int off, int size)
>  	/* The verifier guarantees that size > 0. */
>  	if (off % size != 0)
>  		return false;
> -	if (size != sizeof(__u32))
> +	if (size != sizeof(__u32) && size != sizeof(__u64))
>  		return false;

Doesn't this have the side-effect that this would kind of let is_valid_access()
callback allow not for narrower but wider access, e.g. on the u32 members when
doing BPF_DW. It would still get ignored later on in the ctx rewrite, though, but
it also means that we could try with 2nd part of u64 member offsets with BPF_W
which would then return a 'verifier is misconfigured' error. I think it would
be better to enforce BPF_DW access only on the u64 types and BPF_W access only
on the u32 types before it becomes uapi instead opening this up generically. E.g.
we do similar approach in pe_prog_is_valid_access() for sample_period (taking the
the narrow access bits aside from there).

Thanks,
Daniel