linux-kernel - Re: [PATCH net-next] net: geneve: accept every ethertype

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: <ZA9T14Ks66HOlwH+@corigine.com>
Date:   Mon, 13 Mar 2023 17:48:23 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Josef Miegl <josef@...gl.cz>
Cc:     Eyal Birger <eyal.birger@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Pravin B Shelar <pshelar@....org>
Subject: Re: [PATCH net-next] net: geneve: accept every ethertype

+Pravin

On Sun, Mar 12, 2023 at 05:37:26PM +0100, Josef Miegl wrote:
> The Geneve encapsulation, as defined in RFC 8926, has a Protocol Type
> field, which states the Ethertype of the payload appearing after the
> Geneve header.
> 
> Commit 435fe1c0c1f7 ("net: geneve: support IPv4/IPv6 as inner protocol")
> introduced a new IFLA_GENEVE_INNER_PROTO_INHERIT flag that allowed the
> use of other Ethertypes than Ethernet. However, it imposed a restriction
> that prohibits receiving payloads other than IPv4, IPv6 and Ethernet.
> 
> This patch removes this restriction, making it possible to receive any
> Ethertype as a payload, if the IFLA_GENEVE_INNER_PROTO_INHERIT flag is
> set.
> 
> This is especially useful if one wants to encapsulate MPLS, because with
> this patch the control-plane traffic (IP, IS-IS) and the data-plane
> traffic (MPLS) can be encapsulated without an Ethernet frame, making
> lightweight overlay networks a possibility.

Hi Josef,

I could be mistaken. But I believe that the thinking at the time,
was based on the idea that it was better to only allow protocols that
were known to work. And allow more as time goes on.

Perhaps we have moved away from that thinking (I have no strong feeling
either way). Or perhaps this is safe because of some other guard. But if
not perhaps it is better to add the MPLS ethertype(s) to the if clause
rather than remove it. This would be after any patches that enhance the
stack to actually support this (I'm thinking of [1], though I haven't
looked at it closely).

[1] [PATCH net-next] net: geneve: set IFF_POINTOPOINT with IFLA_GENEVE_INNER_PROTO_INHERIT
    Link: https://lore.kernel.org/netdev/20230312164557.55354-1-josef@miegl.cz/


> Signed-off-by: Josef Miegl <josef@...gl.cz>
> ---
>  drivers/net/geneve.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 89ff7f8e8c7e..32684e94eb4f 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -365,13 +365,6 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>  	if (unlikely(geneveh->ver != GENEVE_VER))
>  		goto drop;
>  
> -	inner_proto = geneveh->proto_type;
> -
> -	if (unlikely((inner_proto != htons(ETH_P_TEB) &&
> -		      inner_proto != htons(ETH_P_IP) &&
> -		      inner_proto != htons(ETH_P_IPV6))))
> -		goto drop;
> -
>  	gs = rcu_dereference_sk_user_data(sk);
>  	if (!gs)
>  		goto drop;
> @@ -380,6 +373,8 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>  	if (!geneve)
>  		goto drop;
>  
> +	inner_proto = geneveh->proto_type;
> +
>  	if (unlikely((!geneve->cfg.inner_proto_inherit &&
>  		      inner_proto != htons(ETH_P_TEB)))) {
>  		geneve->dev->stats.rx_dropped++;
> -- 
> 2.37.1
>