linux-kernel - Re: [ovs-dev] [PATCH net] net: openvswitch: fix parsing of nw

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: <509C48AE-595C-4C21-91CD-33D2893F3577@redhat.com>
Date:   Thu, 23 Jun 2022 11:36:11 +0200
From:   Eelco Chaudron <echaudro@...hat.com>
To:     "Rosemarie O'Riorden" <roriorden@...hat.com>
Cc:     netdev@...r.kernel.org, dev@...nvswitch.org,
        linux-kernel@...r.kernel.org, i.maximets@....org,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH net] net: openvswitch: fix parsing of nw_proto
 for IPv6 fragments



On 21 Jun 2022, at 22:48, Rosemarie O'Riorden wrote:

> When a packet enters the OVS datapath and does not match any existing
> flows installed in the kernel flow cache, the packet will be sent to
> userspace to be parsed, and a new flow will be created. The kernel and
> OVS rely on each other to parse packet fields in the same way so that
> packets will be handled properly.
>
> As per the design document linked below, OVS expects all later IPv6
> fragments to have nw_proto=44 in the flow key, so they can be correctly
> matched on OpenFlow rules. OpenFlow controllers create pipelines based
> on this design.
>
> This behavior was changed by the commit in the Fixes tag so that
> nw_proto equals the next_header field of the last extension header.
> However, there is no counterpart for this change in OVS userspace,
> meaning that this field is parsed differently between OVS and the
> kernel. This is a problem because OVS creates actions based on what is
> parsed in userspace, but the kernel-provided flow key is used as a match
> criteria, as described in Documentation/networking/openvswitch.rst. This
> leads to issues such as packets incorrectly matching on a flow and thus
> the wrong list of actions being applied to the packet. Such changes in
> packet parsing cannot be implemented without breaking the userspace.
>
> The offending commit is partially reverted to restore the expected
> behavior.
>
> The change technically made sense and there is a good reason that it was
> implemented, but it does not comply with the original design of OVS.
> If in the future someone wants to implement such a change, then it must
> be user-configurable and disabled by default to preserve backwards
> compatibility with existing OVS versions.
>
> Cc: stable@...r.kernel.org
> Fixes: fa642f08839b ("openvswitch: Derive IP protocol number for IPv6 later frags")
> Link: https://docs.openvswitch.org/en/latest/topics/design/#fragments
> Signed-off-by: Rosemarie O'Riorden <roriorden@...hat.com>
> ---
>  net/openvswitch/flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 372bf54a0ca9..e20d1a973417 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -407,7 +407,7 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>  	if (flags & IP6_FH_F_FRAG) {
>  		if (frag_off) {
>  			key->ip.frag = OVS_FRAG_TYPE_LATER;
> -			key->ip.proto = nexthdr;
> +			key->ip.proto = NEXTHDR_FRAGMENT;
>  			return 0;
>  		}
>  		key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -- 
> 2.35.3

Thanks Rosemarie, for fixing this. The change looks good to me!

Acked-by: Eelco Chaudron <echaudro@...hat.com>