Vincent Gross <vgr...@openbsd.org> writes:
> On Wed, 31 Aug 2016 15:26:53 +0200
> Vincent Gross <vgr...@openbsd.org> wrote:
>
>> On Thu, 11 Aug 2016 16:57:27 +0100
>> Stuart Henderson <s...@spacehopper.org> wrote:
>>
>> > On 2016/06/27 13:00, Jérémie Courrèges-Anglas wrote:
>> [...]
>> > >
>> > > I also gave my ok to vgross by IM.
>> > >
>> > > I know that some concerns have been exposed privately, I was not
>> > > Cc'd, thus I have no idea what is the current status of that
>> > > discussion. To the people concerned, please keep me / us updated
>> > > about that discussion and Cc us.
>> >
>> > How are things looking with IN_SENDSRCADDR now, are there any
>> > remaining concerns that need fixing before it could be committed?
>> > (Also if anyone has a share-able diff to use this with iked it
>> > would be quite handy..)
>> >
>>
>> Tested locally with two iked on two distinct rdomains plus a bit of
>> LD_PRELOAD goop. Unfortunately I couldn't ping from one rdom to the
>> other, but I also have this problem without my patch, so I am
>> confident this ping problem is unrelated.
>>
>> I would be very grateful if someone could test this.
>>
>
> Take two, unmangled version :
Looks good. Nits only below, I'm not an iked user.
> Index: sbin/iked/iked.h
> ===================================================================
> RCS file: /cvs/src/sbin/iked/iked.h,v
> retrieving revision 1.96
> diff -u -p -r1.96 iked.h
> --- sbin/iked/iked.h 1 Jun 2016 11:16:41 -0000 1.96
> +++ sbin/iked/iked.h 31 Aug 2016 13:19:10 -0000
> @@ -898,6 +898,8 @@ int socket_setport(struct sockaddr *, i
> int socket_getaddr(int, struct sockaddr_storage *);
> int socket_bypass(int, struct sockaddr *);
> int udp_bind(struct sockaddr *, in_port_t);
> +ssize_t sendtofrom(int, void *, size_t, int, struct sockaddr *,
> + socklen_t, struct sockaddr *, socklen_t);
> ssize_t recvfromto(int, void *, size_t, int, struct sockaddr *,
> socklen_t *, struct sockaddr *, socklen_t *);
> const char *
> Index: sbin/iked/ikev2_msg.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 ikev2_msg.c
> --- sbin/iked/ikev2_msg.c 19 Oct 2015 11:25:35 -0000 1.45
> +++ sbin/iked/ikev2_msg.c 31 Aug 2016 13:19:10 -0000
> @@ -319,9 +319,11 @@ ikev2_msg_send(struct iked *env, struct
> msg->msg_offset += sizeof(natt);
> }
>
> - if ((sendto(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
> - (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen)) == -1) {
> - log_warn("%s: sendto", __func__);
> + if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
> + (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
> + (struct sockaddr *)&msg->msg_local, msg->msg_locallen) <
> + ibuf_size(buf)) {
You're moving those write checks from "== -1" to "< ibuf_size(buf)"
(causing additional warnings). Aren't UDP sockets supposed to be "all
or nothing" in this regard?
> + log_warn("%s: sendtofrom", __func__);
> return (-1);
> }
>
> @@ -969,10 +971,12 @@ int
> ikev2_msg_retransmit_response(struct iked *env, struct iked_sa *sa,
> struct iked_message *msg)
> {
> - if ((sendto(msg->msg_fd, ibuf_data(msg->msg_data),
> - ibuf_size(msg->msg_data), 0, (struct sockaddr *)&msg->msg_peer,
> - msg->msg_peerlen)) == -1) {
> - log_warn("%s: sendto", __func__);
> + if (sendtofrom(msg->msg_fd, ibuf_data(msg->msg_data),
> + ibuf_size(msg->msg_data), 0,
> + (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
> + (struct sockaddr *)&msg->msg_local, msg->msg_locallen) <
Trailing whitespace.
> + ibuf_size(msg->msg_data)) {
> + log_warn("%s: sendtofrom", __func__);
> return (-1);
> }
>
> @@ -996,11 +1000,12 @@ ikev2_msg_retransmit_timeout(struct iked
> struct iked_sa *sa = msg->msg_sa;
>
> if (msg->msg_tries < IKED_RETRANSMIT_TRIES) {
> - if ((sendto(msg->msg_fd, ibuf_data(msg->msg_data),
> + if (sendtofrom(msg->msg_fd, ibuf_data(msg->msg_data),
> ibuf_size(msg->msg_data), 0,
> - (struct sockaddr *)&msg->msg_peer,
> - msg->msg_peerlen)) == -1) {
> - log_warn("%s: sendto", __func__);
> + (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
> + (struct sockaddr *)&msg->msg_local, msg->msg_locallen) <
> + ibuf_size(msg->msg_data)) {
> + log_warn("%s: sendtofrom", __func__);
> sa_free(env, sa);
> return;
> }
> Index: sbin/iked/util.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/util.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 util.c
> --- sbin/iked/util.c 23 Nov 2015 19:28:34 -0000 1.30
> +++ sbin/iked/util.c 31 Aug 2016 13:19:10 -0000
> @@ -287,6 +287,57 @@ sockaddr_cmp(struct sockaddr *a, struct
> }
>
> ssize_t
> +sendtofrom(int s, void *buf, size_t len, int flags, struct sockaddr *to,
> + socklen_t tolen, struct sockaddr *from, socklen_t fromlen)
> +{
> + struct iovec iov;
> + struct msghdr msg;
> + struct cmsghdr *cmsg;
> + struct in6_pktinfo *pkt6;
> + struct sockaddr_in *in;
> + struct sockaddr_in6 *in6;
> + union {
In base the standard way to handle this is to add
struct cmsghdr hdr;
to the union. Even if not actually needed here.
> + char inbuf[CMSG_SPACE(sizeof(struct in_addr))];
> + char in6buf[CMSG_SPACE(sizeof(struct in6_pktinfo))];
> + } cmsgbuf;
> +
> + bzero(&msg, sizeof(msg));
> + bzero(&cmsgbuf, sizeof(cmsgbuf));
> +
> + iov.iov_base = buf;
> + iov.iov_len = len;
> + msg.msg_iov = &iov;
> + msg.msg_iovlen = 1;
> + msg.msg_name = to;
> + msg.msg_namelen = tolen;
> + msg.msg_control = &cmsgbuf;
> + msg.msg_controllen = sizeof(cmsgbuf);
> +
> + cmsg = CMSG_FIRSTHDR(&msg);
> + switch (to->sa_family) {
> + case AF_INET:
> + msg.msg_controllen = sizeof(cmsgbuf.inbuf);
> + cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_addr));
> + cmsg->cmsg_level = IPPROTO_IP;
> + cmsg->cmsg_type = IP_SENDSRCADDR;
> + in = (struct sockaddr_in *)from;
> + memcpy(CMSG_DATA(cmsg), &in->sin_addr, sizeof(struct in_addr));
> + break;
> + case AF_INET6:
> + msg.msg_controllen = sizeof(cmsgbuf.in6buf);
> + cmsg->cmsg_len = CMSG_LEN(sizeof(struct in6_pktinfo));
> + cmsg->cmsg_level = IPPROTO_IPV6;
> + cmsg->cmsg_type = IPV6_PKTINFO;
> + in6 = (struct sockaddr_in6 *)from;
> + pkt6 = (struct in6_pktinfo *)CMSG_DATA(cmsg);
> + pkt6->ipi6_addr = in6->sin6_addr;
memcpy, or direct struct assignement?
> + break;
> + }
> +
> + return sendmsg(s, &msg, 0);
Even if previously sendto() was used with a 0 flags argument, better not
ignore the `flags' argument here.
> +}
> +
> +ssize_t
> recvfromto(int s, void *buf, size_t len, int flags, struct sockaddr *from,
> socklen_t *fromlen, struct sockaddr *to, socklen_t *tolen)
> {
>
>
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
