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

[Kohei Enju <enjuk@amazon.com>]

>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