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

[Liu Chao <liuc63@xiaopeng.com>]

Hi Andy,

On Wed, Nov 04, 2026 at 12:19:28PM +0200, Andy Shevchenko wrote:
> Hmm... indeed. Perhaps we want to continue supporting those.

Agreed for the raw notation. v3 restores it with a hex2bin() fallback.

>   bool valid;
>
>   /* Try standard format of MAC address */
>   valid = parse_mac_pton(str, dev_addr, ".:");
>   if (valid && is_valid_ether_addr(dev_addr))
>                         return 0;
>
>   /* Fallback to raw format */
>  // check, if it correctly handles the case you want to fix here
>  // I believe it does handle it correctly
>   if (bin2hex(...) && is_valid_ether_addr(dev_addr))
>                         return 0;

It does handle it correctly, and I checked why rather than assuming it:
hex2bin() validates every character and returns as soon as it sees one
that is not a hex digit, and the NUL terminator is not a hex digit, so it
cannot read past the end of the string. If it does manage to read
2 * ETH_ALEN characters then the buffer holds at least that many. So no
strnlen() guard is needed in front of it.

Confirmed on a KASAN build of exactly what v3 contains: dev_addr=ab,
dev_addr=aazz:cc:dd:ee:ff and dev_addr=aabb.ccdd.eeff all come through
with no report and fall back to a random address, dev_addr=aabbccddeeff
now gives aa:bb:cc:dd:ee:ff, and dev_addr=aa:bb:cc:dd:ee:ff still gives
aa:bb:cc:dd:ee:ff.

Two details about the helper itself. The direction we need is hex2bin(),
not bin2hex(), which turns binary into hex text. And hex2bin() returns 0
on success and -EINVAL on the first character that is not a hex digit,
whereas bin2hex() returns a char * that is never NULL. v3 therefore uses

	valid = hex2bin(dev_addr, str, ETH_ALEN) == 0;

> The function itself was introduced in 331a2d82bcaf ("[PATCH] USB:
> ethernet/rndis gadget address params"), and not in the USB network
> core split as your Fixes points to.

You are right that commit 2b3d942c4878 ("usb ethernet gadget: split out
network core") does not introduce it; it copies get_ether_addr() into the
new u_ether.c, and the function is already at
drivers/usb/gadget/ether.c:2240 in its parent.

331a2d82bcaf does not resolve for me, though. It is not among the 104530
ancestors of 2b3d942c4878, and that range reaches all the way down to
the git root, and cgit returns 404 for it. The loop is already present
verbatim at drivers/usb/gadget/ether.c:2236 in commit 1da177e4c3f4
("Linux-2.6.12-rc2"), so it looks pre-git to me, presumably a BitKeeper
sha.

Nothing in mainline history uses 1da177e4c3f4 as a Fixes target, so v3
drops the tag and records the provenance in the log instead. Say the
word if you know a tree where 331a2d82bcaf resolves and I will put it
back.

On the Cisco notation, agreed: keeping "aabb.ccdd.eeff" would need a
parser of its own for a grouping that nothing else in the kernel uses.
v3 spells out in the log that it and irregular groupings such as
"123456:789abc" stop being accepted, so the narrowing is on the record
rather than implicit.

1/2 is unchanged apart from picking up your Reviewed-by. Thanks for that
and for the review.

Liu Chao