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

From: Vasily Averin <vvs@virtuozzo.com>
To: Trond Myklebust <trondmy@hammerspace.com>,
	"bfields@fieldses.org" <bfields@fieldses.org>
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: Mon, 24 Dec 2018 11:59:33 +0300	[thread overview]
Message-ID: <29ca1423-619d-ace5-53b3-d6567abf067b@virtuozzo.com> (raw)
In-Reply-To: <6051cc19f72f9104806f628666ce54b31a5b184a.camel@hammerspace.com>

On 12/24/18 11:21 AM, Trond Myklebust wrote:
> On Mon, 2018-12-24 at 09:05 +0300, Vasily Averin wrote:
>> On 12/24/18 8:51 AM, Vasily Averin wrote:
>>> On 12/24/18 2:56 AM, Trond Myklebust wrote:
>>>> On Sat, 2018-12-22 at 20:46 +0300, Vasily Averin wrote:
>>>>> On 12/21/18 4:00 AM, bfields@fieldses.org wrote:
>>>>>> On Tue, Dec 18, 2018 at 02:55:15PM +0000, Trond Myklebust
>>>>>> wrote:
>>>>>>> No. We don't care about xpt_flags for the back channel
>>>>>>> because
>>>>>>> there is
>>>>>>> no "server transport". The actual transport is stored in
>>>>>>> the
>>>>>>> 'struct
>>>>>>> rpc_rqst', and is the struct rpc_xprt corresponding to the
>>>>>>> client
>>>>>>> socket or RDMA channel.
>>>>>>>
>>>>>>> IOW: All we really need in svc_process_common() is to be
>>>>>>> able to
>>>>>>> run
>>>>>>> rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(), and that can
>>>>>>> be
>>>>>>> passed
>>>>>>> either as a pointer to the struct svc_xprt_ops itself.
>>>>>>
>>>>>> For what it's worth, I'd rather get rid of that op--it's an
>>>>>> awfully
>>>>>> roundabout way just to do "svc_putnl(resv, 0);" in the tcp
>>>>>> case.
>>>>>
>>>>> Do you mean that svc_create_xprt(serv, "tcp-bc", ...) was used
>>>>> ONLY
>>>>> to call 
>>>>> svc_tcp_prep_reply_hdr() in svc_process_common() ?
>>>>> And according call for rdma-bc does nothing useful at all? 
>>>>>
>>>>> I've just tried to remove svc_create_xprt() from xs_tcp_bc_up()
>>>>> and
>>>>> just 
>>>>> provide pointer to svc_tcp_prep_reply_hdr()
>>>>> in  svc_process_common() 
>>>>> via per-netns sunrpc_net -- and seems it was enough, my
>>>>> testcase
>>>>> worked correctly.
>>>>
>>>> I don't see how that function is related to net namespaces. As
>>>> far as I
>>>> can tell, it only signals whether or not the type of transport
>>>> uses the
>>>> TCP record marking scheme.
>>>
>>> We need to know which kind of transport is used in specified net
>>> namespace,
>>> for example init_ns can use RDMA transport and netns "second" can
>>> use 
>>> TCP transport at the same time.
>>> If you do not like an idea to use function pointer as a mark -- ok
>>> I can save only some boolean flag on sunrpc_net, check it in
>>> svc_process_common() 
>>> and if it is set -- call svc_tcp_prep_reply_hdr() directly.
> 
> I'm not against the idea of using a function pointer, but I'm saying
> that the transport is not unique per-netns. Instead, the transport is
> usually per NFS mount, but you can always retrieve a pointer to it
> directly in bc_svc_process() from req->rq_xprt. 

You're right, I was wrong because I was focused on creation of fake transport svc_xprt.
Yes, we cannot use per-netns pointer here.

>> moreover, I can do not change sunrpc_net at all,
>> I can check in bc_svc_common() which transport uses incoming svc_req
>> and provide such flag as new parameter to svc_process_common().
> 
> The function or flag used by bc_svc_common() could be added to req-
>> rq_xprt->ops as another 'bc_' field and then passed to
> svc_process_common() as the parameter.

Can I just check rqstp->rq_prot ? It is inherited from incoming svc_req,
and it seems it enough to check its propo, it isn't? 

svc_process_common()
...
        /* Setup reply header */
        if (rqstp->rq_prot == IPPROTO_TCP)
                svc_tcp_prep_reply_hdr(rqstp);