linux-kernel - Re: [PATCH] nfsd: fix arithmetic expression overflow in decode

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-prev] [day] [month] [year] [list]

Message-id: <175884537000.1696783.18278273149263057351@noble.neil.brown.name>
Date: Fri, 26 Sep 2025 10:09:30 +1000
From: NeilBrown <neilb@...mail.net>
To: "Alexandr Sapozhnkiov" <alsp705@...il.com>
Cc: "Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
 "Olga Kornievskaia" <kolga@...app.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, linux-nfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, "Alexandr Sapozhnikov" <alsp705@...il.com>,
 lvc-project@...uxtesting.org
Subject:
 Re: [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()

On Fri, 26 Sep 2025, Alexandr Sapozhnkiov wrote:
> From: Alexandr Sapozhnikov <alsp705@...il.com>
> 
> The value of an arithmetic expression tmp2 * NSEC_PER_USEC 
> is a subject to overflow because its operands are not cast 
> to a larger data type before performing arithmetic.
> If tmp2 == 17,000,000 then the expression tmp2 * NSEC_PER_USEC
> will overflow because expression is of type u32.
> If tmp2 > 1,000,000 then tv_nsec will give be greater 
> than 1 second.

You didn't answer my question: why should we be bothered by an over
flow? What harm does it cause?

If we are going to bother detecting an incorrect value, we should
respond to it by returning false.  That would tell the client that the
numbers it provided weren't a valid time.



> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Alexandr Sapozhnikov <alsp705@...il.com>
> ---
>  fs/nfsd/nfsxdr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 5777f40c7353..df62ed5099de 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -172,6 +172,8 @@ svcxdr_decode_sattr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  	tmp1 = be32_to_cpup(p++);
>  	tmp2 = be32_to_cpup(p++);
>  	if (tmp1 != (u32)-1 && tmp2 != (u32)-1) {
> +		if (tmp2 > 999999)
> +			tmp2 = 999999;
>  		iap->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
>  		iap->ia_atime.tv_sec = tmp1;
>  		iap->ia_atime.tv_nsec = tmp2 * NSEC_PER_USEC;
> @@ -180,6 +182,8 @@ svcxdr_decode_sattr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  	tmp1 = be32_to_cpup(p++);
>  	tmp2 = be32_to_cpup(p++);
>  	if (tmp1 != (u32)-1 && tmp2 != (u32)-1) {
> +		if (tmp2 > 1000000)
> +			tmp2 = 999999;

Why are the two code fragments different?  This isn't an AI writing the
patch is it?

NeilBrown

>  		iap->ia_valid |= ATTR_MTIME | ATTR_MTIME_SET;
>  		iap->ia_mtime.tv_sec = tmp1;
>  		iap->ia_mtime.tv_nsec = tmp2 * NSEC_PER_USEC;
> -- 
> 2.43.0
> 
> 
>