[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

Re: [PATCH v3 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 kernel-doc this patch adds for parse_mac_pton() in
  lib/net_utils.c does not match what the function does.

--

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

This commit moves the body of mac_pton() into a new parse_mac_pton(),
which takes a string of accepted delimiter characters. mac_pton() becomes
a static inline wrapper that passes ":", so existing callers keep their
current behaviour.

Link: https://lore.kernel.org/all/20261104025058.2650911-2-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]
This isn't a bug, but does this kernel-doc match what parse_mac_pton()
actually checks?

The only length check in the function is:

	if (strnlen(s, MAC_ADDR_STR_LEN) < MAC_ADDR_STR_LEN)
		return false;

This rejects strings that are too short. Nothing checks that
s[MAC_ADDR_STR_LEN] is '\0', and the validation loop only reads offsets
0 through 16:

	for (i = 0; i < ETH_ALEN; i++) {
		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
			return false;
		if (i != ETH_ALEN - 1 && !strchr(delim, s[i * 3 + 2]))
			return false;
	}

So strings such as "aa:bb:cc:dd:ee:ff:00" or "aa:bb:cc:dd:ee:ffGARBAGE"
return true and fill @mac. The trailing characters are silently ignored.

mac_pton() already worked this way before this patch, so the code itself
is unchanged. What is new is the comment, which says @s must be exactly
MAC_ADDR_STR_LEN characters long and that malformed input returns false.
New callers that use other delimiters might trust that and skip their own
check for trailing data.

The only new caller in this series is get_ether_addr() in "usb: gadget:
u_ether: fix out-of-bounds read in get_ether_addr". Its old open-coded
parser also ignored trailing characters, so it sees no change. That later
patch does not touch lib/net_utils.c, so this mismatch is still there at
the end of the series.

Could the comment say something like "@s must begin with a
MAC_ADDR_STR_LEN-character MAC address; trailing characters are
ignored"? Changing the code instead would change mac_pton() behaviour for
existing callers.

[ ... ]

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

[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

Re: [PATCH v3 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 3 potential
issue(s) to consider.

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

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

- [Medium] The commit message misdescribes what users see after the
  change.
- [Low] get_ether_addr() still accepts malformed addresses that have
  trailing characters, and silently cuts them down to their first six…
- [Low] The patch fixes a slab-out-of-bounds read that KASAN reports in
  get_ether_addr(), reachable with `insmod g_ether.ko dev_addr=ab`, but…

--

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

This patch replaces the open coded six iteration parsing loop in
get_ether_addr() with parse_mac_pton() for ':' and '.' delimited
addresses, and with hex2bin() for the separator-less form. The goal is
to fix an out-of-bounds read on short strings and to stop accepting
non-hex digits. It also makes USB_U_ETHER select GENERIC_NET_UTILS.

Link: https://lore.kernel.org/all/20261104025058.2650911-3-liuc63@xiaopeng.com/

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

[ ... ]

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

[Severity: Medium]
Is the random address fallback accurate for the configfs path?

For "aabb.ccdd.eeff", parse_mac_pton() rejects the string because it is
shorter than MAC_ADDR_STR_LEN. hex2bin() rejects it at the '.' at offset
4. get_ether_addr() then returns 1, and the caller discards the random
address it wrote into new_addr:

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 _HOST_ADDR() in
u_ether_configfs.h pass that error back to userspace:

		ret = gether_set_dev_addr(opts->net, page);
		mutex_unlock(&opts->lock);
		if (!ret)
			ret = len;
		return ret;

So a gadget setup script that used to write "aabb.ccdd.eeff" without
error will now get EINVAL from write(2), and the old address stays in
place. It does not get a random address. Only the legacy module
parameter users (ether.c, cdc2.c, multi.c, ncm.c, g_ffs.c) ignore the
failure and keep the address that was randomised at creation.

Five functions use these configfs macros: f_ecm, f_eem, f_ncm, f_rndis
and f_subset, not just ECM and NCM.

Could the commit message describe this configfs ABI change and its
scope more precisely?

[ ... ]

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

[Severity: Low]
Should this still have a Fixes: tag? For bugs that predate git, the
usual convention is:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

2b3d942c4878 would also work. Without a tag, and with Cc: stable
dropped in v2, stable and distro backport tooling will not pick up this
fix for the out-of-bounds read that KASAN reports.

If the fix is backported, the commit that adds parse_mac_pton(),
e5765bbb2de1 ("lib/net_utils: allow mac_pton() to take a custom
delimiter"), also needs to be marked as a stable prerequisite. This
patch does not build without it.

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

[Severity: Low]
Can malformed addresses with trailing characters still get through
here? parse_mac_pton() only checks for a minimum length and never looks
past the 17th character:

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

hex2bin() reads exactly 12 characters and returns 0 no matter what
comes after them. get_ether_addr() does not check that the string ends
after the parsed MAC, optionally with a trailing newline.

As a result, "aa:bb:cc:dd:ee:ff:11", "aa:bb:cc:dd:ee:ffzz",
"aabbccddeeff00" and "112233445566778899" are all accepted and silently
cut down to their first six bytes. On the module parameter path the
address is marked NET_ADDR_SET. On the configfs path, which passes the
whole page, the store reports success.

The old loop also ignored anything after the sixth byte. However, the
commit message says "parse_mac_pton() checks the length". The
parse_mac_pton() kernel-doc also says "@s must be MAC_ADDR_STR_LEN
characters long" and "false if @s is malformed", which promises more
than the code enforces.

Would it make sense to reject trailing characters in get_ether_addr()?

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