netdev - Re: [PATCH v2 bpf-next 1/5] bpf: Hooks for sys

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: <b1d94917-d362-f872-a3f7-09d3bc770543@iogearbox.net>
Date:   Fri, 25 May 2018 02:59:37 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrey Ignatov <rdna@...com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, kafai@...com, ast@...nel.org,
        kernel-team@...com
Subject: Re: [PATCH v2 bpf-next 1/5] bpf: Hooks for sys_sendmsg

On 05/23/2018 01:40 AM, Andrey Ignatov wrote:
[...]
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index ff4d4ba..a1f9ba2 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -900,6 +900,7 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  {
>  	struct inet_sock *inet = inet_sk(sk);
>  	struct udp_sock *up = udp_sk(sk);
> +	DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
>  	struct flowi4 fl4_stack;
>  	struct flowi4 *fl4;
>  	int ulen = len;
> @@ -954,8 +955,7 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  	/*
>  	 *	Get and verify the address.
>  	 */
> -	if (msg->msg_name) {
> -		DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
> +	if (usin) {
>  		if (msg->msg_namelen < sizeof(*usin))
>  			return -EINVAL;
>  		if (usin->sin_family != AF_INET) {
> @@ -1009,6 +1009,22 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		rcu_read_unlock();
>  	}
>  
> +	if (!connected) {
> +		err = BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk,
> +					    (struct sockaddr *)usin, &ipc.addr);
> +		if (err)
> +			goto out_free;
> +		if (usin) {
> +			if (usin->sin_port == 0) {
> +				/* BPF program set invalid port. Reject it. */
> +				err = -EINVAL;
> +				goto out_free;
> +			}
> +			daddr = usin->sin_addr.s_addr;
> +			dport = usin->sin_port;
> +		}
> +	}
> +
>  	saddr = ipc.addr;
>  	ipc.addr = faddr = daddr;
>  
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 2839c1b..67c44b5 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1315,6 +1315,29 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		fl6.saddr = np->saddr;
>  	fl6.fl6_sport = inet->inet_sport;
>  
> +	if (!connected) {
> +		err = BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk,
> +					   (struct sockaddr *)sin6, &fl6.saddr);
> +		if (err)
> +			goto out_no_dst;
> +		if (sin6) {
> +			if (ipv6_addr_v4mapped(&sin6->sin6_addr)) {
> +				/* BPF program rewrote IPv6-only by IPv4-mapped
> +				 * IPv6. It's currently unsupported.
> +				 */
> +				err = -ENOTSUPP;
> +				goto out_no_dst;
> +			}
> +			if (sin6->sin6_port == 0) {
> +				/* BPF program set invalid port. Reject it. */
> +				err = -EINVAL;
> +				goto out_no_dst;
> +			}
> +			fl6.fl6_dport = sin6->sin6_port;
> +			fl6.daddr = sin6->sin6_addr;
> +		}

Hmm, this extra work here and in v4 case should probably all be done under
the static key? Otherwise we'll do the extra work for checking sin6 and
setting up fl6 twice? Also, when not enabled, couldn't we run into the case
of ipv6_addr_v4mapped() as well? If I'm spotting this right, then we would
bail out though we shouldn't normally?

> +	}
> +
>  	final_p = fl6_update_dst(&fl6, opt, &final);
>  	if (final_p)
>  		connected = false;
> @@ -1394,6 +1417,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  
>  out:
>  	dst_release(dst);
> +out_no_dst:
>  	fl6_sock_release(flowlabel);
>  	txopt_put(opt_to_free);
>  	if (!err)
>