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

[Kohei Enju <enjuk@amazon.com>]

> Message-ID: <20250625095005.66148-2-enjuk@amazon.com> (raw)
> 
> There are two bugs in rose_rt_device_down() that can lead to
> 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, B, A) and A is being
> removed:
> - 1st iteration (i=0): A is removed, array becomes (B, A, A), count=2
> - 2nd iteration (i=1): We now check A instead of B, skipping B entirely
> - 3rd iteration (i=2): Loop terminates early due to count=2
> 
> This leaves the second A in the array with count=2, but the rose_neigh
> structure has been freed. Accessing code assumes that the first `count`
> entries are valid pointers, causing a use-after-free when it accesses
> the dangling pointer.

(Resending because I forgot to cite the patch, please ignore the former 
reply from me. Sorry for messing up.)

The example ([Senario2] below) in the commit message was incorrect. 
Correctly, UAF will happen in the [Senario1] below.

Let me clarify those senarios.

When the entries to be removed (A) are consecutive, the second A is not 
checked, leading to UAF.
[Senario1]
    (A, A, B) with count=3
    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)
    ===> A remains with count=2 although A was freed, so UAF will happen.


When the entries to be removed (A) are not consecutive, all A entries are 
removed luckily.
[Senario2]
    (A, B, A) with count=3
    i=0: 
         (A, B, A) -> (B, A) with count=2
          ^ checked
    i=1: 
         (B, A) -> (B) with count=1
             ^ checked (A, not B)
    i=2: (doesn't occur because i < count is false)
    ===> No A remains. No UAF in this case.

Although, even in the senario2, the fundamental issue remains 
because B is never checked.
The fix addresses issues by preventing unintended skips.

Please let me know if I'm overlooking something or my understanding is 
incorrect. 
Thanks!

> 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 the iteration of subsequent entries.
> 
> 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>
> ---
>  net/rose/rose_route.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
> index 2dd6bd3a3011..a488fd8c4710 100644
> --- a/net/rose/rose_route.c
> +++ b/net/rose/rose_route.c
> @@ -479,7 +479,7 @@ void rose_rt_device_down(struct net_device *dev)
>  {
>  	struct rose_neigh *s, *rose_neigh;
>  	struct rose_node  *t, *rose_node;
> -	int i;
> +	int i, j;
>  
>  	spin_lock_bh(&rose_node_list_lock);
>  	spin_lock_bh(&rose_neigh_list_lock);
> @@ -497,22 +497,14 @@ 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;
> -				}
> +				for (j = i; j < t->count; j++)
> +					t->neighbour[j] = t->neighbour[j + 1];
>  			}
>  
>  			if (t->count <= 0)
> -- 
> 2.48.1