link_addr: only accept '.' and ':' as separators

[ivy]

Summary

the command 'ifconfig epair2a link 10.1.2.200/28' has a somewhat
surprising result:

% ifconfig epair2a link 10.1.2.200/28
% ifconfig epair2a
epair2a: flags=1008842<BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500

options=8<VLAN_MTU>
ether 10:01:02:20:00:28
hwaddr 02:25:24:d4:38:0a

this is because link_addr(3) considers any unrecognised character, such
as '.' or '/', to be a delimiter in the Ethernet address. usually this
wouldn't be a significant problem, because people should just not do
this.

however, consider a system which is IPv6-only, i.e., the kernel supports
INET6 but not INET. in this case ifconfig(8) defaults to the 'link'
address family instead of the 'inet' address family. then suppose the
user has an existing ifconfig_xxx entry in /etc/rc.conf:

ifconfig_hn0="10.1.2.200/28"

because ifconfig defaults to 'link', two things will occur:

• ifconfig will silently accept the IP address as an Ethernet address.
• ifconfig will change the Ethernet address of the interface, causing all networking (including IPv6) to break.

to fix this, change link_addr() to return an int (instead of void), and
return -1 if it detects characters other than '.' or ':' used as
delimiters. this preserves support common Ethernet address syntaxes,
but prevents accepting an IP address with a netmask as a link address.

bump __FreeBSD_version so link_addr() consumers can detect the change.

[ivy]

i am not 100% sure about this one because i suspect link_addr/sockaddr_dl may be used in ways i'm not aware of. however, it does fix the original issue that i ran into, which was extremely confusing.

[kp]

in this case ifconfig(8) defaults to the 'link' address family instead of the 'inet' address family.

I wonder if that's worth changing too. My gut reaction here is that for an INET6-only build we ought to be defaulting to inet6, rather than to link. Or possibly defaulting to nothing at all, because defaulting to 'inet' has been causing headaches for years. We've not wanted to change it because there are any number of rc.conf files out there that rely on that, but that's less likely to be the case for the INET6-only build (especially since relying on it changing the link address is much more obviously the wrong thing to do).

lib/libc/net/linkaddr.c

72

Should we even be accepting '.' as a MAC address delimiter?

75

style(9): return (-1);

sys/sys/param.h

76

I don't know if this is significant enough to bump the version, but on the other hand, numbers are cheap and we buy them in bulk.

It may be worth noting this in RELNOTES though. (There too it's probably better to err on the side of including edge cases. The people who write the release notes can still decide not to mention it later.)

[ivy]

lib/libc/net/linkaddr.c

72

i believe this original code was intended to accept MAC addresses in the form '0011.2233.4455', in which case, yes.

sys/sys/param.h

76

i did this so code could use __FreeBSD_version to check whether it could check for a return value from link_addr(). in practice i don't know how useful that is, but...

[ivy]

In D49936#1138707, @kp wrote:

I wonder if that's worth changing too.

so... i'm not opposed to changing that. actually, i think that makes a lot of sense. however, i think this change to link_addr() also makes sense regardless of that.

[ivy]

style fixes

[kp]

In D49936#1138714, @ivy wrote:

In D49936#1138707, @kp wrote:

I wonder if that's worth changing too.

so... i'm not opposed to changing that. actually, i think that makes a lot of sense. however, i think this change to link_addr() also makes sense regardless of that.

Agreed.

[des]

I would suggest also accepting dashes as separators. I've seen plenty of software that displays Ethernet addresses with dashes instead of colons, which raises the possibility that someone might be using them here as well, but they don't show up in IPv4 or IPv6 addresses, so there's minimal risk of confusion.

[des]

sys/sys/param.h

76

It may be worth noting this in RELNOTES though.

Definitely. Lexi, when you get that far, please include Relnotes: yes in the commit footer.