From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sowmini Varadhan Subject: Re: [PATCH] rds: fix use-after-free read in rds_find_bound Date: Sun, 31 Dec 2017 07:33:54 -0500 Message-ID: <20171231123354.GA9129@oracle.com> References: <1514662599-14491-1-git-send-email-santosh.shilimkar@oracle.com> <20171230202631.GB27855@oracle.com> <27c5708f-5d56-b2bd-c9b8-82a3e5f728f9@oracle.com> <20171230223238.GC27855@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, davem@davemloft.net To: "santosh.shilimkar@oracle.com" Return-path: Received: from userp2130.oracle.com ([156.151.31.86]:38637 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751123AbdLaMeE (ORCPT ); Sun, 31 Dec 2017 07:34:04 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On (12/30/17 21:09), santosh.shilimkar@oracle.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: