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

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]

Date:   Wed, 29 Jun 2022 08:54:37 -0400
From:   Dan Cross <crossd@...il.com>
To:     duoming@....edu.cn
Cc:     Paolo Abeni <pabeni@...hat.com>, linux-hams@...r.kernel.org,
        ralf@...ux-mips.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [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@....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