Re: net: use-after-free in recvmmsg

Em Fri, Jan 22, 2016 at 09:39:53PM +0100, Dmitry Vyukov escreveu:

While running syzkaller fuzzer I've hit the following use-after-free:

<SNIP>
 

Call Trace:
 [<ffffffff8175ea0e>] __asan_report_load8_noabort+0x3e/0x40
mm/kasan/report.c:295
 [<ffffffff851cc31a>] __sys_recvmmsg+0x6fa/0x7f0 net/socket.c:2261
 [<     inline     >] SYSC_recvmmsg net/socket.c:2281
 [<ffffffff851cc57f>] SyS_recvmmsg+0x16f/0x180 net/socket.c:2270
 [<ffffffff86332bb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
==================================================================

I cannot reproduce it, but looking at __sys_recvmmsg code, it seems
that sock is not necessary live after fput_light:

out_put:
    fput_light(sock->file, fput_needed);

    if (err == 0)
        return datagrams;

    if (datagrams != 0) {
        /*
         * We may return less entries than requested (vlen) if the
         * sock is non block and there aren't enough datagrams...
         */
        if (err != -EAGAIN) {
            /*
             * ... or  if recvmsg returns an error after we
             * received some datagrams, where we record the
             * error to return on the next call or if the
             * app asks about it using getsockopt(SO_ERROR).
             */
            sock->sk->sk_err = -err;
        }

        return datagrams;
    }

    return err;
}

I am on commit 30f05309bde49295e02e45c7e615f73aa4e0ccc2 (Jan 20).
Seems to be added in commit a2e2725541fad72416326798c2d7fa4dafb7d337
(Oct 2009).

Maybe this helps? Compile testing now...

diff --git a/net/socket.c b/net/socket.c
index 91c2de6f5020..03e57ad7ec9f 100644
--- a/net/socket.c
+++ b/net/socket.c

@@ -2240,31 +2240,31 @@ int __sys_recvmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 		cond_resched();
 	}
 
-out_put:
-	fput_light(sock->file, fput_needed);
-
 	if (err == 0)
-		return datagrams;
+		goto out_put;
 
-	if (datagrams != 0) {
+	if (datagrams == 0) {
+		datagrams = err;
+		goto out_put;
+	}
+
+	/*
+	 * We may return less entries than requested (vlen) if the
+	 * sock is non block and there aren't enough datagrams...
+	 */
+	if (err != -EAGAIN) {
 		/*
-		 * We may return less entries than requested (vlen) if the
-		 * sock is non block and there aren't enough datagrams...
+		 * ... or  if recvmsg returns an error after we
+		 * received some datagrams, where we record the
+		 * error to return on the next call or if the
+		 * app asks about it using getsockopt(SO_ERROR).
 		 */
-		if (err != -EAGAIN) {
-			/*
-			 * ... or  if recvmsg returns an error after we
-			 * received some datagrams, where we record the
-			 * error to return on the next call or if the
-			 * app asks about it using getsockopt(SO_ERROR).
-			 */
-			sock->sk->sk_err = -err;
-		}
-
-		return datagrams;
+		sock->sk->sk_err = -err;
 	}
+out_put:
+	fput_light(sock->file, fput_needed);
 
-	return err;
+	return datagrams;
 }
 
 SYSCALL_DEFINE5(recvmmsg, int, fd, struct mmsghdr __user *, mmsg,