diff mbox series

[net,v3] mptcp: pm: Fix uaf in __timer_delete_sync

Message ID tencent_F85DEC5DED99554FB28DEF258F8DB8120D07@qq.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,v3] mptcp: pm: Fix uaf in __timer_delete_sync | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 35 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Edward Adam Davis Sept. 5, 2024, 12:27 p.m. UTC
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).

Keeping a reference to add_timer inside the lock, and calling
sk_stop_timer_sync() with this reference, instead of "entry->add_timer".

Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
Cc: stable@vger.kernel.org
Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 net/mptcp/pm_netlink.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Matthieu Baerts (NGI0) Sept. 6, 2024, 6:55 p.m. UTC | #1
Hi Edward,

On 05/09/2024 14:27, 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).
> 
> Keeping a reference to add_timer inside the lock, and calling
> sk_stop_timer_sync() with this reference, instead of "entry->add_timer".
> 
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Cc: stable@vger.kernel.org
> Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

According to the doc [1], a 'Co-dev' tag is supposed to be added before
this SoB:

Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

But I'm sure that's fine without it.

[1]
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by


This v3 now looks good to me. I don't know if it needs to be reviewed by
someone else as I'm now listed as co-developed.

Cheers,
Matt
Jakub Kicinski Sept. 6, 2024, 10:02 p.m. UTC | #2
On Fri, 6 Sep 2024 20:55:20 +0200 Matthieu Baerts wrote:
> > Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> > Cc: stable@vger.kernel.org
> > Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>  
> 
> According to the doc [1], a 'Co-dev' tag is supposed to be added before
> this SoB:
> 
> Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> But I'm sure that's fine without it.

To be clear, would you like us to pick this up directly for net?
Or you'll send it back to us with other MPTCP patches?
Matthieu Baerts (NGI0) Sept. 9, 2024, 7:01 a.m. UTC | #3
Hi Jakub,

On 07/09/2024 00:02, Jakub Kicinski wrote:
> On Fri, 6 Sep 2024 20:55:20 +0200 Matthieu Baerts wrote:
>>> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
>>> Cc: stable@vger.kernel.org
>>> Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>  
>>
>> According to the doc [1], a 'Co-dev' tag is supposed to be added before
>> this SoB:
>>
>> Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>
>> But I'm sure that's fine without it.
> 
> To be clear, would you like us to pick this up directly for net?

Sorry, I forgot that: yes, can you pick this up directly please?

I think that's best to do that for fixes that are ready.

Cheers,
Matt
Paolo Abeni Sept. 9, 2024, 1:07 p.m. UTC | #4
On 9/5/24 14:27, 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).
> 
> Keeping a reference to add_timer inside the lock, and calling
> sk_stop_timer_sync() with this reference, instead of "entry->add_timer".
> 
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Cc: stable@vger.kernel.org
> Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>   net/mptcp/pm_netlink.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 3e4ad801786f..7ddb373cc6ad 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -329,17 +329,21 @@ struct mptcp_pm_add_entry *
>   mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>   		       const struct mptcp_addr_info *addr, bool check_id)
>   {
> -	struct mptcp_pm_add_entry *entry;
>   	struct sock *sk = (struct sock *)msk;
> +	struct timer_list *add_timer = NULL;
> +	struct mptcp_pm_add_entry *entry;
>   
>   	spin_lock_bh(&msk->pm.lock);
>   	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> -	if (entry && (!check_id || entry->addr.id == addr->id))
> +	if (entry && (!check_id || entry->addr.id == addr->id)) {
>   		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> +		add_timer = &entry->add_timer;
> +	}
>   	spin_unlock_bh(&msk->pm.lock);
>   
> -	if (entry && (!check_id || entry->addr.id == addr->id))
> -		sk_stop_timer_sync(sk, &entry->add_timer);
> +	/* no lock, because sk_stop_timer_sync() is calling del_timer_sync() */
> +	if (add_timer)
> +		sk_stop_timer_sync(sk, add_timer);
>   
>   	return entry;
>   }
> @@ -1430,8 +1434,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);

I'm sorry for the late feedback.

I think this is not enough to fix races for good, i.e.

mptcp_nl_remove_subflow_and_signal_addr() -> mptcp_pm_remove_anno_addr()
-> remove_anno_list_by_saddr()

could race with:

mptcp_pm_remove_addrs() -> remove_anno_list_by_saddr()

and both CPUs could see the same 'entry' returned by
mptcp_pm_del_add_timer().

I think the list_del() in remove_anno_list_by_saddr() should moved under
the pm lock protection inside mptcp_pm_del_add_timer(), and no need to 
add spin_lock_bh() around the kfree call.

Thanks,

Paolo
Edward Adam Davis Sept. 10, 2024, 3:59 a.m. UTC | #5
On Mon, 9 Sep 2024 15:07:21 +0200, Paolo Abeni wrote:
> On 9/5/24 14:27, 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).
> > 
> > Keeping a reference to add_timer inside the lock, and calling
> > sk_stop_timer_sync() with this reference, instead of "entry->add_timer".
> > 
> > Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> > Cc: stable@vger.kernel.org
> > Reported-and-tested-by: syzbot+f3a31fb909db9b2a5c4d@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=f3a31fb909db9b2a5c4d
> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >   net/mptcp/pm_netlink.c | 14 ++++++++++----
> >   1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 3e4ad801786f..7ddb373cc6ad 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -329,17 +329,21 @@ struct mptcp_pm_add_entry *
> >   mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> >   		       const struct mptcp_addr_info *addr, bool check_id)
> >   {
> > -	struct mptcp_pm_add_entry *entry;
> >   	struct sock *sk = (struct sock *)msk;
> > +	struct timer_list *add_timer = NULL;
> > +	struct mptcp_pm_add_entry *entry;
> >   
> >   	spin_lock_bh(&msk->pm.lock);
> >   	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> > -	if (entry && (!check_id || entry->addr.id == addr->id))
> > +	if (entry && (!check_id || entry->addr.id == addr->id)) {
> >   		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> > +		add_timer = &entry->add_timer;
> > +	}
> >   	spin_unlock_bh(&msk->pm.lock);
> >   
> > -	if (entry && (!check_id || entry->addr.id == addr->id))
> > -		sk_stop_timer_sync(sk, &entry->add_timer);
> > +	/* no lock, because sk_stop_timer_sync() is calling del_timer_sync() */
> > +	if (add_timer)
> > +		sk_stop_timer_sync(sk, add_timer);
> >   
> >   	return entry;
> >   }
> > @@ -1430,8 +1434,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);
> 
> I'm sorry for the late feedback.
> 
> I think this is not enough to fix races for good, i.e.
> 
> mptcp_nl_remove_subflow_and_signal_addr() -> mptcp_pm_remove_anno_addr()
> -> remove_anno_list_by_saddr()
> 
> could race with:
> 
> mptcp_pm_remove_addrs() -> remove_anno_list_by_saddr()
> 
> and both CPUs could see the same 'entry' returned by
> mptcp_pm_del_add_timer().
Yes, you are right.
> 
> I think the list_del() in remove_anno_list_by_saddr() should moved under
> the pm lock protection inside mptcp_pm_del_add_timer(), and no need to 
> add spin_lock_bh() around the kfree call.
Agreed, I will update the patch.

BR,
Edward
diff mbox series

Patch

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3e4ad801786f..7ddb373cc6ad 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -329,17 +329,21 @@  struct mptcp_pm_add_entry *
 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 		       const struct mptcp_addr_info *addr, bool check_id)
 {
-	struct mptcp_pm_add_entry *entry;
 	struct sock *sk = (struct sock *)msk;
+	struct timer_list *add_timer = NULL;
+	struct mptcp_pm_add_entry *entry;
 
 	spin_lock_bh(&msk->pm.lock);
 	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
-	if (entry && (!check_id || entry->addr.id == addr->id))
+	if (entry && (!check_id || entry->addr.id == addr->id)) {
 		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
+		add_timer = &entry->add_timer;
+	}
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry && (!check_id || entry->addr.id == addr->id))
-		sk_stop_timer_sync(sk, &entry->add_timer);
+	/* no lock, because sk_stop_timer_sync() is calling del_timer_sync() */
+	if (add_timer)
+		sk_stop_timer_sync(sk, add_timer);
 
 	return entry;
 }
@@ -1430,8 +1434,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);
 		return true;
 	}