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

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

[Kohei Enju]

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.

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

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

[Kohei Enju]

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!

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

[Kohei Enju]

> 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

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

[Paolo Abeni]

On 6/25/25 3:38 PM, Kohei Enju wrote:
>> 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.)

This resend was not needed.

> 
> The example ([Senario2] below) in the commit message was incorrect. 

Please send an updated version of the patch including the correct
description in the commit message.

[...]
>> @@ -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];

You can possibly use memmove() here instead of adding another loop.

/P

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

[Kohei Enju]

>Message-ID: <7a25c9c4-610c-4e93-8855-1ec335cd2b64@redhat.com> (raw)
>In-Reply-To: <20250625133911.29344-1-enjuk@amazon.com>
>
>On 6/25/25 3:38 PM, Kohei Enju wrote:
>>> 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.)
>
>This resend was not needed.

Acknowledged.

>> The example ([Senario2] below) in the commit message was incorrect. 
>
>Please send an updated version of the patch including the correct
>description in the commit message.

Sure, I'll update and send as V2.

>[...]
>>> @@ -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];
>
>You can possibly use memmove() here instead of adding another loop.

That sounds like a good suggestion. Thank you for reviewing!

>/P

Regards,
Kohei