From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1F8FAC77B7F for ; Mon, 8 May 2023 06:29:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232656AbjEHG3c (ORCPT ); Mon, 8 May 2023 02:29:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52054 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232011AbjEHG3a (ORCPT ); Mon, 8 May 2023 02:29:30 -0400 Received: from mail-m127104.qiye.163.com (mail-m127104.qiye.163.com [115.236.127.104]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 25D1D150C0; Sun, 7 May 2023 23:29:14 -0700 (PDT) Received: from [0.0.0.0] (unknown [172.96.223.238]) by mail-m127104.qiye.163.com (Hmail) with ESMTPA id 37CF1A403BC; Mon, 8 May 2023 14:29:00 +0800 (CST) Message-ID: <04e00a96-5923-cd8f-78ab-752a6b34f8af@sangfor.com.cn> Date: Mon, 8 May 2023 14:28:55 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [RFC PATCH] SUNRPC: Fix UAF in svc_tcp_listen_data_ready() Content-Language: en-US To: Chuck Lever III Cc: "jlayton@kernel.org" , "trond.myklebust@hammerspace.com" , "anna@kernel.org" , "davem@davemloft.net" , "edumazet@google.com" , "kuba@kernel.org" , "pabeni@redhat.com" , Bruce Fields , Linux NFS Mailing List , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20230507091131.23540-1-dinghui@sangfor.com.cn> <19f9a9bb-7164-dca0-1aff-da4a46b0ee74@sangfor.com.cn> <47C36B96-581F-4D51-8247-3ED9F1B4B948@oracle.com> From: Ding Hui In-Reply-To: <47C36B96-581F-4D51-8247-3ED9F1B4B948@oracle.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-HM-Spam-Status: e1kfGhgUHx5ZQUpXWQgPGg8OCBgUHx5ZQUlOS1dZFg8aDwILHllBWSg2Ly tZV1koWUFITzdXWS1ZQUlXWQ8JGhUIEh9ZQVlDTEhPVkpKSENKTBpIT0JMH1UTARMWGhIXJBQOD1 lXWRgSC1lBWUpMSVVCTVVJSUhVSUhDWVdZFhoPEhUdFFlBWU9LSFVKSktISkxVSktLVUtZBg++ X-HM-Tid: 0a87fa0ca994b282kuuu37cf1a403bc X-HM-MType: 1 X-HM-Sender-Digest: e1kMHhlZQR0aFwgeV1kSHx4VD1lBWUc6NBw6HQw5SD0XSjcpLy4XLEg6 DD4wCTVVSlVKTUNITklMSE9MT0xJVTMWGhIXVR8SFRwTDhI7CBoVHB0UCVUYFBZVGBVFWVdZEgtZ QVlKTElVQk1VSUlIVUlIQ1lXWQgBWUFPT05CNwY+ Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2023/5/8 12:00, Chuck Lever III wrote: > > >> On May 7, 2023, at 6:32 PM, Ding Hui wrote: >> >> On 2023/5/7 23:26, Chuck Lever III wrote: >>>> On May 7, 2023, at 5:11 AM, Ding Hui wrote: >>>> >>>> After the listener svc_sock freed, and before invoking svc_tcp_accept() >>>> for the established child sock, there is a window that the newsock >>>> retaining a freed listener svc_sock in sk_user_data which cloning from >>>> parent. In the race windows if data is received on the newsock, we will >>>> observe use-after-free report in svc_tcp_listen_data_ready(). >>> My thought is that not calling sk_odata() for the newsock >>> could potentially result in missing a data_ready event, >>> resulting in a hung client on that socket. >> >> I checked the vmcore, found that sk_odata points to sock_def_readable(), >> and the sk_wq of newsock is NULL, which be assigned by sk_clone_lock() >> unconditionally. >> >> Calling sk_odata() for the newsock maybe do not wake up any sleepers. >> >>> IMO the preferred approach is to ensure that svsk is always >>> safe to dereference in tcp_listen_data_ready. I haven't yet >>> thought carefully about how to do that. >> >> Agree, but I don't have a good way for now. > > Would a smartly-placed svc_xprt_get() hold the listener in place > until accept processing completes? > It is difficult and complicated to me. I think it's a little bit out of SUNRPC's control for the newsocks before accepted, e.g.: we don't know how many they have. Back to this RFC, I checked the code and thought it is safe by skipping sk_odata() for the newsocks before accepted in **svc_tcp_listen_data_ready()**, since these newsocks's sk_wq must be NULL, and will be assigned new one in sock_alloc_inode() called by kernel_accept(), so we can say if the child sock is not be accepted, there is nothing to be waked up. > >>>> Reproduce by two tasks: >>>> ... -- Thanks, - Ding Hui