Re: [PATCH v2 2/2] usb: gadget: u_ether: fix out-of-bounds read in get_ether_addr

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new parse_mac_pton() call in get_ether_addr()
  (drivers/usb/gadget/function/u_ether.c:691) still accepts malformed…
- [Medium] The commit message misdescribes the user-visible effect of the
  stricter syntax.

--

usb: gadget: u_ether: fix out-of-bounds read in get_ether_addr

This patch replaces the open coded MAC parsing loop in get_ether_addr()
with parse_mac_pton(). The old loop read past the end of short strings
and ignored invalid hex digits. It also selects GENERIC_NET_UTILS so that
parse_mac_pton() is built in.

> The configfs dev_addr and host_addr attributes of the ECM and NCM
> functions reach the same parser through gether_set_dev_addr() and
> gether_set_host_addr().

[ ... ]

> Note that this makes the accepted syntax stricter: the old loop also took
> a separator-less string such as "aabbccddeeff" and the Cisco notation
> "aabb.ccdd.eeff", both of which parse_mac_pton() rejects, so such input
> now falls back to a random address instead of being parsed.

[Severity: Medium]
Is this description accurate for the configfs path?

Only the legacy module parameter path, through gether_setup_name(), uses
the random fallback. On the configfs path, gether_set_dev_addr() discards
the random bytes and returns an error:

drivers/usb/gadget/function/u_ether.c:gether_set_dev_addr() {
	...
	if (get_ether_addr(dev_addr, new_addr))
		return -EINVAL;
	...
}

gether_set_host_addr() does the same. The store handlers generated by
USB_ETHERNET_CONFIGFS_ITEM_ATTR_DEV_ADDR() and
USB_ETHERNET_CONFIGFS_ITEM_ATTR_HOST_ADDR() pass -EINVAL back to
userspace.

So "echo aabbccddeeff > functions/ecm.usb0/dev_addr" used to succeed. With
this patch it fails with EINVAL, the previous address stays in place, and
no random address is assigned.

The message also names only ECM and NCM. The same store macros are used by
f_ecm.c, f_eem.c, f_ncm.c, f_rndis.c and f_subset.c.

Could the commit message describe the new EINVAL behaviour on configfs
writes, and list all the affected functions?

> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 59d85d6a84a89..26471cb086b37 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -686,18 +686,10 @@ static int eth_stop(struct net_device *net)
>  static int get_ether_addr(const char *str, u8 *dev_addr)
>  {
>  	if (str) {
> -		unsigned	i;
> +		bool valid;
>  
> -		for (i = 0; i < 6; i++) {
> -			unsigned char num;
> -
> -			if ((*str == '.') || (*str == ':'))
> -				str++;
> -			num = hex_to_bin(*str++) << 4;
> -			num |= hex_to_bin(*str++);
> -			dev_addr [i] = num;
> -		}
> -		if (is_valid_ether_addr(dev_addr))
> +		valid = parse_mac_pton(str, dev_addr, ".:");
> +		if (valid && is_valid_ether_addr(dev_addr))
>  			return 0;

[Severity: High]
Can malformed addresses with trailing characters still be accepted here?

parse_mac_pton() only enforces a minimum length, and then validates
indices 0 through 16:

lib/net_utils.c:parse_mac_pton() {
	if (strnlen(s, MAC_ADDR_STR_LEN) < MAC_ADDR_STR_LEN)
		return false;
	...
}

Nothing from str[MAC_ADDR_STR_LEN] onward is examined. Inputs such as
"aa:bb:cc:dd:ee:ffzz", "aa:bb:cc:dd:ee:ff:11:22" or the typo
"02:11:22:33:44:556" all return true. The truncated 6-byte prefix is then
used as the address.

This covers the module parameter path through gether_setup_name(). It also
covers the configfs dev_addr and host_addr stores of ECM, EEM, NCM, RNDIS
and subset, where gether_set_dev_addr() returns 0 and sets
NET_ADDR_SET. That seems to contradict the u_ether.h kernel-doc for
gether_set_dev_addr() and gether_set_host_addr():

 * Returns negative errno if the new address is incorrect.

The commit message says parse_mac_pton() "checks the length and every
digit", but that only holds for the first 17 characters.

netconsole's remote_mac_store() handles this with an explicit terminator
check after mac_pton():

	if (!mac_pton(buf, remote_mac))
		goto out_unlock;
	if (buf[MAC_ADDR_STR_LEN] && buf[MAC_ADDR_STR_LEN] != '\n')
		goto out_unlock;

Should get_ether_addr() do the same? A single trailing '\n' would still
need to be allowed, since configfs writes from echo end with one.

>  	}
>  	eth_random_addr(dev_addr);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104025058.2650911-1-liuc63%40xiaopeng.com