Re: [PATCH v3 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:47PM +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 whole configfs
> buffer 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() for the delimited
> notations and hex2bin() for the raw one, and keep the existing
> eth_random_addr() fallback for everything both of them reject.
> parse_mac_pton() checks the length and every digit before writing
> anything to dev_addr, and hex2bin() stops at the first character that is
> not a hex digit, which the NUL terminator is, so neither reads past the
> end of the string.
> 
> 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.

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.
(So, some kind of summary of our discussion in v1 round.)

> Select GENERIC_NET_UTILS so that parse_mac_pton() is built in.
> 
> The loop is older than the git history: it is already present in
> drivers/usb/gadget/ether.c in commit 1da177e4c3f4 ("Linux-2.6.12-rc2")
> and was copied into u_ether.c by commit 2b3d942c4878 ("usb ethernet
> gadget: split out network core"), hence no Fixes tag.

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

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)

-- 
With Best Regards,
Andy Shevchenko