netdev - Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

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: <20231205083646.h2tqkwourtdyzdee@skbuf>
Date: Tue, 5 Dec 2023 10:36:46 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Danzberger <dd@...edd.com>
Cc: woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com,
	netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on
 platform init

On Tue, Dec 05, 2023 at 09:00:39AM +0100, Daniel Danzberger wrote:
> > Is this all that's necessary for instantiating the ksz driver through
> > ds->dev->platform_data? I suppose not, so can you post it all, please?
> Yes, that NULL pointer was the only issue I encountered.
> 
> Here is the module I use to instantiate the switch, which works fine so far in our
> linux v6.1 x86_64 builds:
> --
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/i2c.h>
> #include <linux/netdevice.h>
> #include <net/dsa.h>
> 
> static struct i2c_client *i2cdev;
> 
> static struct dsa_chip_data ksz9477_dsa = {
> 	.port_names = {
> 		"cpu",
> 		"lan1",
> 		"lan2",
> 		"lan3",
> 		"lan4"
> 	}
> };
> 
> static struct i2c_board_info t2t_ngr421_i2c_board_info = {
> 	I2C_BOARD_INFO("ksz9477-switch", 0x5f),
> 	.platform_data	= &ksz9477_dsa,
> };
> 
> static int __init t2t_ngr421_platform_init(void)
> {
> 	struct i2c_adapter *adapter = i2c_get_adapter(11);
> 	struct net_device *netdev_cpu = NULL, *nd;
> 
> 	if (adapter == NULL) {
> 		pr_err("t2t-ngr421: Missing FT260 I2C Adapter");
> 		return -ENODEV;
> 	}
> 
> 	read_lock(&dev_base_lock);
> 	for_each_netdev(&init_net, nd) {
> 		if (!strcmp(nd->name, "eth0")) {
> 			netdev_cpu = nd;
> 			break;
> 		}
> 	}
> 	read_unlock(&dev_base_lock);
> 
> 	if (netdev_cpu == NULL) {
> 		pr_err("t2t-ngr421: Missing netdev eth0");
> 		return -ENODEV;
> 	}
> 		
> 	ksz9477_dsa.netdev[0] = &netdev_cpu->dev;
> 	i2cdev = i2c_new_client_device(adapter, &t2t_ngr421_i2c_board_info);
> 	return i2cdev ? 0 : -ENODEV;
> }
> 
> static void t2t_ngr421_platform_deinit(void)
> {
> 	if (i2cdev)
> 		i2c_unregister_device(i2cdev);
> }
> 
> module_init(t2t_ngr421_platform_init);
> module_exit(t2t_ngr421_platform_deinit);
> 
> MODULE_AUTHOR("Daniel Danzberger <dd@...edd.com>");
> MODULE_DESCRIPTION("T2T NGR421 platform driver");
> MODULE_LICENSE("GPL v2");
> --

Pfff, I hate that "eth0" search. If you have a udev naming rule and the
driver is built as a module, you break it. Although you don't even need
that. Insert a USB to Ethernet adapter and all bets are off regarding
which one is eth0 and which one is eth1. It's good as prototyping code
and not much more.

Admittedly, that's the only thing that DSA offers currently when there's
no firmware description of the switch, but I think it wouldn't even be
that hard to do better. Someone needs to take a close look at Marcin
Wojtas' work of converting DSA to fwnode APIs
https://lore.kernel.org/netdev/20230116173420.1278704-1-mw@semihalf.com/
then we could replace the platform_data with software nodes and references.
That should actually be in our own best interest as maintainers, since
it should unify the handling of the 2 probing cases in the DSA core.
I might be able to find some time to do that early next year.

Except for dsa_loop_bdinfo.c which is easy to test by anyone, I don't
see any other board init code physically present in mainline. So please
do _not_ submit the board code, so I can pretend I didn't see it, and
for the responsibility of converting it to the new API to fall on you :)

(or, of course, you may want to take on the bigger task yourself ahead
of me, case in which your board code, edited to use fwnode_create_software_node(),
would be perfectly welcome in mainline)

But let's do something about the ksz driver's use of the platform_data
structures, since it wasn't even on my radar of something that might be
able to support that use case. More below.

> > Looking at dsa_switch_probe() -> dsa_switch_parse(), it expects
> > ds->dev->platform_data to contain a struct dsa_chip_data. This is in
> > contrast with ksz_spi.c, ksz9477_i2c.c and ksz8863_smi.c, which expect
> > the dev->platform_data to have the struct ksz_platform_data type.
> > But struct ksz_platform_data does not contain struct dsa_chip_data as
> > first element.
> 
> Noticed that as well.
> But hence the 'struct ksz_platform_data' isn't used anywhere, I passed (see module above) 'struct
> dsa_chip_data' directly.

What do you mean struct ksz_platform_data isn't used anywhere? What about this?

int ksz_switch_register(struct ksz_device *dev)
{
	const struct ksz_chip_data *info;
	struct device_node *port, *ports;
	phy_interface_t interface;
	unsigned int port_num;
	int ret;
	int i;

	if (dev->pdata)
		dev->chip_id = dev->pdata->chip_id;

with dev->pdata assigned like this:

static int ksz9477_i2c_probe(struct i2c_client *i2c)
{
	struct ksz_device *dev;

	dev = ksz_switch_alloc(&i2c->dev, i2c);
	if (!dev)
		return -ENOMEM;

	if (i2c->dev.platform_data)
		dev->pdata = i2c->dev.platform_data;

What is your dev->chip_id before and after this? The code dereferences
the first 4 bytes of struct dsa_chip_data as if it was the chip_id field
of struct ksz_platform_data. There is a bunch of code that depends on
dev->chip_id at runtime.