On Thu, 2018-12-20 at 04:39 +0300, Vasily Averin wrote: > Dear Trond, > Red Hat security believes the problem is quite important security > issue: > https://access.redhat.com/security/cve/cve-2018-16884 > > Fix should be backported to affected distributions. > > Could you please approve my first patch and push it to stable@ ? > From my PoV it is correctly fixes the problem, it breaks nothing and > easy for backports, > lightly modified it can be even live-patched. > > Other patches including switch to using empty rqst->rq_xprt can wait. > That patch is not acceptable for upstream. > Thank you, > Vasily Averin > > On 12/19/18 2:25 PM, Vasily Averin wrote: > > On 12/18/18 11:43 PM, Trond Myklebust wrote: > > > On Tue, 2018-12-18 at 23:02 +0300, Vasily Averin wrote: > > > > On 12/18/18 5:55 PM, Trond Myklebust wrote: > > > > > > > It probably also requires us to store a pointer to struct > > > > > > > net > > > > > > > in > > > > > > > the > > > > > > > struct svc_rqst so that nfs4_callback_compound() and > > > > > > > svcauth_gss_accept() can find it, but that should be OK > > > > > > > since > > > > > > > the > > > > > > > transport already has that referenced. > > > > > > > > Ok, I can fix these functions and their sub-calls. > > > > However rqst->rq_xprt is used in other functions that seems > > > > can be > > > > called inside svc_process_common() > > > > - in trace_svc_process(rqstp, progp->pg_name); > > > > - in svc_reserve_auth(rqstp, ...) -> svc_reserve() > > > > - svc_authorise() -> svcauth_gss_release() > > > > > > > > It seems I should fix these places too, it isn't? > > > > could you please advise how to fix svc_reserve() ? > > > > > > We don't want svc_reserve() to run at all for the back channel, > > > so I > > > guess that a test for rqstp->rq_xprt != NULL is appropriate there > > > too. > > > > > > svcauth_gss_release() is just using rqstp->rq_xprt to find the > > > net > > > namespace, so if you add a pointer rqstp->rq_net to fix > > > nfs4_callback_compound, then that will fix the gss case as well. > > > > > > For trace_svc_process(), maybe pull rqst->rq_xprt->xpt_remotebuf > > > out of > > > the tracepoint definition in include/trace/events/sunrpc.h and > > > make it > > > a tracepoint argument that is allowed to be NULL? > > > > This one seems works, could you please check it before formal > > submit ? > > NFSv4 callback-1644 [002] .... 4731.064372: svc_process: > > addr=(null) xid=0x0b0924e3 service=NFSv4 callback vers=1 proc=1 > > > > Frankly speaking I'm afraid that I missed something, > > rqstp->rq_xprt is widely used and nobody expect that it can be > > NULL. > > > > And even I missed nothing -- it's quite tricky anyway. > > Future cahnges can add new calls or execute old non-empty-xprt- > > aware > > functions and trigger crash in some exotic configuration. > > > > Thank you, > > Vasily Averin > > -- Trond Myklebust CTO, Hammerspace Inc 4300 El Camino Real, Suite 105 Los Altos, CA 94022 www.hammer.space
