| Message ID | 68c6032edaf880a958bf7f65afc7e9520f39d5cd.1620754971.git.dcaratti@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | [net] mptcp: validate 'id' when stopping the ADD_ADDR retransmit timer | expand |
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
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!
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
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;
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(-)