linux-kernel - Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver

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]

Message-ID: <9b194ad6-d277-44e5-54b0-c7e453a6fb0c@linux.intel.com>
Date:   Fri, 26 Jan 2018 14:26:04 +0800
From:   "Wang, Haiyue" <haiyue.wang@...ux.intel.com>
To:     minyard@....org, joel@....id.au, avifishman70@...il.com,
        openbmc@...ts.ozlabs.org, openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Cc:     andriy.shevchenko@...el.com
Subject: Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC
 driver



On 2018-01-25 01:48, Corey Minyard wrote:
> On 01/24/2018 10:06 AM, Haiyue Wang wrote:
>> The KCS (Keyboard Controller Style) interface is used to perform in-band
>> IPMI communication between a server host and its BMC (BaseBoard 
>> Management
>> Controllers).
>>
>> This driver exposes the KCS interface on ASpeed SOCs (AST2400 and 
>> AST2500)
>> as a character device. Such SOCs are commonly used as BMCs and this 
>> driver
>> implements the BMC side of the KCS interface.
>>
>> Signed-off-by: Haiyue Wang <haiyue.wang@...ux.intel.com>
>>
>> ---
>
>> +
>> +static ssize_t kcs_bmc_read(struct file *filp, char *buf,
>> +                size_t count, loff_t *offset)
>> +{
>> +    struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
>> +    ssize_t ret = -EAGAIN;
>> +
>
> This function still has some issues.
>
> You can't call copy_to_user() with a spinlock held or interrupts 
> disabled.
> To handle readers, you probably need a separate mutex.
>
> Also, this function can return -EAGAIN even if O_NONBLOCK is not set if
> kcs_bmc->data_in_avail changes between when you wait on the event
> and when you check it under the lock.
>
> You also clear data_in_avail even if the copy_to_user() fails, which is
> wrong.
>
> I believe the best way to handle this would be to have the spinlock
> protect the inner workings of the state machine and a mutex handle
> copying data out, setting/clearing the running flag (thus a mutex
> instead of spinlock in open and release) and the ioctl settings (except
> for abort where you will need to grab the spinlock).
>
> After the wait event below, grab the mutex.  If data is not available
> and O_NONBLOCK is not set, drop the mutex and retry.  Otherwise
> this is the only place (besides release) that sets data_in_avail to 
> false.
> Do the copy_to_user(), grab the spinlock, clear data_in_avail and
> data_in_idx, then release the lock and mutex.  If you are really
> adventurous you can do this without grabbing the lock using
> barriers, but it's probably not necessary here.
>
The main race is data_in and data_out memory copy from & to between one 
user-land (ipmid) and
the irq handler. If separates the copy_to_user into two parts: check the 
'access_ok(VERIFY_WRITE, to, n)',
if no errors, then grap the spinlock and irq disabled, then 
'memcpy((void __force *)to, from, n);' It it right
calling ?

I will add a mutex to avoid spinlcok using as possible.
>> +    if (!(filp->f_flags & O_NONBLOCK))
>> +        wait_event_interruptible(kcs_bmc->queue,
>> +                     kcs_bmc->data_in_avail);
>> +
>> +    spin_lock_irq(&kcs_bmc->lock);
>> +
>> +    if (kcs_bmc->data_in_avail) {
>> +        kcs_bmc->data_in_avail = false;
>> +
>> +        if (count > kcs_bmc->data_in_idx)
>> +            count = kcs_bmc->data_in_idx;
>> +
>> +        if (!copy_to_user(buf, kcs_bmc->data_in, count))
>> +            ret = count;
>> +        else
>> +            ret = -EFAULT;
>> +    }
>> +
>> +    spin_unlock_irq(&kcs_bmc->lock);
>> +
>> +    return ret;
>> +}
>> +
>
>> +    }
>> +
>> +    spin_unlock_irq(&kcs_bmc->lock);
>> +
>> +    return ret;
>> +}
>