Date:   Mon, 24 Dec 2018 11:59:30 -0500
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Vasily Averin <vvs@virtuozzo.com>
Cc:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Jeff Layton <jlayton@kernel.org>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Chuck Lever <chuck.lever@oracle.com>,
        linux-nfs@vger.kernel.org,
        Evgenii Shatokhin <eshatokhin@virtuozzo.com>
Subject: Re: [PATCH v4 02/10] sunrpc: use-after-free in svc_process_common()
Message-ID: <20181224165930.GA11596@fieldses.org>
References: <cover.1545648882.git.vvs@virtuozzo.com>
 <2d71abdb-2e79-4b13-21a6-3d439ddd96c5@virtuozzo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2d71abdb-2e79-4b13-21a6-3d439ddd96c5@virtuozzo.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Mon, Dec 24, 2018 at 02:44:52PM +0300, Vasily Averin wrote:
> if node have NFSv41+ mounts inside several net namespaces
> it can lead to use-after-free in svc_process_common()
> 
> svc_process_common()
>         /* Setup reply header */
>         rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp); <<< HERE
> 
> svc_process_common() can use incorrect rqstp->rq_xprt,
> its caller function bc_svc_process() takes it from serv->sv_bc_xprt.
> The problem is that serv is global structure but sv_bc_xprt
> is assigned per-netnamespace.
> 
> According to Trond, the whole "let's set up rqstp->rq_xprt
> for the back channel" is nothing but a giant hack in order
> to work around the fact that svc_process_common() uses it
> to find the xpt_ops, and perform a couple of (meaningless
> for the back channel) tests of xpt_flags.
> 
> All we really need in svc_process_common() is to be able to run
> rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr()
> 
> Bruce J Fields points that this xpo_prep_reply_hdr() call
> is an awfully roundabout way just to do "svc_putnl(resv, 0);"
> in the tcp case.
> 
> This patch does not initialiuze rqstp->rq_xprt in bc_svc_process(),
> now it calls svc_process_common() with rqstp->rq_xprt = NULL.
> 
> To adjust reply header svc_process_common() just check
> rqstp->rq_prot and calls svc_tcp_prep_reply_hdr() for tcp case.
> 
> To handle rqstp->rq_xprt = NULL case in functions called from
> svc_process_common() patch intruduces net namespace pointer
> svc_rqst->rq_bc_net and adjust SVC_NET() definition.
> Some other function was also adopted to properly handle described case.
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  include/linux/sunrpc/svc.h    | 5 ++++-
>  include/trace/events/sunrpc.h | 6 ++++--
>  net/sunrpc/svc.c              | 9 +++++----
>  net/sunrpc/svc_xprt.c         | 5 +++--
>  net/sunrpc/svcsock.c          | 2 +-
>  5 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 73e130a840ce..fdb6b317d974 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -295,9 +295,12 @@ struct svc_rqst {
>  	struct svc_cacherep *	rq_cacherep;	/* cache info */
>  	struct task_struct	*rq_task;	/* service thread */
>  	spinlock_t		rq_lock;	/* per-request lock */
> +	struct net		*rq_bc_net;	/* pointer to backchannel's
> +						 * net namespace
> +						 */
>  };
>  
> -#define SVC_NET(svc_rqst)	(svc_rqst->rq_xprt->xpt_net)
> +#define SVC_NET(rqst) (rqst->rq_xprt ? rqst->rq_xprt->xpt_net : rqst->rq_bc_net)
>  
>  /*
>   * Rigorous type checking on sockaddr type conversions
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index 28e384186c35..8617f4fd6b70 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -569,7 +569,8 @@ TRACE_EVENT(svc_process,
>  		__field(u32, vers)
>  		__field(u32, proc)
>  		__string(service, name)
> -		__string(addr, rqst->rq_xprt->xpt_remotebuf)
> +		__string(addr, rqst->rq_xprt ?
> +			 rqst->rq_xprt->xpt_remotebuf : "(null)")
>  	),
>  
>  	TP_fast_assign(
> @@ -577,7 +578,8 @@ TRACE_EVENT(svc_process,
>  		__entry->vers = rqst->rq_vers;
>  		__entry->proc = rqst->rq_proc;
>  		__assign_str(service, name);
> -		__assign_str(addr, rqst->rq_xprt->xpt_remotebuf);
> +		__assign_str(addr, rqst->rq_xprt ?
> +			     rqst->rq_xprt->xpt_remotebuf : "(null)");
>  	),
>  
>  	TP_printk("addr=%s xid=0x%08x service=%s vers=%u proc=%u",
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index d13e05f1a990..fb647bc01fc5 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1172,7 +1172,8 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>  	clear_bit(RQ_DROPME, &rqstp->rq_flags);
>  
>  	/* Setup reply header */
> -	rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp);
> +	if (rqstp->rq_prot == IPPROTO_TCP)
> +		svc_tcp_prep_reply_hdr(rqstp);

svc_tcp_prep_reply_hdr is a one-liner, I'd just copy the svc_putnl() and
comment here.

--b.

>  
>  	svc_putu32(resv, rqstp->rq_xid);
>  
> @@ -1244,7 +1245,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>  	 * for lower versions. RPC_PROG_MISMATCH seems to be the closest
>  	 * fit.
>  	 */
> -	if (versp->vs_need_cong_ctrl &&
> +	if (versp->vs_need_cong_ctrl && rqstp->rq_xprt &&
>  	    !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags))
>  		goto err_bad_vers;
>  
> @@ -1336,7 +1337,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>  	return 0;
>  
>   close:
> -	if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
> +	if (rqstp->rq_xprt && test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
>  		svc_close_xprt(rqstp->rq_xprt);
>  	dprintk("svc: svc_process close\n");
>  	return 0;
> @@ -1459,10 +1460,10 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
>  	dprintk("svc: %s(%p)\n", __func__, req);
>  
>  	/* Build the svc_rqst used by the common processing routine */
> -	rqstp->rq_xprt = serv->sv_bc_xprt;
>  	rqstp->rq_xid = req->rq_xid;
>  	rqstp->rq_prot = req->rq_xprt->prot;
>  	rqstp->rq_server = serv;
> +	rqstp->rq_bc_net = req->rq_xprt->xprt_net;
>  
>  	rqstp->rq_addrlen = sizeof(req->rq_xprt->addr);
>  	memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen);
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 51d36230b6e3..bd42da287c26 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -468,10 +468,11 @@ static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
>   */
>  void svc_reserve(struct svc_rqst *rqstp, int space)
>  {
> +	struct svc_xprt *xprt = rqstp->rq_xprt;
> +
>  	space += rqstp->rq_res.head[0].iov_len;
>  
> -	if (space < rqstp->rq_reserved) {
> -		struct svc_xprt *xprt = rqstp->rq_xprt;
> +	if (xprt && space < rqstp->rq_reserved) {
>  		atomic_sub((rqstp->rq_reserved - space), &xprt->xpt_reserved);
>  		rqstp->rq_reserved = space;
>  
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 986f3ed7d1a2..793149ba1bda 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1173,7 +1173,7 @@ static int svc_tcp_sendto(struct svc_rqst *rqstp)
>  /*
>   * Setup response header. TCP has a 4B record length field.
>   */
> -static void svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
> +void svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
>  {
>  	struct kvec *resv = &rqstp->rq_res.head[0];
>  
> -- 
> 2.17.1