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

[NeilBrown <neilb@suse.com>]

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