linux-kernel - Re: [PATCH] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <7a462cee-bf09-4ff3-a047-198b4790fa31@huawei.com>
Date: Tue, 8 Apr 2025 11:05:05 +0800
From: Wang Zhaolong <wangzhaolong1@...wei.com>
To: <trondmy@...nel.org>, <anna@...nel.org>, <dhowells@...hat.com>,
	<viro@...iv.linux.org.uk>
CC: <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<yi.zhang@...wei.com>, <yangerkun@...wei.com>
Subject: Re: [PATCH] NFS: Fix shift-out-of-bounds UBSAN warning with negative
 retrans

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@...wei.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.