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

[NeilBrown <neilb@suse.com>]

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