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