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

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

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 gets the effect of the dropped notations
  wrong for configfs.
- [Low] The new get_ether_addr() still silently accepts malformed
  addresses that have extra characters after the MAC, in both branches.
- [Low] The patch fixes a real slab-out-of-bounds read reported by KASAN
  (with the old loop, `insmod g_ether.ko dev_addr=ab` reads str[3] and…
- [Low] The kernel-doc for parse_mac_pton() does not match its
  implementation, and the new caller depends on the undocumented…

--

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

This commit replaces the open-coded MAC parsing loop in get_ether_addr()
with parse_mac_pton() for the ':' and '.' delimited notation and hex2bin()
for the raw 12 digit notation. This fixes an out-of-bounds read on short
strings and the silent acceptance of non-hex characters. It also makes
USB_U_ETHER select GENERIC_NET_UTILS.

Link: https://lore.kernel.org/all/20261105072147.3383565-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().

[ ... ]

> The notations that keep working are the byte pair ones, ':' and '.', in
> any mixture, and the separator-less "aabbccddeeff".  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 "falls back to a random address" part accurate for configfs?

It holds for the legacy module parameters, which reach get_ether_addr()
through gether_setup_name(). The configfs path goes through
gether_set_dev_addr() and gether_set_host_addr() instead, and both throw
away the random bytes:

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

The store handlers generated by USB_ETHERNET_CONFIGFS_ITEM_ATTR_DEV_ADDR and
USB_ETHERNET_CONFIGFS_ITEM_ATTR_HOST_ADDR in u_ether_configfs.h return that
error to the writer.

"echo aabb.ccdd.eeff > functions/ecm.usb0/dev_addr" used to succeed. The
old loop skipped the '.' and produced aa:bb:cc:dd:ee:ff. Now
parse_mac_pton() fails the length check and hex2bin() fails at the '.'.
The write therefore returns EINVAL and the attribute keeps its old value.
"aabb:cc:dd:ee:ff" behaves the same way.

The message also names only ECM and NCM. The same macros are used in
f_ecm.c, f_eem.c, f_ncm.c, f_rndis.c and f_subset.c. All five are
documented in Documentation/ABI/testing/configfs-usb-gadget-{ecm,eem,ncm,
rndis,subset} with no format restriction.

Could the commit message describe the configfs behaviour change and list
all five affected functions?

> 331a2d82bcaf predates the git conversion and only resolves in the
> historic tree history.git, so a Fixes: tag naming it would not resolve
> for most readers and is left out.

[Severity: Low]
Should this still carry a Fixes: tag?

The function this patch changes, get_ether_addr() in u_ether.c, was
introduced by commit 2b3d942c4878 ("usb ethernet gadget: split out network
core"), and that commit is in git. The usual convention for bugs older
than git is also available:

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

Cc: stable was dropped in v2, and there is no Fixes: tag either. Stable
tooling has nothing to match on, so the out-of-bounds read from
"insmod g_ether.ko dev_addr=ab" stays in all maintained stable trees.

The fix also depends on parse_mac_pton() from the preceding commit
"lib/net_utils: allow mac_pton() to take a custom delimiter".

Would a Cc: stable with a dependency note, or a separate minimal fix for
stable, help backporters here?

> 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, ".:");

[Severity: Low]
This isn't a bug today, but the kernel-doc for parse_mac_pton() added in
the preceding commit "lib/net_utils: allow mac_pton() to take a custom
delimiter" says:

 * @s must be MAC_ADDR_STR_LEN characters long and use a character from
 * @delim at every separator position, e.g. "aa.bb:cc.dd:ee.ff" with ".:".
 ...
 * Return: true on success, false if @s is malformed.

The implementation only rejects strings shorter than MAC_ADDR_STR_LEN and
ignores everything after the 17th character.

This caller depends on that prefix-only behaviour. The configfs store
handlers in u_ether_configfs.h pass the raw page down, and it normally ends
in '\n':

_f_##_opts_dev_addr_store()
    gether_set_dev_addr(opts->net, page)
        get_ether_addr()
            parse_mac_pton(str, dev_addr, ".:")

So "echo aa:bb:cc:dd:ee:ff > dev_addr" passes 18 characters to
parse_mac_pton().

Suppose the helper were later changed to enforce the exact length that its
documentation describes. Wouldn't every colon notation configfs write then
fail with -EINVAL? The hex2bin() fallback would not help, because it
rejects ':'.

Could the kernel-doc say that only the first MAC_ADDR_STR_LEN characters
are checked? Alternatively, could this caller strip the trailing newline
itself?

> +		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 both branches still silently accept an address with extra characters
after the MAC?

parse_mac_pton() only enforces a minimum length:

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

It never looks at s[17] or beyond. "aa:bb:cc:dd:ee:ff:11",
"aa:bb:cc:dd:ee:ffff" and "aa:bb:cc:dd:ee:ffZZZ" all return true.

hex2bin(dev_addr, str, ETH_ALEN) consumes exactly 12 hex digits and returns
0 regardless of what follows. "aabbccddeeff0", "aabbccddeeff00112233" and
"aabbccddeeffzz" are all accepted as aa:bb:cc:dd:ee:ff.

In each of these cases get_ether_addr() returns 0, gether_setup_name()
marks the address NET_ADDR_SET, and the configfs store reports success.

The commit message says parse_mac_pton() "checks the length", but that
check is only a lower bound.

Would it make sense to require str[17] in the delimited case, or str[12] in
the raw case, to be '\0' or '\n'? A single trailing '\n' has to be allowed,
because the configfs store passes the raw page buffer from echo.

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