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 Wed, Nov 04, 2026 at 10:50:58AM +0800, Liu Chao wrote:
> get_ether_addr() walks the MAC address string with a fixed six iteration
> loop, calling hex_to_bin() twice per iteration, and never checks for the
> terminating NUL.  A short string therefore makes it read past the end of
> the buffer.  The dev_addr and host_addr module parameters of the legacy
> gadget drivers are copied into their own kmalloc'ed buffer by
> param_set_charp(), so the overrun crosses a slab boundary and KASAN
> reports it at insmod time:
> 
>   # insmod g_ether.ko dev_addr=ab
> 
>   BUG: KASAN: slab-out-of-bounds in get_ether_addr+0x1ec/0x230
>   Read of size 1 at addr ffff888009f91f53 by task insmod/70
>   ...
>    get_ether_addr+0x1ec/0x230
>    gether_set_dev_addr+0x6b/0x180
>    eth_bind+0xfd/0x3b0 [g_ether]
>    composite_bind+0x1bb/0x7b0
>    gadget_bind_driver+0x229/0xb00
>   ...
>   Allocated by task 70:
>    __kmalloc_noprof+0x1d1/0x4a0
>    param_set_charp+0x4a/0x220
>    parse_args+0x267/0x5c0
>    load_module+0x4f6a/0x6600
>   ...
>   The buggy address is located 0 bytes to the right of
>    allocated 19-byte region [ffff888009f91f40, ffff888009f91f53)
> 
> Besides reading past the end of the string, the loop ignores what
> hex_to_bin() returns: it yields -1 for a character that is not a hex
> digit, and that value is shifted and OR'ed into dev_addr unchecked, so a
> malformed address can be accepted as a valid one and the interface comes
> up with an address that nobody asked for:
> 
>   # insmod g_ether.ko dev_addr=aazz:cc:dd:ee:ff
>   # cat /sys/class/net/usb0/address
>   aa:ff:cc:dd:ee:ff
> 
> 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().  Their store handler passes the page-sized
> configfs buffer straight on, so an overrun there stays inside the same
> object and is never reported, but a malformed address is silently
> accepted all the same.
> 
> Replace the open coded loop with parse_mac_pton(), which checks the
> length and every digit before writing anything to dev_addr, and keep the
> existing eth_random_addr() fallback for all failures.

> Note that this makes the accepted syntax stricter: the old loop also took
> a separator-less string such as "aabbccddeeff" and the Cisco notation
> "aabb.ccdd.eeff", both of which parse_mac_pton() rejects, so such input
> now falls back to a random address instead of being parsed.

Hmm... indeed. Perhaps we want to continue supporting those.
Web browsing and AI assistance suggests that there are only few
formats that can be met IRL: raw (no delimiters), Cisco (4-digit-dot),
Windows (with hyphen instead of colon), and quite rarely OUI split
(3 bytes + 3 bytes with one of the standard delimiters ':', '.',
or '-').

Examples in the kernel and comments are only suggested about standard
hexadecimal 2-byte-colon.

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. The commit message doesn't tell
anything about supported formats. Documentation also doesn't
clarify it anyhow.

AI gives this table (take, of course, it with grain of salt):

MAC Address Format Structure	Example Pattern in g_ether Context	Approximate Number of Public Web Resources	Usage Percentage	Primary Source Context

Raw / Continuous Hex (No Delimiters)	g_ether.host_addr=123456789abc	~110 references	~3.1%	U-Boot automated environment macros, hardware factory provisioning notes, custom embedded board bringing-up logs.

Cisco Dot Notation (3 blocks of 4 digits)	g_ether.host_addr=1234.5678.9abc	~15 references	~0.4%	Hybrid enterprise routing guides, blog posts authored by legacy network engineers config-switching to Linux gadgets.

Semi-Grouped (Irregular byte clustering)	g_ether.host_addr=123456:789abc	~3 references	< 0.1%	Highly specific hardware diagnostic scripts or open source issue trackers validating parser limits.

So, taking it into account as a trend, we can really forget about semi-grouped
and Cisco cases. The Raw is easy to support.

		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;

-- 
With Best Regards,
Andy Shevchenko