[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]>
---
fs/nfsd/nfs4xdr.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
Hi Bruce-
This version is cleaned up slightly and has been tested as follows:
- I am not able to crash a patched server using rtm's nfsd_1
- No new FAILUREs with NFSv4.1 pynfs tests
- No new failures with xfstests on NFSv4.1 or NFSv4.2
- No new failures with the git regression suite on NFSv4.1 or NFSv4.2
- No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr
Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be
confident that it is working properly.
I tested this on top of my XDR tracepoint series, so the line numbers
might be a little off from your current tree. However, it should
otherwise apply cleanly.
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 5ff481e9c85d..479d3452ce6c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -288,11 +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;
}
On Sun, Nov 14, 2021 at 03:16:04PM -0500, Chuck Lever wrote:
> [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]>
> ---
> fs/nfsd/nfs4xdr.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> Hi Bruce-
>
> This version is cleaned up slightly and has been tested as follows:
>
> - I am not able to crash a patched server using rtm's nfsd_1
> - No new FAILUREs with NFSv4.1 pynfs tests
> - No new failures with xfstests on NFSv4.1 or NFSv4.2
> - No new failures with the git regression suite on NFSv4.1 or NFSv4.2
> - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr
>
> Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be
> confident that it is working properly.
>
> I tested this on top of my XDR tracepoint series, so the line numbers
> might be a little off from your current tree. However, it should
> otherwise apply cleanly.
Got it, thanks.--b.
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 5ff481e9c85d..479d3452ce6c 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -288,11 +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;
> }
On Sun, 2021-11-14 at 15:16 -0500, Chuck Lever wrote:
> [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]>
> ---
> fs/nfsd/nfs4xdr.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> Hi Bruce-
>
> This version is cleaned up slightly and has been tested as follows:
>
> - I am not able to crash a patched server using rtm's nfsd_1
> - No new FAILUREs with NFSv4.1 pynfs tests
> - No new failures with xfstests on NFSv4.1 or NFSv4.2
> - No new failures with the git regression suite on NFSv4.1 or NFSv4.2
> - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr
>
> Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be
> confident that it is working properly.
>
> I tested this on top of my XDR tracepoint series, so the line numbers
> might be a little off from your current tree. However, it should
> otherwise apply cleanly.
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 5ff481e9c85d..479d3452ce6c 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -288,11 +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;
> }
>
Why not just convert it to use xdr_stream_decode_uint32_array() instead
of maintaining this separate open coded version?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
> On Nov 14, 2021, at 4:58 PM, Trond Myklebust <[email protected]> wrote:
>
> On Sun, 2021-11-14 at 15:16 -0500, Chuck Lever wrote:
>> [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]>
>> ---
>> fs/nfsd/nfs4xdr.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> Hi Bruce-
>>
>> This version is cleaned up slightly and has been tested as follows:
>>
>> - I am not able to crash a patched server using rtm's nfsd_1
>> - No new FAILUREs with NFSv4.1 pynfs tests
>> - No new failures with xfstests on NFSv4.1 or NFSv4.2
>> - No new failures with the git regression suite on NFSv4.1 or NFSv4.2
>> - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr
>>
>> Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be
>> confident that it is working properly.
>>
>> I tested this on top of my XDR tracepoint series, so the line numbers
>> might be a little off from your current tree. However, it should
>> otherwise apply cleanly.
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 5ff481e9c85d..479d3452ce6c 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -288,11 +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;
>> }
>>
>
> Why not just convert it to use xdr_stream_decode_uint32_array() instead
> of maintaining this separate open coded version?
Pulling the chain a little more, perhaps all decode_bitmap4 call
sites should be converted to use xdr_stream_decode_uint32_array.
We could consider that as a clean up later; the current fix has
been kept surgical so it can be backported.
--
Chuck Lever