Re: [PATCH net v1] rose: fix dangling neighbour pointers in rose_rt_device_down()
From: Paolo Abeni <pabeni@redhat.com>
Date: 2025-06-26 09:31:11
Also in:
linux-hams
On 6/25/25 3:38 PM, Kohei Enju wrote:
quoted
Message-ID: [ref] (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. [...]
quoted
@@ -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