| 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: <TU4PR8401MB03828F31D13FDD425974507487370@TU4PR8401MB0382.NAMPRD84.PROD.OUTLOOK.COM>
Date: Thu, 23 Aug 2018 18:30:20 +0000
From: "Ernst, Justin" <justin.ernst@....com>
To: "minyard@....org" <minyard@....org>
CC: Corey Minyard <cminyard@...sta.com>,
"Banman, Andrew" <abanman@....com>,
"Anderson, Russ" <russ.anderson@....com>,
"openipmi-developer@...ts.sourceforge.net"
<openipmi-developer@...ts.sourceforge.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] ipmi: Rework SMI registration failure
Tested-by: Justin Ernst <justin.ernst@....com>
Passed the testing. Thanks Corey!
-Justin
> -----Original Message-----
> From: Corey Minyard [mailto:tcminyard@...il.com] On Behalf Of
> minyard@....org
> Sent: Wednesday, August 22, 2018 12:20 PM
> To: Ernst, Justin <justin.ernst@....com>
> Cc: Corey Minyard <cminyard@...sta.com>; Banman, Andrew
> <abanman@....com>; Anderson, Russ <russ.anderson@....com>;
> openipmi-developer@...ts.sourceforge.net; linux-kernel@...r.kernel.org
> Subject: [PATCH] ipmi: Rework SMI registration failure
>
> From: Corey Minyard <cminyard@...sta.com>
>
> There were certain situations where ipmi_register_smi() would return a
> failure, but the interface would still be registered and would need to be
> unregistered. This is obviously a bad design and resulted in an oops in certain
> failure cases.
>
> If the interface is started up in ipmi_register_smi(), then an error occurs, shut
> down the interface there so the cleanup can be done properly.
>
> Fix the various smi users, too.
>
> Signed-off-by: Corey Minyard <cminyard@...sta.com>
> Reported-by: Justin Ernst <justin.ernst@....com>
> Cc: Andrew Banman <abanman@....com>
> Cc: Russ Anderson <russ.anderson@....com>
> Cc: openipmi-developer@...ts.sourceforge.net
> Cc: linux-kernel@...r.kernel.org
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 53 +++++++++++++++++++++--------
> --------
> drivers/char/ipmi/ipmi_si_intf.c | 17 +++---------
> drivers/char/ipmi/ipmi_ssif.c | 13 +++------
> 3 files changed, 37 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 46ab1ba..04f64c5 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -3378,39 +3378,45 @@ int ipmi_register_smi(const struct
> ipmi_smi_handlers *handlers,
>
> rv = handlers->start_processing(send_info, intf);
> if (rv)
> - goto out;
> + goto out_err;
>
> rv = __bmc_get_device_id(intf, NULL, &id, NULL, NULL, i);
> if (rv) {
> dev_err(si_dev, "Unable to get the device id: %d\n", rv);
> - goto out;
> + goto out_err_started;
> }
>
> mutex_lock(&intf->bmc_reg_mutex);
> rv = __scan_channels(intf, &id);
> mutex_unlock(&intf->bmc_reg_mutex);
> + if (rv)
> + goto out_err_bmc_reg;
>
> - out:
> - if (rv) {
> - ipmi_bmc_unregister(intf);
> - list_del_rcu(&intf->link);
> - mutex_unlock(&ipmi_interfaces_mutex);
> - synchronize_srcu(&ipmi_interfaces_srcu);
> - cleanup_srcu_struct(&intf->users_srcu);
> - kref_put(&intf->refcount, intf_free);
> - } else {
> - /*
> - * Keep memory order straight for RCU readers. Make
> - * sure everything else is committed to memory before
> - * setting intf_num to mark the interface valid.
> - */
> - smp_wmb();
> - intf->intf_num = i;
> - mutex_unlock(&ipmi_interfaces_mutex);
> + /*
> + * Keep memory order straight for RCU readers. Make
> + * sure everything else is committed to memory before
> + * setting intf_num to mark the interface valid.
> + */
> + smp_wmb();
> + intf->intf_num = i;
> + mutex_unlock(&ipmi_interfaces_mutex);
>
> - /* After this point the interface is legal to use. */
> - call_smi_watchers(i, intf->si_dev);
> - }
> + /* After this point the interface is legal to use. */
> + call_smi_watchers(i, intf->si_dev);
> +
> + return 0;
> +
> + out_err_bmc_reg:
> + ipmi_bmc_unregister(intf);
> + out_err_started:
> + if (intf->handlers->shutdown)
> + intf->handlers->shutdown(intf->send_info);
> + out_err:
> + list_del_rcu(&intf->link);
> + mutex_unlock(&ipmi_interfaces_mutex);
> + synchronize_srcu(&ipmi_interfaces_srcu);
> + cleanup_srcu_struct(&intf->users_srcu);
> + kref_put(&intf->refcount, intf_free);
>
> return rv;
> }
> @@ -3501,7 +3507,8 @@ void ipmi_unregister_smi(struct ipmi_smi *intf)
> }
> srcu_read_unlock(&intf->users_srcu, index);
>
> - intf->handlers->shutdown(intf->send_info);
> + if (intf->handlers->shutdown)
> + intf->handlers->shutdown(intf->send_info);
>
> cleanup_smi_msgs(intf);
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 2194b4c..bb7d8af 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2102,18 +2102,9 @@ static int try_smi_init(struct smi_info *new_smi)
> si_to_str[new_smi->io.si_type]);
>
> WARN_ON(new_smi->io.dev->init_name != NULL);
> - kfree(init_name);
> -
> - return 0;
> -
> -out_err:
> - if (new_smi->intf) {
> - ipmi_unregister_smi(new_smi->intf);
> - new_smi->intf = NULL;
> - }
>
> + out_err:
> kfree(init_name);
> -
> return rv;
> }
>
> @@ -2246,6 +2237,8 @@ static void shutdown_smi(void *send_info)
>
> kfree(smi_info->si_sm);
> smi_info->si_sm = NULL;
> +
> + smi_info->intf = NULL;
> }
>
> /*
> @@ -2259,10 +2252,8 @@ static void cleanup_one_si(struct smi_info
> *smi_info)
>
> list_del(&smi_info->link);
>
> - if (smi_info->intf) {
> + if (smi_info->intf)
> ipmi_unregister_smi(smi_info->intf);
> - smi_info->intf = NULL;
> - }
>
> if (smi_info->pdev) {
> if (smi_info->pdev_registered)
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index fddf646..3574322 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1218,18 +1218,11 @@ static void shutdown_ssif(void *send_info)
> complete(&ssif_info->wake_thread);
> kthread_stop(ssif_info->thread);
> }
> -
> - /*
> - * No message can be outstanding now, we have removed the
> - * upper layer and it permitted us to do so.
> - */
> - kfree(ssif_info);
> }
>
> static int ssif_remove(struct i2c_client *client) {
> struct ssif_info *ssif_info = i2c_get_clientdata(client);
> - struct ipmi_smi *intf;
> struct ssif_addr_info *addr_info;
>
> if (!ssif_info)
> @@ -1239,9 +1232,7 @@ static int ssif_remove(struct i2c_client *client)
> * After this point, we won't deliver anything asychronously
> * to the message handler. We can unregister ourself.
> */
> - intf = ssif_info->intf;
> - ssif_info->intf = NULL;
> - ipmi_unregister_smi(intf);
> + ipmi_unregister_smi(ssif_info->intf);
>
> list_for_each_entry(addr_info, &ssif_infos, link) {
> if (addr_info->client == client) {
> @@ -1250,6 +1241,8 @@ static int ssif_remove(struct i2c_client *client)
> }
> }
>
> + kfree(ssif_info);
> +
> return 0;
> }
>
> --
> 2.7.4
Powered by blists - more mailing lists