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

[netdev-bot+sashiko@kernel.org]

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