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>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240221051608.43241-1-dmantipov@yandex.ru>
Date: Wed, 21 Feb 2024 08:16:08 +0300
From: Dmitry Antipov <dmantipov@...dex.ru>
To: Wenjia Zhang <wenjia@...ux.ibm.com>
Cc: Jan Karcher <jaka@...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()

I've tracked https://syzkaller.appspot.com/bug?extid=5f1acda7e06a2298fae6
down to the problem which may be illustrated by the following pseudocode:

int sock;

/* thread 1 */

while (1) {
       struct msghdr msg = { ... };
       sock = socket(AF_SMC, SOCK_STREAM, 0);
       sendmsg(sock, &msg, MSG_FASTOPEN);
       close(sock);
}

/* thread 2 */

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

That is, something in thread 1 may cause 'smc_switch_to_fallback()' and
swap kernel sockets (of 'struct smc_sock') behind 'sock' between 'ioctl()'
calls in thread 2, so this becomes an attempt to add fasync entry to one
socket but remove from another one. 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 we end up with leaked 'struct fasync_struct' object
linked to the base socket, and this object is noticed by '__sock_release()'
("fasync list not empty"). Of course using 'fasync_remove_entry()' in such
a way is extremely ugly, but what else we can do without touching generic
socket code, '__fput()', etc.? Comments are highly appreciated.

Signed-off-by: Dmitry Antipov <dmantipov@...dex.ru>
---
 net/smc/af_smc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 0f53a5c6fd9d..68cde9db5d2f 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -337,9 +337,13 @@ static int smc_release(struct socket *sock)
 	else
 		lock_sock(sk);

-	if (old_state == SMC_INIT && sk->sk_state == SMC_ACTIVE &&
-	    !smc->use_fallback)
+	if (smc->use_fallback) {
+		/* FIXME: ugly and should be done in some other way */
+		if (sock->wq.fasync_list)
+			fasync_remove_entry(sock->file, &sock->wq.fasync_list);
+	} else if (old_state == SMC_INIT && sk->sk_state == SMC_ACTIVE) {
 		smc_close_active_abort(smc);
+	}

 	rc = __smc_release(smc);

-- 
2.43.2