Re: [PATCH] tcp: md5: Fix overlap between vrf and non-vrf keys

On 10/8/21 9:51 AM, Leonard Crestez wrote:

On 07.10.2021 21:27, David Ahern wrote:

quoted

On 10/7/21 12:41 AM, Leonard Crestez wrote:

quoted

On 07.10.2021 04:14, David Ahern wrote:

quoted

On 10/6/21 11:48 AM, Leonard Crestez wrote:

quoted

@@ -1103,11 +1116,11 @@ static struct tcp_md5sig_key

*tcp_md5_do_lookup_exact(const struct sock *sk,
   #endif
       hlist_for_each_entry_rcu(key, &md5sig->head, node,
                    lockdep_sock_is_held(sk)) {
           if (key->family != family)
               continue;
-        if (key->l3index && key->l3index != l3index)
+        if (key->l3index != l3index)

That seems like the bug fix there. The L3 reference needs to match for
new key and existing key. I think the same change is needed in
__tcp_md5_do_lookup.

Current behavior is that keys added without tcpm_ifindex will match
connections both inside and outside VRFs. Changing this might break real
applications, is it really OK to claim that this behavior was a bug all
along?

no.

It's been a few years. I need to refresh on the logic and that is not
going to happen before this weekend.

It seems that always doing a strict key->l3index != l3index condition
inside of __tcp_md5_do_lookup breaks the usecase of binding one listener
to each VRF and not specifying the ifindex for each key.

This is a very valid usecase, maybe the most common way to use md5 with
vrf.

Ways to fix this:
* Make this comparison only take effect if TCP_MD5SIG_FLAG_IFINDEX is set.
* Make this comparison only take effect if tcp_l3mdev_accept=1
* Add a new flag?

Right now passing TCP_MD5SIG_FLAG_IFINDEX and ifindex == 0 results in an
error but maybe it should be accepted to mean "key applies only for
default VRF".

I think I remember the history now: prior to the set
98c8147648fa..5cad8bce26e0 MD5 lookups for VRF and default VRF both
succeed on an address or prefix match because the L3 domain was not
checked. That set did not want to break the legacy behavior which is why
the change is based on db key having l3index set and matching the
ingress domain.

That means the limitation (hole depending on perspective) is a default
VRF and a VRF having overlap addresses with a key installed with the L3
index set (can't since default VRF does not have one) - which I believe
is your point.

So, yes, one option is to have a flag that indicates strict checking to
close the legacy path.