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

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

[Liu Chao]

Rework of

  [PATCH] usb: gadget: u_ether: fix out-of-bounds read in get_ether_addr
  https://lore.kernel.org/all/20261023110113.2969708-1-liuc63@xiaopeng.com/

following Andy's review: 1/2 prepares a parse_mac_pton() helper in
lib/net_utils.c, 2/2 uses it in get_ether_addr().

The two patches belong to different trees: 1/2 touches lib/ and should go
via netdev or -mm, 2/2 is for the USB tree.  2/2 depends on 1/2, so it
can only be applied once 1/2 has reached Linus' tree.

Changes since v1:
  - rework the fix on top of parse_mac_pton() as suggested by Andy
    instead of open coding the bounds checks in get_ether_addr()
  - add the KASAN report and the malformed address example that v1 was
    missing, both collected on a KASAN build by loading g_ether with a
    short and with a non-hex dev_addr
  - select GENERIC_NET_UTILS for USB_U_ETHER
  - drop Cc: stable: 2/2 calls parse_mac_pton(), which 1/2 introduces,
    so it cannot be backported to any stable tree on its own
  - drop the Reviewed-by, the implementation changed completely

One user visible effect worth pointing out: the old loop also accepted 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.  Details are in the log of 2/2.

Liu Chao (2):
  lib/net_utils: allow mac_pton() to take a custom delimiter
  usb: gadget: u_ether: fix out-of-bounds read in get_ether_addr

 drivers/usb/gadget/Kconfig            |  1 +
 drivers/usb/gadget/function/u_ether.c | 14 +++-----------
 include/linux/hex.h                   |  7 ++++++-
 lib/net_utils.c                       | 17 ++++++++++++++---
 4 files changed, 24 insertions(+), 15 deletions(-)


base-commit: a5ebb76233b79db01e82e061173cb73c6d2b5c6b
-- 
2.50.1

[PATCH v2 1/2] lib/net_utils: allow mac_pton() to take a custom delimiter

[Liu Chao]

mac_pton() hardcodes ':' as the only accepted separator, which pushes
callers that need a different notation to open-code their own parser.

Split the body out into parse_mac_pton() taking the set of accepted
delimiters as a string, and turn mac_pton() into a static inline wrapper
passing ":" so that every existing caller keeps its current behaviour.
The strnlen() guard still runs before any delimiter is inspected, so
strchr() is never handed a NUL character.

No functional change intended.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
v2: new patch, split out of the u_ether fix at Andy's suggestion.
Link: https://lore.kernel.org/all/20261023110113.2969708-1-liuc63@xiaopeng.com/

 include/linux/hex.h |  7 ++++++-
 lib/net_utils.c     | 17 ++++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/linux/hex.h b/include/linux/hex.h
index 2618382e5b0c..408825478cda 100644
--- a/include/linux/hex.h
+++ b/include/linux/hex.h
@@ -30,6 +30,11 @@ extern int hex_to_bin(unsigned char ch);
 extern int __must_check hex2bin(u8 *dst, const char *src, size_t count);
 extern char *bin2hex(char *dst, const void *src, size_t count);
 
-bool mac_pton(const char *s, u8 *mac);
+bool parse_mac_pton(const char *s, u8 *mac, const char *delim);
+
+static inline bool mac_pton(const char *s, u8 *mac)
+{
+	return parse_mac_pton(s, mac, ":");
+}
 
 #endif
diff --git a/lib/net_utils.c b/lib/net_utils.c
index 215cda672fee..011857cc978b 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -5,7 +5,18 @@
 #include <linux/export.h>
 #include <linux/hex.h>
 
-bool mac_pton(const char *s, u8 *mac)
+/**
+ * parse_mac_pton - convert a MAC address string using a custom delimiter
+ * @s: string to be parsed
+ * @mac: buffer to store the result, left untouched if @s is malformed
+ * @delim: characters accepted as a separator between the byte pairs
+ *
+ * @s must be MAC_ADDR_STR_LEN characters long and use a character from
+ * @delim at every separator position, e.g. "aa.bb:cc.dd:ee.ff" with ".:".
+ *
+ * Return: true on success, false if @s is malformed.
+ */
+bool parse_mac_pton(const char *s, u8 *mac, const char *delim)
 {
 	int i;
 
@@ -16,7 +27,7 @@ bool mac_pton(const char *s, u8 *mac)
 	for (i = 0; i < ETH_ALEN; i++) {
 		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
 			return false;
-		if (i != ETH_ALEN - 1 && s[i * 3 + 2] != ':')
+		if (i != ETH_ALEN - 1 && !strchr(delim, s[i * 3 + 2]))
 			return false;
 	}
 	for (i = 0; i < ETH_ALEN; i++) {
@@ -24,4 +35,4 @@ bool mac_pton(const char *s, u8 *mac)
 	}
 	return true;
 }
-EXPORT_SYMBOL(mac_pton);
+EXPORT_SYMBOL(parse_mac_pton);
-- 
2.50.1

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

[Liu Chao]

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.

Select GENERIC_NET_UTILS so that parse_mac_pton() is built in.

Fixes: 2b3d942c4878 ("usb ethernet gadget: split out network core")
Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
v2: rework on top of parse_mac_pton() instead of open coding the bounds

 drivers/usb/gadget/Kconfig            |  1 +
 drivers/usb/gadget/function/u_ether.c | 14 +++-----------
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 76521555e3c1..b83c42be686b 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -158,6 +158,7 @@ config USB_U_SERIAL
 
 config USB_U_ETHER
 	tristate
+	select GENERIC_NET_UTILS
 
 config USB_U_AUDIO
 	tristate
diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 59d85d6a84a8..26471cb086b3 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -686,18 +686,10 @@ static int eth_stop(struct net_device *net)
 static int get_ether_addr(const char *str, u8 *dev_addr)
 {
 	if (str) {
-		unsigned	i;
+		bool valid;
 
-		for (i = 0; i < 6; i++) {
-			unsigned char num;
-
-			if ((*str == '.') || (*str == ':'))
-				str++;
-			num = hex_to_bin(*str++) << 4;
-			num |= hex_to_bin(*str++);
-			dev_addr [i] = num;
-		}
-		if (is_valid_ether_addr(dev_addr))
+		valid = parse_mac_pton(str, dev_addr, ".:");
+		if (valid && is_valid_ether_addr(dev_addr))
 			return 0;
 	}
 	eth_random_addr(dev_addr);
-- 
2.50.1

Re: [PATCH v2 1/2] lib/net_utils: allow mac_pton() to take a custom delimiter

[Andy Shevchenko]

On Wed, Nov 04, 2026 at 10:50:57AM +0800, Liu Chao wrote:
> mac_pton() hardcodes ':' as the only accepted separator, which pushes
> callers that need a different notation to open-code their own parser.
> 
> Split the body out into parse_mac_pton() taking the set of accepted
> delimiters as a string, and turn mac_pton() into a static inline wrapper
> passing ":" so that every existing caller keeps its current behaviour.
> The strnlen() guard still runs before any delimiter is inspected, so
> strchr() is never handed a NUL character.
> 
> No functional change intended.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Exactly what I had in mind, thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Wed, Nov 04, 2026 at 12:19:34PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 04, 2026 at 10:50:58AM +0800, Liu Chao wrote:

...

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

Of course it should be !bin2hex(dev_addr, str, ETH_ALEN). Alternatively

		valid = bin2hex(dev_addr, str, ETH_ALEN) == 0;
		if (valid && is_valid_ether_addr(dev_addr))
			return 0;

>                         return 0;

-- 
With Best Regards,
Andy Shevchenko