2021-11-13 20:58:47

by rtm

[permalink] [raw]
Subject: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

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

I've attached a demo in which the client's EXCHANGE_ID RPC supplies an
address (0x400) that nfsd4_decode_bitmap4() writes into
nii_domain.data due to overflowing bmval[]. The EXCHANGE_ID RPC also
supplies a zero-length eia_client_impl_id<>. The result is that
copy_impl_id() (called by nfsd4_exchange_id()) tries to read from
address 0x400.

# cc nfsd_1.c
# uname -a
Linux (none) 5.15.0-rc7-dirty #64 SMP Sat Nov 13 20:10:21 UTC 2021 riscv64 riscv64 riscv64 GNU/Linux
# ./nfsd_1
...
[ 16.600786] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000400
[ 16.643621] epc : __memcpy+0x3c/0xf8
[ 16.650154] ra : kmemdup+0x2c/0x3c
[ 16.657733] epc : ffffffff803667bc ra : ffffffff800e80fe sp : ffffffd000553c20
[ 16.777502] status: 0000000200000121 badaddr: 0000000000000400 cause: 000000000000000d
[ 16.788193] [<ffffffff803667bc>] __memcpy+0x3c/0xf8
[ 16.796504] [<ffffffff8028cf0e>] nfsd4_exchange_id+0xe6/0x406
[ 16.806159] [<ffffffff8027c352>] nfsd4_proc_compound+0x2b4/0x4e8
[ 16.815721] [<ffffffff80266782>] nfsd_dispatch+0x118/0x172
[ 16.823405] [<ffffffff807633fa>] svc_process_common+0x2de/0x62c
[ 16.832935] [<ffffffff8076380c>] svc_process+0xc4/0x102
[ 16.840421] [<ffffffff802661de>] nfsd+0x102/0x16a
[ 16.848520] [<ffffffff80025b60>] kthread+0xfe/0x110
[ 16.856648] [<ffffffff80003054>] ret_from_exception+0x0/0xc


Attachments:
nfsd_1.c (4.40 kB)

2021-11-13 21:06:15

by Chuck Lever III

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()


> On Nov 13, 2021, at 3:58 PM, [email protected] 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).

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


> I've attached a demo in which the client's EXCHANGE_ID RPC supplies an
> address (0x400) that nfsd4_decode_bitmap4() writes into
> nii_domain.data due to overflowing bmval[]. The EXCHANGE_ID RPC also
> supplies a zero-length eia_client_impl_id<>. The result is that
> copy_impl_id() (called by nfsd4_exchange_id()) tries to read from
> address 0x400.
>
> # cc nfsd_1.c
> # uname -a
> Linux (none) 5.15.0-rc7-dirty #64 SMP Sat Nov 13 20:10:21 UTC 2021 riscv64 riscv64 riscv64 GNU/Linux
> # ./nfsd_1
> ...
> [ 16.600786] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000400
> [ 16.643621] epc : __memcpy+0x3c/0xf8
> [ 16.650154] ra : kmemdup+0x2c/0x3c
> [ 16.657733] epc : ffffffff803667bc ra : ffffffff800e80fe sp : ffffffd000553c20
> [ 16.777502] status: 0000000200000121 badaddr: 0000000000000400 cause: 000000000000000d
> [ 16.788193] [<ffffffff803667bc>] __memcpy+0x3c/0xf8
> [ 16.796504] [<ffffffff8028cf0e>] nfsd4_exchange_id+0xe6/0x406
> [ 16.806159] [<ffffffff8027c352>] nfsd4_proc_compound+0x2b4/0x4e8
> [ 16.815721] [<ffffffff80266782>] nfsd_dispatch+0x118/0x172
> [ 16.823405] [<ffffffff807633fa>] svc_process_common+0x2de/0x62c
> [ 16.832935] [<ffffffff8076380c>] svc_process+0xc4/0x102
> [ 16.840421] [<ffffffff802661de>] nfsd+0x102/0x16a
> [ 16.848520] [<ffffffff80025b60>] kthread+0xfe/0x110
> [ 16.856648] [<ffffffff80003054>] ret_from_exception+0x0/0xc
>
> <nfsd_1.c>

--
Chuck Lever




2021-11-13 21:25:47

by J. Bruce Fields

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

On Sat, Nov 13, 2021 at 09:06:03PM +0000, Chuck Lever III wrote:
>
> > On Nov 13, 2021, at 3:58 PM, [email protected] 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 <[email protected]>
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: <[email protected]>
Cc: [email protected]
Fixes: d1c263a031e8 "NFSD: Replace READ* macros in nfsd4_decode_fattr()"
Signed-off-by: J. Bruce Fields <[email protected]>

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)

2021-11-13 21:31:59

by Chuck Lever III

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()



> On Nov 13, 2021, at 4:25 PM, Bruce Fields <[email protected]> wrote:
>
> On Sat, Nov 13, 2021 at 09:06:03PM +0000, Chuck Lever III wrote:
>>
>>> On Nov 13, 2021, at 3:58 PM, [email protected] 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 <[email protected]>
> 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: <[email protected]>
> Cc: [email protected]
> Fixes: d1c263a031e8 "NFSD: Replace READ* macros in nfsd4_decode_fattr()"
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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()

[email protected] 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: <[email protected]>
Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in nfsd4_decode_fattr()")
Signed-off-by: Chuck Lever <[email protected]>

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




2021-11-13 21:33:07

by J. Bruce Fields

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

By the way, thanks for this work. Just out of curiosity: did anything
in particular prompt this? And do you have some tool that's finding
these, or is it manual code inspection, or some combination?

--b.

On Sat, Nov 13, 2021 at 03:58:42PM -0500, [email protected] 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().
>
> I've attached a demo in which the client's EXCHANGE_ID RPC supplies an
> address (0x400) that nfsd4_decode_bitmap4() writes into
> nii_domain.data due to overflowing bmval[]. The EXCHANGE_ID RPC also
> supplies a zero-length eia_client_impl_id<>. The result is that
> copy_impl_id() (called by nfsd4_exchange_id()) tries to read from
> address 0x400.
>
> # cc nfsd_1.c
> # uname -a
> Linux (none) 5.15.0-rc7-dirty #64 SMP Sat Nov 13 20:10:21 UTC 2021 riscv64 riscv64 riscv64 GNU/Linux
> # ./nfsd_1
> ...
> [ 16.600786] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000400
> [ 16.643621] epc : __memcpy+0x3c/0xf8
> [ 16.650154] ra : kmemdup+0x2c/0x3c
> [ 16.657733] epc : ffffffff803667bc ra : ffffffff800e80fe sp : ffffffd000553c20
> [ 16.777502] status: 0000000200000121 badaddr: 0000000000000400 cause: 000000000000000d
> [ 16.788193] [<ffffffff803667bc>] __memcpy+0x3c/0xf8
> [ 16.796504] [<ffffffff8028cf0e>] nfsd4_exchange_id+0xe6/0x406
> [ 16.806159] [<ffffffff8027c352>] nfsd4_proc_compound+0x2b4/0x4e8
> [ 16.815721] [<ffffffff80266782>] nfsd_dispatch+0x118/0x172
> [ 16.823405] [<ffffffff807633fa>] svc_process_common+0x2de/0x62c
> [ 16.832935] [<ffffffff8076380c>] svc_process+0xc4/0x102
> [ 16.840421] [<ffffffff802661de>] nfsd+0x102/0x16a
> [ 16.848520] [<ffffffff80025b60>] kthread+0xfe/0x110
> [ 16.856648] [<ffffffff80003054>] ret_from_exception+0x0/0xc
>



2021-11-13 21:57:09

by J. Bruce Fields

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

On Sat, Nov 13, 2021 at 09:31:40PM +0000, Chuck Lever III wrote:
> This allows the client to send bitmaps larger than bmval[],
> as the old decoder did,

Oh, thanks, right, I guess rejecting too-large bitmaps outright might
cause compatibility problems with future implementations.

(Hm, ideally, shouldn't we be checking whether bits are set beyond where
we expect so that e.g. we can return NFS4ERR_ATTRNOTSUPP on operations
that set attributes? Perhaps that's more than is necessary; it's a
separate issue, anyway.)

--b.

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

2021-11-14 02:44:42

by Chuck Lever III

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()



> On Nov 13, 2021, at 4:57 PM, Bruce Fields <[email protected]> wrote:
>
> On Sat, Nov 13, 2021 at 09:31:40PM +0000, Chuck Lever III wrote:
>> This allows the client to send bitmaps larger than bmval[],
>> as the old decoder did,
>
> Oh, thanks, right, I guess rejecting too-large bitmaps outright might
> cause compatibility problems with future implementations.
>
> (Hm, ideally, shouldn't we be checking whether bits are set beyond where
> we expect so that e.g. we can return NFS4ERR_ATTRNOTSUPP on operations
> that set attributes? Perhaps that's more than is necessary; it's a
> separate issue, anyway.)

The spec might call for those bits to be ignored. The server would
simply not set those in the response. I believe that's how unsupported
bits are handled anyway, rather than returning an error response.

Also note that the "count > 1000" sanity check might be unneeded. If
the count is unrealistically large, it will cause the subsequent
xdr_inline_decode() to fail. I could send a separate patch to remove
that check if you agree.


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

--
Chuck Lever




2021-12-13 02:10:51

by J. Bruce Fields

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

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()
>
> [email protected] 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: <[email protected]>
> Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in nfsd4_decode_fattr()")
> Signed-off-by: Chuck Lever <[email protected]>
>
> 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.

2021-12-13 04:21:56

by Chuck Lever III

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()


> On Dec 12, 2021, at 9:10 PM, Bruce Fields <[email protected]> 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()
>>
>> [email protected] 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: <[email protected]>
>> Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in nfsd4_decode_fattr()")
>> Signed-off-by: Chuck Lever <[email protected]>
>>
>> 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.


2021-12-13 04:52:45

by Trond Myklebust

[permalink] [raw]
Subject: Re: NFS client can crash server due to overrun in nfsd4_decode_bitmap4()

On Mon, 2021-12-13 at 04:21 +0000, Chuck Lever III wrote:
>
> > On Dec 12, 2021, at 9:10 PM, Bruce Fields <[email protected]>
> > 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()
> > >
> > >    [email protected] 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: <[email protected]>
> > >    Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in
> > > nfsd4_decode_fattr()")
> > >    Signed-off-by: Chuck Lever <[email protected]>
> > >
> > > 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
[email protected]