netdev - Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port information from INCL

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: <20230626221828.qzjeo6dedjnyme6k@skbuf>
Date: Tue, 27 Jun 2023 01:18:28 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Simon Horman <simon.horman@...igine.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/2] net: dsa: tag_sja1105: always prefer source port
 information from INCL_SRCPT

Hi Simon,

On Mon, Jun 26, 2023 at 08:11:53PM +0200, Simon Horman wrote:
> Hi Vladimir,
> 
> A similar comment to that made for [1], though the code is somewhat
> different to that case: are you sure vid is initialised here?
> GCC 12 and Smatch seem unsure about it.
> 
> [1] Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q
>     https://lore.kernel.org/all/ZJg2M+Qvg3Fv73CH@corigine.com/

"vid" can be uninitialized if the tagger is fed a junk packet (a
non-link-local, non-meta packet that also has no tag_8021q header).

The immediate answer that comes to mind is: it depends on how the driver
configures the hardware to send packets to the CPU (and it will never
configure the switch in that way).

But, between the sja1105 driver configuring the switch in a certain way
and the tag_sja1105 driver seeing the results of that, there's also the
DSA master driver (can be any net_device) which can alter the packet in
a nonsensical way, like remove the VLAN header for some reason.

Considering the fact that the DSA master can have tc rules on its
ingress path which do just that, it would probably be wise to be
defensive about this. So I can probably add:

	if (sja1105_skb_has_tag_8021q(skb)) {
		... // existing call to sja1105_vlan_rcv() here
	} else if (source_port == -1 && switch_id == -1) {
		/* Packets with no source information have no chance of
		 * getting accepted, drop them straight away.
		 */
		return NULL;
	}

This "else if" block should ensure that when "vid" is uninitialized,
either "source_port" and "switch_id", or "vbid", always have valid values.