| 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
| ||
|
Message-ID: <87y1rmhyc2.fsf@cloudflare.com>
Date: Mon, 05 Dec 2022 11:24:39 +0100
From: Jakub Sitnicki <jakub@...udflare.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Tom Parkin <tparkin@...alix.com>,
Haowei Yan <g1042620637@...il.com>,
Roopa Prabhu <roopa@...dia.com>,
Nikolay Aleksandrov <nikolay@...dia.com>
Subject: Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with
sk_callback_lock
On Fri, Dec 02, 2022 at 05:50 PM +08, Hangbin Liu wrote:
> On Mon, Nov 14, 2022 at 08:16:19PM +0100, Jakub Sitnicki wrote:
>> sk->sk_user_data has multiple users, which are not compatible with each
>> other. Writers must synchronize by grabbing the sk->sk_callback_lock.
>>
>> l2tp currently fails to grab the lock when modifying the underlying tunnel
>> socket fields. Fix it by adding appropriate locking.
>>
>> We err on the side of safety and grab the sk_callback_lock also inside the
>> sk_destruct callback overridden by l2tp, even though there should be no
>> refs allowing access to the sock at the time when sk_destruct gets called.
>>
>> v4:
>> - serialize write to sk_user_data in l2tp sk_destruct
>>
>> v3:
>> - switch from sock lock to sk_callback_lock
>> - document write-protection for sk_user_data
>>
>> v2:
>> - update Fixes to point to origin of the bug
>> - use real names in Reported/Tested-by tags
>>
>> Cc: Tom Parkin <tparkin@...alix.com>
>> Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")
>> Reported-by: Haowei Yan <g1042620637@...il.com>
>> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
>> ---
>>
>> This took me forever. Sorry about that.
>>
>> include/net/sock.h | 2 +-
>> net/l2tp/l2tp_core.c | 19 +++++++++++++------
>> 2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 5db02546941c..e0517ecc6531 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -323,7 +323,7 @@ struct sk_filter;
>> * @sk_tskey: counter to disambiguate concurrent tstamp requests
>> * @sk_zckey: counter to order MSG_ZEROCOPY notifications
>> * @sk_socket: Identd and reporting IO signals
>> - * @sk_user_data: RPC layer private data
>> + * @sk_user_data: RPC layer private data. Write-protected by @sk_callback_lock.
>> * @sk_frag: cached page frag
>> * @sk_peek_off: current peek_offset value
>> * @sk_send_head: front of stuff to transmit
>> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
>> index 7499c51b1850..754fdda8a5f5 100644
>> --- a/net/l2tp/l2tp_core.c
>> +++ b/net/l2tp/l2tp_core.c
>> @@ -1150,8 +1150,10 @@ static void l2tp_tunnel_destruct(struct sock *sk)
>> }
>>
>> /* Remove hooks into tunnel socket */
>> + write_lock_bh(&sk->sk_callback_lock);
>> sk->sk_destruct = tunnel->old_sk_destruct;
>> sk->sk_user_data = NULL;
>> + write_unlock_bh(&sk->sk_callback_lock);
>>
>> /* Call the original destructor */
>> if (sk->sk_destruct)
>
> Hi Jakub,
>
> I have a similar issue with vxlan driver. Similar with commit
> ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link
> delete"). There is still a race condition on vxlan that when receive a packet
> while deleting a VXLAN device. In vxlan_ecn_decapsulate(), the
> vxlan_get_sk_family() call panic as sk is NULL.
>
> #0 [ffffa25ec6978a38] machine_kexec at ffffffff8c669757
> #1 [ffffa25ec6978a90] __crash_kexec at ffffffff8c7c0a4d
> #2 [ffffa25ec6978b58] crash_kexec at ffffffff8c7c1c48
> #3 [ffffa25ec6978b60] oops_end at ffffffff8c627f2b
> #4 [ffffa25ec6978b80] page_fault_oops at ffffffff8c678fcb
> #5 [ffffa25ec6978bd8] exc_page_fault at ffffffff8d109542
> #6 [ffffa25ec6978c00] asm_exc_page_fault at ffffffff8d200b62
> [exception RIP: vxlan_ecn_decapsulate+0x3b]
> RIP: ffffffffc1014e7b RSP: ffffa25ec6978cb0 RFLAGS: 00010246
> RAX: 0000000000000008 RBX: ffff8aa000888000 RCX: 0000000000000000
> RDX: 000000000000000e RSI: ffff8a9fc7ab803e RDI: ffff8a9fd1168700
> RBP: ffff8a9fc7ab803e R8: 0000000000700000 R9: 00000000000010ae
> R10: ffff8a9fcb748980 R11: 0000000000000000 R12: ffff8a9fd1168700
> R13: ffff8aa000888000 R14: 00000000002a0000 R15: 00000000000010ae
> ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> #7 [ffffa25ec6978ce8] vxlan_rcv at ffffffffc10189cd [vxlan]
> #8 [ffffa25ec6978d90] udp_queue_rcv_one_skb at ffffffff8cfb6507
> #9 [ffffa25ec6978dc0] udp_unicast_rcv_skb at ffffffff8cfb6e45
> #10 [ffffa25ec6978dc8] __udp4_lib_rcv at ffffffff8cfb8807
> #11 [ffffa25ec6978e20] ip_protocol_deliver_rcu at ffffffff8cf76951
> #12 [ffffa25ec6978e48] ip_local_deliver at ffffffff8cf76bde
> #13 [ffffa25ec6978ea0] __netif_receive_skb_one_core at ffffffff8cecde9b
> #14 [ffffa25ec6978ec8] process_backlog at ffffffff8cece139
> #15 [ffffa25ec6978f00] __napi_poll at ffffffff8ceced1a
> #16 [ffffa25ec6978f28] net_rx_action at ffffffff8cecf1f3
> #17 [ffffa25ec6978fa0] __softirqentry_text_start at ffffffff8d4000ca
> #18 [ffffa25ec6978ff0] do_softirq at ffffffff8c6fbdc3
> --- <IRQ stack> ---
>
>> struct socket ffff8a9fd1168700
> struct socket {
> state = SS_FREE,
> type = 0,
> flags = 0,
> file = 0xffff8a9fcb748000,
> sk = 0x0,
> ops = 0x0,
>
> So I'm wondering if we should also have locks in udp_tunnel_sock_release().
> Or should we add a checking in sk state before calling vxlan_get_sk_family()?
This is how like to think about it:
To know when it is safe to load vs->sock->sk->sk_family, we have to ask:
1. What ensures that the objects remain alive/valid in our scope?
2. What protects the objects from being mutated?
In case of vxlan_sock object in the context of vxlan_ecn_decapsulate():
1. We are in an RCU read side section (ip_local_deliver_finish).
2. RCU-protected objects are not to be mutated while readers exist.
The classic "What is RCU, Fundamentally?" article explains it much
better than I ever could:
https://lwn.net/Articles/262464/
As to where the problem lies. I belive udp_tunnel_sock_release() is not
keeping the (2) promise.
After unpublishing the sk_user_data, we should wait for any existing
readers accessing the vxlan_sock to finish with synchronize_rcu(),
before releaseing the socket.
That is:
--- a/net/ipv4/udp_tunnel_core.c
+++ b/net/ipv4/udp_tunnel_core.c
@@ -176,6 +176,7 @@ EXPORT_SYMBOL_GPL(udp_tunnel_xmit_skb);
void udp_tunnel_sock_release(struct socket *sock)
{
rcu_assign_sk_user_data(sock->sk, NULL);
+ synchronize_rcu();
kernel_sock_shutdown(sock, SHUT_RDWR);
sock_release(sock);
}
Otherwise accessing vxlan_sock state doesn't look safe to me.
Powered by blists - more mailing lists