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

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

[Liu Chao]

Rework of

  [PATCH v2] usb: gadget: u_ether: fix out-of-bounds read in get_ether_addr
  https://lore.kernel.org/all/20261104025058.2650911-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 v2:
  - 1/2: pick up Andy's Reviewed-by, no code change
  - 2/2: keep the separator-less notation "aabbccddeeff" working by
    falling back to hex2bin() when parse_mac_pton() does not match, as
    suggested by Andy
  - 2/2: drop the Fixes tag.  commit 2b3d942c4878 ("usb ethernet gadget:
    split out network core") only copies the loop into the new
    u_ether.c; in drivers/usb/gadget/ether.c it is already present in
    commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), so the defect predates the
    git history and there is no mainline commit to point at
  - 2/2: spell out in the log which notations stop being accepted

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

One user visible effect remains: the Cisco notation "aabb.ccdd.eeff" and
irregular groupings such as "123456:789abc" are no longer parsed and now
fall 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 | 18 ++++++++----------
 include/linux/hex.h                   |  7 ++++++-
 lib/net_utils.c                       | 17 ++++++++++++++---
 4 files changed, 29 insertions(+), 14 deletions(-)


base-commit: a5ebb76233b79db01e82e061173cb73c6d2b5c6b
-- 
2.50.1

[PATCH v3 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
v3: pick up Andy's Reviewed-by, no code change.
v2: new patch, split out of the u_ether fix at Andy's suggestion.
Link: https://lore.kernel.org/all/20261104025058.2650911-2-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 v3 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 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.

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.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
v3: fall back to hex2bin() so that the separator-less notation keeps
    working, as suggested by Andy; drop the Fixes tag, the loop predates
    the git history.
v2: rework on top of parse_mac_pton(), add the KASAN report and the
    malformed address example, select GENERIC_NET_UTILS, drop Cc: stable
    and the Reviewed-by.
Link: https://lore.kernel.org/all/20261104025058.2650911-3-liuc63@xiaopeng.com/

 drivers/usb/gadget/Kconfig            |  1 +
 drivers/usb/gadget/function/u_ether.c | 18 ++++++++----------
 2 files changed, 9 insertions(+), 10 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..deedea8a19fd 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -686,18 +686,16 @@ 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;
+		/* Try the delimited formats first */
+		valid = parse_mac_pton(str, dev_addr, ".:");
+		if (valid && is_valid_ether_addr(dev_addr))
+			return 0;
 
-			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))
+		/* Fall back to the raw format */
+		valid = hex2bin(dev_addr, str, ETH_ALEN) == 0;
+		if (valid && is_valid_ether_addr(dev_addr))
 			return 0;
 	}
 	eth_random_addr(dev_addr);
-- 
2.50.1

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

[Andy Shevchenko]

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

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

[Liu Chao]

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