* [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()
@ 2025-09-25 16:28 Alexandr Sapozhnkiov
2025-09-26 0:09 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: Alexandr Sapozhnkiov @ 2025-09-25 16:28 UTC (permalink / raw)
To: Neil Brown, Chuck Lever, Jeff Layton, Olga Kornievskaia, Dai Ngo,
Tom Talpey, linux-nfs, linux-kernel
Cc: Alexandr Sapozhnikov, lvc-project
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()
2025-09-25 16:28 [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr() Alexandr Sapozhnkiov
@ 2025-09-26 0:09 ` NeilBrown
0 siblings, 0 replies; 4+ messages in thread
From: NeilBrown @ 2025-09-26 0:09 UTC (permalink / raw)
To: Alexandr Sapozhnkiov
Cc: Chuck Lever, Jeff Layton, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-kernel, Alexandr Sapozhnikov, lvc-project
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()
@ 2025-09-25 7:56 Alexandr Sapozhnkiov
2025-09-25 10:04 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: Alexandr Sapozhnkiov @ 2025-09-25 7:56 UTC (permalink / raw)
To: Chuck Lever, Jeff Layton, Neil Brown, Olga Kornievskaia, Dai Ngo,
Tom Talpey, linux-nfs, linux-kernel
Cc: Alexandr Sapozhnikov, lvc-project
From: Alexandr Sapozhnikov <alsp705@gmail.com>
The value of an arithmetic expression 'tmp1 * NSEC_PER_USEC'
is a subject to overflow because its operands are not cast
to a larger data type before performing arithmetic
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 > 1000000)
+ tmp2 = 1000000;
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr()
2025-09-25 7:56 Alexandr Sapozhnkiov
@ 2025-09-25 10:04 ` NeilBrown
0 siblings, 0 replies; 4+ messages in thread
From: NeilBrown @ 2025-09-25 10:04 UTC (permalink / raw)
To: Alexandr Sapozhnkiov
Cc: Chuck Lever, Jeff Layton, Olga Kornievskaia, Dai Ngo, Tom Talpey,
linux-nfs, linux-kernel, Alexandr Sapozhnikov, lvc-project
On Thu, 25 Sep 2025, Alexandr Sapozhnkiov wrote:
> From: Alexandr Sapozhnikov <alsp705@gmail.com>
>
> The value of an arithmetic expression 'tmp1 * NSEC_PER_USEC'
> is a subject to overflow because its operands are not cast
> to a larger data type before performing arithmetic
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
Does this matter? What will break if tv_nsec is greater than
999,999,999 ??
>
> 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 > 1000000)
> + tmp2 = 1000000;
1000000 isn't a valud value is it?
> 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;
This is even more weird. 1000001 will become 999999, but 1000000 will
stay as it is.
Why is it even different?
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-26 0:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-25 16:28 [PATCH] nfsd: fix arithmetic expression overflow in decode_saddr() Alexandr Sapozhnkiov
2025-09-26 0:09 ` NeilBrown
-- strict thread matches above, loose matches on Subject: below --
2025-09-25 7:56 Alexandr Sapozhnkiov
2025-09-25 10:04 ` NeilBrown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.