Re: [PATCH net v3 2/2] net: rose: fix null-ptr-deref caused by rose_kill_by_neigh
From: Dan Cross <hidden>
Date: 2022-06-29 12:55:18
Also in:
linux-hams, lkml
On Tue, Jun 28, 2022 at 11:59 PM [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Hello, On Tue, 28 Jun 2022 13:12:40 +0200 Paolo Abeni wrote:quoted
[snip] I'm sorry, I likely was not clear enough in my previous reply. This is broken. If a list is [spin_]lock protected, you can't release the lock, reacquire it and continue traversing the list from the [now invalid] same iterator. e.g. if s is removed from the list, even if the sock is not de- allocated due to the addtional refcount, the traversing will errnously stop after this sock, instead of continuing processing the remaining socks in the list.I understand. The following is a new solution:diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c index bf2d986a6bc..24dcbde88fb 100644 --- a/net/rose/af_rose.c +++ b/net/rose/af_rose.c@@ -165,13 +165,21 @@ void rose_kill_by_neigh(struct rose_neigh *neigh) struct sock *s; spin_lock_bh(&rose_list_lock); +again: sk_for_each(s, &rose_list) { struct rose_sock *rose = rose_sk(s); if (rose->neighbour == neigh) { + sock_hold(s); + spin_unlock_bh(&rose_list_lock); + lock_sock(s); rose_disconnect(s, ENETUNREACH, ROSE_OUT_OF_ORDER, 0); rose->neighbour->use--; rose->neighbour = NULL; + release_sock(s); + spin_lock_bh(&rose_list_lock); + sock_put(s); + goto again;
It may be worthwhile noting that this changes the time complexity
of the algorithm to be O(n^2) in the number of entries in `rose_list`,
instead of linear. But as that number is extremely unlikely to ever
be large, it probably makes no practical difference.
- Dan C.
quoted hunk ↗ jump to hunk
} } spin_unlock_bh(&rose_list_lock);diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c index fee6409c2bb..b116828b422 100644 --- a/net/rose/rose_route.c +++ b/net/rose/rose_route.c@@ -827,7 +827,9 @@ void rose_link_failed(ax25_cb *ax25, int reason) ax25_cb_put(ax25); rose_del_route_by_neigh(rose_neigh); + spin_unlock_bh(&rose_neigh_list_lock); rose_kill_by_neigh(rose_neigh); + return; } spin_unlock_bh(&rose_neigh_list_lock); }If s is removed from the list, the traversing will not stop erroneously. Best regards, Duoming Zhou