Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] ipmi_si: Fix crash when using hard-coded device
To:     <minyard@acm.org>
References: <20190221183343.13554-1-minyard@acm.org>
CC:     <openipmi-developer@lists.sourceforge.net>,
        <linux-kernel@vger.kernel.org>, <honglei5@huawei.com>,
        Corey Minyard <cminyard@mvista.com>
From:   Yang Yingliang <yangyingliang@huawei.com>
Message-ID: <5C6FBCD5.7000000@huawei.com>
Date:   Fri, 22 Feb 2019 17:11:49 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.1
MIME-Version: 1.0
In-Reply-To: <20190221183343.13554-1-minyard@acm.org>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Tested-by: Yang Yingliang <yangyingliang@huawei.com>

[  128.069528] ipmi_si: IPMI System Interface driver
[  128.069556] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS
[  128.069559] ipmi_platform: ipmi_si: SMBIOS: io 0xe4 regsize 1 spacing 
1 irq 0
[  128.069561] ipmi_si: Adding SMBIOS-specified bt state machine
[  128.069657] ipmi_si hisi-lpc-ipmi.1.auto: ipmi_platform: probing via ACPI
[  128.069694] ipmi_si hisi-lpc-ipmi.1.auto: ipmi_platform: [io 
0xffc0e3-0xffc0e7] regsize 1 spacing 1 irq 0
[  128.069696] ipmi_si hisi-lpc-ipmi.1.auto: Hard-coded device at this 
address already exists
[  128.069815] ipmi_si hardcode-ipmi-si.0: ipmi_platform: probing via 
hardcoded
[  128.069817] ipmi_platform: ipmi_si: hardcoded: io 0xffc0e3 regsize 1 
spacing 1 irq 0
[  128.069819] ipmi_si: Adding hardcoded-specified bt state machine
[  128.069892] ipmi_si: Trying SMBIOS-specified bt state machine at i/o 
address 0xe4, slave address 0x0, irq 0
[  128.069896] ipmi_si dmi-ipmi-si.0: Could not set up I/O space
[  128.069900] ipmi_si: Trying hardcoded-specified bt state machine at 
i/o address 0xffc0e3, slave address 0x0, irq 0
[  128.081782] ipmi_si hardcode-ipmi-si.0: bt cap response too short: 3
[  128.081784] ipmi_si hardcode-ipmi-si.0: using default values
[  128.081786] ipmi_si hardcode-ipmi-si.0: req2rsp=5 secs retries=2
[  128.157855] ipmi_si hardcode-ipmi-si.0: IPMI message handler: Found 
new BMC (man_id: 0x0007db, prod_id: 0x0001, dev_id: 0x01)
[  128.441860] ipmi_si hardcode-ipmi-si.0: IPMI bt interface initialized

On 2019/2/22 2:33, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> When excuting a command like:
>    modprobe ipmi_si ports=0xffc0e3 type=bt
> The system would get an oops.
>
> The trouble here is that ipmi_si_hardcode_find_bmc() is called before
> ipmi_si_platform_init(), but initialization of the hard-coded device
> creates an IPMI platform device, which won't be initialized yet.
>
> The real trouble is that hard-coded devices aren't created with
> any device, and the fixup is done later.  So do it right, create the
> hard-coded devices as normal platform devices.
>
> This required adding some new resource types to the IPMI platform
> code for passing information required by the hard-coded device
> and adding some code to remove the hard-coded platform devices
> on module removal.
>
> To enforce the "hard-coded devices passed by the user take priority
> over firmware devices" rule, some special code was added to check
> and see if a hard-coded device already exists.
>
> Reported-by: Yang Yingliang <yangyingliang@huawei.com>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>
> I believe this is the right fix.  Can you test/review and send the
> results?
>
> Thanks,
>
> -corey
>
>   drivers/char/ipmi/ipmi_si.h          |   4 +-
>   drivers/char/ipmi/ipmi_si_hardcode.c | 236 ++++++++++++++++++++-------
>   drivers/char/ipmi/ipmi_si_intf.c     |  23 ++-
>   drivers/char/ipmi/ipmi_si_platform.c |  25 ++-
>   4 files changed, 214 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
> index 52f6152d1fcb..7ae52c17618e 100644
> --- a/drivers/char/ipmi/ipmi_si.h
> +++ b/drivers/char/ipmi/ipmi_si.h
> @@ -25,7 +25,9 @@ void ipmi_irq_finish_setup(struct si_sm_io *io);
>   int ipmi_si_remove_by_dev(struct device *dev);
>   void ipmi_si_remove_by_data(int addr_space, enum si_type si_type,
>   			    unsigned long addr);
> -int ipmi_si_hardcode_find_bmc(void);
> +void ipmi_hardcode_init(void);
> +void ipmi_si_hardcode_exit(void);
> +int ipmi_si_hardcode_match(int addr_type, unsigned long addr);
>   void ipmi_si_platform_init(void);
>   void ipmi_si_platform_shutdown(void);
>   
> diff --git a/drivers/char/ipmi/ipmi_si_hardcode.c b/drivers/char/ipmi/ipmi_si_hardcode.c
> index 487642809c58..1e5783961b0d 100644
> --- a/drivers/char/ipmi/ipmi_si_hardcode.c
> +++ b/drivers/char/ipmi/ipmi_si_hardcode.c
> @@ -3,6 +3,7 @@
>   #define pr_fmt(fmt) "ipmi_hardcode: " fmt
>   
>   #include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
>   #include "ipmi_si.h"
>   
>   /*
> @@ -12,23 +13,22 @@
>   
>   #define SI_MAX_PARMS 4
>   
> -static char          *si_type[SI_MAX_PARMS];
>   #define MAX_SI_TYPE_STR 30
> -static char          si_type_str[MAX_SI_TYPE_STR];
> +static char          si_type_str[MAX_SI_TYPE_STR] __initdata;
>   static unsigned long addrs[SI_MAX_PARMS];
>   static unsigned int num_addrs;
>   static unsigned int  ports[SI_MAX_PARMS];
>   static unsigned int num_ports;
> -static int           irqs[SI_MAX_PARMS];
> -static unsigned int num_irqs;
> -static int           regspacings[SI_MAX_PARMS];
> -static unsigned int num_regspacings;
> -static int           regsizes[SI_MAX_PARMS];
> -static unsigned int num_regsizes;
> -static int           regshifts[SI_MAX_PARMS];
> -static unsigned int num_regshifts;
> -static int slave_addrs[SI_MAX_PARMS]; /* Leaving 0 chooses the default value */
> -static unsigned int num_slave_addrs;
> +static int           irqs[SI_MAX_PARMS] __initdata;
> +static unsigned int num_irqs __initdata;
> +static int           regspacings[SI_MAX_PARMS] __initdata;
> +static unsigned int num_regspacings __initdata;
> +static int           regsizes[SI_MAX_PARMS] __initdata;
> +static unsigned int num_regsizes __initdata;
> +static int           regshifts[SI_MAX_PARMS] __initdata;
> +static unsigned int num_regshifts __initdata;
> +static int slave_addrs[SI_MAX_PARMS] __initdata;
> +static unsigned int num_slave_addrs __initdata;
>   
>   module_param_string(type, si_type_str, MAX_SI_TYPE_STR, 0);
>   MODULE_PARM_DESC(type, "Defines the type of each interface, each"
> @@ -73,12 +73,133 @@ MODULE_PARM_DESC(slave_addrs, "Set the default IPMB slave address for"
>   		 " overridden by this parm.  This is an array indexed"
>   		 " by interface number.");
>   
> -int ipmi_si_hardcode_find_bmc(void)
> +static struct platform_device *ipmi_hc_pdevs[SI_MAX_PARMS];
> +
> +static void __init ipmi_hardcode_init_one(const char *si_type_str,
> +					  unsigned int i,
> +					  unsigned long addr,
> +					  unsigned int flags)
>   {
> -	int ret = -ENODEV;
> -	int             i;
> -	struct si_sm_io io;
> +	struct platform_device *pdev;
> +	unsigned int num_r = 1, size;
> +	struct resource r[4];
> +	struct property_entry p[6];
> +	enum si_type si_type;
> +	unsigned int regspacing, regsize;
> +	int rv;
> +
> +	memset(p, 0, sizeof(p));
> +	memset(r, 0, sizeof(r));
> +
> +	if (!si_type_str || !*si_type_str || strcmp(si_type_str, "kcs") == 0) {
> +		size = 2;
> +		si_type = SI_KCS;
> +	} else if (strcmp(si_type_str, "smic") == 0) {
> +		size = 2;
> +		si_type = SI_SMIC;
> +	} else if (strcmp(si_type_str, "bt") == 0) {
> +		size = 3;
> +		si_type = SI_BT;
> +	} else if (strcmp(si_type_str, "invalid") == 0) {
> +		/*
> +		 * Allow a firmware-specified interface to be
> +		 * disabled.
> +		 */
> +		size = 1;
> +		si_type = SI_TYPE_INVALID;
> +	} else {
> +		pr_warn("Interface type specified for interface %d, was invalid: %s\n",
> +			i, si_type_str);
> +		return;
> +	}
> +
> +	regsize = regsizes[i];
> +	if (regsize == 0)
> +		regsize = DEFAULT_REGSIZE;
> +
> +	p[0] = PROPERTY_ENTRY_U8("ipmi-type", si_type);
> +	p[1] = PROPERTY_ENTRY_U8("slave-addr", slave_addrs[i]);
> +	p[2] = PROPERTY_ENTRY_U8("addr-source", SI_HARDCODED);
> +	p[3] = PROPERTY_ENTRY_U8("reg-shift", regshifts[i]);
> +	p[4] = PROPERTY_ENTRY_U8("reg-size", regsize);
> +	/* Last entry must be left NULL to terminate it. */
> +
> +	/*
> +	 * Register spacing is derived from the resources in
> +	 * the IPMI platform code.
> +	 */
> +	regspacing = regspacings[i];
> +	if (regspacing == 0)
> +		regspacing = regsize;
> +
> +	r[0].start = addr;
> +	r[0].end = r[0].start + regsize - 1;
> +	r[0].name = "IPMI Address 1";
> +	r[0].flags = flags;
> +
> +	if (size > 1) {
> +		r[1].start = r[0].start + regspacing;
> +		r[1].end = r[1].start + regsize - 1;
> +		r[1].name = "IPMI Address 2";
> +		r[1].flags = flags;
> +		num_r++;
> +	}
> +
> +	if (size > 2) {
> +		r[2].start = r[1].start + regspacing;
> +		r[2].end = r[2].start + regsize - 1;
> +		r[2].name = "IPMI Address 3";
> +		r[2].flags = flags;
> +		num_r++;
> +	}
> +
> +	if (irqs[i]) {
> +		r[num_r].start = irqs[i];
> +		r[num_r].end = irqs[i];
> +		r[num_r].name = "IPMI IRQ";
> +		r[num_r].flags = IORESOURCE_IRQ;
> +		num_r++;
> +	}
> +
> +	pdev = platform_device_alloc("hardcode-ipmi-si", i);
> +	if (!pdev) {
> +		pr_err("Error allocating IPMI platform device %d\n", i);
> +		return;
> +	}
> +
> +	rv = platform_device_add_resources(pdev, r, num_r);
> +	if (rv) {
> +		dev_err(&pdev->dev,
> +			"Unable to add hard-code resources: %d\n", rv);
> +		goto err;
> +	}
> +
> +	rv = platform_device_add_properties(pdev, p);
> +	if (rv) {
> +		dev_err(&pdev->dev,
> +			"Unable to add hard-code properties: %d\n", rv);
> +		goto err;
> +	}
> +
> +	rv = platform_device_add(pdev);
> +	if (rv) {
> +		dev_err(&pdev->dev,
> +			"Unable to add hard-code device: %d\n", rv);
> +		goto err;
> +	}
> +
> +	ipmi_hc_pdevs[i] = pdev;
> +	return;
> +
> +err:
> +	platform_device_put(pdev);
> +}
> +
> +void __init ipmi_hardcode_init(void)
> +{
> +	unsigned int i;
>   	char *str;
> +	char *si_type[SI_MAX_PARMS];
>   
>   	/* Parse out the si_type string into its components. */
>   	str = si_type_str;
> @@ -95,54 +216,45 @@ int ipmi_si_hardcode_find_bmc(void)
>   		}
>   	}
>   
> -	memset(&io, 0, sizeof(io));
>   	for (i = 0; i < SI_MAX_PARMS; i++) {
> -		if (!ports[i] && !addrs[i])
> -			continue;
> -
> -		io.addr_source = SI_HARDCODED;
> -		pr_info("probing via hardcoded address\n");
> -
> -		if (!si_type[i] || strcmp(si_type[i], "kcs") == 0) {
> -			io.si_type = SI_KCS;
> -		} else if (strcmp(si_type[i], "smic") == 0) {
> -			io.si_type = SI_SMIC;
> -		} else if (strcmp(si_type[i], "bt") == 0) {
> -			io.si_type = SI_BT;
> -		} else {
> -			pr_warn("Interface type specified for interface %d, was invalid: %s\n",
> -				i, si_type[i]);
> -			continue;
> -		}
> +		if (i < num_ports && ports[i])
> +			ipmi_hardcode_init_one(si_type[i], i, ports[i],
> +					       IORESOURCE_IO);
> +		if (i < num_addrs && addrs[i])
> +			ipmi_hardcode_init_one(si_type[i], i, addrs[i],
> +					       IORESOURCE_MEM);
> +	}
> +}
>   
> -		if (ports[i]) {
> -			/* An I/O port */
> -			io.addr_data = ports[i];
> -			io.addr_type = IPMI_IO_ADDR_SPACE;
> -		} else if (addrs[i]) {
> -			/* A memory port */
> -			io.addr_data = addrs[i];
> -			io.addr_type = IPMI_MEM_ADDR_SPACE;
> -		} else {
> -			pr_warn("Interface type specified for interface %d, but port and address were not set or set to zero\n",
> -				i);
> -			continue;
> -		}
> +void ipmi_si_hardcode_exit(void)
> +{
> +	unsigned int i;
>   
> -		io.addr = NULL;
> -		io.regspacing = regspacings[i];
> -		if (!io.regspacing)
> -			io.regspacing = DEFAULT_REGSPACING;
> -		io.regsize = regsizes[i];
> -		if (!io.regsize)
> -			io.regsize = DEFAULT_REGSIZE;
> -		io.regshift = regshifts[i];
> -		io.irq = irqs[i];
> -		if (io.irq)
> -			io.irq_setup = ipmi_std_irq_setup;
> -		io.slave_addr = slave_addrs[i];
> -
> -		ret = ipmi_si_add_smi(&io);
> +	for (i = 0; i < SI_MAX_PARMS; i++) {
> +		if (ipmi_hc_pdevs[i])
> +			platform_device_unregister(ipmi_hc_pdevs[i]);
>   	}
> -	return ret;
> +}
> +
> +/*
> + * Returns true of the given address exists as a hardcoded address,
> + * false if not.
> + */
> +int ipmi_si_hardcode_match(int addr_type, unsigned long addr)
> +{
> +	unsigned int i;
> +
> +	if (addr_type == IPMI_IO_ADDR_SPACE) {
> +		for (i = 0; i < num_ports; i++) {
> +			if (ports[i] == addr)
> +				return 1;
> +		}
> +	} else {
> +		for (i = 0; i < num_addrs; i++) {
> +			if (addrs[i] == addr)
> +				return 1;
> +		}
> +	}
> +
> +	return 0;
>   }
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index ae99d6a14789..abbd526626d5 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1865,6 +1865,18 @@ int ipmi_si_add_smi(struct si_sm_io *io)
>   	int rv = 0;
>   	struct smi_info *new_smi, *dup;
>   
> +	/*
> +	 * If the user gave us a hard-coded device at the same
> +	 * address, they presumably want us to use it and not what is
> +	 * in the firmware.
> +	 */
> +	if (io->addr_source != SI_HARDCODED &&
> +	    ipmi_si_hardcode_match(io->addr_type, io->addr_data)) {
> +		dev_info(io->dev,
> +			 "Hard-coded device at this address already exists");
> +		return -ENODEV;
> +	}
> +
>   	if (!io->io_setup) {
>   		if (io->addr_type == IPMI_IO_ADDR_SPACE) {
>   			io->io_setup = ipmi_si_port_setup;
> @@ -2097,7 +2109,7 @@ static int try_smi_init(struct smi_info *new_smi)
>   	return rv;
>   }
>   
> -static int init_ipmi_si(void)
> +static int __init init_ipmi_si(void)
>   {
>   	struct smi_info *e;
>   	enum ipmi_addr_src type = SI_INVALID;
> @@ -2105,11 +2117,9 @@ static int init_ipmi_si(void)
>   	if (initialized)
>   		return 0;
>   
> -	pr_info("IPMI System Interface driver\n");
> +	ipmi_hardcode_init();
>   
> -	/* If the user gave us a device, they presumably want us to use it */
> -	if (!ipmi_si_hardcode_find_bmc())
> -		goto do_scan;
> +	pr_info("IPMI System Interface driver\n");
>   
>   	ipmi_si_platform_init();
>   
> @@ -2121,7 +2131,6 @@ static int init_ipmi_si(void)
>   	   with multiple BMCs we assume that there will be several instances
>   	   of a given type so if we succeed in registering a type then also
>   	   try to register everything else of the same type */
> -do_scan:
>   	mutex_lock(&smi_infos_lock);
>   	list_for_each_entry(e, &smi_infos, link) {
>   		/* Try to register a device if it has an IRQ and we either
> @@ -2307,6 +2316,8 @@ static void cleanup_ipmi_si(void)
>   	list_for_each_entry_safe(e, tmp_e, &smi_infos, link)
>   		cleanup_one_si(e);
>   	mutex_unlock(&smi_infos_lock);
> +
> +	ipmi_si_hardcode_exit();
>   }
>   module_exit(cleanup_ipmi_si);
>   
> diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> index 15cf819f884f..f8f85416d47a 100644
> --- a/drivers/char/ipmi/ipmi_si_platform.c
> +++ b/drivers/char/ipmi/ipmi_si_platform.c
> @@ -128,8 +128,6 @@ ipmi_get_info_from_resources(struct platform_device *pdev,
>   		if (res_second->start > io->addr_data)
>   			io->regspacing = res_second->start - io->addr_data;
>   	}
> -	io->regsize = DEFAULT_REGSIZE;
> -	io->regshift = 0;
>   
>   	return res;
>   }
> @@ -137,7 +135,7 @@ ipmi_get_info_from_resources(struct platform_device *pdev,
>   static int platform_ipmi_probe(struct platform_device *pdev)
>   {
>   	struct si_sm_io io;
> -	u8 type, slave_addr, addr_source;
> +	u8 type, slave_addr, addr_source, regsize, regshift;
>   	int rv;
>   
>   	rv = device_property_read_u8(&pdev->dev, "addr-source", &addr_source);
> @@ -149,7 +147,7 @@ static int platform_ipmi_probe(struct platform_device *pdev)
>   	if (addr_source == SI_SMBIOS) {
>   		if (!si_trydmi)
>   			return -ENODEV;
> -	} else {
> +	} else if (addr_source != SI_HARDCODED) {
>   		if (!si_tryplatform)
>   			return -ENODEV;
>   	}
> @@ -169,11 +167,23 @@ static int platform_ipmi_probe(struct platform_device *pdev)
>   	case SI_BT:
>   		io.si_type = type;
>   		break;
> +	case SI_TYPE_INVALID: /* User disabled this in hardcode. */
> +		return -ENODEV;
>   	default:
>   		dev_err(&pdev->dev, "ipmi-type property is invalid\n");
>   		return -EINVAL;
>   	}
>   
> +	io.regsize = DEFAULT_REGSIZE;
> +	rv = device_property_read_u8(&pdev->dev, "reg-size", &regsize);
> +	if (!rv)
> +	    io.regsize = regsize;
> +
> +	io.regshift = 0;
> +	rv = device_property_read_u8(&pdev->dev, "reg-shift", &regshift);
> +	if (!rv)
> +	    io.regshift = regshift;
> +
>   	if (!ipmi_get_info_from_resources(pdev, &io))
>   		return -EINVAL;
>   
> @@ -193,7 +203,8 @@ static int platform_ipmi_probe(struct platform_device *pdev)
>   
>   	io.dev = &pdev->dev;
>   
> -	pr_info("ipmi_si: SMBIOS: %s %#lx regsize %d spacing %d irq %d\n",
> +	pr_info("ipmi_si: %s: %s %#lx regsize %d spacing %d irq %d\n",
> +		ipmi_addr_src_to_str(addr_source),
>   		(io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
>   		io.addr_data, io.regsize, io.regspacing, io.irq);
>   
> @@ -358,6 +369,9 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
>   		goto err_free;
>   	}
>   
> +	io.regsize = DEFAULT_REGSIZE;
> +	io.regshift = 0;
> +
>   	res = ipmi_get_info_from_resources(pdev, &io);
>   	if (!res) {
>   		rv = -EINVAL;
> @@ -421,6 +435,7 @@ static int ipmi_remove(struct platform_device *pdev)
>   
>   static const struct platform_device_id si_plat_ids[] = {
>       { "dmi-ipmi-si", 0 },
> +    { "hardcode-ipmi-si", 0 },
>       { }
>   };
>