| Message ID | 20220314140635.GD30883@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: check for negative "len" values in nfssvc_decode_writeargs() | expand |
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
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
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;
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(-)