| 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
| ||
|
Message-ID: <6df264b9-7d87-4b35-8f56-b593097124a6@huawei.com>
Date: Thu, 10 Apr 2025 21:59:58 +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>, <yi.zhang@...wei.com>, <yangerkun@...wei.com>
Subject: Re: [PATCH RFC] nfs: Fix shift-out-of-bounds UBSAN warning with
negative retrans
Friendly ping.
Perhaps the easiest way to avoid the UBSAN warning is to add a check at
the location where the warning is triggered. If a check is to be added during
the NFS mount process, then the calculation results of the two parameters passed
from user space must first be verified. This approach, however, seems somewhat
redundant.
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 0eab15465511..b714671fa418 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -654,13 +654,20 @@ static unsigned long xprt_abs_ktime_to_jiffies(ktime_t abstime)
static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req,
const struct rpc_timeout *to)
{
unsigned long majortimeo = req->rq_timeout;
- if (to->to_exponential)
- majortimeo <<= to->to_retries;
- else
+ if (to->to_exponential) {
+ unsigned long shifted_value;
+
+ /* Use safe left shift to avoid undefined behavior */
+ if (check_shl_overflow(majortimeo, to->to_retries,
+ &shifted_value))
+ majortimeo = to->to_maxval;
+ else
+ majortimeo = shifted_value;
+ } else
majortimeo += to->to_increment * to->to_retries;
if (majortimeo > to->to_maxval || majortimeo == 0)
majortimeo = to->to_maxval;
return majortimeo;
}
--
2.34.3
Best regards,
Wang Zhaolong
> 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;
Powered by blists - more mailing lists