| 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
| ||
|
Date: Fri, 18 Mar 2022 14:47:14 +0100
From: Eelco Chaudron <echaudro@...hat.com>
To: Aaron Conole <aconole@...hat.com>
Cc: netdev@...r.kernel.org, dev@...nvswitch.org, pshelar@....org,
Ilya Maximets <i.maximets@....org>,
Dumitru Ceara <dceara@...hat.com>,
Numan Siddique <nusiddiq@...hat.com>
Subject: Re: [PATCH net v2] openvswitch: always update flow key after nat
On 18 Mar 2022, at 13:43, 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
> 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>
You were right about the diff, it really looks messy and I had to apply it to review it :)
The patch looks fine to me!!
Acked-by: Eelco Chaudron <echaudro@...hat.com>
> ---
> v1->v2: removed forward decl., moved the ovs_nat_update_key function
> made sure it compiles with NF_NAT disabled and enabled
>
> net/openvswitch/conntrack.c | 118 ++++++++++++++++++------------------
> 1 file changed, 59 insertions(+), 59 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c07afff57dd3..4a947c13c813 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -734,6 +734,57 @@ static bool skb_nfct_cached(struct net *net,
> }
>
> #if IS_ENABLED(CONFIG_NF_NAT)
> +static void ovs_nat_update_key(struct sw_flow_key *key,
> + const struct sk_buff *skb,
> + enum nf_nat_manip_type maniptype)
> +{
> + if (maniptype == NF_NAT_MANIP_SRC) {
> + __be16 src;
> +
> + key->ct_state |= OVS_CS_F_SRC_NAT;
> + if (key->eth.type == htons(ETH_P_IP))
> + key->ipv4.addr.src = ip_hdr(skb)->saddr;
> + else if (key->eth.type == htons(ETH_P_IPV6))
> + memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
> + sizeof(key->ipv6.addr.src));
> + else
> + return;
> +
> + if (key->ip.proto == IPPROTO_UDP)
> + src = udp_hdr(skb)->source;
> + else if (key->ip.proto == IPPROTO_TCP)
> + src = tcp_hdr(skb)->source;
> + else if (key->ip.proto == IPPROTO_SCTP)
> + src = sctp_hdr(skb)->source;
> + else
> + return;
> +
> + key->tp.src = src;
> + } else {
> + __be16 dst;
> +
> + key->ct_state |= OVS_CS_F_DST_NAT;
> + if (key->eth.type == htons(ETH_P_IP))
> + key->ipv4.addr.dst = ip_hdr(skb)->daddr;
> + else if (key->eth.type == htons(ETH_P_IPV6))
> + memcpy(&key->ipv6.addr.dst, &ipv6_hdr(skb)->daddr,
> + sizeof(key->ipv6.addr.dst));
> + else
> + return;
> +
> + if (key->ip.proto == IPPROTO_UDP)
> + dst = udp_hdr(skb)->dest;
> + else if (key->ip.proto == IPPROTO_TCP)
> + dst = tcp_hdr(skb)->dest;
> + else if (key->ip.proto == IPPROTO_SCTP)
> + dst = sctp_hdr(skb)->dest;
> + else
> + return;
> +
> + key->tp.dst = dst;
> + }
> +}
> +
> /* Modelled after nf_nat_ipv[46]_fn().
> * range is only used for new, uninitialized NAT state.
> * Returns either NF_ACCEPT or NF_DROP.
> @@ -741,7 +792,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,58 +864,11 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> push:
> skb_push_rcsum(skb, nh_off);
>
> - return err;
> -}
> -
> -static void ovs_nat_update_key(struct sw_flow_key *key,
> - const struct sk_buff *skb,
> - enum nf_nat_manip_type maniptype)
> -{
> - if (maniptype == NF_NAT_MANIP_SRC) {
> - __be16 src;
> -
> - key->ct_state |= OVS_CS_F_SRC_NAT;
> - if (key->eth.type == htons(ETH_P_IP))
> - key->ipv4.addr.src = ip_hdr(skb)->saddr;
> - else if (key->eth.type == htons(ETH_P_IPV6))
> - memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
> - sizeof(key->ipv6.addr.src));
> - else
> - return;
> -
> - if (key->ip.proto == IPPROTO_UDP)
> - src = udp_hdr(skb)->source;
> - else if (key->ip.proto == IPPROTO_TCP)
> - src = tcp_hdr(skb)->source;
> - else if (key->ip.proto == IPPROTO_SCTP)
> - src = sctp_hdr(skb)->source;
> - else
> - return;
> -
> - key->tp.src = src;
> - } else {
> - __be16 dst;
> -
> - key->ct_state |= OVS_CS_F_DST_NAT;
> - if (key->eth.type == htons(ETH_P_IP))
> - key->ipv4.addr.dst = ip_hdr(skb)->daddr;
> - else if (key->eth.type == htons(ETH_P_IPV6))
> - memcpy(&key->ipv6.addr.dst, &ipv6_hdr(skb)->daddr,
> - sizeof(key->ipv6.addr.dst));
> - else
> - return;
> -
> - if (key->ip.proto == IPPROTO_UDP)
> - dst = udp_hdr(skb)->dest;
> - else if (key->ip.proto == IPPROTO_TCP)
> - dst = tcp_hdr(skb)->dest;
> - else if (key->ip.proto == IPPROTO_SCTP)
> - dst = sctp_hdr(skb)->dest;
> - else
> - return;
> + /* Update the flow key if NAT successful. */
> + if (err == NF_ACCEPT)
> + ovs_nat_update_key(key, skb, maniptype);
>
> - key->tp.dst = dst;
> - }
> + return err;
> }
>
> /* Returns NF_DROP if the packet should be dropped, NF_ACCEPT otherwise. */
> @@ -906,7 +910,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 +920,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 */
> --
> 2.31.1
Powered by blists - more mailing lists