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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[quantumvoid]

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.

Always check len >= MAX_ALLOWEDIPS_DEPTH and only gate the
WARN splat on DEBUG.

Signed-off-by: quantumvoid0 <quantumvoid0@proton.me>
---
 drivers/net/wireguard/allowedips.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireguard/allowedips.c b/drivers/net/wireguard/allowedips.c
index 5ece9ac..dc383f5 100644
--- a/drivers/net/wireguard/allowedips.c
+++ b/drivers/net/wireguard/allowedips.c
@@ -42,8 +42,10 @@ static void push_rcu(struct allowedips_node **stack,
 		     struct allowedips_node __rcu *p, unsigned int *len)
 {
 	if (rcu_access_pointer(p)) {
-		if (WARN_ON(IS_ENABLED(DEBUG) && *len >= MAX_ALLOWEDIPS_DEPTH))
+		if (unlikely(*len >= MAX_ALLOWEDIPS_DEPTH)) {
+			WARN_ON(IS_ENABLED(DEBUG));
 			return;
+		}
 		stack[(*len)++] = rcu_dereference_raw(p);
 	}
 }
--
2.55.0

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

[netdev-bot+sashiko]

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

Re: [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@kernel.org <netdev-bot+sashiko@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@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
>

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

[Jason A. Donenfeld]

This is not reachable. The DEBUG is there because the point of the
selftests is to reach it, but not for this to run in production. I
coded it this way intentionally. I don't want to remove the DEBUG
test.

Jason

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

[quantumvoid0]

Understood, thanks for explaining. Ill drop this.

quantumvoid0

On Monday, October 5th, 2026 at 12:19 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> This is not reachable. The DEBUG is there because the point of the
> selftests is to reach it, but not for this to run in production. I
> coded it this way intentionally. I don't want to remove the DEBUG
> test.
> 
> Jason
>