Re: [PATCH] net/sched: fix potential stack infoleak in em_text_dump()

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The `Fixes:` tag and the commit message body misattribute the
  stack infoleak to b04202d6065c ("net/sched: replace strncpy with…

Pre-existing issues:
- [High] Out-of-bounds read on the request (parse) side of the same ABI
  field: em_text_change() (net/sched/em_text.c:50) passes the raw, user-…

--

net/sched: fix potential stack infoleak in em_text_dump()

This zero-initialises the on-stack struct tcf_em_text in em_text_dump()
so that the bytes of conf.algo[] not written by strscpy() are not copied
to user space by nla_put_nohdr().  KMSAN reported the leak on both
"tc filter show" and a raw RTM_GETTFILTER dump.

Link: https://lore.kernel.org/netdev/20250327143733.187438-1-richard120310@gmail.com/

A couple of questions below, one about the Fixes attribution and one
about the matching parse path.

> The original code used strncpy() which zero-pads to the destination size.
> Commit b04202d6065c ("net/sched: replace strncpy with strscpy") replaced
> it with strscpy(), which does not pad, creating this condition.
> Zero-initialising the struct closes it.
>
> Fixes: b04202d6065c ("net/sched: replace strncpy with strscpy")

[Severity: Medium]
Is this attribution accurate?  The call site that b04202d6065c replaced
was bounded by sizeof(conf.algo) - 1:

net/sched/em_text.c before b04202d6065c:
	strncpy(conf.algo, tm->config->ops->name, sizeof(conf.algo) - 1);

strncpy() zero-pads only up to the supplied bound n, not to the
destination size, so with a bound of 15 on the 16-byte algo[] the last
byte was never written:

include/uapi/linux/tc_ematch/tc_em_text.h:
	struct tcf_em_text {
		char		algo[TC_EM_TEXT_ALGOSIZ];
		...

Doesn't that mean nla_put_nohdr(skb, sizeof(conf), &conf) already copied
one uninitialised stack byte to user space before b04202d6065c, and that
b04202d6065c only widened the window (12 bytes for a three character name
such as "kmp", which matches the quoted "Bytes 168-179 of 199")?

If so, could the Fixes tag point at the introduction of em_text.c, or the
patch carry Cc: stable@vger.kernel.org covering the older trees?  As
written, stable selection driven by this Fixes tag will skip every tree
older than b04202d6065c, which keeps leaking the final byte of algo[] on
every dump of a text ematch.

Could the sentence "The original code used strncpy() which zero-pads to
the destination size ... creating this condition" also be adjusted, since
the old call padded only to the bound of 15?

The code change itself looks correct and complete for the current tree:
the uAPI struct has no interior padding (algo[0..15], from_offset,
to_offset, pattern_len, the layer bitfield byte, pad, sizeof == 24), so
= {} clears every transmitted byte.

> diff --git a/net/sched/em_text.c b/net/sched/em_text.c
> index 343f1aebeec2..4132f8c3c5fc 100644
> --- a/net/sched/em_text.c
> +++ b/net/sched/em_text.c
> @@ -113,7 +113,7 @@ static void em_text_destroy(struct tcf_ematch *m)
>  static int em_text_dump(struct sk_buff *skb, struct tcf_ematch *m)
>  {
>  	struct text_match *tm = EM_TEXT_PRIV(m);
> -	struct tcf_em_text conf;
> +	struct tcf_em_text conf = {};
>  
>  	strscpy(conf.algo, tm->config->ops->name);

[Severity: High]
This is a pre-existing issue and is not introduced by this patch, but
since it concerns the same algo[] field it may be worth handling in the
same series.

After this change the emitter always produces a NUL-terminated,
zero-padded algo[], while the parser accepts an algo[] with no NUL at
all.  em_text_change() validates only len, the layers and the offsets:

net/sched/em_text.c:em_text_change() {
	if (len < sizeof(*conf) || len < (sizeof(*conf) + conf->pattern_len))
		return -EINVAL;
	if (conf->from_layer > conf->to_layer)
		return -EINVAL;
	...
retry:
	ts_conf = textsearch_prepare(conf->algo, (u8 *) conf + sizeof(*conf),
				     conf->pattern_len, GFP_KERNEL, flags);
	...
		flags |= TS_AUTOLOAD;
		goto retry;
}

data here points straight into the user-supplied rtnetlink message, with
no copy and no termination:

net/sched/ematch.c:tcf_em_validate() {
	void *data = (void *) em_hdr + sizeof(*em_hdr);
	...
}

On the TS_AUTOLOAD retry the unterminated name reaches:

lib/textsearch.c:textsearch_prepare() {
	...
		request_module("ts_%s", algo);
}

and vsnprintf's %s handler scans for a zero byte with no length limit on
the source side:

lib/vsprintf.c:string_nocheck() {
	int lim = spec.precision;
	...
	while (lim--) {
		char c = *s++;
		if (!c)
			break;
	...
}

with spec.precision == -1 for a plain %s.  If a caller sets every byte of
algo[], from_offset, to_offset, pattern_len, the layer bitfield, pad and
the pattern to non-zero values, can this read past the end of algo[] and
past the end of the netlink message into adjacent kernel memory, and
embed those bytes into the modprobe request name?  For requests large
enough that netlink_alloc_large_skb() uses vmalloc, the scan would be
walking towards the guard page.

The reachable path is RTM_NEWTFILTER with an ematch tree containing a
TCF_EM_TEXT match whose algo does not resolve to a registered ts_ops, so
the first textsearch_prepare() returns -ENOENT and the retry with
TS_AUTOLOAD is taken.  That needs only CAP_NET_ADMIN in a network
namespace, plus CONFIG_MODULES.

Would rejecting an algo[] without a NUL byte in em_text_change() be the
right companion fix, so both ends of the uAPI agree on the format?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918133953.12494-1-bernard.ladenthin%40gmail.com