linux-kernel - Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG

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] [day] [month] [year] [list]

Date:   Thu, 29 Jun 2017 07:52:04 +0000
From:   Horia Geantă <horia.geanta@....com>
To:     Logan Gunthorpe <logang@...tatee.com>,
        Arnd Bergmann <arnd@...db.de>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        "linux-ntb@...glegroups.com" <linux-ntb@...glegroups.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jyri Sarha <jsarha@...com>,
        Stephen Bates <sbates@...thlin.com>,
        Dan Douglass <dan.douglass@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when
 using io{read|write}64

On 6/28/2017 7:51 PM, Logan Gunthorpe wrote:
> 
> 
> On 28/06/17 04:20 AM, Arnd Bergmann wrote:
>> On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@...tatee.com> wrote:
>>>  #include <linux/types.h>
>>>  #include <linux/bitops.h>
>>> -#include <linux/io.h>
>>> +#include <linux/io-64-nonatomic-hi-lo.h>
>>
>> Here you include the hi-lo variant unconditionally.
>>
Arnd, thanks for spotting this.

This was not in the patch I signed off.
The lo-hi variant should be used instead for CAAM, see further below.

>>> -#else /* CONFIG_64BIT */
>>> -static inline void wr_reg64(void __iomem *reg, u64 data)
>>> -{
>>> -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
>>> -       if (caam_little_end) {
>>> -               wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
>>> -               wr_reg32((u32 __iomem *)(reg), data);
>>> -       } else
>>>  #endif
>>> -       {
>>> -               wr_reg32((u32 __iomem *)(reg), data >> 32);
>>> -               wr_reg32((u32 __iomem *)(reg) + 1, data);
>>> -       }
>>> +               iowrite64be(data, reg);
>>>  }
>>
>> However, the #else path here uses lo-hi instead. I guess we have
>> to decide how to define iowrite64be_lo_hi() first: it could
>> either byteswap the 64-bit value first, then write the two halves,
>> or it could write the two halves, doing a 32-bit byte swap on
>> each.
> 
> Ok, I studied this a bit more:
> 
> The lo_hi/hi_lo functions seem to always refer to the data being written
> or read not to the address operated on. So, in the v3 version of this
> set, which I'm working on, I've defined:
> 
> static inline void iowrite64_hi_lo(u64 val, void __iomem *addr)
> {
>         iowrite32(val >> 32, addr + sizeof(u32));
>         iowrite32(val, addr);
> }
> 
> static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr)
> {
>         iowrite32be(val >> 32, addr);
>         iowrite32be(val, addr + sizeof(u32));
> }
> 
> So the two hi_lo functions match both paths of the #if and thus, I
> believe, the patch will be correct in v3 without changes.
> 
To be consistent with CAAM engine HW spec: in case of 64-bit registers,
irrespective of device endianness, the lower address should be read from
/ written to first, followed by the upper address.

Indeed the I/O accessors in CAAM driver currently don't follow the spec,
however this is a good opportunity to fix the code.
I don't consider this requires a separate patch, as we haven't noticed
any problem. I'd say a simple note in the commit message mentioning the
change (lo-hi r/w order replacing hi-lo for little endian case) is enough.

Thanks,
Horia