| Message ID | 20170421211253.GE19775@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 21 2017, J. Bruce Fields wrote: > On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote: >> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote: >> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote: >> > > I can't say that I like this patch at all. >> > > >> > > The problem is that: >> > > >> > > pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply. >> > > * We assume one is at most one page >> > > */ >> > > >> > > this assumption is never verified. >> > > To my mind, the "obvious" way to verify this assumption is that an >> > > attempt to generate a multi-page reply should fail if there was a >> > > multi-page request. >> > >> > A third option, by the way, which Ari Kauppi argued for, is adding a >> > null check each time we increment rq_next_page, since we seem to arrange >> > for the page array to always be NULL-terminated. >> > >> > > Failing if there was a little bit of extra noise at the end of the >> > > request seems harsher than necessary, and could result in a regression. >> > >> > You're worrying there might be a weird old client out there somewhere? >> > I guess it seems like a small enough risk to me. I'm more worried the >> > extra garbage might violate assumptions elsewhere in the code. >> > >> > But, this looks good too: >> >> But, I'm not too happy about putting that NFSv2/v3-specific check in >> common rpc code. > > Well, but it should work just as well in nfsd_dispatch, I think? > (Untested). So, maybe that's simplest as a first step: > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > index 31e1f9593457..b6298d30a01f 100644 > --- a/fs/nfsd/nfssvc.c > +++ b/fs/nfsd/nfssvc.c > @@ -759,6 +759,22 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp) > rqstp->rq_vers, rqstp->rq_proc); > proc = rqstp->rq_procinfo; > > + if (rqstp->rq_vers < 4 && > + (proc->pc_xdrressize == 0 > + || proc->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) > + && rqstp->rq_arg.len > PAGE_SIZE) { > + /* > + * NFSv2 and v3 assume that an operation may have either a > + * large argument, or a large reply, but never both. > + * > + * NFSv4 may handle compounds with both argument and > + * reply larger than a reply; it has more xdr careful > + * xdr decoding which can handle such calls safely. > + */ > + dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers); > + *statp = rpc_garbage_args; > + return 1; > + } > /* > * Give the xdr decoder a chance to change this if it wants > * (necessary in the NFSv4.0 compound case) I like this. I think this should be the basis of what goes to -stable, and other improvements should stay in mainline. The only change I would suggest would be to be explicit about where the nfsacl protocol fits with this. We could change "rqstp->rq_vers < 4" to "rqstp->rq_prog == NFS_PROGRAM && rqstp->rq_vers < 4" or we could change the text: NFSv2 and v3 assume ... to NFSv2 and v3, along with NFSASL, assume ... and possibly change "rqstp->rq_vers < 4" to "!nfsd_v4client(rqstp)". I believe none of this applies to lockd as none of that code ever looks beyond a single page. Thanks, NeilBrown
On Mon, Apr 24, 2017 at 08:21:36AM +1000, NeilBrown wrote: > On Fri, Apr 21 2017, J. Bruce Fields wrote: > > > On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote: > >> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote: > >> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote: > >> > > I can't say that I like this patch at all. > >> > > > >> > > The problem is that: > >> > > > >> > > pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply. > >> > > * We assume one is at most one page > >> > > */ > >> > > > >> > > this assumption is never verified. > >> > > To my mind, the "obvious" way to verify this assumption is that an > >> > > attempt to generate a multi-page reply should fail if there was a > >> > > multi-page request. > >> > > >> > A third option, by the way, which Ari Kauppi argued for, is adding a > >> > null check each time we increment rq_next_page, since we seem to arrange > >> > for the page array to always be NULL-terminated. > >> > > >> > > Failing if there was a little bit of extra noise at the end of the > >> > > request seems harsher than necessary, and could result in a regression. > >> > > >> > You're worrying there might be a weird old client out there somewhere? > >> > I guess it seems like a small enough risk to me. I'm more worried the > >> > extra garbage might violate assumptions elsewhere in the code. > >> > > >> > But, this looks good too: > >> > >> But, I'm not too happy about putting that NFSv2/v3-specific check in > >> common rpc code. > > > > Well, but it should work just as well in nfsd_dispatch, I think? > > (Untested). So, maybe that's simplest as a first step: > > > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > > index 31e1f9593457..b6298d30a01f 100644 > > --- a/fs/nfsd/nfssvc.c > > +++ b/fs/nfsd/nfssvc.c > > @@ -759,6 +759,22 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp) > > rqstp->rq_vers, rqstp->rq_proc); > > proc = rqstp->rq_procinfo; > > > > + if (rqstp->rq_vers < 4 && > > + (proc->pc_xdrressize == 0 > > + || proc->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) > > + && rqstp->rq_arg.len > PAGE_SIZE) { > > + /* > > + * NFSv2 and v3 assume that an operation may have either a > > + * large argument, or a large reply, but never both. > > + * > > + * NFSv4 may handle compounds with both argument and > > + * reply larger than a reply; it has more xdr careful > > + * xdr decoding which can handle such calls safely. > > + */ > > + dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers); > > + *statp = rpc_garbage_args; > > + return 1; > > + } > > /* > > * Give the xdr decoder a chance to change this if it wants > > * (necessary in the NFSv4.0 compound case) > > I like this. I think this should be the basis of what goes to -stable, > and other improvements should stay in mainline. > > The only change I would suggest would be to be explicit about where the > nfsacl protocol fits with this. Oh, good point, I'd forgotten nfsd_dispatch handles multiple protocols! > We could change "rqstp->rq_vers < 4" to > "rqstp->rq_prog == NFS_PROGRAM && rqstp->rq_vers < 4" > or we could change the text: > NFSv2 and v3 assume ... > to > NFSv2 and v3, along with NFSASL, assume ... > > and possibly change "rqstp->rq_vers < 4" to "!nfsd_v4client(rqstp)". > > I believe none of this applies to lockd as none of that code ever looks > beyond a single page. That makes sense. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 31e1f9593457..b6298d30a01f 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -759,6 +759,22 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp) rqstp->rq_vers, rqstp->rq_proc); proc = rqstp->rq_procinfo; + if (rqstp->rq_vers < 4 && + (proc->pc_xdrressize == 0 + || proc->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) + && rqstp->rq_arg.len > PAGE_SIZE) { + /* + * NFSv2 and v3 assume that an operation may have either a + * large argument, or a large reply, but never both. + * + * NFSv4 may handle compounds with both argument and + * reply larger than a reply; it has more xdr careful + * xdr decoding which can handle such calls safely. + */ + dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers); + *statp = rpc_garbage_args; + return 1; + } /* * Give the xdr decoder a chance to change this if it wants * (necessary in the NFSv4.0 compound case)