lkml.org 
[lkml]   [2022]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH net v3 2/2] net: rose: fix null-ptr-deref caused by rose_kill_by_neigh
On Tue, Jun 28, 2022 at 11:59 PM <duoming@zju.edu.cn> wrote:
> Hello,
>
> On Tue, 28 Jun 2022 13:12:40 +0200 Paolo Abeni wrote:
> > [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.

> }
> }
> 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

\
 
 \ /
  Last update: 2022-06-29 14:56    [W:1.214 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site