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: <ZJrEtw15g3a7nyLN@corigine.com>
Date: Tue, 27 Jun 2023 13:15:03 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Vladimir Oltean <vladimir.oltean@....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

On Tue, Jun 27, 2023 at 01:18:28AM +0300, Vladimir Oltean wrote:
> 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.

This is kind of complex :)

Can I clarify that either:

1. Both source_port and switch_id are -1; or
2. Neither source_port nor switch_id are -1

If so, I agree with your proposal.