2024-05-21 10:35:04

by Yewon Choi

Subject: [PATCH net v2] tls: fix missing memory barrier in tls_init

From: Dae R. Jeong <[email protected]>

In tls_init(), a write memory barrier is missing, and store-store
reordering may cause NULL dereference in tls_{setsockopt,getsockopt}.

CPU0 CPU1
----- -----
// In tls_init()
// In tls_ctx_create()
ctx = kzalloc()
ctx->sk_proto = READ_ONCE(sk->sk_prot) -(1)

// In update_sk_prot()
WRITE_ONCE(sk->sk_prot, tls_prots) -(2)

// In sock_common_setsockopt()
READ_ONCE(sk->sk_prot)->setsockopt()

// In tls_{setsockopt,getsockopt}()
ctx->sk_proto->setsockopt() -(3)

In the above scenario, when (1) and (2) are reordered, (3) can observe
the NULL value of ctx->sk_proto, causing NULL dereference.

To fix it, we rely on rcu_assign_pointer() which implies the release
barrier semantic. By moving rcu_assign_pointer() after ctx->sk_proto is
initialized, we can ensure that ctx->sk_proto are visible when
changing sk->sk_prot.

Fixes: d5bee7374b68 ("net/tls: Annotate access to sk_prot with READ_ONCE/WRITE_ONCE")
Signed-off-by: Yewon Choi <[email protected]>
Signed-off-by: Dae R. Jeong <[email protected]>
Link: https://lore.kernel.org/netdev/ZU4OJG56g2V9z_H7@dragonet/T/
---
v2:
- We don't get rid of tls_ctx_create() because it is called in multiple
places (tls_init(), tls_toe_bypass()). Instead, just move
rcu_assign_pointer() to the last of tls_ctx_create(). If needed, removing
tls_ctx_create() can be considered as later patch.
- Added Fixes tag
v1: https://lore.kernel.org/all/ZU4Mk_RfzvRpwkmX@dragonet/

net/tls/tls_main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index b4674f03d71a..90b7f253d363 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -816,9 +816,17 @@ struct tls_context *tls_ctx_create(struct sock *sk)
return NULL;

mutex_init(&ctx->tx_lock);
- rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
ctx->sk_proto = READ_ONCE(sk->sk_prot);
ctx->sk = sk;
+ /* Release semantic of rcu_assign_pointer() ensures that
+ * ctx->sk_proto is visible before changing sk->sk_prot in
+ * update_sk_prot(), and prevents reading uninitialized value in
+ * tls_{getsockopt, setsockopt}. Note that we do not need a
+ * read barrier in tls_{getsockopt,setsockopt} as there is an
+ * address dependency between sk->sk_proto->{getsockopt,setsockopt}
+ * and ctx->sk_proto.
+ */
+ rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
return ctx;
}

--
2.43.0

2024-05-23 10:10:46

by patchwork-bot+netdevbpf

Subject: Re: [PATCH net v2] tls: fix missing memory barrier in tls_init

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

On Tue, 21 May 2024 19:34:38 +0900 you wrote:
> From: Dae R. Jeong <[email protected]>
>
> In tls_init(), a write memory barrier is missing, and store-store
> reordering may cause NULL dereference in tls_{setsockopt,getsockopt}.
>
> CPU0 CPU1
>
> [...]

Here is the summary with links:
- [net,v2] tls: fix missing memory barrier in tls_init
https://git.kernel.org/netdev/net/c/91e61dd7a0af

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html