Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2021-12-13 at 04:21 +0000, Chuck Lever III wrote:
> 
> > On Dec 12, 2021, at 9:10 PM, Bruce Fields <bfields@fieldses.org>
> > 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@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>
> > > 
> > > 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@hammerspace.com