sizeof is a tricky operator, which may return a larger value than you would expect. This depends on the allocation scheme of the compiler, which knows how to pad a structure to the alignment and addressing requirements of the underlying hardware. So in for each of your cases it had to be checked if the comparison should be "==", "<", or ">=" to cope with remote data structures generated externally compiled software.

In D29519#661804, @donner wrote:

sizeof is a tricky operator, which may return a larger value than you would expect. This depends on the allocation scheme of the compiler, which knows how to pad a structure to the alignment and addressing requirements of the underlying hardware. So in for each of your cases it had to be checked if the comparison should be "==", "<", or ">=" to cope with remote data structures generated externally compiled software.

I don't quite follow - the ABI assumes that different compilers will generate binary-compatible layouts of these structures. In some cases sizeof is indeed inappropriate, as in ng_socket.c for example. In many of these cases we were already performing checks of the form if (sin->sin_len != sizeof(struct sockaddr_in)), they were just happening too late.

Mark,

Thanks for doing this! It looks like a very positive change, and I'm sure there was a lot of effort put into finding the right way to clean up the code.

I have a general question about IPv4 addresses on IPv6 sockets which is really larger than your change. However, I think it may be worth resolving it prior to committing this patch. (Or, alternatively, it is probably worth at least making sure that this patch doesn't make an unintended change in the way IPv4 addresses work when used on IPv6 sockets.)

Jonathan

Ping? Any comments on the overall approach, or on the details of the change?

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

(1) On an AF_INET socket, only sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.
(2) On an AF_INET6 socket, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a non-mapped address can be used.
(3) On an AF_INET6 socket which is V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can not be used.
(4) On an AF_INET6 socket which is not V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can be used.
(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

Are the corresponding checks OK for the various socket options which pass in IP-addresses? It looks like the diff covers only bind()/connect()/sendto()?

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

I believe @tuexen summarized it well. I appreciate him following up; I was supposed to add a note from the last meeting to this review, but hadn't yet done so.

Mark, you are more than welcome to attend the next transport call (April 22 at 1500 UTC) if you would like to discuss this in real time.

Jonathan

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

(1) On an AF_INET socket, only sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.
(2) On an AF_INET6 socket, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a non-mapped address can be used.
(3) On an AF_INET6 socket which is V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can not be used.
(4) On an AF_INET6 socket which is not V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can be used.
(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Are the corresponding checks OK for the various socket options which pass in IP-addresses? It looks like the diff covers only bind()/connect()/sendto()?

That's a good question. in6_control() (an ioctl handler) does not validate the length, but that is less concerning since struct in6_ifreq and struct in6_aliasreq embed a struct sockaddr_in6.

The IPV6_2292NEXTHOP and IPV6_NEXTHOP sockopt handlers do indeed check specifically for AF_INET6. I do not see any other options which operate on sockaddrs. I could easily have missed something.

In D29519#666613, @jtl wrote:

Mark, you are more than welcome to attend the next transport call (April 22 at 1500 UTC) if you would like to discuss this in real time.

I will try to make that one.

In D29519#666666, @markj wrote:

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

(1) On an AF_INET socket, only sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.
(2) On an AF_INET6 socket, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a non-mapped address can be used.
(3) On an AF_INET6 socket which is V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can not be used.
(4) On an AF_INET6 socket which is not V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can be used.
(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Maybe the IPPROTO_SCTP level socket options? There is at least a IPPROTO_SCTP level socket option SCTP_I_WANT_MAPPED_V4_ADDR (see https://tools.ietf.org/html/rfc6458#section-8.1.15 for the specification), but this handles to kernel -> userland direction. I think (that is what I need to double check) we support a mixture of sockaddr_in and sockaddr_in6 in calls like sctp_bindx() and sctp_connectx(), but I'm not sure.

Are the corresponding checks OK for the various socket options which pass in IP-addresses? It looks like the diff covers only bind()/connect()/sendto()?

That's a good question. in6_control() (an ioctl handler) does not validate the length, but that is less concerning since struct in6_ifreq and struct in6_aliasreq embed a struct sockaddr_in6.

The IPV6_2292NEXTHOP and IPV6_NEXTHOP sockopt handlers do indeed check specifically for AF_INET6. I do not see any other options which operate on sockaddrs. I could easily have missed something.

In D29519#666613, @jtl wrote:

Mark, you are more than welcome to attend the next transport call (April 22 at 1500 UTC) if you would like to discuss this in real time.

I will try to make that one.

In D29519#666669, @tuexen wrote:

In D29519#666666, @markj wrote:

In D29519#666595, @tuexen wrote:

(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Maybe the IPPROTO_SCTP level socket options? There is at least a IPPROTO_SCTP level socket option SCTP_I_WANT_MAPPED_V4_ADDR (see https://tools.ietf.org/html/rfc6458#section-8.1.15 for the specification), but this handles to kernel -> userland direction. I think (that is what I need to double check) we support a mixture of sockaddr_in and sockaddr_in6 in calls like sctp_bindx() and sctp_connectx(), but I'm not sure.

I see, thanks. Indeed, those SCTP-specific entry points seem to handle both address families (and include corresponding checks on the sockaddrs). As far as I can tell, SCTP is unique in this regard. I believe this diff won't change anything with respect to sctp_bindx() and _connectx().

In D29519#666731, @markj wrote:

In D29519#666669, @tuexen wrote:

In D29519#666666, @markj wrote:

In D29519#666595, @tuexen wrote:

(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Maybe the IPPROTO_SCTP level socket options? There is at least a IPPROTO_SCTP level socket option SCTP_I_WANT_MAPPED_V4_ADDR (see https://tools.ietf.org/html/rfc6458#section-8.1.15 for the specification), but this handles to kernel -> userland direction. I think (that is what I need to double check) we support a mixture of sockaddr_in and sockaddr_in6 in calls like sctp_bindx() and sctp_connectx(), but I'm not sure.

I see, thanks. Indeed, those SCTP-specific entry points seem to handle both address families (and include corresponding checks on the sockaddrs). As far as I can tell, SCTP is unique in this regard. I believe this diff won't change anything with respect to sctp_bindx() and _connectx().

I don't think either. But the point of the discussion was to get some consistent behaviour... That is why we wanted to look at the code before coming to a conclusion.

I have a general question about compatibility.
Recently I landed a similar change (D28668) in the rtsock space. It turned out that (older) applications do a lot of weird things: some send the length less than expected (recall sockaddr_in has a weird sin_zero field), some send zero-length, etc.
I'm pretty sure that some applications will break in some weird ways - just because we didn't have these checks in place before.
What's our position here? Do we want to let the apps break and insisting on fixing the code, or we consider guarding these checks under, for example #ifndef COMPAT_FREEBSD12?

In D29519#672110, @melifaro wrote:

I have a general question about compatibility.
Recently I landed a similar change (D28668) in the rtsock space. It turned out that (older) applications do a lot of weird things: some send the length less than expected (recall sockaddr_in has a weird sin_zero field), some send zero-length, etc.
I'm pretty sure that some applications will break in some weird ways - just because we didn't have these checks in place before.
What's our position here? Do we want to let the apps break and insisting on fixing the code, or we consider guarding these checks under, for example #ifndef COMPAT_FREEBSD12?

I think the situation here is not as bad as the routing socket code:

1. In common cases we were already validating sockaddr lengths, just not early enough.
2. getsockaddr() is responsible for allocating all of the sockaddrs covered by this diff. It does some basic validation (really just guards against zero-length sockaddrs). Note that when passing a sockaddr via bind(2), connect(2), etc.., it ignores the sa_len in the user-provided sockaddr.
3. These are standard APIs, and other operating systems perform this validation as well. Linux appears to be more relaxed at a cursory glance, it just requires that addrlen >= sizeof(struct sockaddr_foo). OpenBSD is strict.

I can see an argument for relaxing these checks to follow Linux's example, but in general I'd expect application behaviour here to be more normalized than it is for routing sockets. My plan is to give a reasonable MFC window to catch fallout, but I don't expect to have to provide compatibility ifdefs.