Vincent Gross <[email protected]> writes:
> 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 ?
I wouldn't mind adding an extra argument and staying closer to
in6_selectsrc.
> $ 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;
>> >
>> >
>>
>
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
