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: Thu, 20 Dec 2018 12:30:18 +0300	[thread overview]
Message-ID: <11b7dec4-cd81-cb23-68f9-d56b5df79337@virtuozzo.com> (raw)
In-Reply-To: <76d71f3412b3104b917aa836eede3447db35bda0.camel@hammerspace.com>

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On 12/20/18 4:58 AM, Trond Myklebust wrote:
> 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.

In this case how about my initial plan B -- make svc_serv per net-namespace?
It executes additional per-netns nfsv4 callback threads 
but does not require any changes in existing sunrpc code?

[-- Attachment #2: diff-ms-nfs-make-svc_serv-per-netns --]
[-- Type: text/plain, Size: 4357 bytes --]

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 509dc5adeb8f..df6939da9d73 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -30,12 +30,6 @@
 
 #define NFSDBG_FACILITY NFSDBG_CALLBACK
 
-struct nfs_callback_data {
-	unsigned int users;
-	struct svc_serv *serv;
-};
-
-static struct nfs_callback_data nfs_callback_info[NFS4_MAX_MINOR_VERSION + 1];
 static DEFINE_MUTEX(nfs_callback_mutex);
 static struct svc_program nfs4_callback_program;
 
@@ -252,22 +246,23 @@ static const struct svc_serv_ops *nfs4_cb_sv_ops[] = {
 };
 #endif
 
-static struct svc_serv *nfs_callback_create_svc(int minorversion)
+static struct svc_serv *nfs_callback_create_svc(int minorversion,
+						struct net *net)
 {
-	struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
+	struct nfs_net *nn = net_generic(net, nfs_net_id);
 	const struct svc_serv_ops *sv_ops;
-	struct svc_serv *serv;
+	struct svc_serv *serv = nn->serv[minorversion];
 
 	/*
 	 * Check whether we're already up and running.
 	 */
-	if (cb_info->serv) {
+	if (serv) {
 		/*
 		 * Note: increase service usage, because later in case of error
 		 * svc_destroy() will be called.
 		 */
-		svc_get(cb_info->serv);
-		return cb_info->serv;
+		svc_get(serv);
+		return serv;
 	}
 
 	switch (minorversion) {
@@ -281,20 +276,12 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
 	if (sv_ops == NULL)
 		return ERR_PTR(-ENOTSUPP);
 
-	/*
-	 * Sanity check: if there's no task,
-	 * we should be the first user ...
-	 */
-	if (cb_info->users)
-		printk(KERN_WARNING "nfs_callback_create_svc: no kthread, %d users??\n",
-			cb_info->users);
-
 	serv = svc_create_pooled(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, sv_ops);
 	if (!serv) {
 		printk(KERN_ERR "nfs_callback_create_svc: create service failed\n");
 		return ERR_PTR(-ENOMEM);
 	}
-	cb_info->serv = serv;
+	nn->serv[minorversion] = serv;
 	/* As there is only one thread we need to over-ride the
 	 * default maximum of 80 connections
 	 */
@@ -308,14 +295,14 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
  */
 int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 {
-	struct svc_serv *serv;
-	struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
-	int ret;
 	struct net *net = xprt->xprt_net;
+	struct nfs_net *nn = net_generic(net, nfs_net_id);
+	struct svc_serv *serv = nn->serv[minorversion];
+	int ret;
 
 	mutex_lock(&nfs_callback_mutex);
 
-	serv = nfs_callback_create_svc(minorversion);
+	serv = nfs_callback_create_svc(minorversion, net);
 	if (IS_ERR(serv)) {
 		ret = PTR_ERR(serv);
 		goto err_create;
@@ -329,7 +316,6 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 	if (ret < 0)
 		goto err_start;
 
-	cb_info->users++;
 	/*
 	 * svc_create creates the svc_serv with sv_nrthreads == 1, and then
 	 * svc_prepare_thread increments that. So we need to call svc_destroy
@@ -337,8 +323,8 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 	 * thread exits.
 	 */
 err_net:
-	if (!cb_info->users)
-		cb_info->serv = NULL;
+	if (!nn->cb_users[minorversion])
+		nn->serv[minorversion] = NULL;
 	svc_destroy(serv);
 err_create:
 	mutex_unlock(&nfs_callback_mutex);
@@ -355,19 +341,18 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
  */
 void nfs_callback_down(int minorversion, struct net *net)
 {
-	struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
+	struct nfs_net *nn = net_generic(net, nfs_net_id);
 	struct svc_serv *serv;
 
 	mutex_lock(&nfs_callback_mutex);
-	serv = cb_info->serv;
+	serv = nn->serv[minorversion];
 	nfs_callback_down_net(minorversion, serv, net);
-	cb_info->users--;
-	if (cb_info->users == 0) {
+	if (nn->cb_users[minorversion] == 0) {
 		svc_get(serv);
 		serv->sv_ops->svo_setup(serv, NULL, 0);
 		svc_destroy(serv);
 		dprintk("nfs_callback_down: service destroyed\n");
-		cb_info->serv = NULL;
+		nn->serv[minorversion] = NULL;
 	}
 	mutex_unlock(&nfs_callback_mutex);
 }
diff --git a/fs/nfs/netns.h b/fs/nfs/netns.h
index fc9978c58265..a49978d2fb0d 100644
--- a/fs/nfs/netns.h
+++ b/fs/nfs/netns.h
@@ -29,6 +29,7 @@ struct nfs_net {
 	unsigned short nfs_callback_tcpport6;
 	int cb_users[NFS4_MAX_MINOR_VERSION + 1];
 #endif
+	struct svc_serv *serv[NFS4_MAX_MINOR_VERSION + 1];
 	spinlock_t nfs_client_lock;
 	ktime_t boot_time;
 #ifdef CONFIG_PROC_FS
