linux-kernel - Re: [PATCH] SUNRPC: Fix refcnt leak in rpc_clnt_test_and_add

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:   Mon, 20 Apr 2020 11:54:43 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "xiyuyang19@...an.edu.cn" <xiyuyang19@...an.edu.cn>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "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:     "tanxin.ctf@...il.com" <tanxin.ctf@...il.com>,
        "yuanxzhang@...an.edu.cn" <yuanxzhang@...an.edu.cn>,
        "kjlu@....edu" <kjlu@....edu>
Subject: Re: [PATCH] SUNRPC: Fix refcnt leak in rpc_clnt_test_and_add_xprt

On Mon, 2020-04-20 at 13:45 +0800, Xiyu Yang wrote:
> rpc_clnt_test_and_add_xprt() invokes xprt_switch_get() and
> xprt_get(),
> which returns a reference of the rpc_xprt_switch object to "data-
> >xps"
> and a reference of the rpc_xprt object to "data->xprt" with increased
> refcount.
> 
> When rpc_clnt_test_and_add_xprt() returns, local variable "data" and
> its
> field "xps" as well as "xprt" becomes invalid, so their refcounts
> should
> be decreased to keep refcount balanced.
> 
> The reference counting issue happens in one exception handling paths
> of
> rpc_clnt_test_and_add_xprt(). When rpc_call_null_helper() returns
> IS_ERR, the refcnt increased by xprt_switch_get() and xprt_get() are
> not
> decreased, causing a refcnt leak.
> 
> Fix this issue by calling rpc_cb_add_xprt_release() to decrease
> related
> refcounted fields in "data" and then release it when
> rpc_call_null_helper() returns IS_ERR.
> 
> Fixes: 7f554890587c ("SUNRPC: Allow addition of new transports to a
> struct rpc_clnt")
> Signed-off-by: Xiyu Yang <xiyuyang19@...an.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@...il.com>
> ---
>  net/sunrpc/clnt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 7324b21f923e..f86d9ae2167f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2803,8 +2803,10 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt
> *clnt,
>  	task = rpc_call_null_helper(clnt, xprt, NULL,
>  			RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC|
> RPC_TASK_NULLCREDS,
>  			&rpc_cb_add_xprt_call_ops, data);
> -	if (IS_ERR(task))
> +	if (IS_ERR(task)) {
> +		rpc_cb_add_xprt_release(data);
>  		return PTR_ERR(task);

We should just get rid of the IS_ERR() condition here. It cannot ever
trigger, which is why rpc_run_task() no longer checks for it, and no
longer calls the ->rpc_release() method on error.

> +	}
>  	rpc_put_task(task);
>  success:
>  	return 1;
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@...merspace.com