[v2,for-4.4,04/16] SUNRPC: fix refcounting problems with auth_gss messages.

Message ID 1492019030-13567-5-git-send-email-sumit.semwal@linaro.org
State New
Headers show
Series
  • Stable commits from Ubuntu Xenial 4.4-lts
Related show

Commit Message

Sumit Semwal April 12, 2017, 5:43 p.m.
From: NeilBrown <neilb@suse.com>


[ Upstream commit 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c ]

There are two problems with refcounting of auth_gss messages.

First, the reference on the pipe->pipe list (taken by a call
to rpc_queue_upcall()) is not counted.  It seems to be
assumed that a message in pipe->pipe will always also be in
pipe->in_downcall, where it is correctly reference counted.

However there is no guaranty of this.  I have a report of a
NULL dereferences in rpc_pipe_read() which suggests a msg
that has been freed is still on the pipe->pipe list.

One way I imagine this might happen is:
- message is queued for uid=U and auth->service=S1
- rpc.gssd reads this message and starts processing.
  This removes the message from pipe->pipe
- message is queued for uid=U and auth->service=S2
- rpc.gssd replies to the first message. gss_pipe_downcall()
  calls __gss_find_upcall(pipe, U, NULL) and it finds the
  *second* message, as new messages are placed at the head
  of ->in_downcall, and the service type is not checked.
- This second message is removed from ->in_downcall and freed
  by gss_release_msg() (even though it is still on pipe->pipe)
- rpc.gssd tries to read another message, and dereferences a pointer
  to this message that has just been freed.

I fix this by incrementing the reference count before calling
rpc_queue_upcall(), and decrementing it if that fails, or normally in
gss_pipe_destroy_msg().

It seems strange that the reply doesn't target the message more
precisely, but I don't know all the details.  In any case, I think the
reference counting irregularity became a measureable bug when the
extra arg was added to __gss_find_upcall(), hence the Fixes: line
below.

The second problem is that if rpc_queue_upcall() fails, the new
message is not freed. gss_alloc_msg() set the ->count to 1,
gss_add_msg() increments this to 2, gss_unhash_msg() decrements to 1,
then the pointer is discarded so the memory never gets freed.

Fixes: 9130b8dbc6ac ("SUNRPC: allow for upcalls for same uid but different gss service")
Cc: stable@vger.kernel.org
Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1011250
Signed-off-by: NeilBrown <neilb@suse.com>

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>

Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

---
 net/sunrpc/auth_gss/auth_gss.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Greg KH April 16, 2017, 7:59 a.m. | #1
On Wed, Apr 12, 2017 at 11:13:38PM +0530, Sumit Semwal wrote:
> From: NeilBrown <neilb@suse.com>

> 

> [ Upstream commit 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c ]

> 

> There are two problems with refcounting of auth_gss messages.

> 

> First, the reference on the pipe->pipe list (taken by a call

> to rpc_queue_upcall()) is not counted.  It seems to be

> assumed that a message in pipe->pipe will always also be in

> pipe->in_downcall, where it is correctly reference counted.

> 

> However there is no guaranty of this.  I have a report of a

> NULL dereferences in rpc_pipe_read() which suggests a msg

> that has been freed is still on the pipe->pipe list.

> 

> One way I imagine this might happen is:

> - message is queued for uid=U and auth->service=S1

> - rpc.gssd reads this message and starts processing.

>   This removes the message from pipe->pipe

> - message is queued for uid=U and auth->service=S2

> - rpc.gssd replies to the first message. gss_pipe_downcall()

>   calls __gss_find_upcall(pipe, U, NULL) and it finds the

>   *second* message, as new messages are placed at the head

>   of ->in_downcall, and the service type is not checked.

> - This second message is removed from ->in_downcall and freed

>   by gss_release_msg() (even though it is still on pipe->pipe)

> - rpc.gssd tries to read another message, and dereferences a pointer

>   to this message that has just been freed.

> 

> I fix this by incrementing the reference count before calling

> rpc_queue_upcall(), and decrementing it if that fails, or normally in

> gss_pipe_destroy_msg().

> 

> It seems strange that the reply doesn't target the message more

> precisely, but I don't know all the details.  In any case, I think the

> reference counting irregularity became a measureable bug when the

> extra arg was added to __gss_find_upcall(), hence the Fixes: line

> below.

> 

> The second problem is that if rpc_queue_upcall() fails, the new

> message is not freed. gss_alloc_msg() set the ->count to 1,

> gss_add_msg() increments this to 2, gss_unhash_msg() decrements to 1,

> then the pointer is discarded so the memory never gets freed.

> 

> Fixes: 9130b8dbc6ac ("SUNRPC: allow for upcalls for same uid but different gss service")

> Cc: stable@vger.kernel.org

> Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1011250

> Signed-off-by: NeilBrown <neilb@suse.com>

> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>

> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

> ---

>  net/sunrpc/auth_gss/auth_gss.c | 7 ++++++-

>  1 file changed, 6 insertions(+), 1 deletion(-)


This patch is already in 4.9.2!  Something went really wrong with your
patch selection here, why are you sending patches I already have?

confused,

greg k-h
Greg KH April 16, 2017, 10:30 a.m. | #2
On Sun, Apr 16, 2017 at 01:34:22PM +0530, Sumit Semwal wrote:
> Hi Greg,

> 

> 

> 

> On Apr 16, 2017 13:29, "Greg KH" <gregkh@linuxfoundation.org> wrote:

> 

>     On Wed, Apr 12, 2017 at 11:13:38PM +0530, Sumit Semwal wrote:

>     > From: NeilBrown <neilb@suse.com>

>     >

>     > [ Upstream commit 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c ]

>     >

>     > There are two problems with refcounting of auth_gss messages.

>     >

>     > First, the reference on the pipe->pipe list (taken by a call

>     > to rpc_queue_upcall()) is not counted.  It seems to be

>     > assumed that a message in pipe->pipe will always also be in

>     > pipe->in_downcall, where it is correctly reference counted.

>     >

>     > However there is no guaranty of this.  I have a report of a

>     > NULL dereferences in rpc_pipe_read() which suggests a msg

>     > that has been freed is still on the pipe->pipe list.

>     >

>     > One way I imagine this might happen is:

>     > - message is queued for uid=U and auth->service=S1

>     > - rpc.gssd reads this message and starts processing.

>     >   This removes the message from pipe->pipe

>     > - message is queued for uid=U and auth->service=S2

>     > - rpc.gssd replies to the first message. gss_pipe_downcall()

>     >   calls __gss_find_upcall(pipe, U, NULL) and it finds the

>     >   *second* message, as new messages are placed at the head

>     >   of ->in_downcall, and the service type is not checked.

>     > - This second message is removed from ->in_downcall and freed

>     >   by gss_release_msg() (even though it is still on pipe->pipe)

>     > - rpc.gssd tries to read another message, and dereferences a pointer

>     >   to this message that has just been freed.

>     >

>     > I fix this by incrementing the reference count before calling

>     > rpc_queue_upcall(), and decrementing it if that fails, or normally in

>     > gss_pipe_destroy_msg().

>     >

>     > It seems strange that the reply doesn't target the message more

>     > precisely, but I don't know all the details.  In any case, I think the

>     > reference counting irregularity became a measureable bug when the

>     > extra arg was added to __gss_find_upcall(), hence the Fixes: line

>     > below.

>     >

>     > The second problem is that if rpc_queue_upcall() fails, the new

>     > message is not freed. gss_alloc_msg() set the ->count to 1,

>     > gss_add_msg() increments this to 2, gss_unhash_msg() decrements to 1,

>     > then the pointer is discarded so the memory never gets freed.

>     >

>     > Fixes: 9130b8dbc6ac ("SUNRPC: allow for upcalls for same uid but

>     different gss service")

>     > Cc: stable@vger.kernel.org

>     > Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1011250

>     > Signed-off-by: NeilBrown <neilb@suse.com>

>     > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>

>     > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>

>     > ---

>     >  net/sunrpc/auth_gss/auth_gss.c | 7 ++++++-

>     >  1 file changed, 6 insertions(+), 1 deletion(-)

> 

>     This patch is already in 4.9.2!  Something went really wrong with your

>     patch selection here, why are you sending patches I already have?

> 

> That's true, it's there in 4.9.2, but this patch is marked for 4.4, right?


Doh, that's what I get for working on patches this early, sorry for the
noise...

I'll get to these after this next round of stable kernels go out.

thanks,

greg k-h

Patch

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 06095cc..1f0687d 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -541,9 +541,13 @@  gss_setup_upcall(struct gss_auth *gss_auth, struct rpc_cred *cred)
 		return gss_new;
 	gss_msg = gss_add_msg(gss_new);
 	if (gss_msg == gss_new) {
-		int res = rpc_queue_upcall(gss_new->pipe, &gss_new->msg);
+		int res;
+		atomic_inc(&gss_msg->count);
+		res = rpc_queue_upcall(gss_new->pipe, &gss_new->msg);
 		if (res) {
 			gss_unhash_msg(gss_new);
+			atomic_dec(&gss_msg->count);
+			gss_release_msg(gss_new);
 			gss_msg = ERR_PTR(res);
 		}
 	} else
@@ -836,6 +840,7 @@  gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
 			warn_gssd();
 		gss_release_msg(gss_msg);
 	}
+	gss_release_msg(gss_msg);
 }
 
 static void gss_pipe_dentry_destroy(struct dentry *dir,