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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

I may go with some variation on Ari's idea, let me give it a try....

--b.

> 
> > 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.
> 
> I'm also not opposed to doing both (or all three).
> 
> --b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux