linux-kernel - [PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device lifetimes

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]

Date:   Fri,  3 Nov 2023 16:45:19 +1030
From:   Andrew Jeffery <andrew@...econstruct.com.au>
To:     minyard@....org, openipmi-developer@...ts.sourceforge.net
Cc:     Andrew Jeffery <andrew@...econstruct.com.au>,
        linux-kernel@...r.kernel.org, Jonathan.Cameron@...wei.com,
        aladyshev22@...il.com, jk@...econstruct.com.au
Subject: [PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device lifetimes

KCS client modules may be removed by actions unrelated to KCS devices.
As usual, removing a KCS client module requires unbinding all client
instances from the underlying devices to prevent further use of the code.

Previously, KCS client resource lifetimes were tied to the underlying
KCS device instance with the use of `devm_k[mz]alloc()` APIs. This
requires the use of `devm_free()` as a consequence. It's necessary to
scrutinise use of explicit `devm_free()`s because they generally
indicate there's a concerning corner-case in play, but that's not really
the case here. Switch to explicit kmalloc()/kfree() to align
expectations with the intent of the code.

Signed-off-by: Andrew Jeffery <andrew@...econstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 28 ++++++++++++++++++---------
 drivers/char/ipmi/kcs_bmc_serio.c     | 20 ++++++++++++-------
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 45ac930172ec..98f231f24c26 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -470,7 +470,7 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 	struct kcs_bmc_ipmi *priv;
 	int rc;
 
-	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -482,26 +482,35 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 	priv->client.ops = &kcs_bmc_ipmi_client_ops;
 
 	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
-	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
-					   kcs_bmc->channel);
-	if (!priv->miscdev.name)
-		return -EINVAL;
+	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
+	if (!priv->miscdev.name) {
+		rc = -ENOMEM;
+		goto cleanup_priv;
+	}
 
 	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
 
 	rc = misc_register(&priv->miscdev);
 	if (rc) {
-		dev_err(kcs_bmc->dev, "Unable to register device: %d\n", rc);
-		return rc;
+		pr_err("Unable to register device: %d\n", rc);
+		goto cleanup_miscdev_name;
 	}
 
 	spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
 	list_add(&priv->entry, &kcs_bmc_ipmi_instances);
 	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
 
-	dev_info(kcs_bmc->dev, "Initialised IPMI client for channel %d", kcs_bmc->channel);
+	pr_info("Initialised IPMI client for channel %d\n", kcs_bmc->channel);
 
 	return 0;
+
+cleanup_miscdev_name:
+	kfree(priv->miscdev.name);
+
+cleanup_priv:
+	kfree(priv);
+
+	return rc;
 }
 
 static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
@@ -523,7 +532,8 @@ static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 
 	misc_deregister(&priv->miscdev);
 	kcs_bmc_disable_device(&priv->client);
-	devm_kfree(kcs_bmc->dev, priv);
+	kfree(priv->miscdev.name);
+	kfree(priv);
 }
 
 static const struct kcs_bmc_driver_ops kcs_bmc_ipmi_driver_ops = {
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 713e847bbc4e..0a68c76da955 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -71,15 +71,18 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 {
 	struct kcs_bmc_serio *priv;
 	struct serio *port;
+	int rc;
 
-	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
 	port = kzalloc(sizeof(*port), GFP_KERNEL);
-	if (!port)
-		return -ENOMEM;
+	if (!port) {
+		rc = -ENOMEM;
+		goto cleanup_priv;
+	}
 
 	port->id.type = SERIO_8042;
 	port->open = kcs_bmc_serio_open;
@@ -98,9 +101,14 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 
 	serio_register_port(port);
 
-	dev_info(kcs_bmc->dev, "Initialised serio client for channel %d", kcs_bmc->channel);
+	pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
 
 	return 0;
+
+cleanup_priv:
+	kfree(priv);
+
+	return rc;
 }
 
 static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
@@ -122,11 +130,9 @@ static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
 
 	/* kfree()s priv->port via put_device() */
 	serio_unregister_port(priv->port);
-
 	/* Ensure the IBF IRQ is disabled if we were the active client */
 	kcs_bmc_disable_device(&priv->client);
-
-	devm_kfree(priv->client.dev->dev, priv);
+	kfree(priv);
 }
 
 static const struct kcs_bmc_driver_ops kcs_bmc_serio_driver_ops = {
-- 
2.39.2