netdev - Re: [PATCH net-next v5 07/16] bpf: Add setsockopt helper function to bpf

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <18C526C9-D95A-4D93-8A1A-95A4E65AC0D0@fb.com>
Date:   Sat, 1 Jul 2017 01:45:35 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        netdev <netdev@...r.kernel.org>
CC:     Kernel Team <Kernel-team@...com>, Blake Matheny <bmatheny@...com>,
        "Alexei Starovoitov" <ast@...com>,
        David Ahern <dsa@...ulusnetworks.com>
Subject: Re: [PATCH net-next v5 07/16] bpf: Add setsockopt helper function to
 bpf

On 6/30/17, 5:01 PM, "Daniel Borkmann" <daniel@...earbox.net> wrote:

    On 06/30/2017 10:06 PM, Lawrence Brakmo wrote:
    [...]
    > @@ -2672,6 +2673,69 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
    >   	.arg1_type      = ARG_PTR_TO_CTX,
    >   };
    >
    > +BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
    > +	   int, level, int, optname, char *, optval, int, optlen)
    > +{
    > +	struct sock *sk = bpf_sock->sk;
    > +	int ret = 0;
    > +	int val;
    > +
    > +	if (!sk_fullsock(sk))
    > +		return -EINVAL;
    > +
    > +	if (level == SOL_SOCKET) {
    > +		/* Only some socketops are supported */
    > +		val = *((int *)optval);
    > +
    > +		switch (optname) {
    > +		case SO_RCVBUF:
    > +			sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
    > +			sk->sk_rcvbuf = max_t(int, val * 2, SOCK_MIN_RCVBUF);
    > +			break;
    > +		case SO_SNDBUF:
    > +			sk->sk_userlocks |= SOCK_SNDBUF_LOCK;
    > +			sk->sk_sndbuf = max_t(int, val * 2, SOCK_MIN_SNDBUF);
    > +			break;
    > +		case SO_MAX_PACING_RATE:
    > +			sk->sk_max_pacing_rate = val;
    > +			sk->sk_pacing_rate = min(sk->sk_pacing_rate,
    > +						 sk->sk_max_pacing_rate);
    > +			break;
    > +		case SO_PRIORITY:
    > +			sk->sk_priority = val;
    > +			break;
    > +		case SO_RCVLOWAT:
    > +			if (val < 0)
    > +				val = INT_MAX;
    > +			sk->sk_rcvlowat = val ? : 1;
    > +			break;
    > +		case SO_MARK:
    > +			sk->sk_mark = val;
    > +			break;
    > +		default:
    > +			ret = -EINVAL;
    > +		}
    > +	} else if (level == SOL_TCP &&
    > +		   sk->sk_prot->setsockopt == tcp_setsockopt) {
    > +		/* Place holder */
    > +		ret = -EINVAL;
    > +	} else {
    > +		ret = -EINVAL;
    > +	}
    > +	return ret;
    > +}
    > +
    > +static const struct bpf_func_proto bpf_setsockopt_proto = {
    > +	.func		= bpf_setsockopt,
    > +	.gpl_only	= true,
    > +	.ret_type	= RET_INTEGER,
    > +	.arg1_type	= ARG_PTR_TO_CTX,
    > +	.arg2_type	= ARG_ANYTHING,
    > +	.arg3_type	= ARG_ANYTHING,
    > +	.arg4_type	= ARG_PTR_TO_MEM,
    > +	.arg5_type	= ARG_CONST_SIZE_OR_ZERO,
    
    Hm, I had some feedback on this in your last revision of the patch
    set [1] that a NULL pointer dereference can be triggered here. Probably
    oversaw it; I mentioned wrt the above:
    
       Any reason you went with the ARG_CONST_SIZE_OR_ZERO type? Semantics
       of this are that allowed [arg4, arg5] pair can be i) [NULL, 0] or
       ii) [non-NULL, non-zero], where in case ii) verifier checks that the
       area is initialized when coming from BPF stack.
    
       So above 'val = *((int *)optval);' would give a NULL pointer deref
       with NULL passed as arg or in case optlen was < sizeof(int) we access
       stack out of bounds potentially. If the [NULL, 0] pair is not required,
       I would just make that a ARG_CONST_SIZE and then check for size before
       accessing optval.
    
    Would be good if you could still address it in a most likely final respin.
    
    Thanks,
    Daniel

Working on it, will have it soon. Thanks for all the feedback!
    
       [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_781800_&d=DwIC-g&c=5VD0RTtNlTh3ycd41b3MUw&r=pq_Mqvzfy-C8ltkgyx1u_g&m=6nijuGf2vMueVsU1CETOPc1IB9xCd3ApP5vUppoXe_A&s=l4VJ1IUAA1qjTa0fMvOxsKRdoa361lM65q5QGKteEMw&e=