netdev - Re: [ovs-dev] [PATCH net] openvswitch: always update flow key after NAT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2DD78E65-AE2E-4F6E-BF80-DCAFC2944E65@redhat.com>
Date:   Thu, 17 Mar 2022 15:30:49 +0100
From:   Eelco Chaudron <echaudro@...hat.com>
To:     Aaron Conole <aconole@...hat.com>
Cc:     netdev@...r.kernel.org, dev@...nvswitch.org,
        Dumitru Ceara <dceara@...hat.com>,
        Ilya Maximets <i.maximets@....org>
Subject: Re: [ovs-dev] [PATCH net] openvswitch: always update flow key after NAT



On 17 Mar 2022, at 15:01, Aaron Conole wrote:

> Eelco Chaudron <echaudro@...hat.com> writes:
>
>> On 17 Mar 2022, at 13:17, Aaron Conole wrote:
>>
>>> During NAT, a tuple collision may occur.  When this happens, openvswitch
>>> will make a second pass through NAT which will perform additional packet
>>> modification.  This will update the skb data, but not the flow key that
>>> OVS uses.  This means that future flow lookups, and packet matches will
>>> have incorrect data.  This has been supported since
>>> commit 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack").
>>>
>>> That commit failed to properly update the sw_flow_key attributes, since
>>> it only called the ovs_ct_nat_update_key once, rather than each time
>>> ovs_ct_nat_execute was called.  As these two operations are linked, the
>>> ovs_ct_nat_execute() function should always make sure that the
>>> sw_flow_key is updated after a successful call through NAT infrastructure.
>>>
>>> Fixes: 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack")
>>> Cc: Dumitru Ceara <dceara@...hat.com>
>>> Cc: Numan Siddique <nusiddiq@...hat.com>
>>> Signed-off-by: Aaron Conole <aconole@...hat.com>
>>> ---
>>>  net/openvswitch/conntrack.c | 20 ++++++++++++--------
>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index c07afff57dd3..461dbb3b7090 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -104,6 +104,10 @@ static bool labels_nonzero(const struct ovs_key_ct_labels *labels);
>>>
>>>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>>>
>>> +static void ovs_nat_update_key(struct sw_flow_key *key,
>>> +			       const struct sk_buff *skb,
>>> +			       enum nf_nat_manip_type maniptype);
>>> +
>>
>> nit?: Rather than adding this would it be simpler to swap the order of
>> ovs_nat_update_key and ovs_ct_nat_execute?
>
> I thought it looked messier that way.
>
>> Also, the function is defined by “#if IS_ENABLED(CONFIG_NF_NAT)” not
>> sure if this will generate warnings if the function is not present?
>
> Good catch, yes it will.  I will submit a v2 with this guard in place if
> you think the foward decl is okay to keep.

I’m fine with either way, but I personally prefer the re-order as the code looks cleaner (but the patch doesn't ;).

>>>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>>>  {
>>>  	switch (ntohs(key->eth.type)) {
>>> @@ -741,7 +745,7 @@ static bool skb_nfct_cached(struct net *net,
>>>  static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>>>  			      enum ip_conntrack_info ctinfo,
>>>  			      const struct nf_nat_range2 *range,
>>> -			      enum nf_nat_manip_type maniptype)
>>> +			      enum nf_nat_manip_type maniptype, struct sw_flow_key *key)
>>>  {
>>>  	int hooknum, nh_off, err = NF_ACCEPT;
>>>
>>> @@ -813,6 +817,10 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>>>  push:
>>>  	skb_push_rcsum(skb, nh_off);
>>>
>>> +	/* Update the flow key if NAT successful. */
>>> +	if (err == NF_ACCEPT)
>>> +		ovs_nat_update_key(key, skb, maniptype);
>>> +
>>>  	return err;
>>>  }
>>>
>>> @@ -906,7 +914,7 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>>>  	} else {
>>>  		return NF_ACCEPT; /* Connection is not NATed. */
>>>  	}
>>> -	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype);
>>> +	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype, key);
>>>
>>>  	if (err == NF_ACCEPT && ct->status & IPS_DST_NAT) {
>>>  		if (ct->status & IPS_SRC_NAT) {
>>> @@ -916,17 +924,13 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>>>  				maniptype = NF_NAT_MANIP_SRC;
>>>
>>>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range,
>>> -						 maniptype);
>>> +						 maniptype, key);
>>>  		} else if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
>>>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, NULL,
>>> -						 NF_NAT_MANIP_SRC);
>>> +						 NF_NAT_MANIP_SRC, key);
>>>  		}
>>>  	}
>>>
>>> -	/* Mark NAT done if successful and update the flow key. */
>>> -	if (err == NF_ACCEPT)
>>> -		ovs_nat_update_key(key, skb, maniptype);
>>> -
>>>  	return err;
>>>  }
>>>  #else /* !CONFIG_NF_NAT */
>>
>> The rest of the patch looks fine to me...