netdev - Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

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]

Date: Mon, 26 Jun 2023 13:37:41 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Simon Horman <simon.horman@...igine.com>
Cc: Pawel Dembicki <paweldembicki@...il.com>, netdev@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...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-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based
 on 8021q

On Sun, Jun 25, 2023 at 02:42:27PM +0200, Simon Horman wrote:
> + Dan Carpenter
> 
> On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> > This patch is simple implementation of 8021q tagging in vsc73xx driver.
> > At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> > family doesn't provide any tag support for external ethernet ports.
> > 
> > The only way is vlan-based tagging. It require constant hardware vlan
> > filtering. VSC73XX family support provider bridging but QinQ only without
> > fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> > 
> > In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> > frames.
> > 
> > Tag driver introduce most simple funcionality required for proper taging
> > support.
> > 
> > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> 
> ...
> 
> > +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> > +			     int *switch_id, int *vbid, u16 *vid)
> > +{
> > +	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> > +		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> > +
> > +	/* Try our best with imprecise RX */
> > +	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> > +}
> > +
> > +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
> > +				   struct net_device *netdev)
> > +{
> > +	int src_port = -1, switch_id = -1, vbid = -1;
> > +	u16 vid;
> > +
> > +	if (skb_vlan_tag_present(skb))
> > +		/* Normal traffic path. */
> > +		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> > +
> > +	if (vbid >= 1)
> > +		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> > +	else if (src_port == -1 || switch_id == -1)
> > +		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
> 
> Hi Pawel,
> 
> Smatch warns that vid may be used uninitialised here.
> And it's not clear to me why that cannot be the case.
> 

The problem is only when skb_vlan_tag_present() is false.

If vsc73xx_vlan_rcv() is called then actually it's fine.  Either vbid,
src_port and switch_id will be set or vid will be initialized.  Smatch
thinks the vbid is set to 0-7, src_port is set to 0-15 and
switch_id is set to 0-7.  Smatch kind of ignores the switch_id == -1
condition because it's known to be false given that we already checked
src_port == -1.

But the concern again is the skb_vlan_tag_present() is false and then
vbid, src_port and switch_id would all be set to -1.

regards,
dan carpenter