Bug#645469: bind() fails for AF_UNIX sockets with EINVAL

[Jonathan Nieder <jrnieder@gmail.com>]

Robert Millan wrote:
> 2011/10/16 Jonathan Nieder <jrnieder@gmail.com>:

>>        unsigned char sun_len;
>>        unsigned char sun_family;
>>        char sun_path[108];     /* Path name. */
>
> Is this 108 the actual length?  ISTR it was just a placeholder.

It's the actual size of the sun_path field of struct sockaddr_un,
if that's what you mean.

> <sys/un.h> has a macro to determine actual length:
>
> /* Evaluate to actual length of the `sockaddr_un' structure.  */
> # define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)        \
>                       + strlen ((ptr)->sun_path))

Ah, thanks for this clarification.  glibc's reference manual says:

	You should compute the "length" parameter for a socket address in
	the local namespace as the sum of the size of the "sun_family"
	component and the string length (/not/ the allocation size!) of
	the file name string.  This can be done using the macro "SUN_LEN":

In other words, you can call bind() with SUN_LEN(ptr) as the addrlen
argument instead of sizeof(struct sockaddr_un) to tell the kernel not to
copy so much, and to avoid depending on the magic number chosen in the
definition of sockaddr_un.

However, the POSIX documentation for bind() gives an example of an AF_UNIX
socket with

	if (bind(sfd, (struct sockaddr *) &my_addr,
		sizeof(struct sockaddr_un)) == -1)

Similarly, the example in the bind(2) manpage from the man-pages project
uses sizeof(struct sockaddr_un) as the addrlen argument.

Regarding the magic number, POSIX provides some more insight:

	The size of "sun_path" has intentionally been left undefined.
	This is because different implementations use different sizes.
	For example, 4.3 BSD uses a size of 108, and 4.4 BSD uses a
	size of 104.  Since most implementations originate from BSD
	versions, the size is typically in the range 92 to 108.

[...]
>> I wonder whether there would be any downside to changing that 104 in
>> the kernel to 108.
>
> Breaking kernel ABI for everyone not using a patched kernel.  Not a good thing.

If it's desirable, the change could be made upstream.

>> Separately from that, it would be helpful to know where the buffer
>> overflowed in #645377 is, since maybe it could be made bigger without
>> changing the layout of struct sockaddr_un.
>
> I don't think this matters, it's an instance of sockaddr_un.

If sockaddr_un is part of the ABI as an argument to some function,
it would matter.

> If you
> make sun_path[] bigger in the kernel, then instead of 160 you can
> overflow it with a larger value
[...]
> The kernel-side of things is now doing the right thing AFAICS.

It's breaking userspace apps that followed documentation to the letter
and worked before.

How about this (untested)?

diff --git i/sys/kern/uipc_syscalls.c w/sys/kern/uipc_syscalls.c
index 3b83e1c..7b4a11e 100644
--- i/sys/kern/uipc_syscalls.c
+++ w/sys/kern/uipc_syscalls.c
@@ -1703,11 +1703,18 @@ getsockaddr(namp, uaddr, len)
 	if (error) {
 		free(sa, M_SONAME);
 	} else {
+		const char *p;
+		size_t datalen;
+
 #if defined(COMPAT_OLDSOCK) && BYTE_ORDER != BIG_ENDIAN
 		if (sa->sa_family == 0 && sa->sa_len < AF_MAX)
 			sa->sa_family = sa->sa_len;
 #endif
 		sa->sa_len = len;
+		datalen = len - offsetof(struct sockaddr, sa_data[0]);
+		p = memchr(sa->sa_data, '\0', datalen);
+		if (p)
+			sa_len = p - (const char *)sa;
 		*namp = sa;
 	}
 	return (error);