On Mon, 13 Jun 2016 19:57:15 +0200 Jeremie Courreges-Anglas <[email protected]> wrote:
> Vincent Gross <[email protected]> writes: > > > Le Mon, 13 Jun 2016 07:35:16 +0200, > > [email protected] (Jeremie Courreges-Anglas) a écrit : > > > >> [email protected] (Jeremie Courreges-Anglas) writes: > >> > >> > cc'ing sthen since he also has interest in IP_SENDSRCADDR > >> > > >> > Jeremie Courreges-Anglas <[email protected]> writes: > >> > > >> >> Vincent Gross <[email protected]> writes: > >> >> > >> >>> This diff adds support for IP_SENDSRCADDR cmsg on UDP sockets. > >> >>> As for udp6_output(), we check that the source address+port is > >> >>> available only if inp_laddr != * > >> >> > >> >> Your last IP_SENDSRCADDR diff didn't have that check, I think > >> >> it is harmful. If the socket is not bound then there is > >> >> effectively no check performed by in_pcbaddrisavail(), thus I > >> >> can use any random address. Other than this additional bypass > >> >> check, your diff looks good to me. > >> >> > > [...] > >> >> > >> >> I haven't checked yet whether udp6_output is also affected. If > >> >> you folks already know that it isn't, please let me know. > >> > >> The answer is "no", a few tests can't trigger the same problem. > >> IIUC in6_selectsrc is responsible for rejection of non-local > >> systems. Maybe we should take the same approach in netinet/, and > >> extend in_selectsrc()? > >> > >> -- > > > > While validating source address inside selection functions is the > > right direction, I don't think it would be a good thing to extend > > further in_selectsrc() prototype. > > I find it nice to have all the source address selection in one place. > Or do you have another refactoring in mind? > Uh, turns out I was operating on obsolete data. I would actually be easy to shrink in_selectsrc() prototype to (int)(struct in_addr **, struct sockaddr_in *, struct in_pcb *). But this looks like a layering violation to me ... What do you think ? $ grep -r in_selectsrc sys/net* sys/netinet/in_pcb.c sys/netinet/in_pcb.h sys/netinet/udp_usrreq.c $ cd sys/netinet $ grep -A2 in_selectsrc error = in_selectsrc(&ina, sin, inp->inp_moptions, &inp->inp_route, &inp->inp_laddr, inp->inp_rtableid); if (error) in_selectsrc(struct in_addr **insrc, struct sockaddr_in *sin, struct ip_moptions *mopts, struct route *ro, struct in_addr *laddr, u_int rtableid) $ grep -A2 in_selectsrc udp_usrreq.c error = in_selectsrc(&laddr, sin, inp->inp_moptions, &inp->inp_route, &inp->inp_laddr, inp->inp_rtableid); if (error) > > However it is easy to add a check while > > processing cmsg. > > > > rev2 below. Ok ? > > Nits below, looks fine otherwise. The checks do detect addresses not > configured on the system and overlaps of bound sockets. > > > > > diff --git a/share/man/man4/ip.4 b/share/man/man4/ip.4 > > index 111432b..154b0d1 100644 > > --- a/share/man/man4/ip.4 > > +++ b/share/man/man4/ip.4 > > @@ -290,6 +290,27 @@ cmsg_len = CMSG_LEN(sizeof(u_int)) > > cmsg_level = IPPROTO_IP > > cmsg_type = IP_RECVRTABLE > > .Ed > > +.Pp > > +If the > > +.Dv IP_SENDSRCADDR > > +option is passed to a > > +.Xr sendmsg 2 > > +call on a > > +.Dv SOCK_DGRAM > > +socket, the address passed along the > > +.Vt cmsghdr > > +structure will be used as the source of the outgoing > > +.Tn UDP > > +datagram. The > > +.Vt cmsghdr > > +fields for > > +.Xr sendmsg 2 > > +have the following values: > > I would have worded it "should have" here, since these are the values > that the developer is supposed to pass. Yes, I have to find a better wording for this part. > > > +.Bd -literal -offset indent > > +cmsg_len = CMSG_LEN(sizeof(struct in_addr)) > > +cmsg_level = IPPROTO_IP > > +cmsg_type = IP_SENDSRCADDR > > +.Ed > > .Ss "Multicast Options" > > .Tn IP > > multicasting is supported only on > > diff --git a/sys/netinet/in.h b/sys/netinet/in.h > > index adb1b30..bf8c95d 100644 > > --- a/sys/netinet/in.h > > +++ b/sys/netinet/in.h > > @@ -307,6 +307,7 @@ struct ip_opts { > > #define IP_RECVRTABLE 35 /* bool; receive rdomain > > w/dgram */ #define IP_IPSECFLOWINFO 36 /* bool; IPsec flow > > info for dgram */ #define IP_IPDEFTTL 37 /* int; > > IP TTL system default */ +#define IP_SENDSRCADDR > > 38 /* struct in_addr; source address to use */ > > #define IP_RTABLE 0x1021 /* int; routing > > table, see SO_RTABLE */ #define IP_DIVERTFL > > 0x1022 /* int; divert direction flag opt */ diff --git > > a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c index > > 1feea11..401ed7a 100644 --- a/sys/netinet/udp_usrreq.c > > +++ b/sys/netinet/udp_usrreq.c > > @@ -888,6 +888,7 @@ udp_output(struct inpcb *inp, struct mbuf *m, > > struct mbuf *addr, struct sockaddr_in *sin = NULL; > > struct udpiphdr *ui; > > u_int32_t ipsecflowinfo = 0; > > + struct sockaddr_in src_sin; > > int len = m->m_pkthdr.len; > > struct in_addr *laddr; > > int error = 0; > > @@ -906,6 +907,8 @@ udp_output(struct inpcb *inp, struct mbuf *m, > > struct mbuf *addr, goto release; > > } > > > > + memset(&src_sin, 0, sizeof(src_sin)); > > + > > if (control) { > > u_int clen; > > struct cmsghdr *cm; > > @@ -939,9 +942,20 @@ udp_output(struct inpcb *inp, struct mbuf *m, > > struct mbuf *addr, cm->cmsg_level == IPPROTO_IP && > > cm->cmsg_type == IP_IPSECFLOWINFO) { > > ipsecflowinfo = *(u_int32_t > > *)CMSG_DATA(cm); > > - break; > > - } > > + } else > > #endif > > + if (cm->cmsg_len == CMSG_LEN(sizeof(struct > > in_addr)) && > > + cm->cmsg_level == IPPROTO_IP && > > + cm->cmsg_type == IP_SENDSRCADDR) { > > + bzero(&src_sin, sizeof(src_sin)); > > No need for bzero here since you unconditionally call memset above. Duh. > > > + memcpy(&src_sin.sin_addr, > > CMSG_DATA(cm), > > + sizeof(struct in_addr)); > > + src_sin.sin_family = AF_INET; > > + src_sin.sin_len = sizeof(src_sin); > > + /* no check on reuse done when > > sin->sin_port == 0 */ > > + if ((error = > > in_pcbaddrisavail(inp, &src_sin, 0, curproc))) > > Lines > 80 chars. > > > + goto release; > > + } > > clen -= CMSG_ALIGN(cm->cmsg_len); > > cmsgs += CMSG_ALIGN(cm->cmsg_len); > > } while (clen); > > @@ -980,6 +994,17 @@ udp_output(struct inpcb *inp, struct mbuf *m, > > struct mbuf *addr, if (error) > > goto release; > > } > > + > > + if (src_sin.sin_len > 0 && > > + src_sin.sin_addr.s_addr != INADDR_ANY && > > + src_sin.sin_addr.s_addr != > > inp->inp_laddr.s_addr) { > > + src_sin.sin_port = inp->inp_lport; > > + if (inp->inp_laddr.s_addr != INADDR_ANY && > > + (error = > > + in_pcbaddrisavail(inp, &src_sin, 0, > > curproc))) > > + goto release; > > + laddr = &src_sin.sin_addr; > > + } > > } else { > > if (inp->inp_faddr.s_addr == INADDR_ANY) { > > error = ENOTCONN; > > > > >
