linux-kernel - Re: [PATCH v6 05/14] crytpo: caam - make use of iowrite64*_hi_lo in wr

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:   Mon, 29 Jul 2019 15:29:36 +0000
From:   Horia Geanta <horia.geanta@....com>
To:     Andrey Smirnov <andrew.smirnov@...il.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>
CC:     Chris Spencer <christopher.spencer@....co.uk>,
        Cory Tusar <cory.tusar@....aero>,
        Chris Healy <cphealy@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Aymen Sghaier <aymen.sghaier@....com>,
        Leonard Crestez <leonard.crestez@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH v6 05/14] crytpo: caam - make use of iowrite64*_hi_lo in
 wr_reg64

On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> In order to be able to unify 64 and 32 bit implementations of
> wr_reg64, let's convert it to use helpers from
> <linux/io-64-nonatomic-hi-lo.h> first. Here are the steps of the
> transformation:
> 
> 1. Inline wr_reg32 helpers:
> 
> 	if (!caam_imx && caam_little_end) {
> 		if (caam_little_end) {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg) + 1);
> 			iowrite32(data, (u32 __iomem *)(reg));
> 		} else {
> 			iowrite32be(data >> 32, (u32 __iomem *)(reg) + 1);
> 			iowrite32be(data, (u32 __iomem *)(reg));
> 		}
> 	} else {
> 		if (caam_little_end) {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg));
> 			iowrite32(data, (u32 __iomem *)(reg) + 1);
> 		} else {
> 			iowrite32be(data >> 32, (u32 __iomem *)(reg));
> 			iowrite32be(data, (u32 __iomem *)(reg) + 1);
> 		}
> 	}
> 
> 2. Transfrom the conditionals such that the check for
> 'caam_little_end' is at the top level:
> 
> 	if (caam_little_end) {
> 		if (!caam_imx) {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg) + 1);
> 			iowrite32(data, (u32 __iomem *)(reg));
> 		} else {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg));
> 			iowrite32(data, (u32 __iomem *)(reg) + 1);
> 		}
> 	} else {
> 		iowrite32be(data >> 32, (u32 __iomem *)(reg));
> 		iowrite32be(data, (u32 __iomem *)(reg) + 1);
> 	}
> 
> 3. Invert the check for !caam_imx:
> 
> 	if (caam_little_end) {
> 		if (caam_imx) {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg));
> 			iowrite32(data, (u32 __iomem *)(reg) + 1);
> 		} else {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg) + 1);
> 			iowrite32(data, (u32 __iomem *)(reg));
> 		}
> 	} else {
> 		iowrite32be(data >> 32, (u32 __iomem *)(reg));
> 		iowrite32be(data, (u32 __iomem *)(reg) + 1);
> 	}
> 
> 4. Make use of iowrite64* helpers from <linux/io-64-nonatomic-hi-lo.h>
> 
> 	if (caam_little_end) {
> 		if (caam_imx) {
> 			iowrite32(data >> 32, (u32 __iomem *)(reg));
> 			iowrite32(data, (u32 __iomem *)(reg) + 1);
> 		} else {
> 			iowrite64(data, reg);
> 		}
> 	} else {
> 		iowrite64be(data, reg);
> 	}
> 
> No functional change intended.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
Reviewed-by: Horia Geantă <horia.geanta@....com>

Just to clarify one thing.

For a previous patch I mentioned:
> 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.
https://lore.kernel.org/linux-crypto/VI1PR0401MB259145C2DFDB5E4084EA5DFC98D20@VI1PR0401MB2591.eurprd04.prod.outlook.com/

I've checked again and actually there is no limitation wrt. the order in which
the two 32-bit parts of 64-bit registers are read from / written to,
except for performance counters (only available on DN parts, not on i.MX).
However, performance counters do not user {rd,wr}_reg64 and should be fixed
separately.

In conclusion, it's ok to use either hi_lo or lo_hi semantics (which is _data_
semantics btw).
It makes more sense for this patch to include io-64-nonatomic-hi-lo.h since
that's what regs.h currently uses.

Horia