[PATCH] ipmi: ssif: initialize ssif_info->client early

[PATCH] ipmi: ssif: initialize ssif_info->client early

[Mian Yousaf Kaukab]

During probe ssif_info->client is dereferenced in error path. However,
it is set when some of the error checking has already been done. This
causes following kernel crash if an error path is taken:

[   30.645593][  T674] ipmi_ssif 0-000e: ipmi_ssif: Not probing, Interface already present
[   30.657616][  T674] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000088
...
[   30.657723][  T674] pc : __dev_printk+0x28/0xa0
[   30.657732][  T674] lr : _dev_err+0x7c/0xa0
...
[   30.657772][  T674] Call trace:
[   30.657775][  T674]  __dev_printk+0x28/0xa0
[   30.657778][  T674]  _dev_err+0x7c/0xa0
[   30.657781][  T674]  ssif_probe+0x548/0x900 [ipmi_ssif 62ce4b08badc1458fd896206d9ef69a3c31f3d3e]
[   30.657791][  T674]  i2c_device_probe+0x37c/0x3c0
...

Initialize ssif_info->client before any error path can be taken. Clear
i2c_client data in the error path to prevent the dangling pointer from
leaking.

Fixes: c4436c9149c5 ("ipmi_ssif: avoid registering duplicate ssif interface")
Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 drivers/char/ipmi/ipmi_ssif.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 0c62e578749e..48aab77abebf 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1659,6 +1659,9 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		}
 	}
 
+	ssif_info->client = client;
+	i2c_set_clientdata(client, ssif_info);
+
 	rv = ssif_check_and_remove(client, ssif_info);
 	/* If rv is 0 and addr source is not SI_ACPI, continue probing */
 	if (!rv && ssif_info->addr_source == SI_ACPI) {
@@ -1679,9 +1682,6 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		ipmi_addr_src_to_str(ssif_info->addr_source),
 		client->addr, client->adapter->name, slave_addr);
 
-	ssif_info->client = client;
-	i2c_set_clientdata(client, ssif_info);
-
 	/* Now check for system interface capabilities */
 	msg[0] = IPMI_NETFN_APP_REQUEST << 2;
 	msg[1] = IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD;
@@ -1881,6 +1881,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 		dev_err(&ssif_info->client->dev,
 			"Unable to start IPMI SSIF: %d\n", rv);
+		i2c_set_clientdata(client, NULL);
 		kfree(ssif_info);
 	}
 	kfree(resp);
-- 
2.31.1

Re: [PATCH] ipmi: ssif: initialize ssif_info->client early

[Corey Minyard]

On Wed, Dec 08, 2021 at 10:32:39AM +0100, Mian Yousaf Kaukab wrote:
> During probe ssif_info->client is dereferenced in error path. However,
> it is set when some of the error checking has already been done. This
> causes following kernel crash if an error path is taken:
> 
> [   30.645593][  T674] ipmi_ssif 0-000e: ipmi_ssif: Not probing, Interface already present
> [   30.657616][  T674] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000088
> ...
> [   30.657723][  T674] pc : __dev_printk+0x28/0xa0
> [   30.657732][  T674] lr : _dev_err+0x7c/0xa0
> ...
> [   30.657772][  T674] Call trace:
> [   30.657775][  T674]  __dev_printk+0x28/0xa0
> [   30.657778][  T674]  _dev_err+0x7c/0xa0
> [   30.657781][  T674]  ssif_probe+0x548/0x900 [ipmi_ssif 62ce4b08badc1458fd896206d9ef69a3c31f3d3e]
> [   30.657791][  T674]  i2c_device_probe+0x37c/0x3c0
> ...
> 
> Initialize ssif_info->client before any error path can be taken. Clear
> i2c_client data in the error path to prevent the dangling pointer from
> leaking.

Sigh, I just sent some fixes up to Linus, so this was bound to happen :).
This is in my for-next tree, I'll send it up in a few days if everything
is ok.

Thanks,

-corey

> 
> Fixes: c4436c9149c5 ("ipmi_ssif: avoid registering duplicate ssif interface")
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
> ---
>  drivers/char/ipmi/ipmi_ssif.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 0c62e578749e..48aab77abebf 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1659,6 +1659,9 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		}
>  	}
>  
> +	ssif_info->client = client;
> +	i2c_set_clientdata(client, ssif_info);
> +
>  	rv = ssif_check_and_remove(client, ssif_info);
>  	/* If rv is 0 and addr source is not SI_ACPI, continue probing */
>  	if (!rv && ssif_info->addr_source == SI_ACPI) {
> @@ -1679,9 +1682,6 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		ipmi_addr_src_to_str(ssif_info->addr_source),
>  		client->addr, client->adapter->name, slave_addr);
>  
> -	ssif_info->client = client;
> -	i2c_set_clientdata(client, ssif_info);
> -
>  	/* Now check for system interface capabilities */
>  	msg[0] = IPMI_NETFN_APP_REQUEST << 2;
>  	msg[1] = IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD;
> @@ -1881,6 +1881,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  		dev_err(&ssif_info->client->dev,
>  			"Unable to start IPMI SSIF: %d\n", rv);
> +		i2c_set_clientdata(client, NULL);
>  		kfree(ssif_info);
>  	}
>  	kfree(resp);
> -- 
> 2.31.1
>