Page MenuHomeFreeBSD

tcp, udp: Permit binding with AF_UNSPEC if the address is INADDR_ANY
ClosedPublic

Authored by markj on May 28 2021, 10:42 PM.

Details

Summary

Prior to commit f161d294b we only checked the sockaddr length, but now
we verify the address family as well. This breaks ttcp. Relax the
check to avoid breaking compatibility too much: permit AF_UNSPEC if the
address is INADDR_ANY. This matches Linux.

Fixes: f161d294b
Reported by: Bakul Shah

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Hmm. What about the corresponding case for IPv6 sockets? ttcp would still fail, since it also provides a sin_len of 0. I was expecting some fallout...

Hmm. What about the corresponding case for IPv6 sockets? ttcp would still fail, since it also provides a sin_len of 0. I was expecting some fallout...

Do you know of any applications that depend on that? ttcp is IPv4-only.

sin_len is a funny case. bind(2), connect(2) and sendto(2) also have a socklen parameter. Our getsockaddr(), which is responsible for copying in sockaddrs from user memory, overwrites the sa_len with the length provided in that parameter. So the sin_len provided by userspace is effectively ignored, for better or worse.

Hmm. What about the corresponding case for IPv6 sockets? ttcp would still fail, since it also provides a sin_len of 0. I was expecting some fallout...

Do you know of any applications that depend on that? ttcp is IPv4-only.

No, I don't. Just prefer consistency, But if you prefer to add a special case only for IPv4, I'm fine.

sin_len is a funny case. bind(2), connect(2) and sendto(2) also have a socklen parameter. Our getsockaddr(), which is responsible for copying in sockaddrs from user memory, overwrites the sa_len with the length provided in that parameter. So the sin_len provided by userspace is effectively ignored, for better or worse.

Ahh, OK. I missed that. Then I'm fine.

This revision is now accepted and ready to land.May 28 2021, 11:48 PM

Hmm. What about the corresponding case for IPv6 sockets? ttcp would still fail, since it also provides a sin_len of 0. I was expecting some fallout...

Do you know of any applications that depend on that? ttcp is IPv4-only.

No, I don't. Just prefer consistency, But if you prefer to add a special case only for IPv4, I'm fine.

Well, this way we are more consistent with other kernels. :)

Linux and OpenBSD are strict about checking sa_family == AF_INET6 so I think it is reasonable to do the same.

Hi Mark,

I'd personally like the approach D29519 . Is it desirable to have a dedicated sysctl variable to flag the compatible mode? The default value could be set to true.

Hi Mark,

I'd personally like the approach D29519 . Is it desirable to have a dedicated sysctl variable to flag the compatible mode? The default value could be set to true.

I'm not sure. This is really a quite minor modification to D29519, I'd personally prefer not to add yet another sysctl for it. Maybe we should make this behaviour conditional on COMPAT_FREEBSD13 instead.

Hi Mark,

I'd personally like the approach D29519 . Is it desirable to have a dedicated sysctl variable to flag the compatible mode? The default value could be set to true.

I'm not sure. This is really a quite minor modification to D29519, I'd personally prefer not to add yet another sysctl for it. Maybe we should make this behaviour conditional on COMPAT_FREEBSD13 instead.

I agree, no sysctl variable. I'm fine with adding it to COMPAT_FREEBSD13.

Hi Mark,

I'd personally like the approach D29519 . Is it desirable to have a dedicated sysctl variable to flag the compatible mode? The default value could be set to true.

I'm not sure. This is really a quite minor modification to D29519, I'd personally prefer not to add yet another sysctl for it. Maybe we should make this behaviour conditional on COMPAT_FREEBSD13 instead.

I agree, no sysctl variable. I'm fine with adding it to COMPAT_FREEBSD13.

I'll do it as a separate change, since this one should be MFCed to stable/13.

This revision was landed with ongoing or failed builds.May 31 2021, 11:10 PM
This revision was automatically updated to reflect the committed changes.