linux-kernel - [PATCH RFC] nfs: Fix shift-out-of-bounds UBSAN warning with negative retrans

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250410073525.1982010-1-wangzhaolong1@huawei.com>
Date: Thu, 10 Apr 2025 15:35:25 +0800
From: Wang Zhaolong <wangzhaolong1@...wei.com>
To: <trondmy@...nel.org>, <anna@...nel.org>, <rdunlap@...radead.org>
CC: <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<chengzhihao1@...wei.com>, <wangzhaolong1@...wei.com>, <yi.zhang@...wei.com>,
	<yangerkun@...wei.com>
Subject: [PATCH RFC] nfs: Fix shift-out-of-bounds UBSAN warning with negative retrans

The previous commit c09f11ef3595 ("NFS: fs_context: validate UDP retrans
to prevent shift out-of-bounds") attempted to address UBSAN warnings by
validating retrans values, but had two key limitations[1]:

1. It only handled binary mount options, not validating string options
2. It failed to account for negative retrans values, which bypass the
   check `data->retrans >= 64` but then get converted to large positive
   numbers when assigned to unsigned context->retrans

When these large numbers are later used as shift amounts in
xprt_calc_majortimeo(), they trigger the UBSAN shift-out-of-bounds
warning. This issue has existed since early kernel versions (2.6.x series)
as the code historically lacks proper validation of retrans values from
userspace.

As the NFS maintainer has previously indicated that fixes to
xprt_calc_majortimeo() wouldn't be accepted for this issue, this patch
takes the approach of properly validating input parameters instead:

This patch:
- Adds a proper validation check in nfs_validate_transport_protocol(),
  which is a common code path for all mount methods (binary or string)
- Defines a reasonable upper limit (15) that is still generous for
  real-world requirements while preventing undefined behavior
- Provides a clearer error message to users when rejecting values
- Removes the incomplete check from the binary mount path

By validating the retrans parameter in a common code path, we ensure
all mount operations have consistent behavior regardless of how the
mount options are provided to the kernel.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=219988
[2] https://lore.kernel.org/all/5850f8a65c59436b607c9d1ac088402d14873577.camel@hammerspace.com/#t

Fixes: c09f11ef3595 ("NFS: fs_context: validate UDP retrans to prevent shift out-of-bounds")
Signed-off-by: Wang Zhaolong <wangzhaolong1@...wei.com>
---
 fs/nfs/fs_context.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 13f71ca8c974..cb3683d5d37f 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -33,10 +33,11 @@
 #else
 #define NFS_DEFAULT_VERSION 2
 #endif
 
 #define NFS_MAX_CONNECTIONS 16
+#define NFS_MAX_UDP_RETRANS 15
 
 enum nfs_param {
 	Opt_ac,
 	Opt_acdirmax,
 	Opt_acdirmin,
@@ -358,10 +359,19 @@ static int nfs_validate_transport_protocol(struct fs_context *fc,
 {
 	switch (ctx->nfs_server.protocol) {
 	case XPRT_TRANSPORT_UDP:
 		if (nfs_server_transport_udp_invalid(ctx))
 			goto out_invalid_transport_udp;
+		/*
+		 * For UDP transport, retrans is used as a shift value in
+		 * xprt_calc_majortimeo(). To prevent shift-out-of-bounds
+		 * and overflow, limit it to 15 bits which is a reasonable
+		 * upper limit for any real-world scenario (typical values
+		 * are 3-5).
+		 */
+		if (ctx->retrans > NFS_MAX_UDP_RETRANS)
+			goto out_invalid_udp_retrans;
 		break;
 	case XPRT_TRANSPORT_TCP:
 	case XPRT_TRANSPORT_RDMA:
 		break;
 	default:
@@ -378,10 +388,13 @@ static int nfs_validate_transport_protocol(struct fs_context *fc,
 	}
 
 	return 0;
 out_invalid_transport_udp:
 	return nfs_invalf(fc, "NFS: Unsupported transport protocol udp");
+out_invalid_udp_retrans:
+	return nfs_invalf(fc, "NFS: UDP protocol requires retrans <= %d (got %d)",
+			  NFS_MAX_UDP_RETRANS, ctx->retrans);
 out_invalid_xprtsec_policy:
 	return nfs_invalf(fc, "NFS: Transport does not support xprtsec");
 }
 
 /*
@@ -1155,19 +1168,10 @@ static int nfs23_parse_monolithic(struct fs_context *fc,
 		memcpy(mntfh->data, data->root.data, mntfh->size);
 		if (mntfh->size < sizeof(mntfh->data))
 			memset(mntfh->data + mntfh->size, 0,
 			       sizeof(mntfh->data) - mntfh->size);
 
-		/*
-		 * for proto == XPRT_TRANSPORT_UDP, which is what uses
-		 * to_exponential, implying shift: limit the shift value
-		 * to BITS_PER_LONG (majortimeo is unsigned long)
-		 */
-		if (!(data->flags & NFS_MOUNT_TCP)) /* this will be UDP */
-			if (data->retrans >= 64) /* shift value is too large */
-				goto out_invalid_data;
-
 		/*
 		 * Translate to nfs_fs_context, which nfs_fill_super
 		 * can deal with.
 		 */
 		ctx->flags	= data->flags & NFS_MOUNT_FLAGMASK;
@@ -1270,13 +1274,10 @@ static int nfs23_parse_monolithic(struct fs_context *fc,
 out_no_address:
 	return nfs_invalf(fc, "NFS: mount program didn't pass remote address");
 
 out_invalid_fh:
 	return nfs_invalf(fc, "NFS: invalid root filehandle");
-
-out_invalid_data:
-	return nfs_invalf(fc, "NFS: invalid binary mount data");
 }
 
 #if IS_ENABLED(CONFIG_NFS_V4)
 struct compat_nfs_string {
 	compat_uint_t len;
-- 
2.39.2