netdev - Re: [PATCH] sunrpc: need delete xprt->timer in xs

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]

Date:   Sun, 7 Jun 2020 01:03:05 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "davem@...emloft.net" <davem@...emloft.net>,
        "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "zhengbin13@...wei.com" <zhengbin13@...wei.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bfields@...ldses.org" <bfields@...ldses.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>
CC:     "yuehaibing@...wei.com" <yuehaibing@...wei.com>,
        "weiyongjun1@...wei.com" <weiyongjun1@...wei.com>
Subject: Re: [PATCH] sunrpc: need delete xprt->timer in xs_destroy

On Fri, 2020-06-05 at 10:10 +0800, Zhengbin (OSKernel) wrote:
> The complete process is like this:
> 
> xprt_destroy
>    wait_on_bit_lock(&xprt->state, XPRT_LOCKED,
> TASK_UNINTERRUPTIBLE)  
> -->getlock
>    del_timer_sync(&xprt->timer)   -->del xprt->timer
>    INIT_WORK(&xprt->task_cleanup, xprt_destroy_cb)
> 
> xprt_destroy_cb
>    xs_destroy(xprt->ops->destroy)
>      cancel_delayed_work_sync     -->will call 
> transport->connect_worker, whose callback is xs_udp_setup_socket
>      xs_xprt_free(xprt)                    -->free xprt
> 
> xs_udp_setup_socket
>    sock = xs_create_sock
>    xprt_unlock_connect
>        if (!test_bit(XPRT_LOCKED, &xprt->state)) -->state is
> XPRT_LOCKED
>        xprt_schedule_autodisconnect
>          mod_timer
>            internal_add_timer  -->insert xprt->timer to base timer
> list

The call to xprt_lock_connect() in xs_connect() is supposed to ensure
exclusion w.r.t. wait_on_bit_lock(). In other words if the transport-
>connect_worker is actually queued, it is also supposed to be holding
the XPRT_LOCK and ensuring that we can't get into the above situation.

Why is that not the case here?

> 
> On 2020/6/4 23:39, Trond Myklebust wrote:
> > On Thu, 2020-06-04 at 22:49 +0800, Zheng Bin wrote:
> > > If RPC use udp as it's transport protocol, transport-
> > > >connect_worker
> > > will call xs_udp_setup_socket.
> > > xs_udp_setup_socket
> > >    sock = xs_create_sock
> > >    if (IS_ERR(sock))
> > >      goto out;
> > >    out:
> > >      xprt_unlock_connect
> > >        xprt_schedule_autodisconnect
> > >          mod_timer
> > >            internal_add_timer  -->insert xprt->timer to base
> > > timer
> > > list
> > > 
> > > xs_destroy
> > >    cancel_delayed_work_sync(&transport->connect_worker)
> > >    xs_xprt_free(xprt)           -->free xprt
> > > 
> > > Thus use-after-free will happen.
> > > 
> > > Signed-off-by: Zheng Bin <zhengbin13@...wei.com>
> > > ---
> > >   net/sunrpc/xprtsock.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > > index 845d0be805ec..c796808e7f7a 100644
> > > --- a/net/sunrpc/xprtsock.c
> > > +++ b/net/sunrpc/xprtsock.c
> > > @@ -1242,6 +1242,7 @@ static void xs_destroy(struct rpc_xprt
> > > *xprt)
> > >   	dprintk("RPC:       xs_destroy xprt %p\n", xprt);
> > > 
> > >   	cancel_delayed_work_sync(&transport->connect_worker);
> > > +	del_timer_sync(&xprt->timer);
> > >   	xs_close(xprt);
> > >   	cancel_work_sync(&transport->recv_worker);
> > >   	cancel_work_sync(&transport->error_worker);
> > > --
> > > 2.26.0.106.g9fadedd
> > > 
> > I'm confused. How can this happen given that xprt_destroy() first
> > takes
> > the XPRT_LOCK, and then deletes xprt->timer?
> > 
> > Right now, the socket code knows nothing about the details of xprt-
> > > timer and what it is used for. I'd prefer to keep it that way if
> > possible.
> > 
-- 
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022
www.hammer.space