[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

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

[Liu Chao]

Hi Andy,

On Wed, Nov 04, 2026 at 12:19:28PM +0200, Andy Shevchenko wrote:
> Hmm... indeed. Perhaps we want to continue supporting those.

Agreed for the raw notation. v3 restores it with a hex2bin() fallback.

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

It does handle it correctly, and I checked why rather than assuming it:
hex2bin() validates every character and returns as soon as it sees one
that is not a hex digit, and the NUL terminator is not a hex digit, so it
cannot read past the end of the string. If it does manage to read
2 * ETH_ALEN characters then the buffer holds at least that many. So no
strnlen() guard is needed in front of it.

Confirmed on a KASAN build of exactly what v3 contains: dev_addr=ab,
dev_addr=aazz:cc:dd:ee:ff and dev_addr=aabb.ccdd.eeff all come through
with no report and fall back to a random address, dev_addr=aabbccddeeff
now gives aa:bb:cc:dd:ee:ff, and dev_addr=aa:bb:cc:dd:ee:ff still gives
aa:bb:cc:dd:ee:ff.

Two details about the helper itself. The direction we need is hex2bin(),
not bin2hex(), which turns binary into hex text. And hex2bin() returns 0
on success and -EINVAL on the first character that is not a hex digit,
whereas bin2hex() returns a char * that is never NULL. v3 therefore uses

	valid = hex2bin(dev_addr, str, ETH_ALEN) == 0;

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

You are right that commit 2b3d942c4878 ("usb ethernet gadget: split out
network core") does not introduce it; it copies get_ether_addr() into the
new u_ether.c, and the function is already at
drivers/usb/gadget/ether.c:2240 in its parent.

331a2d82bcaf does not resolve for me, though. It is not among the 104530
ancestors of 2b3d942c4878, and that range reaches all the way down to
the git root, and cgit returns 404 for it. The loop is already present
verbatim at drivers/usb/gadget/ether.c:2236 in commit 1da177e4c3f4
("Linux-2.6.12-rc2"), so it looks pre-git to me, presumably a BitKeeper
sha.

Nothing in mainline history uses 1da177e4c3f4 as a Fixes target, so v3
drops the tag and records the provenance in the log instead. Say the
word if you know a tree where 331a2d82bcaf resolves and I will put it
back.

On the Cisco notation, agreed: keeping "aabb.ccdd.eeff" would need a
parser of its own for a grouping that nothing else in the kernel uses.
v3 spells out in the log that it and irregular groupings such as
"123456:789abc" stop being accepted, so the narrowing is on the record
rather than implicit.

1/2 is unchanged apart from picking up your Reviewed-by. Thanks for that
and for the review.

Liu Chao

Re: [PATCH v2 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:44PM +0800, Liu Chao wrote:
> On Wed, Nov 04, 2026 at 12:19:28PM +0200, Andy Shevchenko wrote:
> > Hmm... indeed. Perhaps we want to continue supporting those.
> 
> Agreed for the raw notation. v3 restores it with a hex2bin() fallback.
> 
> >   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;
> 
> It does handle it correctly, and I checked why rather than assuming it:
> hex2bin() validates every character and returns as soon as it sees one
> that is not a hex digit, and the NUL terminator is not a hex digit, so it
> cannot read past the end of the string. If it does manage to read
> 2 * ETH_ALEN characters then the buffer holds at least that many. So no
> strnlen() guard is needed in front of it.
> 
> Confirmed on a KASAN build of exactly what v3 contains: dev_addr=ab,
> dev_addr=aazz:cc:dd:ee:ff and dev_addr=aabb.ccdd.eeff all come through
> with no report and fall back to a random address, dev_addr=aabbccddeeff
> now gives aa:bb:cc:dd:ee:ff, and dev_addr=aa:bb:cc:dd:ee:ff still gives
> aa:bb:cc:dd:ee:ff.

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

> Two details about the helper itself. The direction we need is hex2bin(),
> not bin2hex(), which turns binary into hex text. And hex2bin() returns 0
> on success and -EINVAL on the first character that is not a hex digit,
> whereas bin2hex() returns a char * that is never NULL. v3 therefore uses
> 
> 	valid = hex2bin(dev_addr, str, ETH_ALEN) == 0;
> 
> > 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.
> 
> You are right that commit 2b3d942c4878 ("usb ethernet gadget: split out
> network core") does not introduce it; it copies get_ether_addr() into the
> new u_ether.c, and the function is already at
> drivers/usb/gadget/ether.c:2240 in its parent.
> 
> 331a2d82bcaf does not resolve for me, though. It is not among the 104530
> ancestors of 2b3d942c4878, and that range reaches all the way down to
> the git root, and cgit returns 404 for it. The loop is already present
> verbatim at drivers/usb/gadget/ether.c:2236 in commit 1da177e4c3f4
> ("Linux-2.6.12-rc2"), so it looks pre-git to me, presumably a BitKeeper
> sha.

You need to add history.git from history group and tglx
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
If you want to have smooth link between all three, add grafts.
$ git replace
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
7a2deb32924142696b8174cdf9b38cd72a11fc96

With that being done you will have the full history from Linux 0.10
till these days.

The above mentioned commit is from v2.6.10-rc1.

> Nothing in mainline history uses 1da177e4c3f4 as a Fixes target, so v3
> drops the tag and records the provenance in the log instead. Say the
> word if you know a tree where 331a2d82bcaf resolves and I will put it
> back.

It depends on the Linux Next checks and USB maintainers, not everybody does
have a full history (as possible). Better just to refer to it [1] in a free
form, like

[1]: 331a2d82bcaf ("[PATCH] USB: ethernet/rndis gadget address params")
     from v2.6.10-rc1 (check out history.git tree)

> On the Cisco notation, agreed: keeping "aabb.ccdd.eeff" would need a
> parser of its own for a grouping that nothing else in the kernel uses.
> v3 spells out in the log that it and irregular groupings such as
> "123456:789abc" stop being accepted, so the narrowing is on the record
> rather than implicit.

-- 
With Best Regards,
Andy Shevchenko

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new kernel-doc for parse_mac_pton() in lib/net_utils.c says
  "@s must be MAC_ADDR_STR_LEN characters long", which reads as an…

--

lib/net_utils: allow mac_pton() to take a custom delimiter

This commit moves the body of mac_pton() into a new exported function,
parse_mac_pton(), which takes a string of accepted delimiter characters.
mac_pton() becomes a static inline wrapper that passes ":", so existing
callers behave as before.

Link: https://lore.kernel.org/all/20261023110113.2969708-1-liuc63@xiaopeng.com/

> diff --git a/lib/net_utils.c b/lib/net_utils.c
> index 215cda672fee1..011857cc978b4 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.
> + */

[Severity: Low]
Does this kernel-doc match what parse_mac_pton() actually checks? The text
"@s must be MAC_ADDR_STR_LEN characters long" reads as an exact length, but
the code only checks for a minimum length:

lib/net_utils.c:parse_mac_pton() {
    ...
	if (strnlen(s, MAC_ADDR_STR_LEN) < MAC_ADDR_STR_LEN)
		return false;
    ...
}

strnlen() is capped at MAC_ADDR_STR_LEN (17), and the validation loop only
reads s[0] to s[16]. Nothing after the 17th character is looked at. So
inputs like "aa:bb:cc:dd:ee:ff:00", "aa:bb:cc:dd:ee:ffXYZ" or
"aa:bb:cc:dd:ee:ff\n" return true and fill @mac.

The Return: line says false means "@s is malformed". A caller could read
that as saying true means the whole string was a well-formed MAC address.

The old mac_pton() parsed only the prefix in the same way, and existing
callers depend on it. Sysfs and configfs input usually ends in a newline,
and some callers parse a MAC address embedded in a longer string. So the
code looks right and the comment is what is off.

Since this is a newly exported general-purpose API, could the comment say
something like "@s must contain at least MAC_ADDR_STR_LEN characters; only
the first MAC_ADDR_STR_LEN are parsed and any trailing characters are
ignored"? Enforcing an exact length would break existing mac_pton() users.

> +bool parse_mac_pton(const char *s, u8 *mac, const char *delim)
>  {
>  	int i;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104025058.2650911-1-liuc63%40xiaopeng.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new parse_mac_pton() call in get_ether_addr()
  (drivers/usb/gadget/function/u_ether.c:691) still accepts malformed…
- [Medium] The commit message misdescribes the user-visible effect of the
  stricter syntax.

--

usb: gadget: u_ether: fix out-of-bounds read in get_ether_addr

This patch replaces the open coded MAC parsing loop in get_ether_addr()
with parse_mac_pton(). The old loop read past the end of short strings
and ignored invalid hex digits. It also selects GENERIC_NET_UTILS so that
parse_mac_pton() is built in.

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

[ ... ]

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

[Severity: Medium]
Is this description accurate for the configfs path?

Only the legacy module parameter path, through gether_setup_name(), uses
the random fallback. On the configfs path, gether_set_dev_addr() discards
the random bytes and returns an error:

drivers/usb/gadget/function/u_ether.c:gether_set_dev_addr() {
	...
	if (get_ether_addr(dev_addr, new_addr))
		return -EINVAL;
	...
}

gether_set_host_addr() does the same. The store handlers generated by
USB_ETHERNET_CONFIGFS_ITEM_ATTR_DEV_ADDR() and
USB_ETHERNET_CONFIGFS_ITEM_ATTR_HOST_ADDR() pass -EINVAL back to
userspace.

So "echo aabbccddeeff > functions/ecm.usb0/dev_addr" used to succeed. With
this patch it fails with EINVAL, the previous address stays in place, and
no random address is assigned.

The message also names only ECM and NCM. The same store macros are used by
f_ecm.c, f_eem.c, f_ncm.c, f_rndis.c and f_subset.c.

Could the commit message describe the new EINVAL behaviour on configfs
writes, and list all the affected functions?

> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 59d85d6a84a89..26471cb086b37 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;

[Severity: High]
Can malformed addresses with trailing characters still be accepted here?

parse_mac_pton() only enforces a minimum length, and then validates
indices 0 through 16:

lib/net_utils.c:parse_mac_pton() {
	if (strnlen(s, MAC_ADDR_STR_LEN) < MAC_ADDR_STR_LEN)
		return false;
	...
}

Nothing from str[MAC_ADDR_STR_LEN] onward is examined. Inputs such as
"aa:bb:cc:dd:ee:ffzz", "aa:bb:cc:dd:ee:ff:11:22" or the typo
"02:11:22:33:44:556" all return true. The truncated 6-byte prefix is then
used as the address.

This covers the module parameter path through gether_setup_name(). It also
covers the configfs dev_addr and host_addr stores of ECM, EEM, NCM, RNDIS
and subset, where gether_set_dev_addr() returns 0 and sets
NET_ADDR_SET. That seems to contradict the u_ether.h kernel-doc for
gether_set_dev_addr() and gether_set_host_addr():

 * Returns negative errno if the new address is incorrect.

The commit message says parse_mac_pton() "checks the length and every
digit", but that only holds for the first 17 characters.

netconsole's remote_mac_store() handles this with an explicit terminator
check after mac_pton():

	if (!mac_pton(buf, remote_mac))
		goto out_unlock;
	if (buf[MAC_ADDR_STR_LEN] && buf[MAC_ADDR_STR_LEN] != '\n')
		goto out_unlock;

Should get_ether_addr() do the same? A single trailing '\n' would still
need to be allowed, since configfs writes from echo end with one.

>  	}
>  	eth_random_addr(dev_addr);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104025058.2650911-1-liuc63%40xiaopeng.com