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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

[-- 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