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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Nov 10, 2026 at 12:54:47PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.

[ ... ]

> > 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?

Okay, what I think the v5 of this patch has to do is to update
the entire documentation where the patched function is used.
It should clarify the accepted formats.

> [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?

Yes, the Fixes to official Git era that starts with v2.6.12-rc2 is fine to have
while leaving the free form of the pointer to the actual commit that introduced
the function.

> [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?

Yes, this was already mentioned in the previous patch, the kernel-doc
should be amended.

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

I think this also needs to be "checks the minimum length" in the documentation.
The original code IIRC doesn't prevent the input be restricted by exact number
of characters.

-- 
With Best Regards,
Andy Shevchenko