[PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Davide Caratti]

when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
the list of "announced" addresses. In case of a positive match, the timer
that handles retransmissions is stopped regardless of the 'Address Id' in
the received packet: this behaviour does not comply with RFC8684 §3.4.1.

Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
Tested using packetdrill, with the following captured output:

 unpatched kernel:

 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
        ^^^ retransmission is stopped here, but 'Address Id' is 90

 patched kernel:

 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
        ^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match

Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/mptcp/options.c    | 2 +-
 net/mptcp/pm_netlink.c | 8 ++++----
 net/mptcp/protocol.h   | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 3428c163299b..55732b8d7e08 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1067,7 +1067,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
 		} else {
 			mptcp_pm_add_addr_echoed(msk, &mp_opt.addr);
-			mptcp_pm_del_add_timer(msk, &mp_opt.addr);
+			mptcp_pm_del_add_timer(msk, &mp_opt.addr, true);
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD);
 		}
 
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d094588afad8..09722598994d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -346,18 +346,18 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
 
 struct mptcp_pm_add_entry *
 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
-		       struct mptcp_addr_info *addr)
+		       struct mptcp_addr_info *addr, bool check_id)
 {
 	struct mptcp_pm_add_entry *entry;
 	struct sock *sk = (struct sock *)msk;
 
 	spin_lock_bh(&msk->pm.lock);
 	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
-	if (entry)
+	if (entry && (!check_id || entry->addr.id == addr->id))
 		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry)
+	if (entry && (!check_id || entry->addr.id == addr->id))
 		sk_stop_timer_sync(sk, &entry->add_timer);
 
 	return entry;
@@ -1070,7 +1070,7 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
 {
 	struct mptcp_pm_add_entry *entry;
 
-	entry = mptcp_pm_del_add_timer(msk, addr);
+	entry = mptcp_pm_del_add_timer(msk, addr, false);
 	if (entry) {
 		list_del(&entry->list);
 		kfree(entry);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 868e878af526..16e50caf200e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -693,7 +693,7 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
 bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
 struct mptcp_pm_add_entry *
 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
-		       struct mptcp_addr_info *addr);
+		       struct mptcp_addr_info *addr, bool check_id);
 struct mptcp_pm_add_entry *
 mptcp_lookup_anno_list_by_saddr(struct mptcp_sock *msk,
 				struct mptcp_addr_info *addr);
-- 
2.31.1

Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]

On Tue, 18 May 2021, Davide Caratti wrote:

> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> the list of "announced" addresses. In case of a positive match, the timer
> that handles retransmissions is stopped regardless of the 'Address Id' in
> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>
> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
> Tested using packetdrill, with the following captured output:
>
> unpatched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
>        ^^^ retransmission is stopped here, but 'Address Id' is 90
>
> patched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
>        ^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match
>
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/options.c    | 2 +-
> net/mptcp/pm_netlink.c | 8 ++++----
> net/mptcp/protocol.h   | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)

Thanks Davide - looks good to me.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 3428c163299b..55732b8d7e08 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1067,7 +1067,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
> 		} else {
> 			mptcp_pm_add_addr_echoed(msk, &mp_opt.addr);
> -			mptcp_pm_del_add_timer(msk, &mp_opt.addr);
> +			mptcp_pm_del_add_timer(msk, &mp_opt.addr, true);
> 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD);
> 		}
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d094588afad8..09722598994d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -346,18 +346,18 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
>
> struct mptcp_pm_add_entry *
> mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> -		       struct mptcp_addr_info *addr)
> +		       struct mptcp_addr_info *addr, bool check_id)
> {
> 	struct mptcp_pm_add_entry *entry;
> 	struct sock *sk = (struct sock *)msk;
>
> 	spin_lock_bh(&msk->pm.lock);
> 	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> -	if (entry)
> +	if (entry && (!check_id || entry->addr.id == addr->id))
> 		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> 	spin_unlock_bh(&msk->pm.lock);
>
> -	if (entry)
> +	if (entry && (!check_id || entry->addr.id == addr->id))
> 		sk_stop_timer_sync(sk, &entry->add_timer);
>
> 	return entry;
> @@ -1070,7 +1070,7 @@ static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
> {
> 	struct mptcp_pm_add_entry *entry;
>
> -	entry = mptcp_pm_del_add_timer(msk, addr);
> +	entry = mptcp_pm_del_add_timer(msk, addr, false);
> 	if (entry) {
> 		list_del(&entry->list);
> 		kfree(entry);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 868e878af526..16e50caf200e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -693,7 +693,7 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
> bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
> struct mptcp_pm_add_entry *
> mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> -		       struct mptcp_addr_info *addr);
> +		       struct mptcp_addr_info *addr, bool check_id);
> struct mptcp_pm_add_entry *
> mptcp_lookup_anno_list_by_saddr(struct mptcp_sock *msk,
> 				struct mptcp_addr_info *addr);
> -- 
> 2.31.1
>
>

--
Mat Martineau
Intel

Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Matthieu Baerts]

Hi Davide, Mat,

On 18/05/2021 19:36, Davide Caratti wrote:
> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> the list of "announced" addresses. In case of a positive match, the timer
> that handles retransmissions is stopped regardless of the 'Address Id' in
> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
> 
> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.

Thank you for the patch and the review!

- 64705a88e6f0: mptcp: validate 'id' when stopping the ADD_ADDR
retransmit timer
- Results: bffd86183cb5..02925519e50e

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210520T183902
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210520T183902

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Davide Caratti]

when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
the list of "announced" addresses. In case of a positive match, the timer
that handles retransmissions is stopped regardless of the 'Address Id' in
the received packet: this behaviour does not comply with RFC8684 §3.4.1.

Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
Tested using packetdrill, with the following captured output:

 unpatched kernel:

 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
        ^^^ retransmission is stopped here, but 'Address Id' is 90

 patched kernel:

 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
 Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
 In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
        ^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match

Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/mptcp/pm_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d094588afad8..4318d4f5902e 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -353,11 +353,11 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 
 	spin_lock_bh(&msk->pm.lock);
 	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
-	if (entry)
+	if (entry && entry->addr.id == addr->id)
 		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry)
+	if (entry && entry->addr.id == addr->id)
 		sk_stop_timer_sync(sk, &entry->add_timer);
 
 	return entry;
-- 
2.31.1

Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

On Tue, 11 May 2021, Davide Caratti wrote:

> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> the list of "announced" addresses. In case of a positive match, the timer
> that handles retransmissions is stopped regardless of the 'Address Id' in
> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>
> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
> Tested using packetdrill, with the following captured output:
>
> unpatched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 3013740213], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0xfd2e62517888fe29,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 3013740213], length 0
>        ^^^ retransmission is stopped here, but 'Address Id' is 90
>
> patched kernel:
>
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 1.2.3.4,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 90 198.51.100.2,mptcp dss ack 1672384568], length 0
> Out <...> Flags [.], ack 1, win 256, options [mptcp add-addr v1 id 1 198.51.100.2 hmac 0x1cf372d59e05f4b8,mptcp dss ack 3007449509], length 0
> In  <...> Flags [.], ack 1, win 257, options [mptcp add-addr v1-echo id 1 198.51.100.2,mptcp dss ack 1672384568], length 0
>        ^^^ retransmission is stopped here, only when both 'Address Id' and 'IP Address' match
>
> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d094588afad8..4318d4f5902e 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -353,11 +353,11 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>
> 	spin_lock_bh(&msk->pm.lock);
> 	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> -	if (entry)
> +	if (entry && entry->addr.id == addr->id)
> 		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> 	spin_unlock_bh(&msk->pm.lock);
>
> -	if (entry)
> +	if (entry && entry->addr.id == addr->id)
> 		sk_stop_timer_sync(sk, &entry->add_timer);
>
> 	return entry;

Hi Davide -

The code path for ADD_ADDR echo ignores the return value of 
mptcp_pm_del_add_timer(), so there's no chance there of leaving 
entry->add_timer running if the addr.id doesn't match.

The function is also called by remove_anno_list_by_saddr(), which will 
free 'entry' if a non-NULL value is returned by mptcp_pm_del_add_timer(). 
There's more of a chance here that the sk_stop_timer_sync() would be 
skipped and entry would be freed anyway. Looking at the various paths to 
this in the path manager it seems like it *should* not happen, but I'm not 
convinced it's impossible. Did you look at the effect of this change on 
these other code paths?


--
Mat Martineau
Intel

Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Davide Caratti]

hello Mat, thanks for reviewing!

On Tue, May 11, 2021 at 03:39:14PM -0700, Mat Martineau wrote:
> On Tue, 11 May 2021, Davide Caratti wrote:
> 
> > when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
> > the list of "announced" addresses. In case of a positive match, the timer
> > that handles retransmissions is stopped regardless of the 'Address Id' in
> > the received packet: this behaviour does not comply with RFC8684 §3.4.1.
> > 
> > Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
> > Tested using packetdrill, with the following captured output:
> > 
> 
> Hi Davide -
> 
> The code path for ADD_ADDR echo ignores the return value of
> mptcp_pm_del_add_timer(), so there's no chance there of leaving
> entry->add_timer running if the addr.id doesn't match.

true, and that's the purpose of this patch. When addr.id doesn't match,
it leaves the timer running to allow another attempt of ADD_ADDR + HMAC.

> The function is also called by remove_anno_list_by_saddr(), which will free
> 'entry' if a non-NULL value is returned by mptcp_pm_del_add_timer(). There's
> more of a chance here that the sk_stop_timer_sync() would be skipped and
> entry would be freed anyway. Looking at the various paths to this in the
> path manager it seems like it *should* not happen, but I'm not convinced
> it's impossible. Did you look at the effect of this change on these other
> code paths?

good point, I only tried packetdrill and the kselftests. I see

mptcp_pm_remove_anno_addr()
  remove_anno_list_by_saddr()

and its callers: in this case,either 'addr.id' is always read from the return
value of 

 __lookup_addr_by_id(pernet, addr.addr.id);

or by iterating over

&pernet->local_addr_list

, so those should all be 'signal' endpoints previously added by netlink:
the value of addr.id should always match and the timer should always
be stopped.

However, I also don't feel 100% safe - because 'entry' is freed by
remove_anno_list_by_saddr() and the value of 'address.id' is never checked
in addresses_equal(). Maybe it's safer to do soemething like:

 347 struct mptcp_pm_add_entry *
 348 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 349                        struct mptcp_addr_info *addr, bool strict)
 350 {
 351         struct mptcp_pm_add_entry *entry;
 352         struct sock *sk = (struct sock *)msk;
 353 
 354         spin_lock_bh(&msk->pm.lock);
 355         entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
 356         if (entry && strict && entry->addr.id == addr->id)
 357                 entry->retrans_times = ADD_ADDR_RETRANS_MAX;
 358         spin_unlock_bh(&msk->pm.lock);
 359 
 360         if (entry && strict && entry->addr.id == addr->id)
 361                 sk_stop_timer_sync(sk, &entry->add_timer);
 362 
 363         return entry;
 364 }


so that mptcp_pm_del_add_timer(..., true) is called only in the receive
path of echo-ed ADD_ADDR. WDYT?

thanks!
-- 
davide

Re: [PATCH net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]

On Fri, 14 May 2021, Davide Caratti wrote:

> hello Mat, thanks for reviewing!
>
> On Tue, May 11, 2021 at 03:39:14PM -0700, Mat Martineau wrote:
>> On Tue, 11 May 2021, Davide Caratti wrote:
>>
>>> when Linux receives an echo-ed ADD_ADDR, it checks the IP address against
>>> the list of "announced" addresses. In case of a positive match, the timer
>>> that handles retransmissions is stopped regardless of the 'Address Id' in
>>> the received packet: this behaviour does not comply with RFC8684 §3.4.1.
>>>
>>> Fix it by validating the 'Address Id' in received echo-ed ADD_ADDRs.
>>> Tested using packetdrill, with the following captured output:
>>>
>>
>> Hi Davide -
>>
>> The code path for ADD_ADDR echo ignores the return value of
>> mptcp_pm_del_add_timer(), so there's no chance there of leaving
>> entry->add_timer running if the addr.id doesn't match.
>
> true, and that's the purpose of this patch. When addr.id doesn't match,
> it leaves the timer running to allow another attempt of ADD_ADDR + HMAC.
>
>> The function is also called by remove_anno_list_by_saddr(), which will free
>> 'entry' if a non-NULL value is returned by mptcp_pm_del_add_timer(). There's
>> more of a chance here that the sk_stop_timer_sync() would be skipped and
>> entry would be freed anyway. Looking at the various paths to this in the
>> path manager it seems like it *should* not happen, but I'm not convinced
>> it's impossible. Did you look at the effect of this change on these other
>> code paths?
>
> good point, I only tried packetdrill and the kselftests. I see
>
> mptcp_pm_remove_anno_addr()
>  remove_anno_list_by_saddr()
>
> and its callers: in this case,either 'addr.id' is always read from the return
> value of
>
> __lookup_addr_by_id(pernet, addr.addr.id);
>
> or by iterating over
>
> &pernet->local_addr_list
>
> , so those should all be 'signal' endpoints previously added by netlink:
> the value of addr.id should always match and the timer should always
> be stopped.
>
> However, I also don't feel 100% safe - because 'entry' is freed by
> remove_anno_list_by_saddr() and the value of 'address.id' is never checked
> in addresses_equal(). Maybe it's safer to do soemething like:
>
> 347 struct mptcp_pm_add_entry *
> 348 mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> 349                        struct mptcp_addr_info *addr, bool strict)
> 350 {
> 351         struct mptcp_pm_add_entry *entry;
> 352         struct sock *sk = (struct sock *)msk;
> 353
> 354         spin_lock_bh(&msk->pm.lock);
> 355         entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> 356         if (entry && strict && entry->addr.id == addr->id)
> 357                 entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> 358         spin_unlock_bh(&msk->pm.lock);
> 359
> 360         if (entry && strict && entry->addr.id == addr->id)
> 361                 sk_stop_timer_sync(sk, &entry->add_timer);
> 362
> 363         return entry;
> 364 }
>
>
> so that mptcp_pm_del_add_timer(..., true) is called only in the receive
> path of echo-ed ADD_ADDR. WDYT?
>

I think that's the right general direction, but (entry && (!strict || 
entry->addr.id == addr->id)) is the logic needed for the !strict callers 
to work like before.

--
Mat Martineau
Intel