[PATCH] NFSv4.2: fix listxattr maximum XDR buffer size

[PATCH] NFSv4.2: fix listxattr maximum XDR buffer size

[Jorge Mora]

Switch order of operations to avoid creating a short XDR buffer:
e.g., buflen = 12, old xdrlen = 12, new xdrlen = 20.

Having a short XDR buffer leads to lxa_maxcount be a few bytes
less than what is needed to retrieve the whole list when using
a buflen as returned by a call with size = 0:
    buflen = listxattr(path, NULL, 0);
    buf = malloc(buflen);
    buflen = listxattr(path, buf, buflen);

For a file with one attribute (name = '123456'), the first call
with size = 0 will return buflen = 12 ('user.123456\x00').
The second call with size = 12, sends LISTXATTRS with
lxa_maxcount = 12 + 8 (cookie) + 4 (array count) = 24. The
XDR buffer needs 8 (cookie) + 4 (array count) + 4 (name count)
+ 6 (name len) + 2 (padding) + 4 (eof) = 28 which is 4 bytes
shorter than the lxa_maxcount provided in the call.

Fixes: 04a5da690e8f ("NFSv4.2: define limits and sizes for user xattr handling")
Signed-off-by: Jorge Mora <mora@netapp.com>
---
 fs/nfs/nfs42.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs42.h b/fs/nfs/nfs42.h
index b59876b01a1e..32ee9ad6a560 100644
--- a/fs/nfs/nfs42.h
+++ b/fs/nfs/nfs42.h
@@ -55,11 +55,14 @@ int nfs42_proc_removexattr(struct inode *inode, const char *name);
  * They would be 7 bytes long in the eventual buffer ("user.x\0"), and
  * 8 bytes long XDR-encoded.
  *
- * Include the trailing eof word as well.
+ * Include the trailing eof word as well and make the result a multiple
+ * of 4 bytes.
  */
 static inline u32 nfs42_listxattr_xdrsize(u32 buflen)
 {
-	return ((buflen / (XATTR_USER_PREFIX_LEN + 2)) * 8) + 4;
+	u32 size = 8 * buflen / (XATTR_USER_PREFIX_LEN + 2) + 4;
+
+	return (size + 3) & ~3;
 }
 #endif /* CONFIG_NFS_V4_2 */
 #endif /* __LINUX_FS_NFS_NFS4_2_H */
-- 
2.43.0

Re: [PATCH] NFSv4.2: fix listxattr maximum XDR buffer size

[Benjamin Coddington]

On 25 Jan 2024, at 9:51, Jorge Mora wrote:

> Switch order of operations to avoid creating a short XDR buffer:
> e.g., buflen = 12, old xdrlen = 12, new xdrlen = 20.
>
> Having a short XDR buffer leads to lxa_maxcount be a few bytes
> less than what is needed to retrieve the whole list when using
> a buflen as returned by a call with size = 0:
>     buflen = listxattr(path, NULL, 0);
>     buf = malloc(buflen);
>     buflen = listxattr(path, buf, buflen);
>
> For a file with one attribute (name = '123456'), the first call
> with size = 0 will return buflen = 12 ('user.123456\x00').
> The second call with size = 12, sends LISTXATTRS with
> lxa_maxcount = 12 + 8 (cookie) + 4 (array count) = 24. The
> XDR buffer needs 8 (cookie) + 4 (array count) + 4 (name count)
> + 6 (name len) + 2 (padding) + 4 (eof) = 28 which is 4 bytes
> shorter than the lxa_maxcount provided in the call.
>
> Fixes: 04a5da690e8f ("NFSv4.2: define limits and sizes for user xattr handling")
> Signed-off-by: Jorge Mora <mora@netapp.com>
> ---
>  fs/nfs/nfs42.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs42.h b/fs/nfs/nfs42.h
> index b59876b01a1e..32ee9ad6a560 100644
> --- a/fs/nfs/nfs42.h
> +++ b/fs/nfs/nfs42.h
> @@ -55,11 +55,14 @@ int nfs42_proc_removexattr(struct inode *inode, const char *name);
>   * They would be 7 bytes long in the eventual buffer ("user.x\0"), and
>   * 8 bytes long XDR-encoded.
>   *
> - * Include the trailing eof word as well.
> + * Include the trailing eof word as well and make the result a multiple
> + * of 4 bytes.
>   */
>  static inline u32 nfs42_listxattr_xdrsize(u32 buflen)
>  {
> -	return ((buflen / (XATTR_USER_PREFIX_LEN + 2)) * 8) + 4;
> +	u32 size = 8 * buflen / (XATTR_USER_PREFIX_LEN + 2) + 4;
> +
> +	return (size + 3) & ~3;

Maybe XDR_QUADLEN(size) here?  Otherwise, looks correct.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

Re: [PATCH] NFSv4.2: fix listxattr maximum XDR buffer size

[Benjamin Coddington]

On 26 Jan 2024, at 15:37, Jorge Mora wrote:

> Thanks Ben,
>
> Do you want me to resubmit the patch using XDR_QUALEN? Something like this?
>
>  static inline u32 nfs42_listxattr_xdrsize(u32 buflen)
>  {
> -       return ((buflen / (XATTR_USER_PREFIX_LEN + 2)) * 8) + 4;
> +       u32 size = 8 * buflen / (XATTR_USER_PREFIX_LEN + 2) + 4;
> +
> +       return XDR_QUADLEN(size) << 2;

I just thought I'd mention it since it's used all over.  I think your
version is just fine.

Ben