On Mon, 2021-12-13 at 04:21 +0000, Chuck Lever III wrote:
>
> > On Dec 12, 2021, at 9:10 PM, Bruce Fields <bfields@xxxxxxxxxxxx>
> > wrote:
> >
> > On Sat, Nov 13, 2021 at 09:31:40PM +0000, Chuck Lever III wrote:
> > > Sure, but that's more restrictive than what the old decoder
> > > did. I have this instead (also yet to be tested):
> > >
> > > NFSD: Fix exposure in nfsd4_decode_bitmap()
> > >
> > > rtm@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> > > Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in
> > > nfsd4_decode_fattr()")
> > > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > >
> > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > index 10883e6d80ac..c2f753233fcf 100644
> > > --- a/fs/nfsd/nfs4xdr.c
> > > +++ b/fs/nfsd/nfs4xdr.c
> > > @@ -288,12 +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;
> > > }
> > >
> > > This allows the client to send bitmaps larger than bmval[],
> > > as the old decoder did, and ensures that decode_bitmap()
> > > cannot write farther than @bmlen into the bmval array.
> >
> > But I notice now that your tree has "NFSD: Replace
> > nfsd4_decode_bitmap4()", which does error out on large bitmaps.
> > (Noticed because pynfs checks for this case (see GATT4s and
> > similar) and
> > is seeing BADXDR returns).
>
> D’oh! I can drop “Replace nfsd4_decode_bitmap4()” or we can update
> the generic helper to handle large bitmaps. Dropping the clean-up
> seems safer.
>
The xdr_stream_decode_uint32_array() generic helper already handles
large bitmaps. It will decode as many entries as will fit in @array,
but return -EMSGSIZE to let you know that size was truncated.
IOW: you should treat -EMSGSIZE as a sign that @array_size elements
were actually decoded, rather than as a sign that no elements were
decoded.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
