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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52dbf9c9-0fa6-d4c6-ed6e-bba39e6e921b@gmail.com>
Date:   Sun, 14 Nov 2021 22:19:29 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     Wells Lu <wellslutw@...il.com>, davem@...emloft.net,
        kuba@...nel.org, robh+dt@...nel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        p.zabel@...gutronix.de
Cc:     Wells Lu <wells.lu@...plus.com>
Subject: Re: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021

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.

> +				// 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 :)

> +	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`.

> +	return 0;
> +}


I haven't read the whole thread, i am sorry if these questions were 
already discussed.



With regards,
Pavel Skripkin