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

[Liu Chao <liuc63@xiaopeng.com>]

Hi Andy,

Thanks for the review and for the Reviewed-by. v4 goes out together with
this mail.

On Thu, Nov 05, 2026 at 02:13:25PM +0200, Andy Shevchenko wrote:
> It is worth to mention that the occurrences of use those formats
> (Cisco and irregular grouping) are more historical and rare to meet
> these days.  Also point out that the original commit didn't explain
> the choice of implementation, most likely it was just for
> simplification and not really allowing non-standard formats to be
> used.

v4 adds a paragraph right after the one that says what stops working:

	Neither of the two should be missed: they are historical
	notations that are rarely met these days, and the commit that
	introduced the loop never said that it accepted them.  That
	commit only explains that it resolves a FIXME by adding module
	parameters for stable addresses, so skipping a delimiter
	wherever one happens to turn up reads as a simplification of
	the parser rather than as support for non-standard formats.

I checked the second half instead of taking it on trust. The log of
331a2d82bcaf has no hit for format, colon, separator, delimiter, hex,
notation, cisco, dot or parse. All it says is that it closes a FIXME by
giving the gadget stable addresses through module parameters.

> As I pointed out, you may use the correct SHA, but you need to point
> the readers out to the history.git tree.

With the history.git pointer from your other mail [1] it does resolve,
and what I wrote back to you was wrong: "cgit returns 404" only holds
for torvalds/linux.git, whose root is 1da177e4c3f4. history.git expands
the short sha to 331a2d82bcaff97ecc4764cca266db0d7465b937, David
Brownell, 2004-05-17, committed by Greg KH, adding get_ether_addr() to
drivers/usb/gadget/ether.c with the loop already in its final shape,
nibble() where we now have hex_to_bin().

v4 names it in the free form you described in the v2 thread:

	The loop is older than the git history.  It was introduced by
	331a2d82bcaf ("[PATCH] USB: ethernet/rndis gadget address
	params") from v2.6.7 in drivers/usb/gadget/ether.c, and commit
	2b3d942c4878 ("usb ethernet gadget: split out network core")
	copied it into u_ether.c.  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.

One correction on the release, though: it is v2.6.7, not v2.6.10-rc1. In
the history.git log of drivers/usb/gadget/ether.c the commit is absent
at v2.6.6 (20 entries) and present at v2.6.7-rc1 (22 entries), where it
is the newest commit on that file, dated 2004-05-17, ahead of v2.6.7 of
2004-06-03. It is of course still there at v2.6.10-rc1 (39 entries),
which is where I would guess the later tag came from.

> Code wise LGTM, so after addressing the commit message and reference
> to the culprit commit, feel free to add
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> (with all awareness of the potential weirdness of the user input)

Added to 2/2, after Signed-off-by. 1/2 already carried it from v3 and is
otherwise unchanged. v4 is message only: the tree of 2/2 is byte
identical to what you reviewed, only the log changed.

Your other mail [1] also has this:

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

It does work in the old loop, and I verified that with the verbatim code
rather than by reading it. The loop advances two characters per
iteration, not three, and skips a delimiter only when one sits at the
start of an iteration:

	for (i = 0; i < 6; i++) {
		if ((*str == '.') || (*str == ':'))
			str++;
		num = hex_to_bin(*str++) << 4;
		num |= hex_to_bin(*str++);
		dev_addr[i] = num;
	}

A Cisco group is exactly four digits, so both dots land at the start of
an iteration and are skipped. Tracing "aabb.ccdd.eeff":

	i=0 pos= 0 skip=0 "aab" -> aa
	i=1 pos= 2 skip=0 "bb." -> bb
	i=2 pos= 4 skip=1 ".cc" -> cc
	i=3 pos= 7 skip=0 "dd." -> dd
	i=4 pos= 9 skip=1 ".ee" -> ee
	i=5 pos=12 skip=0 "ff"  -> ff

All 14 characters are consumed and the result is aa:bb:cc:dd:ee:ff, so
the old code did accept the notation. That is why the v3 log lists it as
something that stops working.

The fixed three character stride belongs to parse_mac_pton(), which
looks at s[i*3], s[i*3+1] and s[i*3+2] and wants a delimiter at the last
of those. There Cisco is rejected, because position 2 holds 'b', and
that rejection is exactly the narrowing the v4 log records.

[1] https://lore.kernel.org/all/auw8sAYE-nkZ-qIT@ashevche-desk.local/

Liu Chao