| Message ID | 87oa0r584c.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 4, 2016 at 11:10 PM, NeilBrown <neilb@suse.com> wrote: > > 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. Correct "service" was/is not a part of the protocol between the kernel and gssd. So that's why the kernel can't match what's coming from gssd to a particular waiting upcall. > - 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. Agreed. This is a problem. Doesn't the problem still exist even with this patch because gss_add_msg() adds the msg onto the in_downcall() list? So gssd in __gss_fin_upcall() can find the 2nd upcall even before the 2nd msg is added to the pipe->pipe()? In the perfect world, the real solution (even for the initial problem) would have been changing the gssd-kernel protocol to include "service" as a part of the upcall. But that's not going to happen... The solution I can thinking of is something where the message is not on "in_downcall" list until it's consumed by the rpc_pipe. I'd need to some time to figure out how to do that... > 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> > --- > net/sunrpc/auth_gss/auth_gss.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c > index 3dfd769dc5b5..16cea00c959b 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, > -- > 2.10.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 06 2016, Olga Kornievskaia wrote: > On Sun, Dec 4, 2016 at 11:10 PM, NeilBrown <neilb@suse.com> wrote: >> >> 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. > > Correct "service" was/is not a part of the protocol between the kernel > and gssd. So that's why the kernel can't match what's coming from gssd > to a particular waiting upcall. > >> - 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. > > Agreed. This is a problem. > > Doesn't the problem still exist even with this patch because > gss_add_msg() adds the msg onto the in_downcall() list? So gssd in > __gss_fin_upcall() can find the 2nd upcall even before the 2nd msg is > added to the pipe->pipe()? The use-after-free problem is solved I think. It doesn't really make any difference if the down-call arrives before or after rpc_queue_upcall() is called. The msg will still not be freed before it is removed from both lists. Thanks, NeilBrown
On Mon, Dec 5, 2016 at 5:04 PM, NeilBrown <neilb@suse.com> wrote: > On Tue, Dec 06 2016, Olga Kornievskaia wrote: > >> On Sun, Dec 4, 2016 at 11:10 PM, NeilBrown <neilb@suse.com> wrote: >>> >>> 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. >> >> Correct "service" was/is not a part of the protocol between the kernel >> and gssd. So that's why the kernel can't match what's coming from gssd >> to a particular waiting upcall. >> >>> - 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. >> >> Agreed. This is a problem. >> >> Doesn't the problem still exist even with this patch because >> gss_add_msg() adds the msg onto the in_downcall() list? So gssd in >> __gss_fin_upcall() can find the 2nd upcall even before the 2nd msg is >> added to the pipe->pipe()? > > The use-after-free problem is solved I think. It doesn't really make > any difference if the down-call arrives before or after > rpc_queue_upcall() is called. The msg will still not be freed before it > is removed from both lists. > Sorry I don't see it. Thread 1 adds an upcall and it's getting processed by gssd. Thread 2 executes gss_add_msg() which puts the message on the in_downcall list. Context switch (before the atomic_inc()!). Upcall comes back from the gssd, finds msg from Thread2 in_downcall list. gss_release_msg() will dec the counter to 0 and will remove the msg. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 07 2016, Olga Kornievskaia wrote: >>> >>> Agreed. This is a problem. >>> >>> Doesn't the problem still exist even with this patch because >>> gss_add_msg() adds the msg onto the in_downcall() list? So gssd in >>> __gss_fin_upcall() can find the 2nd upcall even before the 2nd msg is >>> added to the pipe->pipe()? >> >> The use-after-free problem is solved I think. It doesn't really make >> any difference if the down-call arrives before or after >> rpc_queue_upcall() is called. The msg will still not be freed before it >> is removed from both lists. >> > > Sorry I don't see it. Maybe we are looking at different code? > > Thread 1 adds an upcall and it's getting processed by gssd. > Thread 2 executes gss_add_msg() which puts the message on the > in_downcall list. Context switch (before the atomic_inc()!). gss_add_msg(), as of 4.9-rc8, is spin_lock(&pipe->lock); old = __gss_find_upcall(pipe, gss_msg->uid, gss_msg->auth); if (old == NULL) { atomic_inc(&gss_msg->count); list_add(&gss_msg->list, &pipe->in_downcall); } else gss_msg = old; spin_unlock(&pipe->lock); so the gss_msg is added to in_downcall *after* the atomic_inc(), and the whole is protected by pipe->lock anyway so even if the atomic_inc() were delayed by the CPU reordering things, there would be no risk of gss_pipe_downcall() finding a gss_msg which didn't have the ->count elevated. NeilBrown > Upcall comes back from the gssd, finds msg from Thread2 in_downcall > list. gss_release_msg() will dec the counter to 0 and will remove the > msg.
On Tue, Dec 6, 2016 at 5:12 PM, NeilBrown <neilb@suse.com> wrote: > On Wed, Dec 07 2016, Olga Kornievskaia wrote: >>>> >>>> Agreed. This is a problem. >>>> >>>> Doesn't the problem still exist even with this patch because >>>> gss_add_msg() adds the msg onto the in_downcall() list? So gssd in >>>> __gss_fin_upcall() can find the 2nd upcall even before the 2nd msg is >>>> added to the pipe->pipe()? >>> >>> The use-after-free problem is solved I think. It doesn't really make >>> any difference if the down-call arrives before or after >>> rpc_queue_upcall() is called. The msg will still not be freed before it >>> is removed from both lists. >>> >> >> Sorry I don't see it. > > Maybe we are looking at different code? > >> >> Thread 1 adds an upcall and it's getting processed by gssd. >> Thread 2 executes gss_add_msg() which puts the message on the >> in_downcall list. Context switch (before the atomic_inc()!). > > gss_add_msg(), as of 4.9-rc8, is > spin_lock(&pipe->lock); > old = __gss_find_upcall(pipe, gss_msg->uid, gss_msg->auth); > if (old == NULL) { > atomic_inc(&gss_msg->count); > list_add(&gss_msg->list, &pipe->in_downcall); > } else > gss_msg = old; > spin_unlock(&pipe->lock); > > so the gss_msg is added to in_downcall *after* the atomic_inc(), and the > whole is protected by pipe->lock anyway so even if the atomic_inc() were > delayed by the CPU reordering things, there would be no risk of > gss_pipe_downcall() finding a gss_msg which didn't have the ->count > elevated. > Ah, I missed the atomic_inc() in the gss_add_msg(). I was only looking at your patch that did another atomic_inc(). Makes sense now. > NeilBrown > >> Upcall comes back from the gssd, finds msg from Thread2 in_downcall >> list. gss_release_msg() will dec the counter to 0 and will remove the >> msg. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c index 3dfd769dc5b5..16cea00c959b 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,
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> --- net/sunrpc/auth_gss/auth_gss.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)