netdev - Re: [PATCH 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: <20180522180414.GA22817@rdna-mbp>
Date:   Tue, 22 May 2018 11:04:18 -0700
From:   Andrey Ignatov <rdna@...com>
To:     Martin KaFai Lau <kafai@...com>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>, <ast@...nel.org>,
        <daniel@...earbox.net>, <kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/5] bpf: Hooks for sys_sendmsg

Martin KaFai Lau <kafai@...com> [Mon, 2018-05-21 16:17 -0700]:
> On Fri, May 18, 2018 at 07:21:09PM -0700, Andrey Ignatov wrote:
[...]
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index 2839c1b..6f580ea 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -1315,6 +1315,22 @@ 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 (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;
> Could the bpf_prog change sin6 to a v4mapped address?

It's a good point.

Yes, bpf_prog can rewrite sin6->sin6_addr with any IPv6 address,
including IPv4-mapped IPv6 and this is not handled in this patch.

I see two possible options to handle this:

1) Return with error similar to how it's done for (sin6_port == 0).

2) Or delegate to udp_sendmsg() similar to how it's done in the
   beginning of udpv6_sendmsg().

By this point udpv6_sendmsg already checked that destination is not
IPv4-mapped IPv6. And I think it might not be a good idea to rewrite
IPv6-only address with IPv4-mapped IPv6 in bpf_prog.

So IMO "1)" is safer since user passed IPv6-only destination
(IPv4-mapped IPv6 passed by user don't reach this point) and both
msg->msg_name and msg->msg_control may have IPv6-only fields, e.g.
sin6_flowinfo in msg->msg_name or SOL_IPV6 options in msg->msg_control.
Also a few steps were already done based on this by that time, e.g.
ancillary data, if present, was already parsed successfully by
ip6_datagram_send_ctl().

As for specific errno, ENOTSUPP can be returned for now so that in the
future there is a way to extend this code to support IPv4-mapped IPv6 if
needed.

I'll send v2 with this change.

> > +		}
> > +	}
> > +
> >  	final_p = fl6_update_dst(&fl6, opt, &final);
> It seems fl6_update_dst() may update fl6->daddr.
> Is it fine (or inline with the expectation after running
> a bpf_prog that has changed user_ip6)?

Yeah, I checked this code and from what I see it should be fine.

If fl6_update_dst updates fl6->daddr, then it returns original daddr
(e.g. that set by bpf_prog) as final_p. Later udpv6_sendmsg calls
ip6_sk_dst_lookup_flow where the new daddr is used to lookup flow and it
is set back to final_p in ip6_dst_lookup_flow:
	if (final_dst)
		fl6->daddr = *final_dst;

So final value of fl6->daddr will be daddr set by bpf_prog.

> >  	if (final_p)
> >  		connected = false;
> > @@ -1394,6 +1410,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >  
> >  out:
> >  	dst_release(dst);
> > +out_no_dst:
> A nit.  If dst is init to NULL, can a new exit label
> be avoided?

Yes, if dst was init to NULL, new label could be avoided. But it's not
initialized when created. It can probably be changed, but I'd need to
check all dst usages to make sure that it's fine to remove `dst = NULL`
from other places. Maybe in another patch.

> >  	fl6_sock_release(flowlabel);
> >  	txopt_put(opt_to_free);
> >  	if (!err)
> > -- 
> > 2.9.5
> > 

-- 
Andrey Ignatov