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

[quantumvoid0]

Thanks, both points are valid.

Medium is correct, not reachable in tree. cidr strictly increases
along root->leaf so depth <=129, DFS stack stays <=129. This is
hardening only so a future bug cant smash stack[129] in production
where WARN_ON(DEBUG && ...) compiles out. Ill reword v2 to say
hardening, no Fixes: tag.

Low is correct; author/Sob name mismatch. Will fix to match in v2.

pw-bot: cr


On Monday, October 5th, 2026 at 4:25 AM, netdev-bot+sashiko at kernel.org <netdev-bot+sashiko at kernel.org> wrote:

> 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 at proton.me>' but
>   the Signed-off-by trailer says 'quantumvoid0 <quantumvoid0 at 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 at 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
>