[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e4a13002-f471-4951-9180-14f0f8b30bd2@kernel.org>
Date: Wed, 4 Sep 2024 22:39:10 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Edward Adam Davis <eadavis@...com>
Cc: davem@...emloft.net, edumazet@...gle.com, geliang@...nel.org,
kuba@...nel.org, linux-kernel@...r.kernel.org, martineau@...nel.org,
mptcp@...ts.linux.dev, netdev@...r.kernel.org, pabeni@...hat.com,
syzbot+f3a31fb909db9b2a5c4d@...kaller.appspotmail.com,
syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V2] mptcp: pm: Fix uaf in __timer_delete_sync
Hello,
Thank you for this patch!
On 04/09/2024 03:01, Edward Adam Davis wrote:
> There are two paths to access mptcp_pm_del_add_timer, result in a race
> condition:
>
> CPU1 CPU2
> ==== ====
> net_rx_action
> napi_poll netlink_sendmsg
> __napi_poll netlink_unicast
> process_backlog netlink_unicast_kernel
> __netif_receive_skb genl_rcv
> __netif_receive_skb_one_core netlink_rcv_skb
> NF_HOOK genl_rcv_msg
> ip_local_deliver_finish genl_family_rcv_msg
> ip_protocol_deliver_rcu genl_family_rcv_msg_doit
> tcp_v4_rcv mptcp_pm_nl_flush_addrs_doit
> tcp_v4_do_rcv mptcp_nl_remove_addrs_list
> tcp_rcv_established mptcp_pm_remove_addrs_and_subflows
> tcp_data_queue remove_anno_list_by_saddr
> mptcp_incoming_options mptcp_pm_del_add_timer
> mptcp_pm_del_add_timer kfree(entry)
>
> In remove_anno_list_by_saddr(running on CPU2), after leaving the critical
> zone protected by "pm.lock", the entry will be released, which leads to the
> occurrence of uaf in the mptcp_pm_del_add_timer(running on CPU1).
>
> Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
Please add a Fixes tag and Cc stable.
And add 'net' after PATCH in the subject:
[PATCH net v3]
> Signed-off-by: Edward Adam Davis <eadavis@...com>
> ---
> net/mptcp/pm_netlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 3e4ad801786f..d4cbf7dcf983 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1430,8 +1430,10 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
>
> entry = mptcp_pm_del_add_timer(msk, addr, false);
> if (entry) {
> + spin_lock_bh(&msk->pm.lock);
> list_del(&entry->list);
> kfree(entry);
> + spin_unlock_bh(&msk->pm.lock);
Mmh, I can understand it would help to reduce issues here, but I don't
think that's enough: in mptcp_pm_del_add_timer(), CPU1 can get the entry
from the list under the lock, then immediately after, the free can
happen on CPU2, while CPU1 is trying to access entry->add_timer outside
the lock, no? Something like this:
CPU1 CPU2
==== ====
entry = (...)
kfree(entry)
entry->add_timer
What about keeping a reference to add_timer inside the lock, and calling
sk_stop_timer_sync() with this reference, instead of "entry->add_timer"?
I'm thinking about something like that to be applied *on top* of your
patch, WDYT?
https://lore.kernel.org/20240904170517.237863-2-matttbe@kernel.org
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Powered by blists - more mailing lists