netdev - [PATCH] [RFC] net: smc: fix fasync leak in smc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20240306134424.64314-1-dmantipov@yandex.ru>
Date: Wed,  6 Mar 2024 16:44:24 +0300
From: Dmitry Antipov <dmantipov@...dex.ru>
To: Jan Karcher <jaka@...ux.ibm.com>
Cc: Wen Gu <guwen@...ux.alibaba.com>,
	Wenjia Zhang <wenjia@...ux.ibm.com>,
	linux-s390@...r.kernel.org,
	netdev@...r.kernel.org,
	lvc-project@...uxtesting.org,
	Dmitry Antipov <dmantipov@...dex.ru>
Subject: [PATCH] [RFC] net: smc: fix fasync leak in smc_release()

https://syzkaller.appspot.com/bug?extid=5f1acda7e06a2298fae6 may be
tracked down to the problem illustrated by the following example:

int sock;

void *loop0(void *arg) {
        struct msghdr msg = {0};

        while (1) {
                sock = socket(AF_SMC, SOCK_STREAM, 0);
                sendmsg(sock, &msg, MSG_FASTOPEN);
                close(sock);
        }
        return NULL;
}

void *loop1(void *arg) {
        int on;

        while (1) {
                on = 1;
                ioctl(sock, FIOASYNC, &on);
                on = 0;
                ioctl(sock, FIOASYNC, &on);
        }

        return NULL;
}

int main(int argc, char *argv[]) {
        pthread_t a, b;
        struct sigaction sa = {0};

        sa.sa_handler = SIG_IGN;
        sigaction(SIGIO, &sa, NULL);

        pthread_create(&a, NULL, loop0, NULL);
        pthread_create(&b, NULL, loop1, NULL);

        pthread_join(a, NULL);
        pthread_join(b, NULL);

        return 0;
}

Running the program above, kernel memory leak (of 'struct fasync_struct'
object) may be triggered by the following scenario:

Thread 0 (user space):      Thread 1 (kernel space):

int on;
...
on = 1;
ioctl(sock, FIOASYNC, &on);
                            ...
                            smc_switch_to_fallback()
                            ...
                            smc->clcsock->file = smc->sk.sk_socket->file;
                            smc->clcsock->file->private_data = smc->clcsock;
                            ...
on = 0;
ioctl(sock, FIOASYNC, &on);

That is, thread 1 may cause 'smc_switch_to_fallback()' and swap kernel
sockets (of 'struct smc_sock') behind 'sock' between 'ioctl()' calls
in thread 0, so thread 0 makes an attempt to add fasync entry to one
socket (base) but remove from another one (CLC). When 'sock' is closing,
'__fput()' calls 'f_op->fasync()' _before_ 'f_op->release()', and it's
too late to revert the trick performed by 'smc_switch_to_fallback()' in
'smc_release()' and below. Finally there is a leaked 'struct fasync_struct'
object linked to the base socket, and this object is noticed by
'__sock_release()' with "fasync list not empty" message.

This patch makes an attempt to address this issue by introducing the wait
queue shared between base and CLC sockets, and using this wait queue in
'sock_async()'. This guarantees that all FIOASYNC changes always touches
the same list of 'struct fasync_struct' objects and thus may be issued
anytime regardless of an underlying base to CLC socket switch performed
by 'smc_switch_to_fallback()'.

Signed-off-by: Dmitry Antipov <dmantipov@...dex.ru>
---
 include/net/sock.h |  4 ++++
 net/smc/af_smc.c   | 10 ++++++----
 net/smc/smc.h      |  3 +++
 net/socket.c       |  4 +++-
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 54ca8dcbfb43..01b17654c289 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -422,6 +422,10 @@ struct sock {
 		struct socket_wq	*sk_wq_raw;
 		/* public: */
 	};
+
+	/* special AF_SMC quirk */
+	struct socket_wq	*sk_wq_shared;
+
 #ifdef CONFIG_XFRM
 	struct xfrm_policy __rcu *sk_policy[2];
 #endif
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 0f53a5c6fd9d..a00b6ae02b48 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -922,10 +922,8 @@ static int smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
 	if (smc->sk.sk_socket && smc->sk.sk_socket->file) {
 		smc->clcsock->file = smc->sk.sk_socket->file;
 		smc->clcsock->file->private_data = smc->clcsock;
-		smc->clcsock->wq.fasync_list =
-			smc->sk.sk_socket->wq.fasync_list;
-		smc->sk.sk_socket->wq.fasync_list = NULL;
-
+		/* shared wq should be used instead */
+		WARN_ON(smc->sk.sk_socket->wq.fasync_list);
 		/* There might be some wait entries remaining
 		 * in smc sk->sk_wq and they should be woken up
 		 * as clcsock's wait queue is woken up.
@@ -3360,6 +3358,10 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
 		smc->clcsock = clcsock;
 	}
 
+	/* use shared wq for sock_async() */
+	sock->sk->sk_wq_shared = &smc->smc_wq;
+	smc->clcsock->sk->sk_wq_shared = &smc->smc_wq;
+
 out:
 	return rc;
 }
diff --git a/net/smc/smc.h b/net/smc/smc.h
index df64efd2dee8..7403b7b467da 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -287,6 +287,9 @@ struct smc_sock {				/* smc sock container */
 						/* protects clcsock of a listen
 						 * socket
 						 * */
+	struct socket_wq	smc_wq;		/* used by both sockets
+						 * in sock_fasync()
+						 */
 };
 
 #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
diff --git a/net/socket.c b/net/socket.c
index ed3df2f749bf..b16d45b8c875 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1437,11 +1437,13 @@ static int sock_fasync(int fd, struct file *filp, int on)
 {
 	struct socket *sock = filp->private_data;
 	struct sock *sk = sock->sk;
-	struct socket_wq *wq = &sock->wq;
+	struct socket_wq *wq;
 
 	if (sk == NULL)
 		return -EINVAL;
 
+	wq = unlikely(sk->sk_wq_shared) ? sk->sk_wq_shared : &sock->wq;
+
 	lock_sock(sk);
 	fasync_helper(fd, filp, on, &wq->fasync_list);
 
-- 
2.44.0