linux-kernel - Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by ssif

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: <c25ed354-5971-389a-0439-369171b5aabc@acm.org>
Date:   Fri, 24 Aug 2018 08:07:44 -0500
From:   Corey Minyard <minyard@....org>
To:     George Cherian <george.cherian@...ium.com>,
        linux-kernel@...r.kernel.org,
        openipmi-developer@...ts.sourceforge.net
Cc:     arnd@...db.de, gregkh@...uxfoundation.org
Subject: Re: [PATCH 1/2] ipmi_ssif: Unregister i2c device only if created by
 ssif

On 08/24/2018 06:10 AM, George Cherian wrote:
> In ssif_probe error path the i2c client is left hanging, so that
> ssif_platform_remove will remove the client. But it is quite
> possible that ssif would never call an i2c_new_device.
> This condition would lead to kernel crash as below.
> To fix this leave only the client ssif registered hanging in error
> path. All other non-registered clients are set to NULL.

I'm having a hard time seeing how this could happen.

The i2c_new_device() call is only done in the case of dmi_ipmi_probe 
(called from
ssif_platform_probe) or a hard-coded entry.  How does 
ssif_platform_remove get
called on a device that was not registered with ssif_platform_probe?

Small style comment inline...

>   CPU: 107 PID: 30266 Comm: rmmod Not tainted 4.18.0+ #80
>   Hardware name: Cavium Inc. Saber/Saber, BIOS Cavium reference firmware version 7.0 08/04/2018
>   pstate: 60400009 (nZCv daif +PAN -UAO)
>   pc : kernfs_find_ns+0x28/0x120
>   lr : kernfs_find_and_get_ns+0x40/0x60
>   sp : ffff00002310fb50
>   x29: ffff00002310fb50 x28: ffff800a8240f800
>   x27: 0000000000000000 x26: 0000000000000000
>   x25: 0000000056000000 x24: ffff000009073000
>   x23: ffff000008998b38 x22: 0000000000000000
>   x21: ffff800ed86de820 x20: 0000000000000000
>   x19: ffff00000913a1d8 x18: 0000000000000000
>   x17: 0000000000000000 x16: 0000000000000000
>   x15: 0000000000000000 x14: 5300737265766972
>   x13: 643d4d4554535953 x12: 0000000000000030
>   x11: 0000000000000030 x10: 0101010101010101
>   x9 : ffff800ea06cc3f9 x8 : 0000000000000000
>   x7 : 0000000000000141 x6 : ffff000009073000
>   x5 : ffff800adb706b00 x4 : 0000000000000000
>   x3 : 00000000ffffffff x2 : 0000000000000000
>   x1 : ffff000008998b38 x0 : ffff000008356760
>   Process rmmod (pid: 30266, stack limit = 0x00000000e218418d)
>   Call trace:
>    kernfs_find_ns+0x28/0x120
>    kernfs_find_and_get_ns+0x40/0x60
>    sysfs_unmerge_group+0x2c/0x6c
>    dpm_sysfs_remove+0x34/0x70
>    device_del+0x58/0x30c
>    device_unregister+0x30/0x7c
>    i2c_unregister_device+0x84/0x90 [i2c_core]
>    ssif_platform_remove+0x38/0x98 [ipmi_ssif]
>    platform_drv_remove+0x2c/0x6c
>    device_release_driver_internal+0x168/0x1f8
>    driver_detach+0x50/0xbc
>    bus_remove_driver+0x74/0xe8
>    driver_unregister+0x34/0x5c
>    platform_driver_unregister+0x20/0x2c
>    cleanup_ipmi_ssif+0x50/0xd82c [ipmi_ssif]
>    __arm64_sys_delete_module+0x1b4/0x220
>    el0_svc_handler+0x104/0x160
>    el0_svc+0x8/0xc
>   Code: aa1e03e0 aa0203f6 aa0103f7 d503201f (7940e280)
>   ---[ end trace 09f0e34cce8e2d8c ]---
>   Kernel panic - not syncing: Fatal exception
>   SMP: stopping secondary CPUs
>   Kernel Offset: disabled
>   CPU features: 0x23800c38
>
> Signed-off-by: George Cherian <george.cherian@...ium.com>
> ---
>   drivers/char/ipmi/ipmi_ssif.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 18e4650..ccdf6b1 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -181,6 +181,7 @@ struct ssif_addr_info {
>   	struct device *dev;
>   	struct i2c_client *client;
>   
> +	bool client_registered;
>   	struct mutex clients_mutex;
>   	struct list_head clients;
>   
> @@ -1658,6 +1659,8 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   		 * the client like it should.
>   		 */
>   		dev_err(&client->dev, "Unable to start IPMI SSIF: %d\n", rv);
> +		if (!addr_info->client_registered)
> +			addr_info->client = NULL;
>   		kfree(ssif_info);
>   	}
>   	kfree(resp);
> @@ -1672,11 +1675,14 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   static int ssif_adapter_handler(struct device *adev, void *opaque)
>   {
>   	struct ssif_addr_info *addr_info = opaque;
> +	struct i2c_client *client;
>   
>   	if (adev->type != &i2c_adapter_type)
>   		return 0;
>   
> -	i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
> +	client = i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo);
> +	if (client)
> +		addr_info->client_registered = true;
>   

How about..
    if (i2c_new_device(to_i2c_adapter(adev), &addr_info->binfo))
        addr_info->client_registered = true;

No need for the client variable.

-corey

>   	if (!addr_info->adapter_name)
>   		return 1; /* Only try the first I2C adapter by default. */