linux-kernel - Re: [RFC PATCH] SUNRPC: Fix UAF in svc_tcp_listen_data

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

Message-ID: <2e8da045-354f-43a1-72b9-9644d1e2f280@sangfor.com.cn>
Date:   Mon, 15 May 2023 09:17:56 +0800
From:   Ding Hui <dinghui@...gfor.com.cn>
To:     Chuck Lever III <chuck.lever@...cle.com>
Cc:     "jlayton@...nel.org" <jlayton@...nel.org>,
        "trond.myklebust@...merspace.com" <trond.myklebust@...merspace.com>,
        "anna@...nel.org" <anna@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] SUNRPC: Fix UAF in svc_tcp_listen_data_ready()

On 2023/5/15 2:29, Chuck Lever III wrote:
> [ Removing the stale address for Bruce from the Cc, as he no longer
>    works at Red Hat. ]
> 
> 
>> On May 7, 2023, at 9:32 PM, Ding Hui <dinghui@...gfor.com.cn> wrote:
>>
>> On 2023/5/7 23:26, Chuck Lever III wrote:
>>>> On May 7, 2023, at 5:11 AM, Ding Hui <dinghui@...gfor.com.cn> 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.
>>
>>>> Reproduce by two tasks:
>>>>
>>>> 1. while :; do rpc.nfsd 0 ; rpc.nfsd; done
>>>> 2. while :; do echo "" | ncat -4 127.0.0.1 2049 ; done
> 
> I haven't been able to reproduce a crash with this snippet. But

KASAN report should be easier to reproduce than real crash.

> I've done some archaeology to understand the problem better.
> 
> I found that svc_tcp_listen_data_ready is actually invoked /three/
> times: once for the listener socket, and /twice/ for the child.
> The big comment, which pre-dates the git era, appears to be
> somewhat stale; or perhaps it's the specifics of this particular
> test that triggers the third call.
> 
> I reviewed several other tcp_listen_data_ready callbacks. They
> generally do not do anything at all with non-listener sockets,
> suggesting that approach would likely be safe for NFSD.
> 
> Prior to commit 939bb7ef901b ("[PATCH] Code cleanups in calbacks
> in svcsock"), this data_ready callback was a complete no-op for
> non-listener sockets as well. That commit is described as only
> a clean-up, but it indeed changes the logic.
> 
> I also note that most other data_ready callbacks take the
> sk_callback_lock, and svc_tcp_listen_data_ready does not. Not
> clear to me whether svc_tcp_listen_data_ready should be taking
> that lock too.
> 

I notice the lock too, IMO the sk_callback_lock should be used
to protect the svsk avoiding be freed during in the callbacks.

Perhaps it can be reproduced by increasing the processing time in
svc_tcp_listen_data_ready(), but anyway, it would be another issue.

> The upshot is that I think it would be reasonable to simply do
> nothing in svc_tcp_listen_data_ready() if state != TCP_LISTEN.
> 

Thanks for the information.

I will send the formal patch soon later.

-- 
Thanks,
- Ding Hui