netdev - RE: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021

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: <29c352ebf87a4ecb854c5599f5282910@sphcmbx02.sunplus.com.tw>
Date:   Wed, 17 Nov 2021 09:28:27 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Pavel Skripkin <paskripkin@...il.com>,
        Wells Lu <wellslutw@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>
Subject: RE: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021

Hi Pavel,

> Hi, Wells!
> 
> On 11/3/21 14:02, Wells Lu wrote:
> 
> [code snip]
> 
> > +		if (comm->dual_nic) {
> > +			struct net_device *net_dev2 = mac->next_netdev;
> > +
> > +			if (!netif_running(net_dev2)) {
> > +				mac_hw_stop(mac);
> > +
> > +				mac2 = netdev_priv(net_dev2);
> > +
> 
> (*)
> 
> > +				// unregister and free net device.
> > +				unregister_netdev(net_dev2);
> > +				free_netdev(net_dev2);
> > +				mac->next_netdev = NULL;
> > +				pr_info(" Unregistered and freed net device \"eth1\"!\n");
> > +
> > +				comm->dual_nic = 0;
> > +				mac_switch_mode(mac);
> > +				rx_mode_set(net_dev);
> > +				mac_hw_addr_del(mac2);
> > +
> 
> mac2 is net_dev2 private data (*), so it will become freed after
> free_netdev() call.
> 
> FWIW the latest `smatch` should warn about this type of bugs.

Yes, this is indeed a bug.
But the code paragraph has been removed thoroughly in [PATCH v2].


> > +				// If eth0 is up, turn on lan 0 and 1 when
> > +				// switching to daisy-chain mode.
> > +				if (comm->enable & 0x1)
> > +					comm->enable = 0x3;
> 
> [code snip]
> 
> > +static int l2sw_remove(struct platform_device *pdev) {
> > +	struct net_device *net_dev;
> > +	struct net_device *net_dev2;
> > +	struct l2sw_mac *mac;
> > +
> > +	net_dev = platform_get_drvdata(pdev);
> > +	if (!net_dev)
> > +		return 0;
> > +	mac = netdev_priv(net_dev);
> > +
> > +	// Unregister and free 2nd net device.
> > +	net_dev2 = mac->next_netdev;
> > +	if (net_dev2) {
> > +		unregister_netdev(net_dev2);
> > +		free_netdev(net_dev2);
> > +	}
> > +
> 
> Is it save here to free mac->next_netdev before unregistering "parent"
> netdev? I haven't checked the whole code, just asking :)

Yes, I think it is save.
netdev2 should be unregistered and freed before net_dev.
If net_dev is unregistered and freed in advance,
mac->next_netdev becomes danger because 'mac' has been freed.


> > +	sysfs_remove_group(&pdev->dev.kobj, &l2sw_attribute_group);
> > +
> > +	mac->comm->enable = 0;
> > +	soc0_stop(mac);
> > +
> > +	napi_disable(&mac->comm->rx_napi);
> > +	netif_napi_del(&mac->comm->rx_napi);
> > +	napi_disable(&mac->comm->tx_napi);
> > +	netif_napi_del(&mac->comm->tx_napi);
> > +
> > +	mdio_remove(net_dev);
> > +
> > +	// Unregister and free 1st net device.
> > +	unregister_netdev(net_dev);
> > +	free_netdev(net_dev);
> > +
> > +	clk_disable(mac->comm->clk);
> > +
> > +	// Free 'common' area.
> > +	kfree(mac->comm);
> 
> Same here with `mac`.

This is indeed a bug.
But the statement, Kfree(mac->comm);, has been removed in [PATCH v2].
In [PATCH v2], structure data 'mac->comm' is allocated by
devm_kzalloc(). No more need to free it here.


> > +	return 0;
> > +}
> 
> 
> I haven't read the whole thread, i am sorry if these questions were already discussed.
> 
> 
> 
> With regards,
> Pavel Skripkin


Thank you very much for your review!

Best regards,
Wells Lu