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:
signature.asc
Description: PGP signature
