ifconfig: Improve VLAN Identifier number parsing

[zlei]

Summary

VLAN Identifier number 0xFFF is reserved.

Also validate during parsing to prevent potential integer overflow.

Fixes: c7cffd65c5d85 Add support for stacked VLANs (IEEE 802.1ad, AKA Q-in-Q)
MFC after: 1 week

Test Plan

root@:~ # ifconfig em0.4095 create
ifconfig: invalid vlan tag

# ifconfig epair create name e
e
# ifconfig e.4294967297 create
ifconfig: invalid vlan tag

[melifaro]

Conceptually LGTM, please see an implementation comment inline.

sbin/ifconfig/ifvlan.c

133–143

I'd suggest splitting parsing and validation to improve readability.
For example, get the vid using strtol() and then check the range explicitly.

[zlei]

sbin/ifconfig/ifvlan.c

133–143

I'd suggest splitting parsing and validation to improve readability

Reasonable

[zlei]

sbin/ifconfig/ifvlan.c

133–143

Well, vid is possible to overflow (without fix).

# ifconfig epair create name e
e
# ifconfig e.4294967297 create
root@:~ # ifconfig e.4294967297
e.4294967297: flags=8842<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
	ether 02:2c:90:bf:36:0a
	groups: vlan
	vlan: 1 vlanproto: 802.1q vlanpcp: 0 parent interface: e
	media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
	status: active
	nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>

So we either need strtol() or validating while parsing

[zlei]

sbin/ifconfig/ifvlan.c

133–143

Just verified that strtol() does not detect overflow, and it ignores leading zeros. strtoimax() is sufficent but it also ignores leading zeros.

Using strtoxx functions need extra effort.

So I think current version is good enought.

@melifaro What do you think ?

[melifaro]

sbin/ifconfig/ifvlan.c

133–143

Thank you for evaluating the options!

I think that the implementation should be easily readable.
IMO, the intent here is that we should provide a good human-readable error for the innocent mistakes.
The kernel will still reject the invalid output.
I'd still suggest doing some sort of str-to-int conversion (even it doesn't detect overflow) and check the result afterwards.

[zlei]

sbin/ifconfig/ifvlan.c

139

@melifaro This is much simpler, do you support me?

[melifaro]

sbin/ifconfig/ifvlan.c

136–143

I was thinking of something like above. I don't have really hard opinion here, so if you believe your version is more readable - go with it.

[zlei]

sbin/ifconfig/ifvlan.c

136–143

I don't have really hard opinion here, so if you believe your version is more readable - go with it.

Do you mean the current patch, or the latter

if (*cp != '\0' || vid == 0xFFF || (vid & ~0xFFF))

?

[zlei]

Use unsigned int for vid.

Inspired by @melifaro 's comment:

// unsigned int vid;

char *invalid_char = NULL;