linux-kernel - net/smc: Buggy reordering scenario in smc socket

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]

Date: Mon, 15 Apr 2024 20:02:05 +0900
From: Yewon Choi <woni9911@...il.com>
To: Wenjia Zhang <wenjia@...ux.ibm.com>, Jan Karcher <jaka@...ux.ibm.com>,
	"D. Wythe" <alibuda@...ux.alibaba.com>,
	Tony Lu <tonylu@...ux.alibaba.com>,
	Wen Gu <guwen@...ux.alibaba.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-s390@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Cc: "Dae R. Jeong" <threeearcat@...il.com>
Subject: net/smc: Buggy reordering scenario in smc socket

Hello,
we suspect some buggy scenario due to memory reordering in concurrent execution 
of setsockopt() and sendmmsg().

(CPU 1) setsockopt():
    case TCP_FASTOPEN_NO_COOKIE:
        ...
        smc_switch_to_fallback():
            clcsock->file = sk.sk_socket->file; // (1)
            clcsock->file->private_data = clcsock; // (2)

(CPU 2) __sys_sendmmsg():
    sockfd_lookup_light():
        sock_from_file():
            sock = file->private_data; // (3)
    ...
    fput_light(sock->file, fput_needed): // (4)
        fput():
            refcount_dec_and_test(sock->file->f_count) // null-ptr-deref

There is no memory barrier between (1) and (2), so (1) might be reordered after 
(2) is written to memory. Then, execution order can be (2)->(3)->(4)->(1) 
and (4) will read uninitialized value which may cause system crash.


This kind of reordering may happen in smc_ulp_init():

(CPU 1) smc_ulp_init():
    ...
    smcsock->file = tcp->file; // (5)
	smcsock->file->private_data = smcsock; // (6)

Execution order can be (6)->(3)->(4)->(5), showing same symptom as above.


One possible solution seems to be adding release semantic in (2) and (6).

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 4b52b3b159c0..37c23ef3e2d5 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -921,7 +921,7 @@ static int smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
        trace_smc_switch_to_fallback(smc, 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;
+               smp_store_release(&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;
@@ -3410,7 +3410,7 @@ static int smc_ulp_init(struct sock *sk)
 
        /* replace tcp socket to smc */
        smcsock->file = tcp->file;
-       smcsock->file->private_data = smcsock;
+       smp_store_release(&smcsock->file->private_data, smcsock);
        smcsock->file->f_inode = SOCK_INODE(smcsock); /* replace inode when sock_close */
        smcsock->file->f_path.dentry->d_inode = SOCK_INODE(smcsock); /* dput() in __fput */
        tcp->file = NULL;

I think we don't need memory barrier between (3) and (4) because there are
critical section between (3) and (4), so lock(lock_sock/release_sock) will do this.


Could you check these? If confirmed to be a bug, we will send a patch.

Best Regards,
Yewon Choi