netdev - Re: [PATCH net v1] rose: fix dangling neighbour pointers in rose_rt_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250628110155.82474-1-enjuk@amazon.com>
Date: Sat, 28 Jun 2025 20:01:37 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <enjuk@...zon.com>,
	<horms@...nel.org>, <kohei.enju@...il.com>, <kuba@...nel.org>,
	<kuniyu@...gle.com>, <linux-hams@...r.kernel.org>, <mingo@...nel.org>,
	<netdev@...r.kernel.org>,
	<syzbot+e04e2c007ba2c80476cb@...kaller.appspotmail.com>, <tglx@...utronix.de>
Subject: Re: [PATCH net v1] rose: fix dangling neighbour pointers in rose_rt_device_down()

>Message-ID: <7a25c9c4-610c-4e93-8855-1ec335cd2b64@...hat.com> (raw)
>In-Reply-To: <20250625133911.29344-1-enjuk@...zon.com>
>
>On 6/25/25 3:38 PM, Kohei Enju wrote:
>>> Message-ID: <20250625095005.66148-2-enjuk@...zon.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