Re: [PATCH 1/4] nfs: use-after-free in svc_process_common()

From: Trond Myklebust <trondmy@hammerspace.com>
To: "bfields@fieldses.org" <bfields@fieldses.org>,
	"vvs@virtuozzo.com" <vvs@virtuozzo.com>
Cc: "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
	"khorenko@virtuozzo.com" <khorenko@virtuozzo.com>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"eshatokhin@virtuozzo.com" <eshatokhin@virtuozzo.com>,
	"chuck.lever@oracle.com" <chuck.lever@oracle.com>,
	"jlayton@kernel.org" <jlayton@kernel.org>
Subject: Re: [PATCH 1/4] nfs: use-after-free in svc_process_common()
Date: Thu, 20 Dec 2018 01:58:49 +0000	[thread overview]
Message-ID: <76d71f3412b3104b917aa836eede3447db35bda0.camel@hammerspace.com> (raw)
In-Reply-To: <2459cc18-3c14-fb68-def6-eb09fc096613@virtuozzo.com>

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