Attachment 312371 Details for Bug 448076 – patch -- prevent mem corruption due to portmap call succeeding after parent rpc_clnt has been freed

[patch] patch -- prevent mem corruption due to portmap call succeeding after parent rpc_clnt has been freed

03-bz-448076-sunrpc-prevent-memor.patch (text/plain), 5.70 KB, created by Jeff Layton on 2008-07-22 17:25:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2008-07-22 17:25:40 UTC

Size: 5.70 KB

patch

obsolete

>BZ#448076: SUNRPC: prevent memory corruption from successful portmap call with dead parent task
>
>From: Jeff Layton <jlayton@redhat.com>
>
>There is possible memory corruption when a RPC client is destroyed while
>it still has a portmap child task in flight. The portmap routines are
>set up to write the result directly to the cl_port field of the rpc_clnt.
>In the right situation, it's possible for this struct to have been freed.
>If the outstanding portmap call subsequently succeeds, it will write the
>result to freed memory.
>
>The following patch fixes this by changing the portmap routines to
>allocate a new "rpc_pmap_result" struct and to write the result there.
>The parent task then copies the result out of that struct and into its
>own when needed. The struct is managed via a kref, which prevents us
>from freeing the result container before all users have finished with
>it.
>---
>
> include/linux/sunrpc/clnt.h |    5 +++++
> net/sunrpc/clnt.c           |    5 +++++
> net/sunrpc/pmap_clnt.c      |   45 ++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 52 insertions(+), 3 deletions(-)
>
>
>diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>index 769e320..2cee67b 100644
>--- a/include/linux/sunrpc/clnt.h
>+++ b/include/linux/sunrpc/clnt.h
>@@ -31,6 +31,7 @@ struct rpc_portmap {
> };
> 
> struct rpc_inode;
>+struct rpc_pmap_result;
> 
> /*
>  * The high-level client handle
>@@ -66,6 +67,9 @@ struct rpc_clnt {
> 	struct rpc_rtt		cl_rtt_default;
> 	struct rpc_portmap	cl_pmap_default;
> 	char			cl_inline_name[32];
>+#ifndef __GENKSYMS__
>+	struct rpc_pmap_result	*cl_pmap_result;
>+#endif /* !__GENKSYMS__ */
> };
> #define cl_timeout		cl_xprt->timeout
> #define cl_prog			cl_pmap->pm_prog
>@@ -118,6 +122,7 @@ struct rpc_clnt *rpc_clone_client(struct rpc_clnt *);
> int		rpc_shutdown_client(struct rpc_clnt *);
> int		rpc_destroy_client(struct rpc_clnt *);
> void		rpc_release_client(struct rpc_clnt *);
>+void		pmap_result_put(struct rpc_pmap_result *pr);
> void		rpc_getport(struct rpc_task *, struct rpc_clnt *);
> int		rpc_register(u32, u32, int, unsigned short, int *);
> 
>diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>index 9aa673a..99e9453 100644
>--- a/net/sunrpc/clnt.c
>+++ b/net/sunrpc/clnt.c
>@@ -209,6 +209,7 @@ rpc_clone_client(struct rpc_clnt *clnt)
> 	if (new->cl_auth)
> 		atomic_inc(&new->cl_auth->au_count);
> 	new->cl_pmap		= &new->cl_pmap_default;
>+	new->cl_pmap_result	= NULL;
> 	rpc_init_wait_queue(&new->cl_pmap_default.pm_bindwait, "bindwait");
> 	return new;
> out_no_clnt:
>@@ -268,6 +269,10 @@ rpc_destroy_client(struct rpc_clnt *clnt)
> 
> 	dprintk("RPC: destroying %s client for %s\n",
> 			clnt->cl_protname, clnt->cl_server);
>+	if (clnt->cl_pmap_result) {
>+		pmap_result_put(clnt->cl_pmap_result);
>+		clnt->cl_pmap_result = NULL;
>+	}
> 	if (clnt->cl_auth) {
> 		rpcauth_destroy(clnt->cl_auth);
> 		clnt->cl_auth = NULL;
>diff --git a/net/sunrpc/pmap_clnt.c b/net/sunrpc/pmap_clnt.c
>index a43cabc..2a276ef 100644
>--- a/net/sunrpc/pmap_clnt.c
>+++ b/net/sunrpc/pmap_clnt.c
>@@ -19,6 +19,7 @@
> #include <linux/sunrpc/clnt.h>
> #include <linux/sunrpc/xprt.h>
> #include <linux/sunrpc/sched.h>
>+#include <linux/kref.h>
> 
> #ifdef RPC_DEBUG
> # define RPCDBG_FACILITY	RPCDBG_PMAP
>@@ -34,6 +35,25 @@ static void			pmap_getport_done(struct rpc_task *);
> extern struct rpc_program	pmap_program;
> static spinlock_t		pmap_lock = SPIN_LOCK_UNLOCKED;
> 
>+struct rpc_pmap_result {
>+	__u16			pr_port;
>+	struct kref		pr_refcount;
>+};
>+
>+static void
>+pmap_result_release(struct kref *ref)
>+{
>+	struct rpc_pmap_result *pr = container_of(ref, struct rpc_pmap_result,
>+						  pr_refcount);
>+	kfree(pr);
>+}
>+
>+void
>+pmap_result_put(struct rpc_pmap_result *pr)
>+{
>+	kref_put(&pr->pr_refcount, pmap_result_release);
>+}
>+
> /*
>  * Obtain the port for a given RPC service on a given host. This one can
>  * be called for an ongoing RPC request.
>@@ -46,7 +66,6 @@ rpc_getport(struct rpc_task *task, struct rpc_clnt *clnt)
> 	struct rpc_message msg = {
> 		.rpc_proc	= &pmap_procedures[PMAP_GETPORT],
> 		.rpc_argp	= map,
>-		.rpc_resp	= &clnt->cl_port,
> 		.rpc_cred	= NULL
> 	};
> 	struct rpc_clnt	*pmap_clnt;
>@@ -68,6 +87,14 @@ rpc_getport(struct rpc_task *task, struct rpc_clnt *clnt)
> 	map->pm_binding = 1;
> 	spin_unlock(&pmap_lock);
> 
>+	clnt->cl_pmap_result = kzalloc(sizeof(*clnt->cl_pmap_result),
>+					GFP_KERNEL);
>+	if (clnt->cl_pmap_result == NULL)
>+		goto bailout;
>+
>+	kref_init(&clnt->cl_pmap_result->pr_refcount);
>+	msg.rpc_resp = clnt->cl_pmap_result;
>+
> 	pmap_clnt = pmap_create(clnt->cl_server, sap, map->pm_prot);
> 	if (IS_ERR(pmap_clnt)) {
> 		task->tk_status = PTR_ERR(pmap_clnt);
>@@ -87,10 +114,16 @@ rpc_getport(struct rpc_task *task, struct rpc_clnt *clnt)
> 	rpc_call_setup(child, &msg, 0);
> 
> 	/* ... and run the child task */
>+	kref_get(&clnt->cl_pmap_result->pr_refcount);
>+	pmap_clnt->cl_pmap_result = clnt->cl_pmap_result;
> 	rpc_run_child(task, child, pmap_getport_done);
> 	return;
> 
> bailout:
>+	if (clnt->cl_pmap_result) {
>+		pmap_result_put(clnt->cl_pmap_result);
>+		clnt->cl_pmap_result = NULL;
>+	}
> 	spin_lock(&pmap_lock);
> 	map->pm_binding = 0;
> 	rpc_wake_up(&map->pm_bindwait);
>@@ -142,6 +175,12 @@ pmap_getport_done(struct rpc_task *task)
> 	dprintk("RPC: %4d pmap_getport_done(status %d)\n",
> 			task->tk_pid, task->tk_status);
> 
>+	if (clnt->cl_port == 0)
>+		clnt->cl_port = clnt->cl_pmap_result->pr_port;
>+
>+	pmap_result_put(clnt->cl_pmap_result);
>+	clnt->cl_pmap_result = NULL;
>+
> 	if (task->tk_status < 0) {
> 		/* Pass error up */
> 		clnt->cl_port = 0;
>@@ -253,9 +292,9 @@ xdr_encode_mapping(struct rpc_rqst *req, u32 *p, struct rpc_portmap *map)
> }
> 
> static int
>-xdr_decode_port(struct rpc_rqst *req, u32 *p, unsigned short *portp)
>+xdr_decode_port(struct rpc_rqst *req, u32 *p, struct rpc_pmap_result *pr)
> {
>-	*portp = (unsigned short) ntohl(*p++);
>+	pr->pr_port = (unsigned short) ntohl(*p++);
> 	return 0;
> }
>

Actions: View | Diff

Attachments on bug 448076: 312371