netdev - Re: [PATCH net-next v2] 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: <ZBiDvW+BZ/qE3EAV@corigine.com>
Date:   Mon, 20 Mar 2023 17:03:09 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Josef Miegl <josef@...gl.cz>
Cc:     Eyal Birger <eyal.birger@...il.com>,
        Pravin B Shelar <pshelar@....org>,
        Josef Miegl <josef@...gl.cz>,
        "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
Subject: Re: [PATCH net-next v2] net: geneve: accept every ethertype

On Sun, Mar 19, 2023 at 11:09:54PM +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 did not get rid of a
> restriction that prohibits receiving payloads other than Ethernet,
> instead the commit white-listed additional Ethertypes, IPv4 and IPv6.
> 
> 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.
> 
> The restriction was set in place back in commit 0b5e8b8eeae4
> ("net: Add Geneve tunneling protocol driver"), which implemented a
> protocol layer driver for Geneve to be used with Open vSwitch. The
> relevant discussion about introducing the Ethertype white-list can be
> found here:
> https://lore.kernel.org/netdev/CAEP_g=_1q3ACX5NTHxLDnysL+dTMUVzdLpgw1apLKEdDSWPztw@mail.gmail.com/
> 
> <quote>
> >> +       if (unlikely(geneveh->proto_type != htons(ETH_P_TEB)))
> >
> > Why? I thought the point of geneve carrying protocol field was to
> > allow protocols other than Ethernet... is this temporary maybe?
> 
> Yes, it is temporary. Currently OVS only handles Ethernet packets but
> this restriction can be lifted once we have a consumer that is capable
> of handling other protocols.
> </quote>
> 
> This white-list was then ported to a generic Geneve netdevice in commit
> 371bd1061d29 ("geneve: Consolidate Geneve functionality in single
> module."). Preserving the Ethertype white-list at this point made sense,
> as the Geneve device could send out only Ethernet payloads anyways.

I'm not sure if it ought to be fixed, but checkpatch complains that:

:379: ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 371bd1061d29 ("geneve: Consolidate Geneve functionality in single module.")'
This white-list was then ported to a generic Geneve netdevice in commit

> However, now that the Geneve netdevice supports encapsulating other
> payloads with IFLA_GENEVE_INNER_PROTO_INHERIT and we have a consumer
> capable of other protocols, it seems appropriate to lift the restriction
> and allow any Geneve payload to be received.
> 
> Signed-off-by: Josef Miegl <josef@...gl.cz>

Above nit not withstanding, and in keeping with the discussion of v1,
I am happy with this patch.

Reviewed-by: Simon Horman <simon.horman@...igine.com>

...