2024-01-25 15:03:19

by Jorge Mora

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

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 <[email protected]>
---
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



2024-01-26 13:50:30

by Benjamin Coddington

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

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 <[email protected]>
> ---
> 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 <[email protected]>

Ben


2024-01-27 13:02:13

by Benjamin Coddington

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

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