Re: [PATCH v3 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 3 potential
issue(s) to consider.

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

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).

- [Medium] The commit message misdescribes what users see after the
  change.
- [Low] get_ether_addr() still accepts malformed addresses that have
  trailing characters, and silently cuts them down to their first six…
- [Low] The patch fixes a slab-out-of-bounds read that KASAN reports in
  get_ether_addr(), reachable with `insmod g_ether.ko dev_addr=ab`, but…

--

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

This patch replaces the open coded six iteration parsing loop in
get_ether_addr() with parse_mac_pton() for ':' and '.' delimited
addresses, and with hex2bin() for the separator-less form. The goal is
to fix an out-of-bounds read on short strings and to stop accepting
non-hex digits. It also makes USB_U_ETHER select GENERIC_NET_UTILS.

Link: https://lore.kernel.org/all/20261104025058.2650911-3-liuc63@xiaopeng.com/

> 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().

[ ... ]

> What stops working
> is the Cisco notation "aabb.ccdd.eeff" and irregular groupings such as
> "123456:789abc", which the old loop only accepted because it skipped a
> delimiter wherever it happened to find one.  Supporting them would need
> a parser per grouping, so such input now falls back to a random address.

[Severity: Medium]
Is the random address fallback accurate for the configfs path?

For "aabb.ccdd.eeff", parse_mac_pton() rejects the string because it is
shorter than MAC_ADDR_STR_LEN. hex2bin() rejects it at the '.' at offset
4. get_ether_addr() then returns 1, and the caller discards the random
address it wrote into new_addr:

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 _HOST_ADDR() in
u_ether_configfs.h pass that error back to userspace:

		ret = gether_set_dev_addr(opts->net, page);
		mutex_unlock(&opts->lock);
		if (!ret)
			ret = len;
		return ret;

So a gadget setup script that used to write "aabb.ccdd.eeff" without
error will now get EINVAL from write(2), and the old address stays in
place. It does not get a random address. Only the legacy module
parameter users (ether.c, cdc2.c, multi.c, ncm.c, g_ffs.c) ignore the
failure and keep the address that was randomised at creation.

Five functions use these configfs macros: f_ecm, f_eem, f_ncm, f_rndis
and f_subset, not just ECM and NCM.

Could the commit message describe this configfs ABI change and its
scope more precisely?

[ ... ]

> The loop is older than the git history: it is already present in
> drivers/usb/gadget/ether.c in commit 1da177e4c3f4 ("Linux-2.6.12-rc2")
> and was copied into u_ether.c by commit 2b3d942c4878 ("usb ethernet
> gadget: split out network core"), hence no Fixes tag.

[Severity: Low]
Should this still have a Fixes: tag? For bugs that predate git, the
usual convention is:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

2b3d942c4878 would also work. Without a tag, and with Cc: stable
dropped in v2, stable and distro backport tooling will not pick up this
fix for the out-of-bounds read that KASAN reports.

If the fix is backported, the commit that adds parse_mac_pton(),
e5765bbb2de1 ("lib/net_utils: allow mac_pton() to take a custom
delimiter"), also needs to be marked as a stable prerequisite. This
patch does not build without it.

> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 59d85d6a84a89..deedea8a19fd7 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -686,18 +686,16 @@ 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;
> +		/* Try the delimited formats first */
> +		valid = parse_mac_pton(str, dev_addr, ".:");
> +		if (valid && is_valid_ether_addr(dev_addr))
> +			return 0;
>  
> -			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))
> +		/* Fall back to the raw format */
> +		valid = hex2bin(dev_addr, str, ETH_ALEN) == 0;
> +		if (valid && is_valid_ether_addr(dev_addr))
>  			return 0;

[Severity: Low]
Can malformed addresses with trailing characters still get through
here? parse_mac_pton() only checks for a minimum length and never looks
past the 17th character:

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

hex2bin() reads exactly 12 characters and returns 0 no matter what
comes after them. get_ether_addr() does not check that the string ends
after the parsed MAC, optionally with a trailing newline.

As a result, "aa:bb:cc:dd:ee:ff:11", "aa:bb:cc:dd:ee:ffzz",
"aabbccddeeff00" and "112233445566778899" are all accepted and silently
cut down to their first six bytes. On the module parameter path the
address is marked NET_ADDR_SET. On the configfs path, which passes the
whole page, the store reports success.

The old loop also ignored anything after the sixth byte. However, the
commit message says "parse_mac_pton() checks the length". The
parse_mac_pton() kernel-doc also says "@s must be MAC_ADDR_STR_LEN
characters long" and "false if @s is malformed", which promises more
than the code enforces.

Would it make sense to reject trailing characters in get_ether_addr()?

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