Message ID | 1450214668-13322-1-git-send-email-kamal@canonical.com |
---|---|
State | New |
Headers | show |
Hi Kamal, This will break the NFS client's callback channel without Trond's fix 756b9b37cfb2e3dc76b2e43a8c097402ac736e07. It shouldn't be added without that one as well. Ben On Tue, 15 Dec 2015, Kamal Mostafa wrote: > This is a note to let you know that I have just added a patch titled > > nfs4: limit callback decoding to received bytes > > to the linux-3.19.y-queue branch of the 3.19.y-ckt extended stable tree > which can be found at: > > http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-3.19.y-queue > > This patch is scheduled to be released in version 3.19.8-ckt12. > > If you, or anyone else, feels it should not be added to this tree, please > reply to this email. > > For more information about the 3.19.y-ckt tree, see > https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable > > Thanks. > -Kamal > > ------ > > From 44e003f516dc4407fee82e3f668369a551284ead 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 > > 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: Kamal Mostafa <kamal@canonical.com> > --- > fs/nfs/callback_xdr.c | 7 +++++-- > net/sunrpc/backchannel_rqst.c | 8 ++++++++ > net/sunrpc/svc.c | 1 + > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > index 02f8d09..1e36635 100644 > --- a/fs/nfs/callback_xdr.c > +++ b/fs/nfs/callback_xdr.c > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) > > 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(struct svc_rqst *rqstp, void *argp, void *r > 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(struct svc_rqst *rqstp, void *argp, void *r > > 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); > diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c > index 28504df..b57cee1 100644 > --- a/net/sunrpc/backchannel_rqst.c > +++ b/net/sunrpc/backchannel_rqst.c > @@ -308,11 +308,19 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) > { > 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); > 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); > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index 91eaef1..b59b6a7 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -1361,6 +1361,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, > 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; > -- > 1.9.1 > >
On Tue, 2015-12-15 at 17:26 -0500, Benjamin Coddington wrote: > Hi Kamal, > > This will break the NFS client's callback channel without Trond's fix > 756b9b37cfb2e3dc76b2e43a8c097402ac736e07. It shouldn't be added without > that one as well. > > Ben Thanks for the heads-up Ben. I'll make sure that follow-up fix gets applied in the same 3.19-stable release. -Kamal > > On Tue, 15 Dec 2015, Kamal Mostafa wrote: > > > This is a note to let you know that I have just added a patch titled > > > > nfs4: limit callback decoding to received bytes > > > > to the linux-3.19.y-queue branch of the 3.19.y-ckt extended stable tree > > which can be found at: > > > > http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-3.19.y-queue > > > > This patch is scheduled to be released in version 3.19.8-ckt12. > > > > If you, or anyone else, feels it should not be added to this tree, please > > reply to this email. > > > > For more information about the 3.19.y-ckt tree, see > > https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable > > > > Thanks. > > -Kamal > > > > ------ > > > > From 44e003f516dc4407fee82e3f668369a551284ead 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 > > > > 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: Kamal Mostafa <kamal@canonical.com> > > --- > > fs/nfs/callback_xdr.c | 7 +++++-- > > net/sunrpc/backchannel_rqst.c | 8 ++++++++ > > net/sunrpc/svc.c | 1 + > > 3 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > > index 02f8d09..1e36635 100644 > > --- a/fs/nfs/callback_xdr.c > > +++ b/fs/nfs/callback_xdr.c > > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) > > > > 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(struct svc_rqst *rqstp, void *argp, void *r > > 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(struct svc_rqst *rqstp, void *argp, void *r > > > > 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); > > diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c > > index 28504df..b57cee1 100644 > > --- a/net/sunrpc/backchannel_rqst.c > > +++ b/net/sunrpc/backchannel_rqst.c > > @@ -308,11 +308,19 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) > > { > > 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); > > 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); > > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > > index 91eaef1..b59b6a7 100644 > > --- a/net/sunrpc/svc.c > > +++ b/net/sunrpc/svc.c > > @@ -1361,6 +1361,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, > > 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; > > -- > > 1.9.1 > > > > >
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c index 02f8d09..1e36635 100644 --- a/fs/nfs/callback_xdr.c +++ b/fs/nfs/callback_xdr.c @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) 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(struct svc_rqst *rqstp, void *argp, void *r 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(struct svc_rqst *rqstp, void *argp, void *r 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); diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c index 28504df..b57cee1 100644 --- a/net/sunrpc/backchannel_rqst.c +++ b/net/sunrpc/backchannel_rqst.c @@ -308,11 +308,19 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) { 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); 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); diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index 91eaef1..b59b6a7 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -1361,6 +1361,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, 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;