netdev - [RFC PATCH] net, sock.h: Make sure accesses to a fullsock when it is indeed one

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: <20240228074308.3623984-2-zegao@tencent.com>
Date: Wed, 28 Feb 2024 02:43:09 -0500
From: Ze Gao <zegao2021@...il.com>
To: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>
Cc: linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	Ze Gao <zegao@...cent.com>,
	Honglin Li <honglinli@...cent.com>
Subject: [RFC PATCH] net, sock.h: Make sure accesses to a fullsock when it is indeed one

We know a pointer that has type struct sock* can actually points to
one of some different sock types which have different memory layouts,
take req_to_sk() for example, and whether a sock is full or not
depends upon ->sk_state which is a shared field among them so that we
see some repeated code pattern similar to this:

	if (sk && sk fullsock(sk) && sk->field_not_shared)

which seems to have no problem at the first glance, but it is actually
unsound in a way that ->field_not_shared is likely uninitialized (or
unmapped) when it's not a full sock, and a compiler is free to reorder
accesses to fields of a struct sock when it can, that is, it could
reorder accesses to ->field_not_shared across ->sk_state or load them
all before the branch test, which leads to unexpected behavior, although
most of them won't do this.

So leave a barrier() in between and force the compiler to keep the
obvious program order.

Cc: Honglin Li <honglinli@...cent.com>
Signed-off-by: Ze Gao <zegao@...cent.com>
---

IIUC, casting a pointer to refer to a bigger object in size is
technically UB, which may lead to unsound code. From the POV of
a compiler, when it is allowed to assume that one struct member
is valid, they all are through a pointer, and thus it's likely
for the compiler to do such optimizations and reorder what we
want to keep in order.

Note this is not a typical way to use barrier(), which only
acts an ok fix to what's already unsound, at least IMO.

Comments are welcome, since I'm not an expert in C and I know
most of compilers won't do this reorder, but I'm being pessimistic
here.

Happy to learn from your sage insights and better solutions (or
no solutions at all if this is indeed not a problem in the first
place)

Regards,
        -- Ze

 include/net/sock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 92f7ea62a915..f7e3960cb5fc 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2815,7 +2815,14 @@ skb_sk_is_prefetched(struct sk_buff *skb)
  */
 static inline bool sk_fullsock(const struct sock *sk)
 {
-	return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV);
+	bool ret = (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV);
+
+	/*
+	 * Make sure all accesses to a full sock happens right
+	 * after ->sk_state.
+	 */
+	barrier();
+	return ret;
 }

 static inline bool
-- 
2.41.0