Re: [PATCH net v3 2/2] net: rose: fix null-ptr-deref caused by rose_kill_by_neigh

[Dan Cross]

On Tue, Jun 28, 2022 at 11:59 PM <duoming@xxxxxxxxxx> 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