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

Message-ID: <bc4755e6c5cee7a326cf06f983907a3170be1649.camel@hammerspace.com>
Date:   Thu, 4 Jun 2020 15:39:58 +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>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "bfields@...ldses.org" <bfields@...ldses.org>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>
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 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
Linux NFS client maintainer, Hammerspace
trond.myklebust@...merspace.com