| 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: <YG/g/poZLwO34QH7@packtop>
Date: Fri, 9 Apr 2021 05:07:11 +0000
From: Zev Weiss <zweiss@...inix.com>
To: Andrew Jeffery <andrew@...id.au>
CC: "openipmi-developer@...ts.sourceforge.net"
<openipmi-developer@...ts.sourceforge.net>,
"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
"minyard@....org" <minyard@....org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"ryan_chen@...eedtech.com" <ryan_chen@...eedtech.com>,
"tmaimon77@...il.com" <tmaimon77@...il.com>,
"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
"avifishman70@...il.com" <avifishman70@...il.com>,
"venture@...gle.com" <venture@...gle.com>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"tali.perry1@...il.com" <tali.perry1@...il.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"lee.jones@...aro.org" <lee.jones@...aro.org>,
"chiawei_wang@...eedtech.com" <chiawei_wang@...eedtech.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"benjaminfair@...gle.com" <benjaminfair@...gle.com>
Subject: Re: [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy
in the kernel
On Fri, Mar 19, 2021 at 01:27:46AM CDT, Andrew Jeffery wrote:
>Soon it will be possible for one KCS device to have multiple associated
>chardevs exposed to userspace (for IPMI and raw-style access). However,
>don't prevent userspace from:
>
>1. Opening more than one chardev at a time, or
>2. Opening the same chardev more than once.
>
>System behaviour is undefined for both classes of multiple access, so
>userspace must manage itself accordingly.
>
>The implementation delivers IBF and OBF events to the first chardev
>client to associate with the KCS device. An open on a related chardev
>cannot associate its client with the KCS device and so will not
>receive notification of events. However, any fd on any chardev may race
>their accesses to the data and status registers.
>
>Signed-off-by: Andrew Jeffery <andrew@...id.au>
>---
> drivers/char/ipmi/kcs_bmc.c | 34 ++++++++++-------------------
> drivers/char/ipmi/kcs_bmc_aspeed.c | 3 +--
> drivers/char/ipmi/kcs_bmc_npcm7xx.c | 3 +--
> 3 files changed, 14 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index 05bbb72418b2..2fafa9541934 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -55,24 +55,12 @@ EXPORT_SYMBOL(kcs_bmc_update_status);
> int kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
> {
> struct kcs_bmc_client *client;
>- int rc;
>+ int rc = KCS_BMC_EVENT_NONE;
>
> spin_lock(&kcs_bmc->lock);
> client = kcs_bmc->client;
>- if (client) {
>+ if (!WARN_ON_ONCE(!client))
> rc = client->ops->event(client);
The double-negation split by a macro seems a bit confusing to me
readability-wise; could we simplify to something like
if (client)
rc = client->ops->event(client);
else
WARN_ONCE();
?
>- } else {
>- u8 status;
>-
>- status = kcs_bmc_read_status(kcs_bmc);
>- if (status & KCS_BMC_STR_IBF) {
>- /* Ack the event by reading the data */
>- kcs_bmc_read_data(kcs_bmc);
>- rc = KCS_BMC_EVENT_HANDLED;
>- } else {
>- rc = KCS_BMC_EVENT_NONE;
>- }
>- }
> spin_unlock(&kcs_bmc->lock);
>
> return rc;
>@@ -81,26 +69,28 @@ EXPORT_SYMBOL(kcs_bmc_handle_event);
>
> int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
> {
>- int rc;
>-
> spin_lock_irq(&kcs_bmc->lock);
>- if (kcs_bmc->client) {
>- rc = -EBUSY;
>- } else {
>+ if (!kcs_bmc->client) {
>+ u8 mask = KCS_BMC_EVENT_TYPE_IBF;
>+
> kcs_bmc->client = client;
>- rc = 0;
>+ kcs_bmc_update_event_mask(kcs_bmc, mask, mask);
> }
> spin_unlock_irq(&kcs_bmc->lock);
>
>- return rc;
>+ return 0;
Since this function appears to be infallible now, should it just return
void? (Might be more churn than it's worth...shrug.)
> }
> EXPORT_SYMBOL(kcs_bmc_enable_device);
>
> void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
> {
> spin_lock_irq(&kcs_bmc->lock);
>- if (client == kcs_bmc->client)
>+ if (client == kcs_bmc->client) {
>+ u8 mask = KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE;
>+
>+ kcs_bmc_update_event_mask(kcs_bmc, mask, 0);
> kcs_bmc->client = NULL;
>+ }
> spin_unlock_irq(&kcs_bmc->lock);
> }
> EXPORT_SYMBOL(kcs_bmc_disable_device);
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
>index 5f26471c038c..271845eb2e26 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -419,8 +419,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, priv);
>
>- aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE),
>- KCS_BMC_EVENT_TYPE_IBF);
>+ aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0);
> aspeed_kcs_enable_channel(kcs_bmc, true);
>
> rc = kcs_bmc_add_device(&priv->kcs_bmc);
>diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>index c2032728a03d..fdf35cad2eba 100644
>--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>@@ -207,8 +207,7 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
> if (rc)
> return rc;
>
>- npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE),
>- KCS_BMC_EVENT_TYPE_IBF);
>+ npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0);
> npcm7xx_kcs_enable_channel(kcs_bmc, true);
>
> pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n",
>--
>2.27.0
>
Powered by blists - more mailing lists