Patch "nfs4: limit callback decoding to received bytes" has been added to the 4.3-stable tree

Patch "nfs4: limit callback decoding to received bytes" has been added to the 4.3-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    nfs4: limit callback decoding to received bytes

to the 4.3-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     nfs4-limit-callback-decoding-to-received-bytes.patch
and it can be found in the queue-4.3 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From 38b7631fbe42e6e247e9fc9879f961b14a687e3b Mon Sep 17 00:00:00 2001
From: Benjamin Coddington <bcodding@redhat.com>
Date: Fri, 20 Nov 2015 09:55:30 -0500
Subject: nfs4: limit callback decoding to received bytes

From: Benjamin Coddington <bcodding@redhat.com>

commit 38b7631fbe42e6e247e9fc9879f961b14a687e3b upstream.

A truncated cb_compound request will cause the client to decode null or
data from a previous callback for nfs4.1 backchannel case, or uninitialized
data for the nfs4.0 case. This is because the path through
svc_process_common() advances the request's iov_base and decrements iov_len
without adjusting the overall xdr_buf's len field.  That causes
xdr_init_decode() to set up the xdr_stream with an incorrect length in
nfs4_callback_compound().

Fixing this for the nfs4.1 backchannel case first requires setting the
correct iov_len and page_len based on the length of received data in the
same manner as the nfs4.0 case.

Then the request's xdr_buf length can be adjusted for both cases based upon
the remaining iov_len and page_len.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 fs/nfs/callback_xdr.c         |    7 +++++--
 net/sunrpc/backchannel_rqst.c |    8 ++++++++
 net/sunrpc/svc.c              |    1 +
 3 files changed, 14 insertions(+), 2 deletions(-)

--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_strea
 
 	p = xdr_inline_decode(xdr, nbytes);
 	if (unlikely(p == NULL))
-		printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n");
+		printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed "
+							"or truncated request.\n");
 	return p;
 }
 
@@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(str
 	struct cb_compound_hdr_arg hdr_arg = { 0 };
 	struct cb_compound_hdr_res hdr_res = { NULL };
 	struct xdr_stream xdr_in, xdr_out;
+	struct xdr_buf *rq_arg = &rqstp->rq_arg;
 	__be32 *p, status;
 	struct cb_process_state cps = {
 		.drc_status = 0,
@@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(str
 
 	dprintk("%s: start\n", __func__);
 
-	xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base);
+	rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len;
+	xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base);
 
 	p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len);
 	xdr_init_encode(&xdr_out, &rqstp->rq_res, p);
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc
 {
 	struct rpc_xprt *xprt = req->rq_xprt;
 	struct svc_serv *bc_serv = xprt->bc_serv;
+	struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf;
 
 	spin_lock(&xprt->bc_pa_lock);
 	list_del(&req->rq_bc_pa_list);
 	xprt_dec_alloc_count(xprt, 1);
 	spin_unlock(&xprt->bc_pa_lock);
 
+	if (copied <= rq_rcv_buf->head[0].iov_len) {
+		rq_rcv_buf->head[0].iov_len = copied;
+		rq_rcv_buf->page_len = 0;
+	} else {
+		rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len;
+	}
+
 	req->rq_private_buf.len = copied;
 	set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
 
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, st
 	memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen);
 	memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg));
 	memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res));
+	rqstp->rq_arg.len = req->rq_private_buf.len;
 
 	/* reset result send buffer "put" position */
 	resv->iov_len = 0;


Patches currently in stable-queue which might be from bcodding@redhat.com are

queue-4.3/nfs4-start-callback_ident-at-idr-1.patch
queue-4.3/nfs4-limit-callback-decoding-to-received-bytes.patch

Re: Patch "nfs4: limit callback decoding to received bytes" has been added to the 4.3-stable tree

[Trond Myklebust]

Hi Greg,

On Fri, Dec 11, 2015 at 12:20 PM,  <gregkh@linuxfoundation.org> wrote:
>
> This is a note to let you know that I've just added the patch titled
>
>     nfs4: limit callback decoding to received bytes
>
> to the 4.3-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>      nfs4-limit-callback-decoding-to-received-bytes.patch
> and it can be found in the queue-4.3 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
> From 38b7631fbe42e6e247e9fc9879f961b14a687e3b Mon Sep 17 00:00:00 2001
> From: Benjamin Coddington <bcodding@redhat.com>
> Date: Fri, 20 Nov 2015 09:55:30 -0500
> Subject: nfs4: limit callback decoding to received bytes
>
> From: Benjamin Coddington <bcodding@redhat.com>
>
> commit 38b7631fbe42e6e247e9fc9879f961b14a687e3b upstream.
>
> A truncated cb_compound request will cause the client to decode null or
> data from a previous callback for nfs4.1 backchannel case, or uninitialized
> data for the nfs4.0 case. This is because the path through
> svc_process_common() advances the request's iov_base and decrements iov_len
> without adjusting the overall xdr_buf's len field.  That causes
> xdr_init_decode() to set up the xdr_stream with an incorrect length in
> nfs4_callback_compound().
>
> Fixing this for the nfs4.1 backchannel case first requires setting the
> correct iov_len and page_len based on the length of received data in the
> same manner as the nfs4.0 case.
>
> Then the request's xdr_buf length can be adjusted for both cases based upon
> the remaining iov_len and page_len.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>  fs/nfs/callback_xdr.c         |    7 +++++--
>  net/sunrpc/backchannel_rqst.c |    8 ++++++++
>  net/sunrpc/svc.c              |    1 +
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_strea
>
>         p = xdr_inline_decode(xdr, nbytes);
>         if (unlikely(p == NULL))
> -               printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n");
> +               printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed "
> +                                                       "or truncated request.\n");
>         return p;
>  }
>
> @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(str
>         struct cb_compound_hdr_arg hdr_arg = { 0 };
>         struct cb_compound_hdr_res hdr_res = { NULL };
>         struct xdr_stream xdr_in, xdr_out;
> +       struct xdr_buf *rq_arg = &rqstp->rq_arg;
>         __be32 *p, status;
>         struct cb_process_state cps = {
>                 .drc_status = 0,
> @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(str
>
>         dprintk("%s: start\n", __func__);
>
> -       xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base);
> +       rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len;
> +       xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base);
>
>         p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len);
>         xdr_init_encode(&xdr_out, &rqstp->rq_res, p);
> --- a/net/sunrpc/backchannel_rqst.c
> +++ b/net/sunrpc/backchannel_rqst.c
> @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc
>  {
>         struct rpc_xprt *xprt = req->rq_xprt;
>         struct svc_serv *bc_serv = xprt->bc_serv;
> +       struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf;
>
>         spin_lock(&xprt->bc_pa_lock);
>         list_del(&req->rq_bc_pa_list);
>         xprt_dec_alloc_count(xprt, 1);
>         spin_unlock(&xprt->bc_pa_lock);
>
> +       if (copied <= rq_rcv_buf->head[0].iov_len) {
> +               rq_rcv_buf->head[0].iov_len = copied;
> +               rq_rcv_buf->page_len = 0;
> +       } else {
> +               rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len;
> +       }
> +
>         req->rq_private_buf.len = copied;
>         set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
>
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, st
>         memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen);
>         memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg));
>         memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res));
> +       rqstp->rq_arg.len = req->rq_private_buf.len;
>
>         /* reset result send buffer "put" position */
>         resv->iov_len = 0;
>

This patch turned out to cause some issues with the NFS callback
channel. I have a fix, but it hasn't been upstreamed yet. Can we
therefore please hold on this patch for now, and remove it from all
the stable queues. I'll resubmit when the fix is in mainline.

Thanks
  Trond

Re: Patch "nfs4: limit callback decoding to received bytes" has been added to the 4.3-stable tree

[Greg Kroah-Hartman]

On Fri, Dec 11, 2015 at 06:14:03PM -0500, Trond Myklebust wrote:
> Hi Greg,
> 
> On Fri, Dec 11, 2015 at 12:20 PM,  <gregkh@linuxfoundation.org> wrote:
> >
> > This is a note to let you know that I've just added the patch titled
> >
> >     nfs4: limit callback decoding to received bytes
> >
> > to the 4.3-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> >
> > The filename of the patch is:
> >      nfs4-limit-callback-decoding-to-received-bytes.patch
> > and it can be found in the queue-4.3 subdirectory.
> >
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> >
> >
> > From 38b7631fbe42e6e247e9fc9879f961b14a687e3b Mon Sep 17 00:00:00 2001
> > From: Benjamin Coddington <bcodding@redhat.com>
> > Date: Fri, 20 Nov 2015 09:55:30 -0500
> > Subject: nfs4: limit callback decoding to received bytes
> >
> > From: Benjamin Coddington <bcodding@redhat.com>
> >
> > commit 38b7631fbe42e6e247e9fc9879f961b14a687e3b upstream.
> >
> > A truncated cb_compound request will cause the client to decode null or
> > data from a previous callback for nfs4.1 backchannel case, or uninitialized
> > data for the nfs4.0 case. This is because the path through
> > svc_process_common() advances the request's iov_base and decrements iov_len
> > without adjusting the overall xdr_buf's len field.  That causes
> > xdr_init_decode() to set up the xdr_stream with an incorrect length in
> > nfs4_callback_compound().
> >
> > Fixing this for the nfs4.1 backchannel case first requires setting the
> > correct iov_len and page_len based on the length of received data in the
> > same manner as the nfs4.0 case.
> >
> > Then the request's xdr_buf length can be adjusted for both cases based upon
> > the remaining iov_len and page_len.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > ---
> >  fs/nfs/callback_xdr.c         |    7 +++++--
> >  net/sunrpc/backchannel_rqst.c |    8 ++++++++
> >  net/sunrpc/svc.c              |    1 +
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > --- a/fs/nfs/callback_xdr.c
> > +++ b/fs/nfs/callback_xdr.c
> > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_strea
> >
> >         p = xdr_inline_decode(xdr, nbytes);
> >         if (unlikely(p == NULL))
> > -               printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n");
> > +               printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed "
> > +                                                       "or truncated request.\n");
> >         return p;
> >  }
> >
> > @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(str
> >         struct cb_compound_hdr_arg hdr_arg = { 0 };
> >         struct cb_compound_hdr_res hdr_res = { NULL };
> >         struct xdr_stream xdr_in, xdr_out;
> > +       struct xdr_buf *rq_arg = &rqstp->rq_arg;
> >         __be32 *p, status;
> >         struct cb_process_state cps = {
> >                 .drc_status = 0,
> > @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(str
> >
> >         dprintk("%s: start\n", __func__);
> >
> > -       xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base);
> > +       rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len;
> > +       xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base);
> >
> >         p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len);
> >         xdr_init_encode(&xdr_out, &rqstp->rq_res, p);
> > --- a/net/sunrpc/backchannel_rqst.c
> > +++ b/net/sunrpc/backchannel_rqst.c
> > @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc
> >  {
> >         struct rpc_xprt *xprt = req->rq_xprt;
> >         struct svc_serv *bc_serv = xprt->bc_serv;
> > +       struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf;
> >
> >         spin_lock(&xprt->bc_pa_lock);
> >         list_del(&req->rq_bc_pa_list);
> >         xprt_dec_alloc_count(xprt, 1);
> >         spin_unlock(&xprt->bc_pa_lock);
> >
> > +       if (copied <= rq_rcv_buf->head[0].iov_len) {
> > +               rq_rcv_buf->head[0].iov_len = copied;
> > +               rq_rcv_buf->page_len = 0;
> > +       } else {
> > +               rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len;
> > +       }
> > +
> >         req->rq_private_buf.len = copied;
> >         set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
> >
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, st
> >         memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen);
> >         memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg));
> >         memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res));
> > +       rqstp->rq_arg.len = req->rq_private_buf.len;
> >
> >         /* reset result send buffer "put" position */
> >         resv->iov_len = 0;
> >
> 
> This patch turned out to cause some issues with the NFS callback
> channel. I have a fix, but it hasn't been upstreamed yet. Can we
> therefore please hold on this patch for now, and remove it from all
> the stable queues. I'll resubmit when the fix is in mainline.

No problem, now dropped from the 4.3 and 4.2 stable queues.

thanks,

greg k-h