[PATCH net v2] rose: fix dangling neighbour pointers in rose_rt_device_down()

[PATCH net v2] rose: fix dangling neighbour pointers in rose_rt_device_down()

[Kohei Enju]

There are two bugs in rose_rt_device_down() that can cause
use-after-free:

1. The loop bound `t->count` is modified within the loop, which can
   cause the loop to terminate early and miss some entries.

2. When removing an entry from the neighbour array, the subsequent entries
   are moved up to fill the gap, but the loop index `i` is still
   incremented, causing the next entry to be skipped.

For example, if a node has three neighbours (A, A, B) with count=3 and A
is being removed, the second A is not checked.

    i=0: (A, A, B) -> (A, B) with count=2
          ^ checked
    i=1: (A, B)    -> (A, B) with count=2
             ^ checked (B, not A!)
    i=2: (doesn't occur because i < count is false)

This leaves the second A in the array with count=2, but the rose_neigh
structure has been freed. Code that accesses these entries assumes that
the first `count` entries are valid pointers, causing a use-after-free
when it accesses the dangling pointer.

Fix both issues by iterating over the array in reverse order with a fixed
loop bound. This ensures that all entries are examined and that the removal
of an entry doesn't affect subsequent iterations.

Reported-by: syzbot+e04e2c007ba2c80476cb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e04e2c007ba2c80476cb
Tested-by: syzbot+e04e2c007ba2c80476cb@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
Changes:
  v2:
    - Change commit message to describe the UAF scenario correctly
    - Replace for loop with memmove() for array shifting
  v1: https://lore.kernel.org/all/20250625095005.66148-2-enjuk@amazon.com/
---
 net/rose/rose_route.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
index 2dd6bd3a3011..b72bf8a08d48 100644
--- a/net/rose/rose_route.c
+++ b/net/rose/rose_route.c
@@ -497,22 +497,15 @@ void rose_rt_device_down(struct net_device *dev)
 			t         = rose_node;
 			rose_node = rose_node->next;
 
-			for (i = 0; i < t->count; i++) {
+			for (i = t->count - 1; i >= 0; i--) {
 				if (t->neighbour[i] != s)
 					continue;
 
 				t->count--;
 
-				switch (i) {
-				case 0:
-					t->neighbour[0] = t->neighbour[1];
-					fallthrough;
-				case 1:
-					t->neighbour[1] = t->neighbour[2];
-					break;
-				case 2:
-					break;
-				}
+				memmove(&t->neighbour[i], &t->neighbour[i + 1],
+					sizeof(t->neighbour[0]) *
+						(t->count - i));
 			}
 
 			if (t->count <= 0)
-- 
2.49.0

Re: [PATCH net v2] rose: fix dangling neighbour pointers in rose_rt_device_down()

[Simon Horman]

On Sun, Jun 29, 2025 at 12:06:31PM +0900, Kohei Enju wrote:
> There are two bugs in rose_rt_device_down() that can cause
> use-after-free:
> 
> 1. The loop bound `t->count` is modified within the loop, which can
>    cause the loop to terminate early and miss some entries.
> 
> 2. When removing an entry from the neighbour array, the subsequent entries
>    are moved up to fill the gap, but the loop index `i` is still
>    incremented, causing the next entry to be skipped.
> 
> For example, if a node has three neighbours (A, A, B) with count=3 and A
> is being removed, the second A is not checked.
> 
>     i=0: (A, A, B) -> (A, B) with count=2
>           ^ checked
>     i=1: (A, B)    -> (A, B) with count=2
>              ^ checked (B, not A!)
>     i=2: (doesn't occur because i < count is false)
> 
> This leaves the second A in the array with count=2, but the rose_neigh
> structure has been freed. Code that accesses these entries assumes that
> the first `count` entries are valid pointers, causing a use-after-free
> when it accesses the dangling pointer.
> 
> Fix both issues by iterating over the array in reverse order with a fixed
> loop bound. This ensures that all entries are examined and that the removal
> of an entry doesn't affect subsequent iterations.
> 
> Reported-by: syzbot+e04e2c007ba2c80476cb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e04e2c007ba2c80476cb
> Tested-by: syzbot+e04e2c007ba2c80476cb@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
> Changes:
>   v2:
>     - Change commit message to describe the UAF scenario correctly
>     - Replace for loop with memmove() for array shifting
>   v1: https://lore.kernel.org/all/20250625095005.66148-2-enjuk@amazon.com/

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v2] rose: fix dangling neighbour pointers in rose_rt_device_down()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 29 Jun 2025 12:06:31 +0900 you wrote:
> There are two bugs in rose_rt_device_down() that can cause
> use-after-free:
> 
> 1. The loop bound `t->count` is modified within the loop, which can
>    cause the loop to terminate early and miss some entries.
> 
> 2. When removing an entry from the neighbour array, the subsequent entries
>    are moved up to fill the gap, but the loop index `i` is still
>    incremented, causing the next entry to be skipped.
> 
> [...]

Here is the summary with links:
  - [net,v2] rose: fix dangling neighbour pointers in rose_rt_device_down()
    https://git.kernel.org/netdev/net/c/34a500caf48c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html