[PATCH] nfsd: check for oversized NFSv2/v3 arguments

[PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

A client can append random data to the end of an NFSv2 or NFSv3 RPC call
without our complaining; we'll just stop parsing at the end of the
expected data and ignore the rest.

Encoded arguments and replies are stored together in an array of pages,
and if a call is too large it could leave inadequate space for the
reply.  This is normally OK because NFS RPC's typically have either
short arguments and long replies (like READ) or long arguments and short
replies (like WRITE).  But a client that sends an incorrectly long reply
can violate those assumptions.  This was observed to cause crashes.

So, insist that the argument not be any longer than we expect.

Also, several operations increment rq_next_page in the decode routine
before checking the argument size, which can leave rq_next_page pointing
well past the end of the page array, causing trouble later in
svc_free_pages.

As followup we may also want to rewrite the encoding routines to check
more carefully that they aren't running off the end of the page array.

Reported-by: Tuomas Haanpää <thaan@synopsys.com>
Reported-by: Ari Kauppi <ari@synopsys.com>
Cc: stable@vger.kernel.org
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs3xdr.c          | 23 +++++++++++++++++------
 fs/nfsd/nfsxdr.c           | 13 ++++++++++---
 include/linux/sunrpc/svc.h |  3 +--
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index dba2ff8eaa68..be66bcadfaea 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -334,8 +334,11 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
 	if (!p)
 		return 0;
 	p = xdr_decode_hyper(p, &args->offset);
-
 	args->count = ntohl(*p++);
+
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
+
 	len = min(args->count, max_blocksize);
 
 	/* set up the kvec */
@@ -349,7 +352,7 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
 		v++;
 	}
 	args->vlen = v;
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 int
@@ -536,9 +539,11 @@ nfs3svc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p,
 	p = decode_fh(p, &args->fh);
 	if (!p)
 		return 0;
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
 	args->buffer = page_address(*(rqstp->rq_next_page++));
 
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 int
@@ -564,10 +569,14 @@ nfs3svc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
 	args->verf   = p; p += 2;
 	args->dircount = ~0;
 	args->count  = ntohl(*p++);
+
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
+
 	args->count  = min_t(u32, args->count, PAGE_SIZE);
 	args->buffer = page_address(*(rqstp->rq_next_page++));
 
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 int
@@ -585,6 +594,9 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
 	args->dircount = ntohl(*p++);
 	args->count    = ntohl(*p++);
 
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
+
 	len = args->count = min(args->count, max_blocksize);
 	while (len > 0) {
 		struct page *p = *(rqstp->rq_next_page++);
@@ -592,8 +604,7 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
 			args->buffer = page_address(p);
 		len -= PAGE_SIZE;
 	}
-
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 int
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 41b468a6a90f..79268369f7b3 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -257,6 +257,9 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
 	len = args->count     = ntohl(*p++);
 	p++; /* totalcount - unused */
 
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
+
 	len = min_t(unsigned int, len, NFSSVC_MAXBLKSIZE_V2);
 
 	/* set up somewhere to store response.
@@ -272,7 +275,7 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
 		v++;
 	}
 	args->vlen = v;
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 int
@@ -360,9 +363,11 @@ nfssvc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p, struct nfsd_readli
 	p = decode_fh(p, &args->fh);
 	if (!p)
 		return 0;
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
 	args->buffer = page_address(*(rqstp->rq_next_page++));
 
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 int
@@ -400,9 +405,11 @@ nfssvc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
 	args->cookie = ntohl(*p++);
 	args->count  = ntohl(*p++);
 	args->count  = min_t(u32, args->count, PAGE_SIZE);
+	if (!xdr_argsize_check(rqstp, p))
+		return 0;
 	args->buffer = page_address(*(rqstp->rq_next_page++));
 
-	return xdr_argsize_check(rqstp, p);
+	return 1;
 }
 
 /*
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index e770abeed32d..6ef19cf658b4 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -336,8 +336,7 @@ xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
 {
 	char *cp = (char *)p;
 	struct kvec *vec = &rqstp->rq_arg.head[0];
-	return cp >= (char*)vec->iov_base
-		&& cp <= (char*)vec->iov_base + vec->iov_len;
+	return cp == (char *)vec->iov_base + vec->iov_len;
 }
 
 static inline int
-- 
2.9.3

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

(Cc'd you, Neil, partly on the off chance you might have a better idea
where this came from.  Looks to me like it may have been there forever,
but, I haven't looked too hard yet.)

--b.

On Fri, Apr 14, 2017 at 11:04:40AM -0400, bfields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> A client can append random data to the end of an NFSv2 or NFSv3 RPC call
> without our complaining; we'll just stop parsing at the end of the
> expected data and ignore the rest.
> 
> Encoded arguments and replies are stored together in an array of pages,
> and if a call is too large it could leave inadequate space for the
> reply.  This is normally OK because NFS RPC's typically have either
> short arguments and long replies (like READ) or long arguments and short
> replies (like WRITE).  But a client that sends an incorrectly long reply
> can violate those assumptions.  This was observed to cause crashes.
> 
> So, insist that the argument not be any longer than we expect.
> 
> Also, several operations increment rq_next_page in the decode routine
> before checking the argument size, which can leave rq_next_page pointing
> well past the end of the page array, causing trouble later in
> svc_free_pages.
> 
> As followup we may also want to rewrite the encoding routines to check
> more carefully that they aren't running off the end of the page array.
> 
> Reported-by: Tuomas Haanpää <thaan@synopsys.com>
> Reported-by: Ari Kauppi <ari@synopsys.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfs3xdr.c          | 23 +++++++++++++++++------
>  fs/nfsd/nfsxdr.c           | 13 ++++++++++---
>  include/linux/sunrpc/svc.h |  3 +--
>  3 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index dba2ff8eaa68..be66bcadfaea 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -334,8 +334,11 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
>  	if (!p)
>  		return 0;
>  	p = xdr_decode_hyper(p, &args->offset);
> -
>  	args->count = ntohl(*p++);
> +
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
> +
>  	len = min(args->count, max_blocksize);
>  
>  	/* set up the kvec */
> @@ -349,7 +352,7 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
>  		v++;
>  	}
>  	args->vlen = v;
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  int
> @@ -536,9 +539,11 @@ nfs3svc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p,
>  	p = decode_fh(p, &args->fh);
>  	if (!p)
>  		return 0;
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
>  	args->buffer = page_address(*(rqstp->rq_next_page++));
>  
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  int
> @@ -564,10 +569,14 @@ nfs3svc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
>  	args->verf   = p; p += 2;
>  	args->dircount = ~0;
>  	args->count  = ntohl(*p++);
> +
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
> +
>  	args->count  = min_t(u32, args->count, PAGE_SIZE);
>  	args->buffer = page_address(*(rqstp->rq_next_page++));
>  
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  int
> @@ -585,6 +594,9 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
>  	args->dircount = ntohl(*p++);
>  	args->count    = ntohl(*p++);
>  
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
> +
>  	len = args->count = min(args->count, max_blocksize);
>  	while (len > 0) {
>  		struct page *p = *(rqstp->rq_next_page++);
> @@ -592,8 +604,7 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
>  			args->buffer = page_address(p);
>  		len -= PAGE_SIZE;
>  	}
> -
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  int
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 41b468a6a90f..79268369f7b3 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -257,6 +257,9 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
>  	len = args->count     = ntohl(*p++);
>  	p++; /* totalcount - unused */
>  
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
> +
>  	len = min_t(unsigned int, len, NFSSVC_MAXBLKSIZE_V2);
>  
>  	/* set up somewhere to store response.
> @@ -272,7 +275,7 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
>  		v++;
>  	}
>  	args->vlen = v;
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  int
> @@ -360,9 +363,11 @@ nfssvc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p, struct nfsd_readli
>  	p = decode_fh(p, &args->fh);
>  	if (!p)
>  		return 0;
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
>  	args->buffer = page_address(*(rqstp->rq_next_page++));
>  
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  int
> @@ -400,9 +405,11 @@ nfssvc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
>  	args->cookie = ntohl(*p++);
>  	args->count  = ntohl(*p++);
>  	args->count  = min_t(u32, args->count, PAGE_SIZE);
> +	if (!xdr_argsize_check(rqstp, p))
> +		return 0;
>  	args->buffer = page_address(*(rqstp->rq_next_page++));
>  
> -	return xdr_argsize_check(rqstp, p);
> +	return 1;
>  }
>  
>  /*
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e770abeed32d..6ef19cf658b4 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -336,8 +336,7 @@ xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
>  {
>  	char *cp = (char *)p;
>  	struct kvec *vec = &rqstp->rq_arg.head[0];
> -	return cp >= (char*)vec->iov_base
> -		&& cp <= (char*)vec->iov_base + vec->iov_len;
> +	return cp == (char *)vec->iov_base + vec->iov_len;
>  }
>  
>  static inline int
> -- 
> 2.9.3
> 

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On Fri, Apr 14 2017, J. Bruce Fields wrote:

> (Cc'd you, Neil, partly on the off chance you might have a better idea
> where this came from.  Looks to me like it may have been there forever,
> but, I haven't looked too hard yet.)
>

Hi Bruce,
 I can't say that I like this patch at all.

The problem is that:

	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
				       * We assume one is at most one page
				       */

this assumption is never verified.
To my mind, the "obvious" way to verify this assumption is that an
attempt to generate a multi-page reply should fail if there was a
multi-page request.
Failing if there was a little bit of extra noise at the end of the
request seems harsher than necessary, and could result in a regression.

We already know how big replies can get, so we can perform a complete
sanity check quite early:

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index a08aeb56b8e4..14f4d425cf8c 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1196,6 +1196,12 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		goto err_bad_proc;
 	rqstp->rq_procinfo = procp;
 
+	if ((procp->pc_xdrressize == 0 ||
+	     procp->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) &&
+	    rqstp->rq_arg.len > PAGE_SIZE)
+		/* The assumption about request/reply sizes in svc_init_buffer() is violated! */
+		goto err_garbage;
+
 	/* Syntactic check complete */
 	serv->sv_stats->rpccnt++;
 

I haven't tested this at all and haven't even convinced myself that
it covers every case (though I cannot immediately think of any likely
corners).

Does it address your test case?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
>  I can't say that I like this patch at all.
> 
> The problem is that:
> 
> 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> 				       * We assume one is at most one page
> 				       */
> 
> this assumption is never verified.
> To my mind, the "obvious" way to verify this assumption is that an
> attempt to generate a multi-page reply should fail if there was a
> multi-page request.

A third option, by the way, which Ari Kauppi argued for, is adding a
null check each time we increment rq_next_page, since we seem to arrange
for the page array to always be NULL-terminated.

> Failing if there was a little bit of extra noise at the end of the
> request seems harsher than necessary, and could result in a regression.

You're worrying there might be a weird old client out there somewhere?
I guess it seems like a small enough risk to me.  I'm more worried the
extra garbage might violate assumptions elsewhere in the code.

But, this looks good too:

> We already know how big replies can get, so we can perform a complete
> sanity check quite early:
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index a08aeb56b8e4..14f4d425cf8c 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1196,6 +1196,12 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>  		goto err_bad_proc;
>  	rqstp->rq_procinfo = procp;
>  
> +	if ((procp->pc_xdrressize == 0 ||
> +	     procp->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) &&
> +	    rqstp->rq_arg.len > PAGE_SIZE)
> +		/* The assumption about request/reply sizes in svc_init_buffer() is violated! */
> +		goto err_garbage;
> +
>  	/* Syntactic check complete */
>  	serv->sv_stats->rpccnt++;
>  
> 
> I haven't tested this at all and haven't even convinced myself that
> it covers every case (though I cannot immediately think of any likely
> corners).
> 
> Does it address your test case?

I'll check, it probably does.

We'd need to limit the test to v2/v3.

I'm also not opposed to doing both (or all three).

--b.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]

On Tue, Apr 18 2017, J. Bruce Fields wrote:

> On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
>>  I can't say that I like this patch at all.
>> 
>> The problem is that:
>> 
>> 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
>> 				       * We assume one is at most one page
>> 				       */
>> 
>> this assumption is never verified.
>> To my mind, the "obvious" way to verify this assumption is that an
>> attempt to generate a multi-page reply should fail if there was a
>> multi-page request.
>
> A third option, by the way, which Ari Kauppi argued for, is adding a
> null check each time we increment rq_next_page, since we seem to arrange
> for the page array to always be NULL-terminated.

Not a bad idea.   That is what nfsaclsvc_encode_getaclres() and
nfs3svc_encode_getaclres do.
Hmm... your change to xdr_argsize_check will break
nfsaclsvc_decode_setaclargs(), won't it?  It performs the check before
the final nfsacl_decode().


>
>> Failing if there was a little bit of extra noise at the end of the
>> request seems harsher than necessary, and could result in a regression.
>
> You're worrying there might be a weird old client out there somewhere?
> I guess it seems like a small enough risk to me.  I'm more worried the
> extra garbage might violate assumptions elsewhere in the code.

Something like that.  Probably no client does that...  I wouldn't be
overly surprised if some old boot-from-NFS code in a some ROM somewhere
took a shortcut like this though.

>
> But, this looks good too:
>
>> We already know how big replies can get, so we can perform a complete
>> sanity check quite early:
>> 
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index a08aeb56b8e4..14f4d425cf8c 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1196,6 +1196,12 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>>  		goto err_bad_proc;
>>  	rqstp->rq_procinfo = procp;
>>  
>> +	if ((procp->pc_xdrressize == 0 ||
>> +	     procp->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) &&
>> +	    rqstp->rq_arg.len > PAGE_SIZE)
>> +		/* The assumption about request/reply sizes in svc_init_buffer() is violated! */
>> +		goto err_garbage;
>> +
>>  	/* Syntactic check complete */
>>  	serv->sv_stats->rpccnt++;
>>  
>> 
>> I haven't tested this at all and haven't even convinced myself that
>> it covers every case (though I cannot immediately think of any likely
>> corners).
>> 
>> Does it address your test case?
>
> I'll check, it probably does.
>
> We'd need to limit the test to v2/v3.

Why?  Does v4 allocate extra pages?  Or is it more careful about using
them?
v4 does need something different, as pc_xdrressize is always zero..

Thanks,
NeilBrown

>
> I'm also not opposed to doing both (or all three).
>
> --b.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Wed, Apr 19, 2017 at 10:17:09AM +1000, NeilBrown wrote:
> On Tue, Apr 18 2017, J. Bruce Fields wrote:
> 
> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
> >>  I can't say that I like this patch at all.
> >> 
> >> The problem is that:
> >> 
> >> 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> >> 				       * We assume one is at most one page
> >> 				       */
> >> 
> >> this assumption is never verified.
> >> To my mind, the "obvious" way to verify this assumption is that an
> >> attempt to generate a multi-page reply should fail if there was a
> >> multi-page request.
> >
> > A third option, by the way, which Ari Kauppi argued for, is adding a
> > null check each time we increment rq_next_page, since we seem to arrange
> > for the page array to always be NULL-terminated.
> 
> Not a bad idea.   That is what nfsaclsvc_encode_getaclres() and
> nfs3svc_encode_getaclres do.
> Hmm... your change to xdr_argsize_check will break
> nfsaclsvc_decode_setaclargs(), won't it?  It performs the check before
> the final nfsacl_decode().

Ugh, I forget that I don't run any tests for NFSv3 ACLs.  Well, that
would be easy enough to fix....

> >> I haven't tested this at all and haven't even convinced myself that
> >> it covers every case (though I cannot immediately think of any likely
> >> corners).
> >> 
> >> Does it address your test case?
> >
> > I'll check, it probably does.
> >
> > We'd need to limit the test to v2/v3.
> 
> Why?  Does v4 allocate extra pages?  Or is it more careful about using
> them?
> v4 does need something different, as pc_xdrressize is always zero..

NFSv4 compounds just don't have that limitation.  You can read and write
in the same compound if you want.  (Why you'd want to, I've no idea.)

(In fact, I think at least in the version >=4.1 case we should probably
only be placing limits on argument and reply sizes individually, so our
current implementation (which also places limits on the sum of the two)
is probably wrong.  This doesn't keep me up at night.)

--b.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3030 bytes --]

On Tue, Apr 18 2017, J. Bruce Fields wrote:

> On Wed, Apr 19, 2017 at 10:17:09AM +1000, NeilBrown wrote:
>> On Tue, Apr 18 2017, J. Bruce Fields wrote:
>> 
>> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
>> >>  I can't say that I like this patch at all.
>> >> 
>> >> The problem is that:
>> >> 
>> >> 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
>> >> 				       * We assume one is at most one page
>> >> 				       */
>> >> 
>> >> this assumption is never verified.
>> >> To my mind, the "obvious" way to verify this assumption is that an
>> >> attempt to generate a multi-page reply should fail if there was a
>> >> multi-page request.
>> >
>> > A third option, by the way, which Ari Kauppi argued for, is adding a
>> > null check each time we increment rq_next_page, since we seem to arrange
>> > for the page array to always be NULL-terminated.
>> 
>> Not a bad idea.   That is what nfsaclsvc_encode_getaclres() and
>> nfs3svc_encode_getaclres do.
>> Hmm... your change to xdr_argsize_check will break
>> nfsaclsvc_decode_setaclargs(), won't it?  It performs the check before
>> the final nfsacl_decode().
>
> Ugh, I forget that I don't run any tests for NFSv3 ACLs.  Well, that
> would be easy enough to fix....
>
>> >> I haven't tested this at all and haven't even convinced myself that
>> >> it covers every case (though I cannot immediately think of any likely
>> >> corners).
>> >> 
>> >> Does it address your test case?
>> >
>> > I'll check, it probably does.
>> >
>> > We'd need to limit the test to v2/v3.
>> 
>> Why?  Does v4 allocate extra pages?  Or is it more careful about using
>> them?
>> v4 does need something different, as pc_xdrressize is always zero..
>
> NFSv4 compounds just don't have that limitation.  You can read and write
> in the same compound if you want.  (Why you'd want to, I've no idea.)

I realise NFSv4 compounds don't have that limitation.
I wondered what code in the NFSv4 server ensures that we don't try to use
more memory than was allocated.

I notice lots of calls to xdr_reserve_space() in nfs4xdr.c.  Many of them
trigger nfserr_resource when xdr_reserve_space() returns NULL.
But not all.
nfsd4_encode_readv() just pops up a warning.  Once.  Then will
(eventually) de-reference the NULL pointer and crash.
So presumably it really cannot happen (should be a BUG_ON anyway)?
So why can this not happen?
I see that nfsd4_encode_read() limits the size of the read to
  xdr->buf->buflen - xdr->buf->len
and nfsd4_encode_readdir() does a similar thing when computing
bytes_left.

So, it is more careful about using the allocated pages than v2/3 is.

Thanks,
NeilBrown

>
> (In fact, I think at least in the version >=4.1 case we should probably
> only be placing limits on argument and reply sizes individually, so our
> current implementation (which also places limits on the sum of the two)
> is probably wrong.  This doesn't keep me up at night.)
>
> --b.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Thu, Apr 20, 2017 at 10:57:10AM +1000, NeilBrown wrote:
> I realise NFSv4 compounds don't have that limitation.
> I wondered what code in the NFSv4 server ensures that we don't try to use
> more memory than was allocated.
> 
> I notice lots of calls to xdr_reserve_space() in nfs4xdr.c.  Many of them
> trigger nfserr_resource when xdr_reserve_space() returns NULL.
> But not all.
> nfsd4_encode_readv() just pops up a warning.  Once.  Then will
> (eventually) de-reference the NULL pointer and crash.
> So presumably it really cannot happen (should be a BUG_ON anyway)?
> So why can this not happen?
> I see that nfsd4_encode_read() limits the size of the read to
>   xdr->buf->buflen - xdr->buf->len
> and nfsd4_encode_readdir() does a similar thing when computing
> bytes_left.
> 
> So, it is more careful about using the allocated pages than v2/3 is.

Yes.  The v4 code was written from the start with overflow checks
preceding any encode or decode.  And I tried to think this all through
carefully when I rewrote the encoding side a few years ago.  But I don't
think that really got much review, and test coverage is poor (a big
thanks here to the synpsys people for their fuzzing work), so additional
skeptical eyes are welcomed....

There's a lot of tricky hand-written code here handling data from the
network.  Every now and then somebody brings up the idea of trying to
autogenerate it, as is traditionally done for rpc programs.  No idea how
practical that is.

--b.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
> >  I can't say that I like this patch at all.
> > 
> > The problem is that:
> > 
> > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> > 				       * We assume one is at most one page
> > 				       */
> > 
> > this assumption is never verified.
> > To my mind, the "obvious" way to verify this assumption is that an
> > attempt to generate a multi-page reply should fail if there was a
> > multi-page request.
> 
> A third option, by the way, which Ari Kauppi argued for, is adding a
> null check each time we increment rq_next_page, since we seem to arrange
> for the page array to always be NULL-terminated.
> 
> > Failing if there was a little bit of extra noise at the end of the
> > request seems harsher than necessary, and could result in a regression.
> 
> You're worrying there might be a weird old client out there somewhere?
> I guess it seems like a small enough risk to me.  I'm more worried the
> extra garbage might violate assumptions elsewhere in the code.
> 
> But, this looks good too:

But, I'm not too happy about putting that NFSv2/v3-specific check in
common rpc code.  Also, I think this check comes too late for some of
the damage.

I may go with some variation on Ari's idea, let me give it a try....

--b.

> 
> > We already know how big replies can get, so we can perform a complete
> > sanity check quite early:
> > 
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index a08aeb56b8e4..14f4d425cf8c 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1196,6 +1196,12 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> >  		goto err_bad_proc;
> >  	rqstp->rq_procinfo = procp;
> >  
> > +	if ((procp->pc_xdrressize == 0 ||
> > +	     procp->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) &&
> > +	    rqstp->rq_arg.len > PAGE_SIZE)
> > +		/* The assumption about request/reply sizes in svc_init_buffer() is violated! */
> > +		goto err_garbage;
> > +
> >  	/* Syntactic check complete */
> >  	serv->sv_stats->rpccnt++;
> >  
> > 
> > I haven't tested this at all and haven't even convinced myself that
> > it covers every case (though I cannot immediately think of any likely
> > corners).
> > 
> > Does it address your test case?
> 
> I'll check, it probably does.
> 
> We'd need to limit the test to v2/v3.
> 
> I'm also not opposed to doing both (or all three).
> 
> --b.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
> > >  I can't say that I like this patch at all.
> > > 
> > > The problem is that:
> > > 
> > > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> > > 				       * We assume one is at most one page
> > > 				       */
> > > 
> > > this assumption is never verified.
> > > To my mind, the "obvious" way to verify this assumption is that an
> > > attempt to generate a multi-page reply should fail if there was a
> > > multi-page request.
> > 
> > A third option, by the way, which Ari Kauppi argued for, is adding a
> > null check each time we increment rq_next_page, since we seem to arrange
> > for the page array to always be NULL-terminated.
> > 
> > > Failing if there was a little bit of extra noise at the end of the
> > > request seems harsher than necessary, and could result in a regression.
> > 
> > You're worrying there might be a weird old client out there somewhere?
> > I guess it seems like a small enough risk to me.  I'm more worried the
> > extra garbage might violate assumptions elsewhere in the code.
> > 
> > But, this looks good too:
> 
> But, I'm not too happy about putting that NFSv2/v3-specific check in
> common rpc code.  Also, I think this check comes too late for some of
> the damage.
> 
> I may go with some variation on Ari's idea, let me give it a try....

In the read case, I think Ari's approach wouldn't catch the error until
nfsd_direct_splice_actor(), which doesn't actually look capable of
handling errors.  Maybe that should be fixed.  Or maybe read just needs
some more checks.  Ugh.

--b.

> 
> --b.
> 
> > 
> > > We already know how big replies can get, so we can perform a complete
> > > sanity check quite early:
> > > 
> > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > > index a08aeb56b8e4..14f4d425cf8c 100644
> > > --- a/net/sunrpc/svc.c
> > > +++ b/net/sunrpc/svc.c
> > > @@ -1196,6 +1196,12 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> > >  		goto err_bad_proc;
> > >  	rqstp->rq_procinfo = procp;
> > >  
> > > +	if ((procp->pc_xdrressize == 0 ||
> > > +	     procp->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE)) &&
> > > +	    rqstp->rq_arg.len > PAGE_SIZE)
> > > +		/* The assumption about request/reply sizes in svc_init_buffer() is violated! */
> > > +		goto err_garbage;
> > > +
> > >  	/* Syntactic check complete */
> > >  	serv->sv_stats->rpccnt++;
> > >  
> > > 
> > > I haven't tested this at all and haven't even convinced myself that
> > > it covers every case (though I cannot immediately think of any likely
> > > corners).
> > > 
> > > Does it address your test case?
> > 
> > I'll check, it probably does.
> > 
> > We'd need to limit the test to v2/v3.
> > 
> > I'm also not opposed to doing both (or all three).
> > 
> > --b.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

On Thu, Apr 20 2017, J. Bruce Fields wrote:

> On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote:
>> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
>> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
>> > >  I can't say that I like this patch at all.
>> > > 
>> > > The problem is that:
>> > > 
>> > > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
>> > > 				       * We assume one is at most one page
>> > > 				       */
>> > > 
>> > > this assumption is never verified.
>> > > To my mind, the "obvious" way to verify this assumption is that an
>> > > attempt to generate a multi-page reply should fail if there was a
>> > > multi-page request.
>> > 
>> > A third option, by the way, which Ari Kauppi argued for, is adding a
>> > null check each time we increment rq_next_page, since we seem to arrange
>> > for the page array to always be NULL-terminated.
>> > 
>> > > Failing if there was a little bit of extra noise at the end of the
>> > > request seems harsher than necessary, and could result in a regression.
>> > 
>> > You're worrying there might be a weird old client out there somewhere?
>> > I guess it seems like a small enough risk to me.  I'm more worried the
>> > extra garbage might violate assumptions elsewhere in the code.
>> > 
>> > But, this looks good too:
>> 
>> But, I'm not too happy about putting that NFSv2/v3-specific check in
>> common rpc code.  Also, I think this check comes too late for some of
>> the damage.

Too late?  It is earlier than anything else.

>> 
>> I may go with some variation on Ari's idea, let me give it a try....
>
> In the read case, I think Ari's approach wouldn't catch the error until
> nfsd_direct_splice_actor(), which doesn't actually look capable of
> handling errors.  Maybe that should be fixed.  Or maybe read just needs
> some more checks.  Ugh.

By the time you get to nfsd_read(), the 'struct kvec' should be set up
and valid.  So we need checks is e.g. nfs3svc_decode_readargs(), but not
deeper.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Fri, Apr 21, 2017 at 08:11:59AM +1000, NeilBrown wrote:
> On Thu, Apr 20 2017, J. Bruce Fields wrote:
> 
> > On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote:
> >> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
> >> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
> >> > >  I can't say that I like this patch at all.
> >> > > 
> >> > > The problem is that:
> >> > > 
> >> > > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> >> > > 				       * We assume one is at most one page
> >> > > 				       */
> >> > > 
> >> > > this assumption is never verified.
> >> > > To my mind, the "obvious" way to verify this assumption is that an
> >> > > attempt to generate a multi-page reply should fail if there was a
> >> > > multi-page request.
> >> > 
> >> > A third option, by the way, which Ari Kauppi argued for, is adding a
> >> > null check each time we increment rq_next_page, since we seem to arrange
> >> > for the page array to always be NULL-terminated.
> >> > 
> >> > > Failing if there was a little bit of extra noise at the end of the
> >> > > request seems harsher than necessary, and could result in a regression.
> >> > 
> >> > You're worrying there might be a weird old client out there somewhere?
> >> > I guess it seems like a small enough risk to me.  I'm more worried the
> >> > extra garbage might violate assumptions elsewhere in the code.
> >> > 
> >> > But, this looks good too:
> >> 
> >> But, I'm not too happy about putting that NFSv2/v3-specific check in
> >> common rpc code.  Also, I think this check comes too late for some of
> >> the damage.
> 
> Too late?  It is earlier than anything else.

D'oh, yes, I had some idea the check happened after encoding.

> >> I may go with some variation on Ari's idea, let me give it a try....
> >
> > In the read case, I think Ari's approach wouldn't catch the error until
> > nfsd_direct_splice_actor(), which doesn't actually look capable of
> > handling errors.  Maybe that should be fixed.  Or maybe read just needs
> > some more checks.  Ugh.
> 
> By the time you get to nfsd_read(), the 'struct kvec' should be set up
> and valid.

That's ignored in the splice case, isn't it?

OK, maybe I need to sleep on it and look again in the morning....

--b.

> So we need checks is e.g. nfs3svc_decode_readargs(), but not
> deeper.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
> > >  I can't say that I like this patch at all.
> > > 
> > > The problem is that:
> > > 
> > > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> > > 				       * We assume one is at most one page
> > > 				       */
> > > 
> > > this assumption is never verified.
> > > To my mind, the "obvious" way to verify this assumption is that an
> > > attempt to generate a multi-page reply should fail if there was a
> > > multi-page request.
> > 
> > A third option, by the way, which Ari Kauppi argued for, is adding a
> > null check each time we increment rq_next_page, since we seem to arrange
> > for the page array to always be NULL-terminated.
> > 
> > > Failing if there was a little bit of extra noise at the end of the
> > > request seems harsher than necessary, and could result in a regression.
> > 
> > You're worrying there might be a weird old client out there somewhere?
> > I guess it seems like a small enough risk to me.  I'm more worried the
> > extra garbage might violate assumptions elsewhere in the code.
> > 
> > But, this looks good too:
> 
> But, I'm not too happy about putting that NFSv2/v3-specific check in
> common rpc code.

Well, but it should work just as well in nfsd_dispatch, I think?
(Untested).  So, maybe that's simplest as a first step:

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 31e1f9593457..b6298d30a01f 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -759,6 +759,22 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 				rqstp->rq_vers, rqstp->rq_proc);
 	proc = rqstp->rq_procinfo;
 
+	if (rqstp->rq_vers < 4 &&
+	    (proc->pc_xdrressize == 0
+			|| proc->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE))
+		&& rqstp->rq_arg.len > PAGE_SIZE) {
+		/*
+		 * NFSv2 and v3 assume that an operation may have either a
+		 * large argument, or a large reply, but never both.
+		 *
+		 * NFSv4 may handle compounds with both argument and
+		 * reply larger than a reply; it has more xdr careful
+		 * xdr decoding which can handle such calls safely.
+		 */
+		dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers);
+		*statp = rpc_garbage_args;
+		return 1;
+	}
 	/*
 	 * Give the xdr decoder a chance to change this if it wants
 	 * (necessary in the NFSv4.0 compound case)

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]

On Fri, Apr 21 2017, J. Bruce Fields wrote:

> On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote:
>> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
>> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
>> > >  I can't say that I like this patch at all.
>> > > 
>> > > The problem is that:
>> > > 
>> > > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
>> > > 				       * We assume one is at most one page
>> > > 				       */
>> > > 
>> > > this assumption is never verified.
>> > > To my mind, the "obvious" way to verify this assumption is that an
>> > > attempt to generate a multi-page reply should fail if there was a
>> > > multi-page request.
>> > 
>> > A third option, by the way, which Ari Kauppi argued for, is adding a
>> > null check each time we increment rq_next_page, since we seem to arrange
>> > for the page array to always be NULL-terminated.
>> > 
>> > > Failing if there was a little bit of extra noise at the end of the
>> > > request seems harsher than necessary, and could result in a regression.
>> > 
>> > You're worrying there might be a weird old client out there somewhere?
>> > I guess it seems like a small enough risk to me.  I'm more worried the
>> > extra garbage might violate assumptions elsewhere in the code.
>> > 
>> > But, this looks good too:
>> 
>> But, I'm not too happy about putting that NFSv2/v3-specific check in
>> common rpc code.
>
> Well, but it should work just as well in nfsd_dispatch, I think?
> (Untested).  So, maybe that's simplest as a first step:
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 31e1f9593457..b6298d30a01f 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -759,6 +759,22 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
>  				rqstp->rq_vers, rqstp->rq_proc);
>  	proc = rqstp->rq_procinfo;
>  
> +	if (rqstp->rq_vers < 4 &&
> +	    (proc->pc_xdrressize == 0
> +			|| proc->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE))
> +		&& rqstp->rq_arg.len > PAGE_SIZE) {
> +		/*
> +		 * NFSv2 and v3 assume that an operation may have either a
> +		 * large argument, or a large reply, but never both.
> +		 *
> +		 * NFSv4 may handle compounds with both argument and
> +		 * reply larger than a reply; it has more xdr careful
> +		 * xdr decoding which can handle such calls safely.
> +		 */
> +		dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers);
> +		*statp = rpc_garbage_args;
> +		return 1;
> +	}
>  	/*
>  	 * Give the xdr decoder a chance to change this if it wants
>  	 * (necessary in the NFSv4.0 compound case)

I like this.  I think this should be the basis of what goes to -stable,
and other improvements should stay in mainline.

The only change I would suggest would be to be explicit about where the
nfsacl protocol fits with this.

We could change "rqstp->rq_vers < 4" to
 "rqstp->rq_prog == NFS_PROGRAM && rqstp->rq_vers < 4"
or we could change the text:
 NFSv2 and v3 assume ...
to
 NFSv2 and v3, along with NFSASL, assume ...

and possibly change "rqstp->rq_vers < 4" to "!nfsd_v4client(rqstp)".

I believe none of this applies to lockd as none of that code ever looks
beyond a single page.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Mon, Apr 24, 2017 at 08:21:36AM +1000, NeilBrown wrote:
> On Fri, Apr 21 2017, J. Bruce Fields wrote:
> 
> > On Thu, Apr 20, 2017 at 12:19:35PM -0400, J. Bruce Fields wrote:
> >> On Tue, Apr 18, 2017 at 01:13:51PM -0400, J. Bruce Fields wrote:
> >> > On Tue, Apr 18, 2017 at 10:25:20AM +1000, NeilBrown wrote:
> >> > >  I can't say that I like this patch at all.
> >> > > 
> >> > > The problem is that:
> >> > > 
> >> > > 	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
> >> > > 				       * We assume one is at most one page
> >> > > 				       */
> >> > > 
> >> > > this assumption is never verified.
> >> > > To my mind, the "obvious" way to verify this assumption is that an
> >> > > attempt to generate a multi-page reply should fail if there was a
> >> > > multi-page request.
> >> > 
> >> > A third option, by the way, which Ari Kauppi argued for, is adding a
> >> > null check each time we increment rq_next_page, since we seem to arrange
> >> > for the page array to always be NULL-terminated.
> >> > 
> >> > > Failing if there was a little bit of extra noise at the end of the
> >> > > request seems harsher than necessary, and could result in a regression.
> >> > 
> >> > You're worrying there might be a weird old client out there somewhere?
> >> > I guess it seems like a small enough risk to me.  I'm more worried the
> >> > extra garbage might violate assumptions elsewhere in the code.
> >> > 
> >> > But, this looks good too:
> >> 
> >> But, I'm not too happy about putting that NFSv2/v3-specific check in
> >> common rpc code.
> >
> > Well, but it should work just as well in nfsd_dispatch, I think?
> > (Untested).  So, maybe that's simplest as a first step:
> >
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 31e1f9593457..b6298d30a01f 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -759,6 +759,22 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
> >  				rqstp->rq_vers, rqstp->rq_proc);
> >  	proc = rqstp->rq_procinfo;
> >  
> > +	if (rqstp->rq_vers < 4 &&
> > +	    (proc->pc_xdrressize == 0
> > +			|| proc->pc_xdrressize > XDR_QUADLEN(PAGE_SIZE))
> > +		&& rqstp->rq_arg.len > PAGE_SIZE) {
> > +		/*
> > +		 * NFSv2 and v3 assume that an operation may have either a
> > +		 * large argument, or a large reply, but never both.
> > +		 *
> > +		 * NFSv4 may handle compounds with both argument and
> > +		 * reply larger than a reply; it has more xdr careful
> > +		 * xdr decoding which can handle such calls safely.
> > +		 */
> > +		dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers);
> > +		*statp = rpc_garbage_args;
> > +		return 1;
> > +	}
> >  	/*
> >  	 * Give the xdr decoder a chance to change this if it wants
> >  	 * (necessary in the NFSv4.0 compound case)
> 
> I like this.  I think this should be the basis of what goes to -stable,
> and other improvements should stay in mainline.
> 
> The only change I would suggest would be to be explicit about where the
> nfsacl protocol fits with this.

Oh, good point, I'd forgotten nfsd_dispatch handles multiple protocols!

> We could change "rqstp->rq_vers < 4" to
>  "rqstp->rq_prog == NFS_PROGRAM && rqstp->rq_vers < 4"
> or we could change the text:
>  NFSv2 and v3 assume ...
> to
>  NFSv2 and v3, along with NFSASL, assume ...
> 
> and possibly change "rqstp->rq_vers < 4" to "!nfsd_v4client(rqstp)".
> 
> I believe none of this applies to lockd as none of that code ever looks
> beyond a single page.

That makes sense.

--b.

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

On Mon, Apr 24, 2017 at 10:06:42AM -0400, J. Bruce Fields wrote:
> On Mon, Apr 24, 2017 at 08:21:36AM +1000, NeilBrown wrote:
> > I like this.  I think this should be the basis of what goes to -stable,
> > and other improvements should stay in mainline.
> > 
> > The only change I would suggest would be to be explicit about where the
> > nfsacl protocol fits with this.
> 
> Oh, good point, I'd forgotten nfsd_dispatch handles multiple protocols!

That was getting to be kind of a pile of conditions for one "if", and
the comments were getting a little long-winded, so I split it out, but
otherwise it's the same idea.

--b.

commit 43e06bcafea8
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Apr 21 16:10:18 2017 -0400

    nfsd: check for oversized NFSv2/v3 arguments
    
    A client can append random data to the end of an NFSv2 or NFSv3 RPC call
    without our complaining; we'll just stop parsing at the end of the
    expected data and ignore the rest.
    
    Encoded arguments and replies are stored together in an array of pages,
    and if a call is too large it could leave inadequate space for the
    reply.  This is normally OK because NFS RPC's typically have either
    short arguments and long replies (like READ) or long arguments and short
    replies (like WRITE).  But a client that sends an incorrectly long reply
    can violate those assumptions.  This was observed to cause crashes.
    
    Also, several operations increment rq_next_page in the decode routine
    before checking the argument size, which can leave rq_next_page pointing
    well past the end of the page array, causing trouble later in
    svc_free_pages.
    
    So, following a suggestion from Neil Brown, add a central check to
    enforce our expectation that no NFSv2/v3 call has both a large call and
    a large reply.
    
    As followup we may also want to rewrite the encoding routines to check
    more carefully that they aren't running off the end of the page array.
    
    We may also consider rejecting calls that have any extra garbage
    appended.  That would be safer, and within our rights by spec, but given
    the age of our server and the NFS protocol, and the fact that we've
    never enforced this before, we may need to balance that against the
    possibility of breaking some oddball client.
    
    Reported-by: Tuomas Haanpää <thaan@synopsys.com>
    Reported-by: Ari Kauppi <ari@synopsys.com>
    Cc: stable@vger.kernel.org
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 31e1f9593457..59979f0bbd4b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -747,6 +747,37 @@ static __be32 map_new_errors(u32 vers, __be32 nfserr)
 	return nfserr;
 }
 
+/*
+ * A write procedure can have a large argument, and a read procedure can
+ * have a large reply, but no NFSv2 or NFSv3 procedure has argument and
+ * reply that can both be larger than a page.  The xdr code has taken
+ * advantage of this assumption to be a sloppy about bounds checking in
+ * some cases.  Pending a rewrite of the NFSv2/v3 xdr code to fix that
+ * problem, we enforce these assumptions here:
+ */
+static bool nfs_request_too_big(struct svc_rqst *rqstp,
+				struct svc_procedure *proc)
+{
+	/*
+	 * The ACL code has more careful bounds-checking and is not
+	 * susceptible to this problem:
+	 */
+	if (rqstp->rq_prog != NFS_PROGRAM)
+		return false;
+	/*
+	 * Ditto NFSv4 (which can in theory have argument and reply both
+	 * more than a page):
+	 */
+	if (rqstp->rq_vers >= 4)
+		return false;
+	/* The reply will be small, we're OK: */
+	if (proc->pc_xdrressize > 0 &&
+	    proc->pc_xdrressize < XDR_QUADLEN(PAGE_SIZE))
+		return false;
+
+	return rqstp->rq_arg.len > PAGE_SIZE;
+}
+
 int
 nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 {
@@ -759,6 +790,11 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 				rqstp->rq_vers, rqstp->rq_proc);
 	proc = rqstp->rq_procinfo;
 
+	if (nfs_request_too_big(rqstp, proc)) {
+		dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers);
+		*statp = rpc_garbage_args;
+		return 1;
+	}
 	/*
 	 * Give the xdr decoder a chance to change this if it wants
 	 * (necessary in the NFSv4.0 compound case)

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

And, another problem spotted by the Synposys folks.

I'll give these some more testing and hope to send a pull request in
another day or two.

--b.

commit a0aa2db91590
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Apr 21 15:26:30 2017 -0400

    nfsd: stricter decoding of write-like NFSv2/v3 ops
    
    The NFSv2/v3 code does not systematically check whether we decode past
    the end of the buffer.  This generally appears to be harmless, but there
    are a few places where we do arithmetic on the pointers involved and
    don't account for the possibility that a length could be negative.  Add
    checks to catch these.
    
    Reported-by: Tuomas Haanpää <thaan@synopsys.com>
    Reported-by: Ari Kauppi <ari@synopsys.com>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index dba2ff8eaa68..41cc47bf9d00 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -358,6 +358,7 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 {
 	unsigned int len, v, hdr, dlen;
 	u32 max_blocksize = svc_max_payload(rqstp);
+	struct kvec *head = rqstp->rq_arg.head;
 
 	p = decode_fh(p, &args->fh);
 	if (!p)
@@ -367,6 +368,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 	args->count = ntohl(*p++);
 	args->stable = ntohl(*p++);
 	len = args->len = ntohl(*p++);
+	if ((void *)p > head->iov_base + head->iov_len)
+		return 0;
 	/*
 	 * The count must equal the amount of data passed.
 	 */
@@ -466,11 +469,15 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p,
 	len = ntohl(*p++);
 	if (len == 0 || len > NFS3_MAXPATHLEN || len >= PAGE_SIZE)
 		return 0;
+	if (!*(rqstp->rq_next_page))
+		return 0;
 	args->tname = new = page_address(*(rqstp->rq_next_page++));
 	args->tlen = len;
 	/* first copy and check from the first page */
 	old = (char*)p;
 	vec = &rqstp->rq_arg.head[0];
+	if ((void *)old > vec->iov_base + vec->iov_len)
+		return 0;
 	avail = vec->iov_len - (old - (char*)vec->iov_base);
 	while (len && avail && *old) {
 		*new++ = *old++;
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 41b468a6a90f..7a0eed7c619d 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -301,6 +301,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 	 * bytes.
 	 */
 	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
+	if (hdr > rqstp->rq_arg.head[0].iov_len)
+		return 0;
 	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
 		- hdr;
 

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3847 bytes --]

On Mon, Apr 24 2017, J. Bruce Fields wrote:

> And, another problem spotted by the Synposys folks.
>
> I'll give these some more testing and hope to send a pull request in
> another day or two.
>
> --b.
>
> commit a0aa2db91590
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Fri Apr 21 15:26:30 2017 -0400
>
>     nfsd: stricter decoding of write-like NFSv2/v3 ops
>     
>     The NFSv2/v3 code does not systematically check whether we decode past
>     the end of the buffer.  This generally appears to be harmless, but there
>     are a few places where we do arithmetic on the pointers involved and
>     don't account for the possibility that a length could be negative.  Add
>     checks to catch these.
>     
>     Reported-by: Tuomas Haanpää <thaan@synopsys.com>
>     Reported-by: Ari Kauppi <ari@synopsys.com>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>

The code looks right ... but ... 

>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index dba2ff8eaa68..41cc47bf9d00 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -358,6 +358,7 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  {
>  	unsigned int len, v, hdr, dlen;
>  	u32 max_blocksize = svc_max_payload(rqstp);
> +	struct kvec *head = rqstp->rq_arg.head;
>  
>  	p = decode_fh(p, &args->fh);
>  	if (!p)
> @@ -367,6 +368,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	args->count = ntohl(*p++);
>  	args->stable = ntohl(*p++);
>  	len = args->len = ntohl(*p++);
> +	if ((void *)p > head->iov_base + head->iov_len)
> +		return 0;

I'm in two minds here.
On the one hand, the change for NFSv2 could be used here unchanged,
which would make the change slightly smaller and easier to review as
two parts would be identical.

On the other hand I think there is value in defining the "head" local
variable, but to fully realize that value you would need to define
"tail" as well, and then change

	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
		+ rqstp->rq_arg.tail[0].iov_len - hdr;

to

        dlen = head->iov_len + rqstp->rq_arg.page_len + tail->iov_len - hdr;

i.e. either keep it simple (like the v2 code) or make it tidy (with head
and tail), but not half-and-half??


>  	/*
>  	 * The count must equal the amount of data passed.
>  	 */
> @@ -466,11 +469,15 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p,
>  	len = ntohl(*p++);
>  	if (len == 0 || len > NFS3_MAXPATHLEN || len >= PAGE_SIZE)
>  		return 0;
> +	if (!*(rqstp->rq_next_page))
> +		return 0;

Why the extra parentheses?  "->" is at the highest precedence level for C.

>  	args->tname = new = page_address(*(rqstp->rq_next_page++));
>  	args->tlen = len;
>  	/* first copy and check from the first page */
>  	old = (char*)p;
>  	vec = &rqstp->rq_arg.head[0];
> +	if ((void *)old > vec->iov_base + vec->iov_len)
> +		return 0;
>  	avail = vec->iov_len - (old - (char*)vec->iov_base);

We seem to be repeating a calculation here.
I would prefer to make 'avail' a signed int then add

        if (avail < 0)
              return 0;

That would seem more "obviously correct".

But the code is correct as it stands.
Reviewed-by: NeilBrown <neilb@suse.com>

Thanks,
NeilBrown


>  	while (len && avail && *old) {
>  		*new++ = *old++;
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 41b468a6a90f..7a0eed7c619d 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -301,6 +301,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	 * bytes.
>  	 */
>  	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
> +	if (hdr > rqstp->rq_arg.head[0].iov_len)
> +		return 0;
>  	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
>  		- hdr;
>  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[J. Bruce Fields]

> > +	if (!*(rqstp->rq_next_page))
> > +		return 0;
> 
> Why the extra parentheses?  "->" is at the highest precedence level for C.

I've been doing this stuff enough years, you'd think I'd have bothered
to memorize the C operator precedence table by now.

Anyway, this change is actually unrelated and not entirely necessary;
dropped.

> i.e. either keep it simple (like the v2 code) or make it tidy (with head
> and tail), but not half-and-half??

What the heck, let's go all out.

You only live once!

--b.

commit db44bac41bbf
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Apr 25 16:21:34 2017 -0400

    nfsd4: minor NFSv2/v3 write decoding cleanup
    
    Use a couple shortcuts that will simplify a following bugfix.
    
    Cc: stable@vger.kernel.org
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index dba2ff8eaa68..d18cfddbe115 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -358,6 +358,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 {
 	unsigned int len, v, hdr, dlen;
 	u32 max_blocksize = svc_max_payload(rqstp);
+	struct kvec *head = rqstp->rq_arg.head;
+	struct kvec *tail = rqstp->rq_arg.tail;
 
 	p = decode_fh(p, &args->fh);
 	if (!p)
@@ -377,9 +379,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 	 * Check to make sure that we got the right number of
 	 * bytes.
 	 */
-	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
-	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
-		+ rqstp->rq_arg.tail[0].iov_len - hdr;
+	hdr = (void*)p - head->iov_base;
+	dlen = head->iov_len + rqstp->rq_arg.page_len + tail->iov_len - hdr;
 	/*
 	 * Round the length of the data which was specified up to
 	 * the next multiple of XDR units and then compare that
@@ -396,7 +397,7 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 		len = args->len = max_blocksize;
 	}
 	rqstp->rq_vec[0].iov_base = (void*)p;
-	rqstp->rq_vec[0].iov_len = rqstp->rq_arg.head[0].iov_len - hdr;
+	rqstp->rq_vec[0].iov_len = head->iov_len - hdr;
 	v = 0;
 	while (len > rqstp->rq_vec[v].iov_len) {
 		len -= rqstp->rq_vec[v].iov_len;
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 41b468a6a90f..59bd88a23a3d 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -280,6 +280,7 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 					struct nfsd_writeargs *args)
 {
 	unsigned int len, hdr, dlen;
+	struct kvec *head = rqstp->rq_arg.head;
 	int v;
 
 	p = decode_fh(p, &args->fh);
@@ -300,9 +301,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 	 * Check to make sure that we got the right number of
 	 * bytes.
 	 */
-	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
-	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
-		- hdr;
+	hdr = (void*)p - head->iov_base;
+	dlen = head->iov_len + rqstp->rq_arg.page_len - hdr;
 
 	/*
 	 * Round the length of the data which was specified up to
@@ -316,7 +316,7 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 		return 0;
 
 	rqstp->rq_vec[0].iov_base = (void*)p;
-	rqstp->rq_vec[0].iov_len = rqstp->rq_arg.head[0].iov_len - hdr;
+	rqstp->rq_vec[0].iov_len = head->iov_len - hdr;
 	v = 0;
 	while (len > rqstp->rq_vec[v].iov_len) {
 		len -= rqstp->rq_vec[v].iov_len;
commit 13bf9fbff0e5
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Apr 21 15:26:30 2017 -0400

    nfsd: stricter decoding of write-like NFSv2/v3 ops
    
    The NFSv2/v3 code does not systematically check whether we decode past
    the end of the buffer.  This generally appears to be harmless, but there
    are a few places where we do arithmetic on the pointers involved and
    don't account for the possibility that a length could be negative.  Add
    checks to catch these.
    
    Reported-by: Tuomas Haanpää <thaan@synopsys.com>
    Reported-by: Ari Kauppi <ari@synopsys.com>
    Reviewed-by: NeilBrown <neilb@suse.com>
    Cc: stable@vger.kernel.org
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index d18cfddbe115..452334694a5d 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -369,6 +369,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 	args->count = ntohl(*p++);
 	args->stable = ntohl(*p++);
 	len = args->len = ntohl(*p++);
+	if ((void *)p > head->iov_base + head->iov_len)
+		return 0;
 	/*
 	 * The count must equal the amount of data passed.
 	 */
@@ -472,6 +474,8 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p,
 	/* first copy and check from the first page */
 	old = (char*)p;
 	vec = &rqstp->rq_arg.head[0];
+	if ((void *)old > vec->iov_base + vec->iov_len)
+		return 0;
 	avail = vec->iov_len - (old - (char*)vec->iov_base);
 	while (len && avail && *old) {
 		*new++ = *old++;
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 59bd88a23a3d..de07ff625777 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -302,6 +302,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
 	 * bytes.
 	 */
 	hdr = (void*)p - head->iov_base;
+	if (hdr > head->iov_len)
+		return 0;
 	dlen = head->iov_len + rqstp->rq_arg.page_len - hdr;
 
 	/*

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5935 bytes --]

On Tue, Apr 25 2017, J. Bruce Fields wrote:

>> > +	if (!*(rqstp->rq_next_page))
>> > +		return 0;
>> 
>> Why the extra parentheses?  "->" is at the highest precedence level for C.
>
> I've been doing this stuff enough years, you'd think I'd have bothered
> to memorize the C operator precedence table by now.
>
> Anyway, this change is actually unrelated and not entirely necessary;
> dropped.
>
>> i.e. either keep it simple (like the v2 code) or make it tidy (with head
>> and tail), but not half-and-half??
>
> What the heck, let's go all out.

Looks good, thanks.


>
> You only live once!

:-)

NeilBrown


>
> --b.
>
> commit db44bac41bbf
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Tue Apr 25 16:21:34 2017 -0400
>
>     nfsd4: minor NFSv2/v3 write decoding cleanup
>     
>     Use a couple shortcuts that will simplify a following bugfix.
>     
>     Cc: stable@vger.kernel.org
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index dba2ff8eaa68..d18cfddbe115 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -358,6 +358,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  {
>  	unsigned int len, v, hdr, dlen;
>  	u32 max_blocksize = svc_max_payload(rqstp);
> +	struct kvec *head = rqstp->rq_arg.head;
> +	struct kvec *tail = rqstp->rq_arg.tail;
>  
>  	p = decode_fh(p, &args->fh);
>  	if (!p)
> @@ -377,9 +379,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	 * Check to make sure that we got the right number of
>  	 * bytes.
>  	 */
> -	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
> -	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
> -		+ rqstp->rq_arg.tail[0].iov_len - hdr;
> +	hdr = (void*)p - head->iov_base;
> +	dlen = head->iov_len + rqstp->rq_arg.page_len + tail->iov_len - hdr;
>  	/*
>  	 * Round the length of the data which was specified up to
>  	 * the next multiple of XDR units and then compare that
> @@ -396,7 +397,7 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  		len = args->len = max_blocksize;
>  	}
>  	rqstp->rq_vec[0].iov_base = (void*)p;
> -	rqstp->rq_vec[0].iov_len = rqstp->rq_arg.head[0].iov_len - hdr;
> +	rqstp->rq_vec[0].iov_len = head->iov_len - hdr;
>  	v = 0;
>  	while (len > rqstp->rq_vec[v].iov_len) {
>  		len -= rqstp->rq_vec[v].iov_len;
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 41b468a6a90f..59bd88a23a3d 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -280,6 +280,7 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  					struct nfsd_writeargs *args)
>  {
>  	unsigned int len, hdr, dlen;
> +	struct kvec *head = rqstp->rq_arg.head;
>  	int v;
>  
>  	p = decode_fh(p, &args->fh);
> @@ -300,9 +301,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	 * Check to make sure that we got the right number of
>  	 * bytes.
>  	 */
> -	hdr = (void*)p - rqstp->rq_arg.head[0].iov_base;
> -	dlen = rqstp->rq_arg.head[0].iov_len + rqstp->rq_arg.page_len
> -		- hdr;
> +	hdr = (void*)p - head->iov_base;
> +	dlen = head->iov_len + rqstp->rq_arg.page_len - hdr;
>  
>  	/*
>  	 * Round the length of the data which was specified up to
> @@ -316,7 +316,7 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  		return 0;
>  
>  	rqstp->rq_vec[0].iov_base = (void*)p;
> -	rqstp->rq_vec[0].iov_len = rqstp->rq_arg.head[0].iov_len - hdr;
> +	rqstp->rq_vec[0].iov_len = head->iov_len - hdr;
>  	v = 0;
>  	while (len > rqstp->rq_vec[v].iov_len) {
>  		len -= rqstp->rq_vec[v].iov_len;
> commit 13bf9fbff0e5
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Fri Apr 21 15:26:30 2017 -0400
>
>     nfsd: stricter decoding of write-like NFSv2/v3 ops
>     
>     The NFSv2/v3 code does not systematically check whether we decode past
>     the end of the buffer.  This generally appears to be harmless, but there
>     are a few places where we do arithmetic on the pointers involved and
>     don't account for the possibility that a length could be negative.  Add
>     checks to catch these.
>     
>     Reported-by: Tuomas Haanpää <thaan@synopsys.com>
>     Reported-by: Ari Kauppi <ari@synopsys.com>
>     Reviewed-by: NeilBrown <neilb@suse.com>
>     Cc: stable@vger.kernel.org
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index d18cfddbe115..452334694a5d 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -369,6 +369,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	args->count = ntohl(*p++);
>  	args->stable = ntohl(*p++);
>  	len = args->len = ntohl(*p++);
> +	if ((void *)p > head->iov_base + head->iov_len)
> +		return 0;
>  	/*
>  	 * The count must equal the amount of data passed.
>  	 */
> @@ -472,6 +474,8 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p,
>  	/* first copy and check from the first page */
>  	old = (char*)p;
>  	vec = &rqstp->rq_arg.head[0];
> +	if ((void *)old > vec->iov_base + vec->iov_len)
> +		return 0;
>  	avail = vec->iov_len - (old - (char*)vec->iov_base);
>  	while (len && avail && *old) {
>  		*new++ = *old++;
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 59bd88a23a3d..de07ff625777 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -302,6 +302,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, __be32 *p,
>  	 * bytes.
>  	 */
>  	hdr = (void*)p - head->iov_base;
> +	if (hdr > head->iov_len)
> +		return 0;
>  	dlen = head->iov_len + rqstp->rq_arg.page_len - hdr;
>  
>  	/*
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] nfsd: check for oversized NFSv2/v3 arguments

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

On Mon, Apr 24 2017, J. Bruce Fields wrote:

> On Mon, Apr 24, 2017 at 10:06:42AM -0400, J. Bruce Fields wrote:
>> On Mon, Apr 24, 2017 at 08:21:36AM +1000, NeilBrown wrote:
>> > I like this.  I think this should be the basis of what goes to -stable,
>> > and other improvements should stay in mainline.
>> > 
>> > The only change I would suggest would be to be explicit about where the
>> > nfsacl protocol fits with this.
>> 
>> Oh, good point, I'd forgotten nfsd_dispatch handles multiple protocols!
>
> That was getting to be kind of a pile of conditions for one "if", and
> the comments were getting a little long-winded, so I split it out, but
> otherwise it's the same idea.
>
> --b.
>
> commit 43e06bcafea8
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Fri Apr 21 16:10:18 2017 -0400
>
>     nfsd: check for oversized NFSv2/v3 arguments
>     
>     A client can append random data to the end of an NFSv2 or NFSv3 RPC call
>     without our complaining; we'll just stop parsing at the end of the
>     expected data and ignore the rest.
>     
>     Encoded arguments and replies are stored together in an array of pages,
>     and if a call is too large it could leave inadequate space for the
>     reply.  This is normally OK because NFS RPC's typically have either
>     short arguments and long replies (like READ) or long arguments and short
>     replies (like WRITE).  But a client that sends an incorrectly long reply
>     can violate those assumptions.  This was observed to cause crashes.
>     
>     Also, several operations increment rq_next_page in the decode routine
>     before checking the argument size, which can leave rq_next_page pointing
>     well past the end of the page array, causing trouble later in
>     svc_free_pages.
>     
>     So, following a suggestion from Neil Brown, add a central check to
>     enforce our expectation that no NFSv2/v3 call has both a large call and
>     a large reply.
>     
>     As followup we may also want to rewrite the encoding routines to check
>     more carefully that they aren't running off the end of the page array.
>     
>     We may also consider rejecting calls that have any extra garbage
>     appended.  That would be safer, and within our rights by spec, but given
>     the age of our server and the NFS protocol, and the fact that we've
>     never enforced this before, we may need to balance that against the
>     possibility of breaking some oddball client.
>     
>     Reported-by: Tuomas Haanpää <thaan@synopsys.com>
>     Reported-by: Ari Kauppi <ari@synopsys.com>
>     Cc: stable@vger.kernel.org
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 31e1f9593457..59979f0bbd4b 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -747,6 +747,37 @@ static __be32 map_new_errors(u32 vers, __be32 nfserr)
>  	return nfserr;
>  }
>  
> +/*
> + * A write procedure can have a large argument, and a read procedure can
> + * have a large reply, but no NFSv2 or NFSv3 procedure has argument and
> + * reply that can both be larger than a page.  The xdr code has taken
> + * advantage of this assumption to be a sloppy about bounds checking in
> + * some cases.  Pending a rewrite of the NFSv2/v3 xdr code to fix that
> + * problem, we enforce these assumptions here:
> + */
> +static bool nfs_request_too_big(struct svc_rqst *rqstp,
> +				struct svc_procedure *proc)
> +{
> +	/*
> +	 * The ACL code has more careful bounds-checking and is not
> +	 * susceptible to this problem:
> +	 */
> +	if (rqstp->rq_prog != NFS_PROGRAM)
> +		return false;
> +	/*
> +	 * Ditto NFSv4 (which can in theory have argument and reply both
> +	 * more than a page):
> +	 */
> +	if (rqstp->rq_vers >= 4)
> +		return false;
> +	/* The reply will be small, we're OK: */
> +	if (proc->pc_xdrressize > 0 &&
> +	    proc->pc_xdrressize < XDR_QUADLEN(PAGE_SIZE))
> +		return false;
> +
> +	return rqstp->rq_arg.len > PAGE_SIZE;
> +}
> +
>  int
>  nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
>  {
> @@ -759,6 +790,11 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
>  				rqstp->rq_vers, rqstp->rq_proc);
>  	proc = rqstp->rq_procinfo;
>  
> +	if (nfs_request_too_big(rqstp, proc)) {
> +		dprintk("nfsd: NFSv%d argument too large\n", rqstp->rq_vers);
> +		*statp = rpc_garbage_args;
> +		return 1;
> +	}
>  	/*
>  	 * Give the xdr decoder a chance to change this if it wants
>  	 * (necessary in the NFSv4.0 compound case)

Yes, that's much nicer :-)
Reviewed-by: NeilBrown <neilb@suse.com>

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]