> On Nov 13, 2021, at 4:25 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> On Sat, Nov 13, 2021 at 09:06:03PM +0000, Chuck Lever III wrote:
>>
>>> On Nov 13, 2021, at 3:58 PM, rtm@xxxxxxxxxxxxx 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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: d1c263a031e8 "NFSD: Replace READ* macros in nfsd4_decode_fattr()"
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> 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)
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.
> return nfserr_bad_xdr;
> p = xdr_inline_decode(argp->xdr, count << 2);
> if (!p)
--
Chuck Lever
