Date:   Sat, 13 Nov 2021 16:25:44 -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: <20211113212544.GA27601@fieldses.org>
References: <97860.1636837122@crash.local>
 <11B4530A-C0A0-4779-A9BA-F0E19B62C5A6@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <11B4530A-C0A0-4779-A9BA-F0E19B62C5A6@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Sat, Nov 13, 2021 at 09:06:03PM +0000, Chuck Lever III wrote:
> 
> > On Nov 13, 2021, at 3:58 PM, rtm@csail.mit.edu wrote:
> > 
> > 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().
> 
> Thanks, I'll look into addressing this for v5.16-rc.
> 
> By the way, can you tell if this exposure was in the code
> before 2548aa784d76 ("NFSD: Add a separate decoder to handle
> state_protect_ops") ? (ie, do we need a separate fix for
> this for pre-5.11 NFSD -- I'm guessing no).

It may not have been an EXCHANGE_ID problem, but:

> Is the current implementation of nfsd4_decode_bitmap() a
> problem for its other consumers?

Yeah, I don't see that there's anything a caller could do that would
prevent it, so the problem starts with the introduction of
nfsd4_decode_bitmap4.

Not actually tested, but I suppose we want the following.

--b.

commit 8211c4817cc0
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Sat Nov 13 16:11:58 2021 -0500

    nfsd: fix overrun in nfsd4_decode_bitmap4
    
    rtm says: "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()."
    
    Reported-by: <rtm@csail.mit.edu>
    Cc: stable@vger.kernel.org
    Fixes: d1c263a031e8 "NFSD: Replace READ* macros in nfsd4_decode_fattr()"
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 9b609aac47e1..7aa97c09b5a9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -282,8 +282,7 @@ nfsd4_decode_bitmap4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen)
 
 	if (xdr_stream_decode_u32(argp->xdr, &count) < 0)
 		return nfserr_bad_xdr;
-	/* request sanity */
-	if (count > 1000)
+	if (count > bmlen)
 		return nfserr_bad_xdr;
 	p = xdr_inline_decode(argp->xdr, count << 2);
 	if (!p)