2024-01-25 14:58:55

by Jorge Mora

Subject: [PATCH] NFSD: fix nfsd4_listxattr_validate_cookie

If LISTXATTRS is sent with a correct cookie but a small maxcount,
this could lead function nfsd4_listxattr_validate_cookie to
return NFS4ERR_BAD_COOKIE. If maxcount = 20, then second check
on function gives RHS = 3 thus any cookie larger than 3 returns
NFS4ERR_BAD_COOKIE.

There is no need to validate the cookie on the return XDR buffer
since attribute referenced by cookie will be the first in the
return buffer.

Fixes: 23e50fe3a5e6 ("nfsd: implement the xattr functions and en/decode logic")
Signed-off-by: Jorge Mora <[email protected]>
---
fs/nfsd/nfs4xdr.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 913d566519bf..7f957061c4d5 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5116,16 +5116,11 @@ nfsd4_listxattr_validate_cookie(struct nfsd4_listxattrs *listxattrs,

/*
* If the cookie is larger than the maximum number we can fit
- * in either the buffer we just got back from vfs_listxattr, or,
- * XDR-encoded, in the return buffer, it's invalid.
+ * in the buffer we just got back from vfs_listxattr, it's invalid.
*/
if (cookie > (listxattrs->lsxa_len) / (XATTR_USER_PREFIX_LEN + 2))
return nfserr_badcookie;

- if (cookie > (listxattrs->lsxa_maxcount /
- (XDR_QUADLEN(XATTR_USER_PREFIX_LEN + 2) + 4)))
- return nfserr_badcookie;
-
*offsetp = (u32)cookie;
return 0;
}
--
2.43.0

2024-01-25 17:34:07

by Jeffrey Layton

Subject: Re: [PATCH] NFSD: fix nfsd4_listxattr_validate_cookie

On Thu, 2024-01-25 at 07:46 -0700, Jorge Mora wrote:
> If LISTXATTRS is sent with a correct cookie but a small maxcount,
> this could lead function nfsd4_listxattr_validate_cookie to
> return NFS4ERR_BAD_COOKIE. If maxcount = 20, then second check
> on function gives RHS = 3 thus any cookie larger than 3 returns
> NFS4ERR_BAD_COOKIE.
>
> There is no need to validate the cookie on the return XDR buffer
> since attribute referenced by cookie will be the first in the
> return buffer.
>
> Fixes: 23e50fe3a5e6 ("nfsd: implement the xattr functions and en/decode logic")
> Signed-off-by: Jorge Mora <[email protected]>
> ---
> fs/nfsd/nfs4xdr.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 913d566519bf..7f957061c4d5 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5116,16 +5116,11 @@ nfsd4_listxattr_validate_cookie(struct nfsd4_listxattrs *listxattrs,
>
> /*
> * If the cookie is larger than the maximum number we can fit
> - * in either the buffer we just got back from vfs_listxattr, or,
> - * XDR-encoded, in the return buffer, it's invalid.
> + * in the buffer we just got back from vfs_listxattr, it's invalid.
> */
> if (cookie > (listxattrs->lsxa_len) / (XATTR_USER_PREFIX_LEN + 2))
> return nfserr_badcookie;
>
> - if (cookie > (listxattrs->lsxa_maxcount /
> - (XDR_QUADLEN(XATTR_USER_PREFIX_LEN + 2) + 4)))
> - return nfserr_badcookie;
> -
> *offsetp = (u32)cookie;
> return 0;
> }


The cookie is generated by the server, and the client generates the
maxcount value. They don't really have anything to do with one another,
so yeah, I agree that this check is nonsensical.

Reviewed-by: Jeff Layton <[email protected]>