From mboxrd@z Thu Jan 1 00:00:00 1970 From: Subash Abhinov Kasiviswanathan Subject: Re: Crash due to destroying TCP request sockets using SOCK_DESTROY Date: Fri, 06 Jul 2018 17:24:04 -0600 Message-ID: <2e8c55a7adff3aff6e62738c793e84e3@codeaurora.org> References: <95aaa3d59cb3c9cc11b6b83880d92fec@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Alistair Strachan , David Ahern To: Eric Dumazet , Lorenzo Colitti Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:51416 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932478AbeGFXYE (ORCPT ); Fri, 6 Jul 2018 19:24:04 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: >> Looks like for a TCP_NEW_SYN_RECV socket, sock_diag_destroy >> essentially ends up doing: >> >> struct request_sock *req = inet_reqsk(sk); >> >> local_bh_disable(); >> >> inet_csk_reqsk_queue_drop_and_put(req->rsk_listener, >> req); >> local_bh_enable(); >> ... >> >> sock_gen_put(sk); >> >> It looks like inet_csk_reqsk_queue_drop_and_put calls reqsk_put(req), >> which frees the socket, and at that point sock_gen_put is a UAF. Do we >> just need: >> >> - >> inet_csk_reqsk_queue_drop_and_put(req->rsk_listener, >> - req); >> + inet_csk_reqsk_queue_drop(req->rsk_listener, >> req); >> >> since sock_gen_put will also end up calling reqsk_put() for a >> TCP_SYN_RECV socket? >> >> Alastair - you're able to reproduce this UAF using net_test on qemu, >> right? If so, could you try that two-line patch above? >> > > Hi Lorenzo > > Your patch makes sense to me, please submit it formally with : > > Fixes: d7226c7a4dd1 ("net: diag: Fix refcnt leak in error path > destroying socket") > Cc: David Ahern > > Thanks ! Thanks Lorenzo and Eric. I will try it out locally. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project