| From 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c Mon Sep 17 00:00:00 2001 |
| From: NeilBrown <neilb@suse.com> |
| Date: Mon, 5 Dec 2016 15:10:11 +1100 |
| Subject: SUNRPC: fix refcounting problems with auth_gss messages. |
| |
| From: NeilBrown <neilb@suse.com> |
| |
| commit 1cded9d2974fe4fe339fc0ccd6638b80d465ab2c upstream. |
| |
| 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") |
| 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| net/sunrpc/auth_gss/auth_gss.c | 7 ++++++- |
| 1 file changed, 6 insertions(+), 1 deletion(-) |
| |
| --- 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_au |
| 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 |
| warn_gssd(); |
| gss_release_msg(gss_msg); |
| } |
| + gss_release_msg(gss_msg); |
| } |
| |
| static void gss_pipe_dentry_destroy(struct dentry *dir, |