linux-kernel - Re: [PATCH v18 6/7] 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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Jul 2018 11:45:44 +0000
From:   Horia Geanta <horia.geanta@....com>
To:     Logan Gunthorpe <logang@...tatee.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Fabio Estevam <festevam@...il.com>,
        Aymen Sghaier <aymen.sghaier@....com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Linux-Arch <linux-arch@...r.kernel.org>,
        "linux-ntb@...glegroups.com" <linux-ntb@...glegroups.com>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Dan Douglass <dan.douglass@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when
 using io{read|write}64

On 7/4/2018 2:58 AM, Logan Gunthorpe wrote:
> 
> 
> On 03/07/18 04:21 PM, Andy Shevchenko wrote:
>> It is an explicit call to BUG().
>> That's why we see wrong instruction trap.
> 
> Ok, I think I see the problem... the code is rather confusing:
> 
> Prior to the patch, IOs were BE depending on caam_little_end but if
> caam_imx was set, then it wrote two LE writes with the high one first.
> After the patch, it writes two BE writes with the high one first.
> 
I think there are two separate issues here:

1. Semantics of operations in io-64-nonatomic-lo-hi.h, io-64-nonatomic-hi-lo.h

Logan, you mentioned the following (which unfortunately I somehow missed):
https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@deltatee.com
The lo_hi/hi_lo functions seem to always refer to the data being written
or read not to the address operated on.

OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
asm-generic/io-64-nonatomic-hi-lo.h mentions:
797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
environment")
- <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
the order of lower address -> higher address
- <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
reversed order

I think we should keep the initial semantics when adding support for
io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.

Actually this is the semantics intended for the CAAM patch, see the note at the
end of the commit message that refers to addresses, not values:
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.

2. CAAM driver I/O accessors for i.MX case

CAAM block in some i.MX parts has broken endianness for 64b registers.
For e.g. for i.MX6S/SL/D/Q even though CAAM is little endian, BARs for I/O rings
have to be programmed as:
I/O Ring BAR+0: unused
I/O Ring BAR+4: IOVA (32-bit little endian)
when the proper layout (for a 64b register) would have been to program IOVA at
BAR+0.

This explains why I/O accessors in CAAM driver handle things differently in case
caam_imx=true.

Since this quirk cannot be accommodated in generic fashion, code dealing with
caam_imx has to stay.

Horia