netdev - Re: [PATCH] rds: fix use-after-free read in rds_find

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171231123354.GA9129@oracle.com>
Date:   Sun, 31 Dec 2017 07:33:54 -0500
From:   Sowmini Varadhan <sowmini.varadhan@...cle.com>
To:     "santosh.shilimkar@...cle.com" <santosh.shilimkar@...cle.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] rds: fix use-after-free read in rds_find_bound

On (12/30/17 21:09), santosh.shilimkar@...cle.com wrote:
> Right. This was loop transport in action so xmit will just flip
> the direction with receive. And rds_recv_incoming() can race with
> socket_release. rds_find_bound() is suppose to add ref count on
> socket for rds_recv_incoming() but by that time socket is DEAD &
> freed by socket release callback.

correct, that makes sense.

> And rds_release is suppose to be synced with rs_recv_lock. But
> release callback is marking the sk orphan before syncing
> up with receive path and updating the bind table. Probably it
> can pushed down further after the socket clean up buut need
> to think bit more.

However, I'm not sure this seals the race..  according to the
bug report rds_recv_incoming->rds_find_bound is being called
in rds_send_worker context and the rds_find_bound code is

     63         rs = rhashtable_lookup_fast(&bind_hash_table, &key, ht_parms);
     64         if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
     65                 rds_sock_addref(rs);
     66         else 
     67                 rs = NULL;
     68 

Since the entire logic of rds_release can interleave between line 63
and 64, (whereas we only addref at line 65), moving the sock_orphan
will not help.

I see that there was an explicic synchornization via the bucket->lock 
before 7b5654349e. I think you need something like that, or some type
or rcu-based hash list.

Patch below may make race-window smaller, but race window is still there.

> 
> 
> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index b405f77..11e1426 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -65,7 +65,6 @@ static int rds_release(struct socket *sock)
> 
>         rs = rds_sk_to_rs(sk);
> 
> -       sock_orphan(sk);
>         /* Note - rds_clear_recv_queue grabs rs_recv_lock, so
>          * that ensures the recv path has completed messing
>          * with the socket. */
> @@ -85,6 +84,7 @@ static int rds_release(struct socket *sock)
> 
>         rds_trans_put(rs->rs_transport);
> 
> +       sock_orphan(sk);
>         sock->sk = NULL;
>         sock_put(sk);
>  out: