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

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

On Thu, Nov 05, 2026 at 03:21:44PM +0800, Liu Chao wrote:
> 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.

Cisco should not work, the index is going over 3 characters on each iteration.

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

You need to add history.git from history group and tglx
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
If you want to have smooth link between all three, add grafts.
$ git replace
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
7a2deb32924142696b8174cdf9b38cd72a11fc96

With that being done you will have the full history from Linux 0.10
till these days.

The above mentioned commit is from v2.6.10-rc1.

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

It depends on the Linux Next checks and USB maintainers, not everybody does
have a full history (as possible). Better just to refer to it [1] in a free
form, like

[1]: 331a2d82bcaf ("[PATCH] USB: ethernet/rndis gadget address params")
     from v2.6.10-rc1 (check out history.git tree)

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

-- 
With Best Regards,
Andy Shevchenko