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()
Date: Mon, 24 Dec 2018 11:59:30 -0500 [thread overview]
Message-ID: <20181224165930.GA11596@fieldses.org> (raw)
In-Reply-To: <2d71abdb-2e79-4b13-21a6-3d439ddd96c5@virtuozzo.com>
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
next prev parent reply other threads:[~2018-12-24 16:59 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1545648882.git.vvs@virtuozzo.com>
2018-12-24 11:44 ` [PATCH v4 01/10] sunrpc: use SVC_NET() in svcauth_gss_* functions Vasily Averin
2018-12-24 11:44 ` [PATCH v4 02/10] sunrpc: use-after-free in svc_process_common() Vasily Averin
2018-12-24 16:59 ` J. Bruce Fields [this message]
2018-12-24 11:45 ` [PATCH v4 03/10] sunrpc: replace svc_serv->sv_bc_xprt by boolean flag Vasily Averin
2018-12-24 11:45 ` [PATCH v4 04/10] sunrpc: remove unused bc_up operation from rpc_xprt_ops Vasily Averin
2018-12-24 11:45 ` [PATCH v4 05/10] sunrpc: remove svc_tcp_bc_class Vasily Averin
2018-12-24 11:45 ` [PATCH v4 06/10] sunrpc: remove svc_rdma_bc_class Vasily Averin
2018-12-24 11:46 ` [PATCH v4 07/10] sunrpc: remove unused xpo_prep_reply_hdr callback Vasily Averin
2018-12-24 11:46 ` [PATCH v4 08/10] sunrpc: make visible processing error in bc_svc_process() Vasily Averin
2018-12-24 11:46 ` [PATCH v4 09/10] sunrpc: fix debug message in svc_create_xprt() Vasily Averin
2018-12-24 11:46 ` [PATCH v4 10/10] nfs: minor typo in nfs4_callback_up_net() Vasily Averin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181224165930.GA11596@fieldses.org \
--to=bfields@fieldses.org \
--cc=anna.schumaker@netapp.com \
--cc=chuck.lever@oracle.com \
--cc=eshatokhin@virtuozzo.com \
--cc=jlayton@kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=trond.myklebust@hammerspace.com \
--cc=vvs@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.