[v3] mptcp: pm: Fix uaf in __timer_delete_sync

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

Posted by Edward Adam Davis 2 months, 4 weeks ago

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);
 		return true;
 	}
 
-- 
2.43.0

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Paolo Abeni 2 months, 3 weeks ago

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

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Edward Adam Davis 2 months, 3 weeks ago

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

[PATCH net V4] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Edward Adam Davis 2 months, 3 weeks ago

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".

Move list_del(&entry->list) to mptcp_pm_del_add_timer and inside the pm lock,
do not directly access any members of the entry outside the pm lock, which
can avoid similar "entry->x" uaf.

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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3e4ad801786f..f195b577c367 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -331,15 +331,21 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 {
 	struct mptcp_pm_add_entry *entry;
 	struct sock *sk = (struct sock *)msk;
+	struct timer_list *add_timer = NULL;
 
 	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;
+	}
+	if (!check_id && entry)
+		list_del(&entry->list);
 	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,7 +1436,6 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
 
 	entry = mptcp_pm_del_add_timer(msk, addr, false);
 	if (entry) {
-		list_del(&entry->list);
 		kfree(entry);
 		return true;
 	}
-- 
2.43.0

Re: [PATCH net V4] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Matthieu Baerts 2 months, 3 weeks ago

Hi Edward,

On 10/09/2024 11:58, 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".
> 
> Move list_del(&entry->list) to mptcp_pm_del_add_timer and inside the pm lock,
> do not directly access any members of the entry outside the pm lock, which
> can avoid similar "entry->x" uaf.
> 
> 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>
It looks also good to me, but no need for me to add a reviewed-by tag I
suppose.

This v4 can be applied in netdev directly.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net V4] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Paolo Abeni 2 months, 3 weeks ago

On 9/10/24 11:58, 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".
> 
> Move list_del(&entry->list) to mptcp_pm_del_add_timer and inside the pm lock,
> do not directly access any members of the entry outside the pm lock, which
> can avoid similar "entry->x" uaf.
> 
> 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>

Acked-by: Paolo Abeni <pabeni@redhat.com>

Re: [PATCH net V4] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by MPTCP CI 2 months, 3 weeks ago

Hi Edward,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10790383980

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/480c7ad533ba
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=888806


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

[PATCH V4] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Edward Adam Davis 2 months, 3 weeks ago

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".

Move list_del(&entry->list) to mptcp_pm_del_add_timer and inside the pm lock,
do not directly access any members of the entry outside the pm lock, which
can avoid similar "entry->x" uaf.

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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3e4ad801786f..f195b577c367 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -331,15 +331,21 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 {
 	struct mptcp_pm_add_entry *entry;
 	struct sock *sk = (struct sock *)msk;
+	struct timer_list *add_timer = NULL;
 
 	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;
+	}
+	if (!check_id && entry)
+		list_del(&entry->list);
 	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,7 +1436,6 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
 
 	entry = mptcp_pm_del_add_timer(msk, addr, false);
 	if (entry) {
-		list_del(&entry->list);
 		kfree(entry);
 		return true;
 	}
-- 
2.43.0

Re: [PATCH V4] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by MPTCP CI 2 months, 3 weeks ago

Hi Edward,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10790389747

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e58211a3272b
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=888810


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Matthieu Baerts 2 months, 4 weeks ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Jakub Kicinski 2 months, 4 weeks ago

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?

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by Matthieu Baerts 2 months, 3 weeks ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by MPTCP CI 2 months, 4 weeks ago

Hi Edward,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 1 failed test(s): mptcp_connect_mmap 🔴
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10721999077

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b6a976b908cd
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=887227


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH net v3] mptcp: pm: Fix uaf in __timer_delete_sync

Posted by MPTCP CI 2 months, 4 weeks ago

Hi Edward,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Script error! ❓
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10720554167

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a896db33c525
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=887227


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)