Hi Dan- > On Mar 14, 2022, at 10:06 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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
