* [PATCH] NFSD: check for negative "len" values in nfssvc_decode_writeargs()
@ 2022-03-14 14:06 Dan Carpenter
2022-03-14 14:42 ` Chuck Lever III
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-03-14 14:06 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, kernel-janitors, Harshit Mogalapalli
This code checks the upper bound of "len" but it needs to check for
negative values as well.
Fixes: a51b5b737a0b ("NFSD: Update the NFSv2 WRITE argument decoder to use struct xdr_stream")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
From static analysis and I am not sure of the implications of this bug.
fs/nfsd/nfsxdr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index aba8520b4b8b..a9f80e30320e 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -336,7 +336,7 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
/* opaque data */
if (xdr_stream_decode_u32(xdr, &args->len) < 0)
return false;
- if (args->len > NFSSVC_MAXBLKSIZE_V2)
+ if (args->len < 0 || args->len > NFSSVC_MAXBLKSIZE_V2)
return false;
if (!xdr_stream_subsegment(xdr, &args->payload, args->len))
return false;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] NFSD: check for negative "len" values in nfssvc_decode_writeargs()
2022-03-14 14:06 [PATCH] NFSD: check for negative "len" values in nfssvc_decode_writeargs() Dan Carpenter
@ 2022-03-14 14:42 ` Chuck Lever III
2022-03-15 9:45 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Chuck Lever III @ 2022-03-14 14:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: Linux NFS Mailing List, kernel-janitors, Harshit Mogalapalli
Hi Dan-
> On Mar 14, 2022, at 10:06 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> This code checks the upper bound of "len" but it needs to check for
> negative values as well.
It doesn't check because nfsd3_writeargs::len is a __u32,
and the NFSv2 code here was copied from that assuming that
nfsd_writeargs::len had the same signage. This is because...
https://datatracker.ietf.org/doc/html/rfc1832#section-3.13 says
that the count field in a variable-length array is supposed to
be unsigned.
Thus IMO nfsd_writeargs::len should be changed to __u32
instead of adding the extra negativity check.
If you resend, make sure the format specifier in the dprintk()
at the top of nfsd_proc_write() is adjusted accordingly.
> Fixes: a51b5b737a0b ("NFSD: Update the NFSv2 WRITE argument decoder to use struct xdr_stream")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis and I am not sure of the implications of this bug.
xdr_stream_subsegment() takes an unsigned int as its third
parameter. A large out-of-bounds value would cause it to return
false, so this bug should have no impact.
The use of "int" for the nfsd_writeargs::len field goes back
to before the git era, so I suppose just "Cc: stable" is adequate.
> fs/nfsd/nfsxdr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index aba8520b4b8b..a9f80e30320e 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -336,7 +336,7 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
> /* opaque data */
> if (xdr_stream_decode_u32(xdr, &args->len) < 0)
> return false;
> - if (args->len > NFSSVC_MAXBLKSIZE_V2)
> + if (args->len < 0 || args->len > NFSSVC_MAXBLKSIZE_V2)
> return false;
> if (!xdr_stream_subsegment(xdr, &args->payload, args->len))
> return false;
> --
> 2.20.1
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] NFSD: check for negative "len" values in nfssvc_decode_writeargs()
2022-03-14 14:42 ` Chuck Lever III
@ 2022-03-15 9:45 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-03-15 9:45 UTC (permalink / raw)
To: Chuck Lever III
Cc: Linux NFS Mailing List, kernel-janitors, Harshit Mogalapalli
On Mon, Mar 14, 2022 at 05:42:58PM +0300, Chuck Lever III wrote:
> Hi Dan-
>
> > On Mar 14, 2022, at 10:06 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > This code checks the upper bound of "len" but it needs to check for
> > negative values as well.
>
> It doesn't check because nfsd3_writeargs::len is a __u32,
> and the NFSv2 code here was copied from that assuming that
> nfsd_writeargs::len had the same signage. This is because...
>
> https://datatracker.ietf.org/doc/html/rfc1832#section-3.13 says
> that the count field in a variable-length array is supposed to
> be unsigned.
>
> Thus IMO nfsd_writeargs::len should be changed to __u32
> instead of adding the extra negativity check.
>
> If you resend, make sure the format specifier in the dprintk()
> at the top of nfsd_proc_write() is adjusted accordingly.
Thanks for this tip. It's weird that GCC doesn't complain if you don't
make this change to the printk. :/
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-15 9:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14 14:06 [PATCH] NFSD: check for negative "len" values in nfssvc_decode_writeargs() Dan Carpenter
2022-03-14 14:42 ` Chuck Lever III
2022-03-15 9:45 ` Dan Carpenter
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.