| Message ID | 163692036074.16710.5678362976688977923.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] NFSD: Fix exposure in nfsd4_decode_bitmap() | expand |
On Sun, Nov 14, 2021 at 03:16:04PM -0500, Chuck Lever wrote: > rtm@csail.mit.edu reports: > > nfsd4_decode_bitmap4() will write beyond bmval[bmlen-1] if the RPC > > directs it to do so. This can cause nfsd4_decode_state_protect4_a() > > to write client-supplied data beyond the end of > > nfsd4_exchange_id.spo_must_allow[] when called by > > nfsd4_decode_exchange_id(). > > Rewrite the loops so nfsd4_decode_bitmap() cannot iterate beyond > @bmlen. > > Reported by: rtm@csail.mit.edu > Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in nfsd4_decode_fattr()") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4xdr.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > Hi Bruce- > > This version is cleaned up slightly and has been tested as follows: > > - I am not able to crash a patched server using rtm's nfsd_1 > - No new FAILUREs with NFSv4.1 pynfs tests > - No new failures with xfstests on NFSv4.1 or NFSv4.2 > - No new failures with the git regression suite on NFSv4.1 or NFSv4.2 > - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr > > Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be > confident that it is working properly. > > I tested this on top of my XDR tracepoint series, so the line numbers > might be a little off from your current tree. However, it should > otherwise apply cleanly. Got it, thanks.--b. > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 5ff481e9c85d..479d3452ce6c 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -288,11 +288,8 @@ nfsd4_decode_bitmap4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen) > p = xdr_inline_decode(argp->xdr, count << 2); > if (!p) > return nfserr_bad_xdr; > - i = 0; > - while (i < count) > - bmval[i++] = be32_to_cpup(p++); > - while (i < bmlen) > - bmval[i++] = 0; > + for (i = 0; i < bmlen; i++) > + bmval[i] = (i < count) ? be32_to_cpup(p++) : 0; > > return nfs_ok; > }
On Sun, 2021-11-14 at 15:16 -0500, Chuck Lever wrote: > rtm@csail.mit.edu reports: > > nfsd4_decode_bitmap4() will write beyond bmval[bmlen-1] if the RPC > > directs it to do so. This can cause nfsd4_decode_state_protect4_a() > > to write client-supplied data beyond the end of > > nfsd4_exchange_id.spo_must_allow[] when called by > > nfsd4_decode_exchange_id(). > > Rewrite the loops so nfsd4_decode_bitmap() cannot iterate beyond > @bmlen. > > Reported by: rtm@csail.mit.edu > Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in > nfsd4_decode_fattr()") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4xdr.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > Hi Bruce- > > This version is cleaned up slightly and has been tested as follows: > > - I am not able to crash a patched server using rtm's nfsd_1 > - No new FAILUREs with NFSv4.1 pynfs tests > - No new failures with xfstests on NFSv4.1 or NFSv4.2 > - No new failures with the git regression suite on NFSv4.1 or NFSv4.2 > - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr > > Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be > confident that it is working properly. > > I tested this on top of my XDR tracepoint series, so the line numbers > might be a little off from your current tree. However, it should > otherwise apply cleanly. > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 5ff481e9c85d..479d3452ce6c 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -288,11 +288,8 @@ nfsd4_decode_bitmap4(struct nfsd4_compoundargs > *argp, u32 *bmval, u32 bmlen) > p = xdr_inline_decode(argp->xdr, count << 2); > if (!p) > return nfserr_bad_xdr; > - i = 0; > - while (i < count) > - bmval[i++] = be32_to_cpup(p++); > - while (i < bmlen) > - bmval[i++] = 0; > + for (i = 0; i < bmlen; i++) > + bmval[i] = (i < count) ? be32_to_cpup(p++) : 0; > > return nfs_ok; > } > Why not just convert it to use xdr_stream_decode_uint32_array() instead of maintaining this separate open coded version?
> On Nov 14, 2021, at 4:58 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Sun, 2021-11-14 at 15:16 -0500, Chuck Lever wrote: >> rtm@csail.mit.edu reports: >>> nfsd4_decode_bitmap4() will write beyond bmval[bmlen-1] if the RPC >>> directs it to do so. This can cause nfsd4_decode_state_protect4_a() >>> to write client-supplied data beyond the end of >>> nfsd4_exchange_id.spo_must_allow[] when called by >>> nfsd4_decode_exchange_id(). >> >> Rewrite the loops so nfsd4_decode_bitmap() cannot iterate beyond >> @bmlen. >> >> Reported by: rtm@csail.mit.edu >> Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in >> nfsd4_decode_fattr()") >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> fs/nfsd/nfs4xdr.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> Hi Bruce- >> >> This version is cleaned up slightly and has been tested as follows: >> >> - I am not able to crash a patched server using rtm's nfsd_1 >> - No new FAILUREs with NFSv4.1 pynfs tests >> - No new failures with xfstests on NFSv4.1 or NFSv4.2 >> - No new failures with the git regression suite on NFSv4.1 or NFSv4.2 >> - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr >> >> Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be >> confident that it is working properly. >> >> I tested this on top of my XDR tracepoint series, so the line numbers >> might be a little off from your current tree. However, it should >> otherwise apply cleanly. >> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >> index 5ff481e9c85d..479d3452ce6c 100644 >> --- a/fs/nfsd/nfs4xdr.c >> +++ b/fs/nfsd/nfs4xdr.c >> @@ -288,11 +288,8 @@ nfsd4_decode_bitmap4(struct nfsd4_compoundargs >> *argp, u32 *bmval, u32 bmlen) >> p = xdr_inline_decode(argp->xdr, count << 2); >> if (!p) >> return nfserr_bad_xdr; >> - i = 0; >> - while (i < count) >> - bmval[i++] = be32_to_cpup(p++); >> - while (i < bmlen) >> - bmval[i++] = 0; >> + for (i = 0; i < bmlen; i++) >> + bmval[i] = (i < count) ? be32_to_cpup(p++) : 0; >> >> return nfs_ok; >> } >> > > Why not just convert it to use xdr_stream_decode_uint32_array() instead > of maintaining this separate open coded version? Pulling the chain a little more, perhaps all decode_bitmap4 call sites should be converted to use xdr_stream_decode_uint32_array. We could consider that as a clean up later; the current fix has been kept surgical so it can be backported. -- Chuck Lever
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 5ff481e9c85d..479d3452ce6c 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -288,11 +288,8 @@ nfsd4_decode_bitmap4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen) p = xdr_inline_decode(argp->xdr, count << 2); if (!p) return nfserr_bad_xdr; - i = 0; - while (i < count) - bmval[i++] = be32_to_cpup(p++); - while (i < bmlen) - bmval[i++] = 0; + for (i = 0; i < bmlen; i++) + bmval[i] = (i < count) ? be32_to_cpup(p++) : 0; return nfs_ok; }