linux-kernel - Re: [PATCH 2/2] [v2] crypto: hisilicon

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]

Date:   Fri, 20 Sep 2019 15:16:41 +0100
From:   John Garry <john.garry@...wei.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Zhou Wang <wangzhou1@...ilicon.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Kenneth Lee <liguozhu@...ilicon.com>,
        "Mao Wenan" <maowenan@...wei.com>, Hao Fang <fanghao11@...wei.com>,
        Shiju Jose <shiju.jose@...wei.com>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Will Deacon <will@...nel.org>
Subject: Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86

On 20/09/2019 14:36, Arnd Bergmann wrote:
> On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <arnd@...db.de> wrote:
>>
>> On Fri, Sep 20, 2019 at 10:34 AM John Garry <john.garry@...wei.com> wrote:
>>
>>>> +     if (!IS_ENABLED(CONFIG_ARM64)) {
>>>> +             memcpy_toio(fun_base, src, 16);
>>>> +             wmb();
>>>> +             return;
>>>> +     }
>>>> +
>>>>       asm volatile("ldp %0, %1, %3\n"
>>>>                    "stp %0, %1, %2\n"
>>>>                    "dsb sy\n"
>>>>
>>>
>>> As I understand, this operation needs to be done atomically. So - even
>>> though your change is just for compile testing - the memcpy_to_io() may
>>> not do the same thing on other archs, right?
>>>
>>> I just wonder if it's right to make that change, or at least warn the
>>> imaginary user of possible malfunction for !arm64.
>>

Hi Arnd,

>> It's probably not necessary here. From what I can tell from the documentation,
>> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
>> don't guarantee that an stp arrives on the bus in one piece either.
>>
>> Usually, hardware like this has no hard requirement on an atomic store,
>> it just needs the individual bits to arrive in a particular order, and then
>> triggers the update on the last bit that gets stored. If that is the case here
>> as well, it might actually be better to use two writeq_relaxed() and
>> a barrier. This would also solve the endianess issue.
>
> See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
> to introduce 128-bit MMIO accessors, this got rejected since they
> are not atomic even on ARMv8.4.

So this is proprietary IP integrated with a proprietary ARMv8 
implementation, so there could be a tight coupling, the like of which 
Will mentioned in that thread, but I'm doubtful.

I'm looking at the electronically translated documentation on this HW, 
and it reads "The Mailbox operation performed by the CPU cannot be 
interleaved", and then tells that software should lock against 
concurrent accesses or alternatively use a 128-bit access. So it seems 
that the 128b op used is only to guarantee software is atomic.

Wang Zhou can confirm my understanding.

If true, I see that we seem to be already guaranteeing mutual exclusion 
in qm_mb(), in taking a mutex.

Thanks,
John


>
>     Arnd
>
> .
>