| 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
| ||
|
Date: Tue, 6 Sep 2016 22:50:38 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Hoan Tran <hotran@....com>
Cc: Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
lkml <linux-kernel@...r.kernel.org>,
Itaru Kitayama <itaru.kitayama@...en.jp>, Loc Ho <lho@....com>,
Duc Dang <dhdang@....com>
Subject: Re: [PATCH] hwmon: xgene: Fix crash when alarm occurs before driver
probe
On 09/06/2016 10:21 PM, Hoan Tran wrote:
> Hi Guenter,
>
> Thank for your quick review !
>
> On Tue, Sep 6, 2016 at 9:35 PM, Guenter Roeck <linux@...ck-us.net> wrote:
>> On 09/06/2016 08:46 PM, Hoan Tran wrote:
>>>
>>> The system crashes during probing xgene-hwmon driver when temperature
>>> alarm interrupt occurs before.
>>> It's because
>>> - xgene_hwmon_probe() requests PCC mailbox channel which also enables
>>> the mailbox interrupt.
>>> - As temperature alarm interrupt is pending, ISR runs and crashes when
>>> accesses
>>> into invalid resource as unmapped PCC shared memory.
>>>
>>> This patch fixes this issue by saving this alarm message and scheduling a
>>> bottom handler after xgene_hwmon_probe() finish.
>>>
>>
>> I am not completely happy with this fix. Main problem I have is that the
>> processing associated with resp_pending doesn't happen until init_flag is
>> set.
>> Since the hwmon functions can be called right after
>> hwmon_device_register_with_groups(),
>> there is now a new race condition between that call and setting init_flag.
>
> I think it's still good if hwmon functions are called right after
> hwmon_device_register_with_groups().
> The response message will be queued into FIFO and be processed later.
>
Yes, but the call to complete() won't happen in this case, or am I missing
something ?
Guenter
>>
>> I am also a bit concerned with init_flag and rx_pending not being atomic and
>> protected.
>> What happens if a second interrupt occurs right after init_flag is set but
>> before
>> (or while) rx_pending is evaluated ?
>
> Yah, that's a good catch. I can re-use the "kfifo_lock" spinlock for
> this atomic protection.
>
>>
>> On top of that, init_flag and thus the added complexity is (unless I am
>> missing
>> something) only needed if acpi is enabled. Penaltizing non-acpi code doesn't
>> seem
>> to be optimal.
>
> I think, with DT, we still need this flag. In a case of temperature
> alarm, the driver needs to set "temp1_critical_alarm" sysfs.
> This "temp1_critical_alarm" should be created before "init_flag" = true.
>
> Thanks
> Hoan
>
>>
>> How do other drivers handle this situation ? This must be a common problem
>> with all mbox users.
>>
>> Thanks,
>> Guenter
>>
>>
>>> Signed-off-by: Hoan Tran <hotran@....com>
>>> Reported-by: Itaru Kitayama <itaru.kitayama@...en.jp>
>>> ---
>>> drivers/hwmon/xgene-hwmon.c | 75
>>> +++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 56 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
>>> index bc78a5d..e3b4e84 100644
>>> --- a/drivers/hwmon/xgene-hwmon.c
>>> +++ b/drivers/hwmon/xgene-hwmon.c
>>> @@ -107,6 +107,8 @@ struct xgene_hwmon_dev {
>>> struct completion rd_complete;
>>> int resp_pending;
>>> struct slimpro_resp_msg sync_msg;
>>> + bool init_flag;
>>> + bool rx_pending;
>>>
>>> struct work_struct workq;
>>> struct kfifo_rec_ptr_1 async_msg_fifo;
>>> @@ -465,13 +467,35 @@ static void xgene_hwmon_evt_work(struct work_struct
>>> *work)
>>> }
>>> }
>>>
>>> +static int xgene_hwmon_rx_ready(struct xgene_hwmon_dev *ctx, void *msg)
>>> +{
>>> + if (!ctx->init_flag) {
>>> + ctx->rx_pending = true;
>>> + /* Enqueue to the FIFO */
>>> + kfifo_in_spinlocked(&ctx->async_msg_fifo, msg,
>>> + sizeof(struct slimpro_resp_msg),
>>> + &ctx->kfifo_lock);
>>> + return -EBUSY;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * This function is called when the SLIMpro Mailbox received a message
>>> */
>>> static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg)
>>> {
>>> struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
>>> - struct slimpro_resp_msg amsg;
>>> +
>>> + /*
>>> + * While the driver registers with the mailbox framework, an
>>> interrupt
>>> + * can be pending before the probe function completes its
>>> + * initialization. If such condition occurs, just queue up the
>>> message
>>> + * as the driver is not ready for servicing the callback.
>>> + */
>>> + if (xgene_hwmon_rx_ready(ctx, msg) < 0)
>>> + return;
>>>
>>> /*
>>> * Response message format:
>>> @@ -500,12 +524,8 @@ static void xgene_hwmon_rx_cb(struct mbox_client *cl,
>>> void *msg)
>>> return;
>>> }
>>>
>>> - amsg.msg = ((u32 *)msg)[0];
>>> - amsg.param1 = ((u32 *)msg)[1];
>>> - amsg.param2 = ((u32 *)msg)[2];
>>> -
>>> /* Enqueue to the FIFO */
>>> - kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg,
>>> + kfifo_in_spinlocked(&ctx->async_msg_fifo, msg,
>>> sizeof(struct slimpro_resp_msg),
>>> &ctx->kfifo_lock);
>>> /* Schedule the bottom handler */
>>> schedule_work(&ctx->workq);
>>> @@ -520,6 +540,15 @@ static void xgene_hwmon_pcc_rx_cb(struct mbox_client
>>> *cl, void *msg)
>>> struct acpi_pcct_shared_memory *generic_comm_base =
>>> ctx->pcc_comm_addr;
>>> struct slimpro_resp_msg amsg;
>>>
>>> + /*
>>> + * While the driver registers with the mailbox framework, an
>>> interrupt
>>> + * can be pending before the probe function completes its
>>> + * initialization. If such condition occurs, just queue up the
>>> message
>>> + * as the driver is not ready for servicing the callback.
>>> + */
>>> + if (xgene_hwmon_rx_ready(ctx, &amsg) < 0)
>>> + return;
>>> +
>>> msg = generic_comm_base + 1;
>>> /* Check if platform sends interrupt */
>>> if (!xgene_word_tst_and_clr(&generic_comm_base->status,
>>> @@ -596,6 +625,17 @@ static int xgene_hwmon_probe(struct platform_device
>>> *pdev)
>>> platform_set_drvdata(pdev, ctx);
>>> cl = &ctx->mbox_client;
>>>
>>> + spin_lock_init(&ctx->kfifo_lock);
>>> + mutex_init(&ctx->rd_mutex);
>>> +
>>> + rc = kfifo_alloc(&ctx->async_msg_fifo,
>>> + sizeof(struct slimpro_resp_msg) *
>>> ASYNC_MSG_FIFO_SIZE,
>>> + GFP_KERNEL);
>>> + if (rc)
>>> + goto out_mbox_free;
>>> +
>>> + INIT_WORK(&ctx->workq, xgene_hwmon_evt_work);
>>> +
>>> /* Request mailbox channel */
>>> cl->dev = &pdev->dev;
>>> cl->tx_done = xgene_hwmon_tx_done;
>>> @@ -676,17 +716,6 @@ static int xgene_hwmon_probe(struct platform_device
>>> *pdev)
>>> ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency;
>>> }
>>>
>>> - spin_lock_init(&ctx->kfifo_lock);
>>> - mutex_init(&ctx->rd_mutex);
>>> -
>>> - rc = kfifo_alloc(&ctx->async_msg_fifo,
>>> - sizeof(struct slimpro_resp_msg) *
>>> ASYNC_MSG_FIFO_SIZE,
>>> - GFP_KERNEL);
>>> - if (rc)
>>> - goto out_mbox_free;
>>> -
>>> - INIT_WORK(&ctx->workq, xgene_hwmon_evt_work);
>>> -
>>> ctx->hwmon_dev = hwmon_device_register_with_groups(ctx->dev,
>>> "apm_xgene",
>>> ctx,
>>> @@ -697,17 +726,25 @@ static int xgene_hwmon_probe(struct platform_device
>>> *pdev)
>>> goto out;
>>> }
>>>
>>> + ctx->init_flag = true;
>>> + if (ctx->rx_pending) {
>>> + /*
>>> + * If there is a pending message, schedule the bottom
>>> handler
>>> + */
>>> + schedule_work(&ctx->workq);
>>> + }
>>> +
>>> dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver
>>> registered\n");
>>>
>>> return 0;
>>>
>>> out:
>>> - kfifo_free(&ctx->async_msg_fifo);
>>> -out_mbox_free:
>>> if (acpi_disabled)
>>> mbox_free_channel(ctx->mbox_chan);
>>> else
>>> pcc_mbox_free_channel(ctx->mbox_chan);
>>> +out_mbox_free:
>>> + kfifo_free(&ctx->async_msg_fifo);
>>>
>>> return rc;
>>> }
>>>
>>
>
Powered by blists - more mailing lists