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

[Ding Hui]

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