[PATCH] NFS: Fix shift-out-of-bounds UBSAN warning with negative retrans

[PATCH] NFS: Fix shift-out-of-bounds UBSAN warning with negative retrans

[Wang Zhaolong]

The previous commit c09f11ef3595 ("NFS: fs_context: validate UDP retrans
to prevent shift out-of-bounds") added a check to prevent shift values
that are too large, but it fails to account for negative retrans values.
When data->retrans is negative, the condition `data->retrans >= 64` is
skipped, allowing negative values to be copied to context->retrans,
which is unsigned. This results in a large positive number that can
trigger the original UBSAN issue[1].

This patch modifies the check to explicitly handle both negative values
and values that are too large.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=219988
Fixes: 9954bf92c0cd ("NFS: Move mount parameterisation bits into their own file")
Signed-off-by: Wang Zhaolong <wangzhaolong1@huawei.com>
---
 fs/nfs/fs_context.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 13f71ca8c974..0703ac0349cb 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -1161,11 +1161,12 @@ static int nfs23_parse_monolithic(struct fs_context *fc,
 		 * 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 */
+			/* Reject invalid retrans values (negative or too large) */
+			if (data->retrans < 0 || data->retrans >= 64)
 				goto out_invalid_data;
 
 		/*
 		 * Translate to nfs_fs_context, which nfs_fill_super
 		 * can deal with.
-- 
2.34.3

Re: [PATCH] NFS: Fix shift-out-of-bounds UBSAN warning with negative retrans

[Wang Zhaolong]

Hello,

I noticed an issue with the Fixes tag in my original patch. The current tag
refers to commit 9954bf92c0cd ("NFS: Move mount parameterisation bits into their
own file"), which is not directly related to the issue being fixed.

While investigating further, I discovered this problem actually traces back to
much earlier kernel versions (as far back as 2.6.x). The shift-out-of-bounds
issue has been present for a very long time.

Commit c09f11ef3595 ("NFS: fs_context: validate UDP retrans to prevent shift
out-of-bounds") attempted to fix part of this issue by checking for values that
are too large, but it didn't account for negative values. My patch completes
this fix by also checking for negative values.

Given this history, I believe the proper Fixes tag should point to c09f11ef3595
as my patch is completing an incomplete fix, while also mentioning the long-term
existence of this issue in the commit message.

I will send a v2 patch with this clarification.

Thanks,
Wang Zhaolong

> The previous commit c09f11ef3595 ("NFS: fs_context: validate UDP retrans
> to prevent shift out-of-bounds") added a check to prevent shift values
> that are too large, but it fails to account for negative retrans values.
> When data->retrans is negative, the condition `data->retrans >= 64` is
> skipped, allowing negative values to be copied to context->retrans,
> which is unsigned. This results in a large positive number that can
> trigger the original UBSAN issue[1].
> 
> This patch modifies the check to explicitly handle both negative values
> and values that are too large.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=219988
> Fixes: 9954bf92c0cd ("NFS: Move mount parameterisation bits into their own file")
> Signed-off-by: Wang Zhaolong <wangzhaolong1@huawei.com>
> ---
>   fs/nfs/fs_context.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 13f71ca8c974..0703ac0349cb 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -1161,11 +1161,12 @@ static int nfs23_parse_monolithic(struct fs_context *fc,
>   		 * 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 */
> +			/* Reject invalid retrans values (negative or too large) */
> +			if (data->retrans < 0 || data->retrans >= 64)
>   				goto out_invalid_data;
>   
>   		/*
>   		 * Translate to nfs_fs_context, which nfs_fill_super
>   		 * can deal with.