| 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 | expand |
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
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
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,