nfsd: fix arithmetic expression overflow in decode_saddr()

[PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()

Posted by Alexandr Sapozhnkiov 6 days, 5 hours ago

From: Alexandr Sapozhnikov <alsp705@gmail.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.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Alexandr Sapozhnikov <alsp705@gmail.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;
 		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

Re: [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()

Posted by NeilBrown 5 days, 22 hours ago

On Fri, 26 Sep 2025, Alexandr Sapozhnkiov wrote:
> From: Alexandr Sapozhnikov <alsp705@gmail.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@gmail.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
> 
> 
>