From mboxrd@z Thu Jan 1 00:00:00 1970 From: "santosh.shilimkar@oracle.com" Subject: Re: [PATCH] rds: fix use-after-free read in rds_find_bound Date: Sun, 31 Dec 2017 14:30:02 -0800 Message-ID: <90022a2a-7edb-b348-1a07-afb8a26ccd7a@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> <20171231123354.GA9129@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, davem@davemloft.net To: Sowmini Varadhan Return-path: Received: from userp2120.oracle.com ([156.151.31.85]:47722 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750952AbdLaWaH (ORCPT ); Sun, 31 Dec 2017 17:30:07 -0500 In-Reply-To: <20171231123354.GA9129@oracle.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/31/17 4:33 AM, Sowmini Varadhan wrote: > 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. > Yea. In fact the earlier point of sk being null and rs not is also possible because socket release explicitly marks its NULL ("sock->sk = NULL"). But it just side effect of the actual race. >> 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. > The rhashtable already has internal bucket lock so those operation like add/remove are synced up. But yes reference addition can still race with receive since receive lock is taken after find bound. > Patch below may make race-window smaller, but race window is still there. >If the receive lock is taken ahead then with sock_orphan change socket release will get synchronized with receive. But preventing socket release to be getting triggered while in receive path by means ref count is better option. Moving socket_orphan down is anyway a good change but its not enough. Will think bit more about it. Thanks for the good discussion. Regards, Santosh