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

[NeilBrown <neilb@suse.com>]

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