Re: [PATCH] wireguard: allowedips: prevent stack overflow without DEBUG

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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 commit message says production builds have "an unchecked
  stack[129] write" in push_rcu() (drivers/net/wireguard/allowedips.c),…
- [Low] The commit author is 'quantumvoid <quantumvoid0@proton.me>' but
  the Signed-off-by trailer says 'quantumvoid0 <quantumvoid0@proton.me>'.

--

wireguard: allowedips: prevent stack overflow without DEBUG

This commit changes push_rcu() so that the len >= MAX_ALLOWEDIPS_DEPTH
check is always evaluated and only the WARN is gated on DEBUG. The commit
message presents this as a fix for an unchecked stack[129] write in
production builds.

> push_rcu() only bounds-checked the traversal stack when
> DEBUG is defined. In production the check is compiled out,
> leaving an unchecked stack[129] write.

[Severity: Medium]
Is the stack[129] write actually reachable in the current tree? The trie
invariants seem to rule it out.

add() rejects a cidr larger than the address width:

    if (unlikely(cidr > bits || !peer))
        return -EINVAL;

On an exact prefix match, node_placement() reuses the existing node. When
a node is split, it gets this cidr:

    cidr = min(cidr, common_bits(down, key, bits));

That value is greater than the parent's cidr and smaller than the
children's cidr. So cidr strictly increases along every root-to-leaf path,
and a node with cidr == bits has no children. That caps any path at 129
nodes.

root_free_rcu() and root_remove_peer_lists() pop one node before pushing
at most two children. While a node at depth d is being handled, the stack
holds at most d + 2 <= 129 entries. A push with *len == 129 never happens,
so stack[129] is never written.

The DEBUG-only gating also looks deliberate. The selftest in
drivers/net/wireguard/selftest/allowedips.c builds a maximal-depth trie to
exercise this assertion. Commit 46622219aae2 ("wireguard: allowedips:
expand maximum node depth") sized the array to 129 for this reason and
fixed the real off-by-one there.

Could the commit message call this hardening of a debug-only invariant
check? The other option is to show a path that breaks the depth invariant.
As written, it reads as a fix for a reachable stack overflow, with no
trigger and no Fixes: tag.

> Signed-off-by: quantumvoid0 <quantumvoid0@proton.me>

[Severity: Low]
This isn't a bug, but the name in the Signed-off-by (quantumvoid0) does
not match the Author name (quantumvoid), even though the email is the
same. checkpatch.pl warns about this mismatch. Could the two be made
consistent?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/TaZxFNfdnpo-3szeC-LoWZBxj-6XGhXkdEqnQi6yyaOHXp8cw4Xtksw07sYrIpludwvJQrukFoCGw3r9rXDcInkdB5QD2EswKPNsXMU3qMQ%3D%40proton.me