Date:   Sun, 12 Dec 2021 21:10:48 -0500
From:   Bruce Fields <bfields@fieldses.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     "rtm@csail.mit.edu" <rtm@csail.mit.edu>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: NFS client can crash server due to overrun in
 nfsd4_decode_bitmap4()
Message-ID: <20211213021048.GA20814@fieldses.org>
References: <97860.1636837122@crash.local>
 <11B4530A-C0A0-4779-A9BA-F0E19B62C5A6@oracle.com>
 <20211113212544.GA27601@fieldses.org>
 <9A3E5D78-AE3C-4F45-8CB1-10F2EDA1D911@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9A3E5D78-AE3C-4F45-8CB1-10F2EDA1D911@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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).

--b.