Re: [RFC PATCH] SUNRPC: Fix UAF in svc_tcp_listen_data_ready()

[Ding Hui <dinghui@sangfor.com.cn>]

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@sangfor.com.cn> wrote:
>>
>> On 2023/5/7 23:26, Chuck Lever III wrote:
>>>> On May 7, 2023, at 5:11 AM, Ding Hui <dinghui@sangfor.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