linux-kernel - Re: [PATCH net v2] ethtool: Drop check for vlan etype and vlan tci when parsing flow

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

Message-ID: <20190529160940.k2hnuoauaa4y2rga@salvia>
Date:   Wed, 29 May 2019 18:09:40 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc:     davem@...emloft.net, Florian Fainelli <f.fainelli@...il.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Antoine Tenart <antoine.tenart@...tlin.com>,
        thomas.petazzoni@...tlin.com
Subject: Re: [PATCH net v2] ethtool: Drop check for vlan etype and vlan tci
 when parsing flow_rule

On Wed, May 29, 2019 at 05:13:44PM +0200, Maxime Chevallier wrote:
> When parsing an ethtool flow spec to build a flow_rule, the code checks
> if both the vlan etype and the vlan tci are specified by the user to add
> a FLOW_DISSECTOR_KEY_VLAN match.
> 
> However, when the user only specified a vlan etype or a vlan tci, this
> check silently ignores these parameters.
> 
> For example, the following rule :
> 
> ethtool -N eth0 flow-type udp4 vlan 0x0010 action -1 loc 0
> 
> will result in no error being issued, but the equivalent rule will be
> created and passed to the NIC driver :
> 
> ethtool -N eth0 flow-type udp4 action -1 loc 0
> 
> In the end, neither the NIC driver using the rule nor the end user have
> a way to know that these keys were dropped along the way, or that
> incorrect parameters were entered.
> 
> This kind of check should be left to either the driver, or the ethtool
> flow spec layer.
> 
> This commit makes so that ethtool parameters are forwarded as-is to the
> NIC driver.
> 
> Since none of the users of ethtool_rx_flow_rule_create are using the
> VLAN dissector, I don't think this qualifies as a regression.
> 
> Fixes: eca4205f9ec3 ("ethtool: add ethtool_rx_flow_spec to flow_rule structure translator")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> ---
> V2: Added Fixes: tag, targetted to -net.
> 
>  net/core/ethtool.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 4a593853cbf2..2fe86893e9b5 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -3010,26 +3010,23 @@ ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input)
>  		const struct ethtool_flow_ext *ext_h_spec = &fs->h_ext;
>  		const struct ethtool_flow_ext *ext_m_spec = &fs->m_ext;
>  
> -		if (ext_m_spec->vlan_etype &&
> -		    ext_m_spec->vlan_tci) {
> -			match->key.vlan.vlan_tpid = ext_h_spec->vlan_etype;
> -			match->mask.vlan.vlan_tpid = ext_m_spec->vlan_etype;
> +		match->key.vlan.vlan_tpid = ext_h_spec->vlan_etype;
> +		match->mask.vlan.vlan_tpid = ext_m_spec->vlan_etype;

Could you just check for ext_m_spec->vlan_etype, then set vlan_tpid
accordingly? ie.

        if (ext_m_spec->vlan_etype) {
		match->key.vlan.vlan_tpid = ext_h_spec->vlan_etype;
		match->mask.vlan.vlan_tpid = ext_m_spec->vlan_etype;
        }
        if (ext_m_spec->vlan_tci) {
                match->key.vlan.vlan_id = ...;
                match->mask.vlan.vlan_id = ...;
                match->key.vlan.vlan_priority = ...;
                match->mask.vlan.vlan_priority = ...;
        }

        if (ext_m_spec->vlan_etype ||
            ext_m_spec->vlan_tci) {
		match->dissector.used_keys |=
			BIT(FLOW_DISSECTOR_KEY_VLAN);
		match->dissector.offset[FLOW_DISSECTOR_KEY_VLAN] =
			offsetof(struct ethtool_rx_flow_key, vlan);
        }

Something like this above.