nfsd: check for oversized NFSv2/v3 arguments

From: "J. Bruce Fields" <[email protected]>

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ää <[email protected]>

Reported-by Ari Kauppi <[email protected]>

Cc [email protected]

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs3xdr.c          | 23 +++++++++++++++++------
 fs/nfsd/nfsxdr.c           | 13 ++++++++++---
 include/linux/sunrpc/svc.h |  3 +--
 3 files changed, 28 insertions(+), 11 deletions(-)

fs/nfsd/nfs3xdr.c +17 -6

index dba2ff8eaa68..be66bcadfaea 100644
@@ -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

fs/nfsd/nfsxdr.c +10 -3

index 41b468a6a90f..79268369f7b3 100644
@@ -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;
 }
 
 /*

include/linux/sunrpc/svc.h +1 -2

index e770abeed32d..6ef19cf658b4 100644
@@ -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

Signature · 1 line

2.9.3

(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.)

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:

net/sunrpc/svc.c +6 -0

index a08aeb56b8e4..14f4d425cf8c 100644
@@ -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?

Quoted 8 lines · NeilBrown

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 */

Quoted 4 lines

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.

Quoted 3 lines

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:

Quoted 25 lines

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

net/sunrpc/svc.c +6 -0

index a08aeb56b8e4..14f4d425cf8c 100644
@@ -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).

Quoted 1 line

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.

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · NeilBrown

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 */

Quoted 8 lines

Quoted 4 lines

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().

Quoted 6 lines

Quoted 5 lines

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?

Quoted 3 lines

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..

Quoted 20 lines · NeilBrown

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · NeilBrown

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 */

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

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....

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 1 line

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.

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.

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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:

Quoted 22 lines

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

net/sunrpc/svc.c +6 -0

index a08aeb56b8e4..14f4d425cf8c 100644
@@ -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++;
 

Quoted 5 lines

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.

Quoted 25 lines · J. Bruce Fields

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 35 lines

Quoted 22 lines

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

net/sunrpc/svc.c +6 -0

index a08aeb56b8e4..14f4d425cf8c 100644
@@ -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++;
 

Quoted 5 lines

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.

Quoted 26 lines · J. Bruce Fields

Quoted 25 lines · J. Bruce Fields

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 27 lines · J. Bruce Fields

Quoted 26 lines · J. Bruce Fields

Quoted 25 lines · J. Bruce Fields

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 21 lines · J. Bruce Fields

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 22 lines · J. Bruce Fields

Quoted 21 lines · J. Bruce Fields

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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:

fs/nfsd/nfssvc.c +9 -0

index 31e1f9593457..b6298d30a01f 100644
@@ -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

Quoted 52 lines · J. Bruce Fields

Quoted 22 lines · J. Bruce Fields

Quoted 21 lines · J. Bruce Fields

Quoted 4 lines · NeilBrown

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

The problem is that:

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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:

fs/nfsd/nfssvc.c +9 -0

index 31e1f9593457..b6298d30a01f 100644
@@ -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

fs/nfsd/nfssvc.c nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp) · line 780

Quoted 10 lines

fs/nfsd/nfssvc.c +7 -0

+		 * 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.

Quoted 5 lines · NeilBrown

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!

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 <[email protected]>
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ää <[email protected]>

Reported-by Ari Kauppi <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs3xdr.c +1 -0

index dba2ff8eaa68..41cc47bf9d00 100644
@@ -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)

fs/nfsd/nfs3xdr.c +0 -0

 	while (len && avail && *old) {
 		*new++ = *old++;

fs/nfsd/nfsxdr.c +2 -0

index 41b468a6a90f..7a0eed7c619d 100644
@@ -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;
 

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (!*(rqstp->rq_next_page))
+		return 0;

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

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

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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.

Quoted 2 lines

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 <[email protected]>
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 [email protected]

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs3xdr.c +5 -4

index dba2ff8eaa68..d18cfddbe115 100644
@@ -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;

fs/nfsd/nfsxdr.c +4 -4

index 41b468a6a90f..59bd88a23a3d 100644
@@ -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 <[email protected]>
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ää <[email protected]>

Reported-by Ari Kauppi <[email protected]>

Reviewed-by NeilBrown <[email protected]>

Cc [email protected]

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs3xdr.c +4 -0

index d18cfddbe115..452334694a5d 100644
@@ -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++;

fs/nfsd/nfsxdr.c +2 -1

index 59bd88a23a3d..de07ff625777 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · J. Bruce Fields

Quoted 5 lines · NeilBrown

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 <[email protected]>
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ää <[email protected]>

Reported-by Ari Kauppi <[email protected]>

Cc [email protected]

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfssvc.c +31 -0

index 31e1f9593457..59979f0bbd4b 100644
@@ -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);