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

[NeilBrown <neilb@suse.com>]

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