Re: [PATCH net] net/sched: em_ipt: reject hook values >= NF_INET_NUMHOOKS

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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 patch is redundant.

--

net/sched: em_ipt: reject hook values >= NF_INET_NUMHOOKS

This patch adds a range check on TCA_EM_IPT_HOOK in em_ipt_change(), so
hook values >= NF_INET_NUMHOOKS are rejected before check_match() uses
im->hook as a shift amount. According to the commit message, an "addrtype"
match can otherwise carry any hook value and trigger a UBSAN
shift-out-of-bounds.

> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
> index 26f1c4269a487..53358886a88c5 100644
> --- a/net/sched/em_ipt.c
> +++ b/net/sched/em_ipt.c
> @@ -168,6 +168,11 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
>  
>  	im->match = match;
>  	im->hook = nla_get_u32(tb[TCA_EM_IPT_HOOK]);
> +	if (im->hook >= NF_INET_NUMHOOKS) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +

[Severity: Medium]
Can this condition ever be true? The tree already contains commit
94cea342e891 ("net: sched: em_ipt: validate hook value from netlink").
That commit added the same check to em_ipt_change(), right after the
required-attribute check:

net/sched/em_ipt.c:em_ipt_change() {
    ...
	if (!tb[TCA_EM_IPT_HOOK] || !tb[TCA_EM_IPT_MATCH_NAME] ||
	    !tb[TCA_EM_IPT_MATCH_DATA] || !tb[TCA_EM_IPT_NFPROTO])
		return -EINVAL;

	if (nla_get_u32(tb[TCA_EM_IPT_HOOK]) >= NF_INET_NUMHOOKS)
		return -EINVAL;
    ...
}

That check runs before get_xt_match() and kzalloc(), for every match name,
"addrtype" included. A hook value of 32 is rejected with -EINVAL long
before check_match() gets to:

	mtpar.hook_mask	= 1 << im->hook;

So the new branch looks like dead code.

For the same reason, some statements in the commit message don't seem to
match the current tree:

  "em_ipt_change() reads the TCA_EM_IPT_HOOK netlink attribute into
  im->hook as an unbounded u32"

  "the only range check on the hook lives in
  policy_validate_match_data()"

  "A filter using match_name "addrtype" can therefore carry an arbitrary
  hook value."

Was the v7.3-rc3 reproduction done on a tree without 94cea342e891?

94cea342e891 has no Fixes: or Cc: stable tag. Would it be better to drop
this patch and ask for 94cea342e891 to be backported to stable instead?

If the range should also be enforced in the netlink policy, em_ipt_policy
still has a bare NLA_U32 entry:

	[TCA_EM_IPT_HOOK]		= { .type = NLA_U32 },

Could this become NLA_POLICY_MAX(NLA_U32, NF_INET_NUMHOOKS - 1) and replace
the open-coded check, instead of adding a second copy of it?

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