Re: [PATCH net-next v1 1/1] eea: Drop temporary buffer by using %pEp directly

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (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).

- [Medium] In eea_adminq_config_host_info()
  (drivers/net/ethernet/alibaba/eea/eea_adminq.c), the patch swaps…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the escape
  flags used in eea_adminq_config_host_info() do not escape anything.

--

eea: Drop temporary buffer by using %pEp directly

In eea_adminq_config_host_info(), this commit replaces the
string_escape_str() call followed by "%s" with a direct "%pEp" specifier.
It also drops the on-stack temporary buffer that held the escaped copy of
the device reply string.

> diff --git a/drivers/net/ethernet/alibaba/eea/eea_adminq.c b/drivers/net/ethernet/alibaba/eea/eea_adminq.c
> index 73d0caf257006..d17575a768900 100644
> --- a/drivers/net/ethernet/alibaba/eea/eea_adminq.c
> +++ b/drivers/net/ethernet/alibaba/eea/eea_adminq.c
> @@ -522,16 +522,8 @@ void eea_adminq_config_host_info(struct eea_net *enet)
>  			dev_warn(dev, "The hardware-driven state validation may be abnormal.\n");
>  
>  		if (rep->has_reply) {
> -			char buf[EEA_HINFO_MAX_REP_LEN] = {0};
> -
>  			rep->reply_str[EEA_HINFO_MAX_REP_LEN - 1] = '\0';
> -
> -			string_escape_str(rep->reply_str, buf, sizeof(buf),
> -					  ESCAPE_NP, NULL);
> -
> -			buf[EEA_HINFO_MAX_REP_LEN - 1] = '\0';
> -
> -			dev_warn(dev, "Device replied: %s\n", buf);
> +			dev_warn(dev, "Device replied: %pEp\n", rep->reply_str);

[Severity: Medium]
Will this print only the first byte of rep->reply_str?

%pE does not look for a NUL terminator. It takes the buffer length from
the field width, and when no width is given it uses 1 byte:

lib/vsprintf.c:escaped_string() {
    ...
	len = spec.field_width < 0 ? 1 : spec.field_width;
    ...
}

Documentation/core-api/printk-formats.rst says the same:

    If field width is omitted then 1 byte only will be escaped.

The removed code escaped the whole string, because string_escape_str()
calls string_escape_mem(src, strlen(src), ...).

With this change, a reply of "abc" would be logged as "Device replied: a".
If reply_str[0] is NUL, the NUL byte is copied raw into the log record,
because ESCAPE_NP alone passes it through unchanged.

The commit message describes this as a like-for-like conversion. Should
the length be passed explicitly, for example:

	dev_warn(dev, "Device replied: %*pEp\n",
		 (int)strnlen(rep->reply_str, EEA_HINFO_MAX_REP_LEN),
		 rep->reply_str);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
does ESCAPE_NP on its own actually escape anything here?

The old string_escape_str(..., ESCAPE_NP, NULL) and the new %pEp both
select only ESCAPE_NP. The 'p' makes flags non-zero, so escaped_string()
never falls back to its ESCAPE_ANY_NP default:

lib/vsprintf.c:escaped_string() {
    ...
		case 'p':
			flags |= ESCAPE_NP;
			break;
    ...
	if (!flags)
		flags = ESCAPE_ANY_NP;
    ...
}

In string_escape_mem(), printable bytes pass through at the ESCAPE_NP
check. A non-printable byte skips the ESCAPE_SPACE, ESCAPE_SPECIAL,
ESCAPE_NULL, ESCAPE_OCTAL and ESCAPE_HEX branches, because none of those
flags is set. The final escape_passthrough() then copies it raw:

lib/string_helpers.c:string_escape_mem() {
    ...
		if (!(is_append && in_dict) && isprint(c) &&
		    flags & ESCAPE_NP && escape_passthrough(c, &p, end))
			continue;
    ...
		escape_passthrough(c, &p, end);
	}
    ...
}

The kernel-doc for string_escape_mem() also notes that "It doesn't make
much sense to use either of them without %ESCAPE_OCTAL or %ESCAPE_HEX".

The device fills rep->reply_str through the admin queue response, and the
driver only forces the last byte to NUL. As a result, CR/LF, ANSI escape
sequences and high-bit bytes from the device reach the kernel log
unescaped.

Since this line is being rewritten anyway, could it use a flag set that
does escape, such as %*pE (ESCAPE_ANY_NP), %*pEhp or %*pEop, along with
an explicit width?

>  		}
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929082722.3368741-1-andriy.shevchenko%40linux.intel.com