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] [thread-next>] [day] [month] [year] [list]

Message-ID: <E3DEE4BD-F8BB-4B41-8310-9214B1431614@redhat.com>
Date:   Thu, 17 Mar 2022 14:42:33 +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 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?

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?

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