Re: [PATCH] net/sched: em_meta: clamp shift to value width

[netdev-bot+sashiko@kernel.org]

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] In net/sched/em_meta.c, em_meta_change() (lines 942-945)
  silently rewrites a user-supplied hdr.shift above BITS_PER_LONG-1 down…
- [Low] Commit-message and DCO problems.
- [Low] The patch has no Fixes: tag even though it fixes user-reachable
  undefined behaviour.

--

net/sched: em_meta: clamp shift to value width

This patch clamps the user-supplied hdr.shift of both em_meta operands to
BITS_PER_LONG - 1 in em_meta_change(). The aim is to avoid an undefined
right shift in meta_int_apply_extras() when the u8 shift is BITS_PER_LONG
or larger.

> Hello,
>
[ ... ]
> Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07), net/sched/em_meta.c:762-770 (meta_int_apply_extras), 906-941 (em_meta_change).
> Longstanding code; the sibling fix pattern exists in act_pedit.
>
> This was found during an assisted (AI-supported) static source review of
> mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
> reporter against that tree. The patch below has NOT been compiled and has
> NOT been runtime tested, and no reproducer was produced - the triggering
> conditions are described above instead. Please treat this as a report for
> verification rather than a battle-tested fix.
>
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>

[Severity: Low]
The commit has no Signed-off-by: line, so it can't be applied under the
DCO. Could it be resent with one?

The only trailer is this Reported-by:, and it names the patch author.
Is it needed?

The greeting, the "Affected version/commit" line with its tree hash and
line numbers, and the paragraph on compile and test status read like
cover letter material. Should they go below the --- line, or be dropped?

The paragraph also says "every code path cited below was re-verified",
but nothing is cited after that sentence.

[Severity: Low]
The changelog only says "Longstanding code". The shift in
meta_int_apply_extras() goes back to the initial git import. Should this
carry a tag so stable trees pick up the fix?

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

> diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
> index 48bdd77df0a3e..6681c6793cb80 100644
> --- a/net/sched/em_meta.c
> +++ b/net/sched/em_meta.c
> @@ -936,6 +936,14 @@ static int em_meta_change(struct net *net, void *data, int len,
>  	memcpy(&meta->lvalue.hdr, &hdr->left, sizeof(hdr->left));
>  	memcpy(&meta->rvalue.hdr, &hdr->right, sizeof(hdr->right));
>  
> +	/* Keep the shift within the value width to avoid undefined
> +	 * behaviour in meta_int_apply_extras(), like act_pedit does.
> +	 */
> +	if (meta->lvalue.hdr.shift > BITS_PER_LONG - 1)
> +		meta->lvalue.hdr.shift = BITS_PER_LONG - 1;
> +	if (meta->rvalue.hdr.shift > BITS_PER_LONG - 1)
> +		meta->rvalue.hdr.shift = BITS_PER_LONG - 1;

[Severity: Medium]
Can this silent clamp change the match result for an out-of-range shift?
meta_int_apply_extras() uses the clamped value directly:

net/sched/em_meta.c:meta_int_apply_extras() {
	if (v->hdr.shift)
		dst->value >>= v->hdr.shift;
	...
}

When shift is BITS_PER_LONG - 1, the top bit survives. The result is 0 or
1, not the 0 you would get if every bit were shifted out.

For example, on a 32-bit kernel, meta(random shift 40 eq 0) would fail
about half the time. On 64-bit, the same request always gives 0 for a
u32-sourced value. The same __u8 uAPI value now gives a verdict that
depends on BITS_PER_LONG.

em_meta_dump() also reports the rewritten shift back to userspace:

net/sched/em_meta.c:em_meta_dump() {
	...
	memcpy(&hdr.left, &meta->lvalue.hdr, sizeof(hdr.left));
	memcpy(&hdr.right, &meta->rvalue.hdr, sizeof(hdr.right));

	if (nla_put(skb, TCA_EM_META_HDR, sizeof(hdr), &hdr))
	...
}

So tc filter show would print 63 or 31 instead of the configured shift.
Is that intended?

The new comment ties the clamp to the value width and to
meta_int_apply_extras(). However, the clamp also rewrites
TCF_META_TYPE_VAR operands. For those, shift is a byte trim count, and
a large value is not undefined:

net/sched/em_meta.c:meta_var_apply_extras() {
	int shift = v->hdr.shift;

	if (shift && shift < dst->len)
		dst->len -= shift;
}

var_dev and var_sk_bound_if are at most 15 bytes long, so VAR matching
does not change. The dump does change, though, and the comment doesn't
describe that path.

In act_pedit the shift is used to compute an offset. Here, the shifted
value is compared directly with the user operand.

Would one of these be a better fit?

- Reject shift >= BITS_PER_LONG for TCF_META_TYPE_INT with -EINVAL.
- Leave hdr untouched, and set dst->value to 0 in meta_int_apply_extras()
  when shift >= BITS_PER_LONG.

Either way, could the changelog describe the behaviour change?

> +
>  	if (!meta_is_supported(&meta->lvalue) ||
>  	    !meta_is_supported(&meta->rvalue)) {
>  		err = -EOPNOTSUPP;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/PonpZoq156a2aiUCGrHWYeg_kl3yFtp5oE2JaeCRJosmPfqWvGCrSlM-dn8lG7kSQpaAJuSaDN1zwr8X9UTC_DxfkFYZQVEQpV9kb-dMWxc%3D%40proton.me