[Patch] mqueue: fix the retry logic for netlink_attachskb()

[Patch] mqueue: fix the retry logic for netlink_attachskb()

[Cong Wang]

The retry logic for netlink_attachskb() inside sys_mq_notify()
is suspicious and vulnerable:

1) The sock refcnt is already released when retry is needed
2) The fd is controllable by user-space because we already
   release the file refcnt

so we when retry and the fd has been closed during this small
window, we end up calling netlink_detachskb() on the error path
which releases the sock again and could lead to a use-after-free.

We don't need to re-get the file pointer in the retry loop, keep
it until we finally succeed.

Reported-by: GeneBlue <geneblue.mail@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 ipc/mqueue.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index e8d41ff..b894d6c 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -1237,15 +1237,15 @@ SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
 			/* TODO: add a header? */
 			skb_put(nc, NOTIFY_COOKIE_LEN);
 			/* and attach it to the socket */
-retry:
 			f = fdget(notification.sigev_signo);
 			if (!f.file) {
 				ret = -EBADF;
 				goto out;
 			}
+retry:
 			sock = netlink_getsockbyfilp(f.file);
-			fdput(f);
 			if (IS_ERR(sock)) {
+				fdput(f);
 				ret = PTR_ERR(sock);
 				sock = NULL;
 				goto out;
@@ -1255,6 +1255,7 @@ SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
 			ret = netlink_attachskb(sock, nc, &timeo, NULL);
 			if (ret == 1)
 				goto retry;
+			fdput(f);
 			if (ret) {
 				sock = NULL;
 				nc = NULL;
-- 
2.5.5

Re: [Patch] mqueue: fix the retry logic for netlink_attachskb()

[Linus Torvalds]

On Fri, Jul 7, 2017 at 11:32 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> The retry logic for netlink_attachskb() inside sys_mq_notify()
> is suspicious and vulnerable:
>
> 1) The sock refcnt is already released when retry is needed
> 2) The fd is controllable by user-space because we already
>    release the file refcnt

Hmm. What's different the second (and third.. and..) time around from
the first time?

I don't dislike your patch (it looks fine), but  avoiding the
fdget/fdput in the retry loop doesn't seem to really change anything -
it's just as if we'd just react to the original thing a bit later.

> so we when retry and the fd has been closed during this small
> window, we end up calling netlink_detachskb() on the error path
> which releases the sock again and could lead to a use-after-free.

So this seems to be a real problem: "sock" is not NULL'ed out in that

                        if (!f.file) {

error case (or alternatively, in the retry case).  Plus, since we did
the "fput()" early, "sock" may be gone by the time we do the
netlink_attachskb() even when it's all successful.

But I don't think this is really so much about the retrying - the
"sock may be gone" case seems to be true even the first time around,
and even if we never retry at all.

Am I reading this correctly?

Basically, I think the patch is fine, but the explanation seems a bit
misleading. This isn't really about the re-trying: that would be fine
if we just cleaned up sock properly.

Can you confirm that? I don't know where the original report is.

And that code is ancient, so we should do a "cc: stable" there too,
and backport it basically forever. I think most of the code in this
area predates the git tree, although Al Viro actually touched some
things around here very recently to make the compat case cleaner.

                 Linus

Re: [Patch] mqueue: fix the retry logic for netlink_attachskb()

[Cong Wang]

On Fri, Jul 7, 2017 at 5:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 7, 2017 at 11:32 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> so we when retry and the fd has been closed during this small
>> window, we end up calling netlink_detachskb() on the error path
>> which releases the sock again and could lead to a use-after-free.
>
> So this seems to be a real problem: "sock" is not NULL'ed out in that
>
>                         if (!f.file) {
>
> error case (or alternatively, in the retry case).  Plus, since we did
> the "fput()" early, "sock" may be gone by the time we do the
> netlink_attachskb() even when it's all successful.
>
> But I don't think this is really so much about the retrying - the
> "sock may be gone" case seems to be true even the first time around,
> and even if we never retry at all.
>
> Am I reading this correctly?


Yes you are correct.

>
> Basically, I think the patch is fine, but the explanation seems a bit
> misleading. This isn't really about the re-trying: that would be fine
> if we just cleaned up sock properly.
>
> Can you confirm that? I don't know where the original report is.

Yes of course, setting 'sock' to NULL before 'goto retry' is sufficient
to fix it, that is in fact my initial thought. And I realized retry'ing
fdget() can't help anything in this situation but increases the
attack vector, so I decided to get rid of it from the retry loop
instead of just NULL'ing 'sock'.

Or do you prefer the simpler fix? Or should I just resend it with
a improved changelog?

BTW, the original report is here:
https://groups.google.com/forum/#!topic/syzkaller/QsmbsGoYPzA


>
> And that code is ancient, so we should do a "cc: stable" there too,
> and backport it basically forever. I think most of the code in this
> area predates the git tree, although Al Viro actually touched some
> things around here very recently to make the compat case cleaner.
>

Yeah, sorry about forgetting it.

Thanks!

Re: [Patch] mqueue: fix the retry logic for netlink_attachskb()

[Linus Torvalds]

On Sat, Jul 8, 2017 at 11:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> Can you confirm that? I don't know where the original report is.
>
> Yes of course, setting 'sock' to NULL before 'goto retry' is sufficient
> to fix it, that is in fact my initial thought. And I realized retry'ing
> fdget() can't help anything in this situation but increases the
> attack vector, so I decided to get rid of it from the retry loop
> instead of just NULL'ing 'sock'.
>
> Or do you prefer the simpler fix? Or should I just resend it with
> a improved changelog?

It was just the combination of that nasty code, your patch, and the
explanation that confused me.

Reading the patch, I actually thought that one of the things you fixed
was moving the "fdput()" later, to after the netlink_attachskb().

And I thought you did that because netlink_attachskb() would need the
file to be still around keeping a reference count to the socket, and
without it the socket could have been dropped in the meantime.

But reading the code more closely, I notice that
netlink_getsockbyfilp() gets a reference to the sock, and it's that
netlink_attachskb() will drop that reference on error or retry.

So the fdput() makes sense after netlink_getsockbyfilp(), but that's
also why the retry code currently includes repeating the fdget()...
And the error handling for the fdget is that then triggers the real
bug.

So the reason you moved the fdput() later wasn't to protect the socket
reference, it was just because of how the whole retry loop needs to
have the file descriptor just to get a new reference to the socket.

That's why I thought you fixed a bug even in the first iteration, but
it turns out that was just me making assumptions based on mis-reading
the patch without looking at all the context and the logic of the
called functions.

Now that I have checked deeper, I realize that your patch description
was actually correct about this only being a retry problem - the first
time around the reference count ends up moving correctly from file to
socket, but then when it repeats and 'sock' may contain a stale
pointer, we may end up doing the wrong thing when the fdget fails.

Honestly, now I feel like either patch is fine, and your original
commit message is fine too - but I just hate that code.

And making it use some nice helper function to clean it up looks
painful too, because the error handling is so odd (ie
mq_netlink_attachskb() will free the skb on error, while the other
error cases won't, so you'd have to have some special handling for the
different errors that can happen).

Honestly, this code is nasty, and right now my feeling is that it
would be good to have a minimal patch that also backports cleanly.
Maybe somebody can clean it up later, but that's a separate windmill
to rail against.

And due to the recent compat cleanups by Al, your bigger patch does
not apply cleanly to current git - but the smaller patch to just
setting 'sock' to NULL before that 'goto retry' should apply cleanly
to all versions of this code.

So purely because of that reason, I think I'd prefer to see that
smaller patch instead. Would you mind re-sending the thing?

Sorry about the whole confusion.

               Linus

Re: [Patch] mqueue: fix the retry logic for netlink_attachskb()

[Cong Wang]

On Sat, Jul 8, 2017 at 11:55 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Jul 8, 2017 at 11:04 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>
>>> Can you confirm that? I don't know where the original report is.
>>
>> Yes of course, setting 'sock' to NULL before 'goto retry' is sufficient
>> to fix it, that is in fact my initial thought. And I realized retry'ing
>> fdget() can't help anything in this situation but increases the
>> attack vector, so I decided to get rid of it from the retry loop
>> instead of just NULL'ing 'sock'.
>>
>> Or do you prefer the simpler fix? Or should I just resend it with
>> a improved changelog?
>
> It was just the combination of that nasty code, your patch, and the
> explanation that confused me.
>
> Reading the patch, I actually thought that one of the things you fixed
> was moving the "fdput()" later, to after the netlink_attachskb().
>
> And I thought you did that because netlink_attachskb() would need the
> file to be still around keeping a reference count to the socket, and
> without it the socket could have been dropped in the meantime.
>
> But reading the code more closely, I notice that
> netlink_getsockbyfilp() gets a reference to the sock, and it's that
> netlink_attachskb() will drop that reference on error or retry.
>
> So the fdput() makes sense after netlink_getsockbyfilp(), but that's
> also why the retry code currently includes repeating the fdget()...
> And the error handling for the fdget is that then triggers the real
> bug.
>
> So the reason you moved the fdput() later wasn't to protect the socket
> reference, it was just because of how the whole retry loop needs to
> have the file descriptor just to get a new reference to the socket.
>
> That's why I thought you fixed a bug even in the first iteration, but
> it turns out that was just me making assumptions based on mis-reading
> the patch without looking at all the context and the logic of the
> called functions.
>
> Now that I have checked deeper, I realize that your patch description
> was actually correct about this only being a retry problem - the first
> time around the reference count ends up moving correctly from file to
> socket, but then when it repeats and 'sock' may contain a stale
> pointer, we may end up doing the wrong thing when the fdget fails.
>
> Honestly, now I feel like either patch is fine, and your original
> commit message is fine too - but I just hate that code.
>
> And making it use some nice helper function to clean it up looks
> painful too, because the error handling is so odd (ie
> mq_netlink_attachskb() will free the skb on error, while the other
> error cases won't, so you'd have to have some special handling for the
> different errors that can happen).
>
> Honestly, this code is nasty, and right now my feeling is that it
> would be good to have a minimal patch that also backports cleanly.
> Maybe somebody can clean it up later, but that's a separate windmill
> to rail against.


Indeed, I spent a few hours to spot this bug, to be honest. ;) We can
always clean up things later.

>
> And due to the recent compat cleanups by Al, your bigger patch does
> not apply cleanly to current git - but the smaller patch to just
> setting 'sock' to NULL before that 'goto retry' should apply cleanly
> to all versions of this code.

Ah, understood, probably because I don't pull your branch so often
but now we are still in merge window...

>
> So purely because of that reason, I think I'd prefer to see that
> smaller patch instead. Would you mind re-sending the thing?

Sure, I will rebase and send the simpler fix.

>
> Sorry about the whole confusion.
>

No worries. Thanks for all the details!