Re: [PATCH V2] mptcp: pm: Fix uaf in __timer_delete_sync
From: Matthieu Baerts
Date: Wed Sep 04 2024 - 16:39:31 EST
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@xxxxxxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxx>
> ---
> 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@xxxxxxxxxx
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.