| 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
| ||
|
Message-ID: <20231103145129.000067d8@Huawei.com>
Date: Fri, 3 Nov 2023 14:51:29 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Andrew Jeffery <andrew@...econstruct.com.au>
CC: <minyard@....org>, <openipmi-developer@...ts.sourceforge.net>,
<linux-kernel@...r.kernel.org>, <aladyshev22@...il.com>,
<jk@...econstruct.com.au>
Subject: Re: [PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device
lifetimes
On Fri, 3 Nov 2023 16:45:19 +1030
Andrew Jeffery <andrew@...econstruct.com.au> wrote:
> 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>
Bit more unrelated white space stuff in here that ideally wouldn't be there.
Otherwise makes sense to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> ---
> 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 = {
Powered by blists - more mailing lists