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

[Daniel Danzberger <dd@embedd.com>]

Hi,

On Mon, 2023-12-04 at 19:43 +0200, Vladimir Oltean wrote:
> Hello Daniel,
> 
> On Mon, Dec 04, 2023 at 04:43:15PM +0100, Daniel Danzberger wrote:
> > Fixes a NULL pointer access when registering a switch device that has
> > not been defined via DTS.
> > 
> > This might happen when the switch is used on a platform like x86 that
> > doesn't use DTS and instantiates devices in platform specific init code.
> > 
> > Signed-off-by: Daniel Danzberger <dd@embedd.com>
> > ---
> >  drivers/net/dsa/microchip/ksz_common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> > index 9545aed905f5..525e13d9e39c 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -1678,7 +1678,7 @@ static int ksz_check_device_id(struct ksz_device *dev)
> >  	dt_chip_data = of_device_get_match_data(dev->dev);
> >  
> >  	/* Check for Device Tree and Chip ID */
> > -	if (dt_chip_data->chip_id != dev->chip_id) {
> > +	if (dt_chip_data && dt_chip_data->chip_id != dev->chip_id) {
> >  		dev_err(dev->dev,
> >  			"Device tree specifies chip %s but found %s, please fix it!\n",
> >  			dt_chip_data->dev_name, dev->info->dev_name);
> > -- 
> > 2.39.2
> > 
> > 
> 
> 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@embedd.com>");
MODULE_DESCRIPTION("T2T NGR421 platform driver");
MODULE_LICENSE("GPL v2");
--

> 
> 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.
Which is what the DSA code at net/dsa/dsa2.c expects in:
 static int dsa_switch_parse(struct dsa_switch *ds, struct dsa_chip_data *cd)

-- 
Regards

Daniel Danzberger
embeDD GmbH, Alter Postplatz 2, CH-6370 Stans