[PATCH] crypto: tcrypt - reschedule during speed tests
Avoid RCU stalls in the case of non-preemptible kernel and lengthy
speed tests by rescheduling when advancing from one block size
to another.
Signed-off-by: Horia Geantă
---
crypto/tcrypt.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 078ec36007bf..bdde95e8d369 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -415,12 +415,14 @@ static void test_mb_aead_speed(const char *algo, int enc,
int secs,
}
- if (secs)
+ if (secs) {
ret = test_mb_aead_jiffies(data, enc, *b_size,
secs, num_mb);
- else
+ cond_resched();
+ } else {
ret = test_mb_aead_cycles(data, enc, *b_size,
num_mb);
+ }
if (ret) {
pr_err("%s() failed return code=%d\n", e, ret);
@@ -660,11 +662,13 @@ static void test_aead_speed(const char *algo, int enc,
unsigned int secs,
*b_size + (enc ? 0 : authsize),
iv);
- if (secs)
+ if (secs) {
ret = test_aead_jiffies(req, enc, *b_size,
secs);
- else
+ cond_resched();
+ } else {
ret = test_aead_cycles(req, enc, *b_size);
+ }
if (ret) {
pr_err("%s() failed return code=%d\n", e, ret);
@@ -876,11 +880,13 @@ static void test_mb_ahash_speed(const char *algo,
unsigned int secs,
i, speed[i].blen, speed[i].plen,
speed[i].blen / speed[i].plen);
- if (secs)
+ if (secs) {
ret = test_mb_ahash_jiffies(data, speed[i].blen, secs,
num_mb);
- else
+ cond_resched();
+ } else {
ret = test_mb_ahash_cycles(data, speed[i].blen, num_mb);
+ }
if (ret) {
@@ -1103,12 +1109,14 @@ static void test_ahash_speed_common(const char *algo,
unsigned int secs,
ahash_request_set_crypt(req, sg, output, speed[i].plen);
- if (secs)
+ if (secs) {
ret = test_ahash_jiffies(req, speed[i].blen,
speed[i].plen, output, secs);
- else
+ cond_resched();
+ } else {
ret = test_ahash_cycles(req, speed[i].blen,
speed[i].plen, output);
+ }
if (ret) {
pr_err("hashing failed ret=%d\n", ret);
@@ -1367,13 +1375,15 @@ static void test_mb_skcipher_speed(const char *algo,
int enc, int secs,
iv);
}
- if (secs)
+ if (secs) {
ret = test_mb_acipher_jiffies(data, enc,
*b_size, secs,
num_mb);
- else
+ cond_resched();
+ } else {
ret = test_mb_acipher_cycles(data, enc,
*b_size, num_mb);
+ }
if (ret) {
pr_err("%s() failed flags=%x\n", e,
@@ -1581,12 +1591,14 @@ static void test_skcipher_speed(const char *algo, int
enc, unsigned int secs,
skcipher_request_set_crypt(req, sg, sg, *b_size, iv);
- if (secs)
+ if (secs) {
ret = test_acipher_jiffies(req, enc,
*b_size, secs);
- else
+ cond_resched();
+ } else {
ret = test_acipher_cycles(req, enc,
*b_size);
+ }
if (ret) {
pr_err("%s() failed flags=%x\n", e,
--
2.16.2
Re: [PATCH 2/3] crypto: hisilicon SEC security accelerator driver
On Fri, 20 Jul 2018 20:17:22 +0200
Stephan Müller wrote:
> Am Montag, 16. Juli 2018, 12:43:41 CEST schrieb Jonathan Cameron:
>
> Hi Jonathan,
>
> > +static int sec_alg_skcipher_setkey_aes_xts(struct crypto_skcipher *tfm,
> > + const u8 *key, unsigned int
> > keylen) +{
> > + enum sec_cipher_alg alg;
> > +
> > + switch (keylen) {
> > + case AES_KEYSIZE_128 * 2:
> > + alg = SEC_C_AES_XTS_128;
> > + break;
> > + case AES_KEYSIZE_256 * 2:
> > + alg = SEC_C_AES_XTS_256;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return sec_alg_skcipher_setkey(tfm, key, keylen, alg);
> > +}
>
> Can you please call the function xts_check_key or xts_verify_key before
> setting the key?
>
Will do.
Thanks,
Jonathan
[PATCH V2 0/3] Hisilicon SEC crypto driver (hip06 / hip07)
The driver provides in kernel support for the Hisilicon SEC accelerator found in the hip06 and hip07 SoCs. There are 4 such units on the D05 board for which an appropriate DT binding has been provided. ACPI also works with an appropriate UEFI build. The hardware does not update the IV in chaining or counting modes. This is done in the drive ron completion of the cipher operation. The driver support AES, DES and 3DES block ciphers in a range of modes (others to follow). Hash and AAED support to follow. Sorry for the delay on this one, other priorities and all that... Changes since V1. 1) DT binding fixes suggested by Rob Herring in patches 1 and 3. 2) Added XTS key check as suggested by Stephan Muller. 3) A trivial use after free found during testing of the above. Changes since RFC. 1) Addition of backlog queuing as needed to support dm-crypt usecases. 2) iommu presence tests now done as Robin Murphy suggested. 3) Hardware limiation to 32MB requests worked aroud in driver so it will now support very large requests (512*32MB). Larger request handling than this would require a longer queue with the associate overheads and is considered unlikely to be necessary. 4) The specific handling related to the inline IV patch set from Stephan has been dropped for now. 5) Interrupt handler was previous more complex than necessary so has been reworked. 6) Use of the bounce buffer for small packeets is dropped for now. This is a performance optimization that made the code harder to review and can be reintroduced as necessary at a later date. 7) Restructuring of some code to simplify hash and aaed (hash implemented but not ready fo upstream at this time) 8) Various minor fixes and reworks of the code * several off by one errors in the cleanup paths * single template for enc and dec * drop dec_key as not used (enc_key was used in both cases) * drop dma pool for IVs as it breaks chaining. * lots of spinlocks changed to mutexes as not taken in atomic context. * nasty memory leak cleaned up. Jonathan Cameron (3): dt-bindings: Add bindings for Hisilicon SEC crypto accelerators. crypto: hisilicon SEC security accelerator driver arm64: dts: hisi: add SEC crypto accelerator nodes for hip07 SoC .../bindings/crypto/hisilicon,hip07-sec.txt| 67 + arch/arm64/boot/dts/hisilicon/hip07.dtsi | 284 + drivers/crypto/Kconfig |2 + drivers/crypto/Makefile|1 + drivers/crypto/hisilicon/Kconfig | 14 + drivers/crypto/hisilicon/Makefile |2 + drivers/crypto/hisilicon/sec/Makefile |3 + drivers/crypto/hisilicon/sec/sec_algs.c| 1122 + drivers/crypto/hisilicon/sec/sec_drv.c | 1323 drivers/crypto/hisilicon/sec/sec_drv.h | 428 +++ 10 files changed, 3246 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/hisilicon,hip07-sec.txt create mode 100644 drivers/crypto/hisilicon/Kconfig create mode 100644 drivers/crypto/hisilicon/Makefile create mode 100644 drivers/crypto/hisilicon/sec/Makefile create mode 100644 drivers/crypto/hisilicon/sec/sec_algs.c create mode 100644 drivers/crypto/hisilicon/sec/sec_drv.c create mode 100644 drivers/crypto/hisilicon/sec/sec_drv.h -- 2.16.2
[PATCH 3/3] arm64: dts: hisi: add SEC crypto accelerator nodes for hip07 SoC
Enable all 4 SEC units available on d05 boards.
Signed-off-by: Jonathan Cameron
---
arch/arm64/boot/dts/hisilicon/hip07.dtsi | 284 +++
1 file changed, 284 insertions(+)
diff --git a/arch/arm64/boot/dts/hisilicon/hip07.dtsi
b/arch/arm64/boot/dts/hisilicon/hip07.dtsi
index 0600a6a84ab7..6d046f4f7019 100644
--- a/arch/arm64/boot/dts/hisilicon/hip07.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hip07.dtsi
@@ -1049,7 +1049,74 @@
num-pins = <2>;
};
};
+ p0_mbigen_alg_a:interrupt-controller@d008 {
+ compatible = "hisilicon,mbigen-v2";
+ reg = <0x0 0xd008 0x0 0x1>;
+ p0_mbigen_sec_a: intc_sec {
+ msi-parent = <&p0_its_dsa_a 0x40400>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <33>;
+ };
+ p0_mbigen_smmu_alg_a: intc_smmu_alg {
+ msi-parent = <&p0_its_dsa_a 0x40b1b>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <3>;
+ };
+ };
+ p0_mbigen_alg_b:interrupt-controller@8,d008 {
+ compatible = "hisilicon,mbigen-v2";
+ reg = <0x8 0xd008 0x0 0x1>;
+
+ p0_mbigen_sec_b: intc_sec {
+ msi-parent = <&p0_its_dsa_b 0x42400>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <33>;
+ };
+ p0_mbigen_smmu_alg_b: intc_smmu_alg {
+ msi-parent = <&p0_its_dsa_b 0x42b1b>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <3>;
+ };
+ };
+ p1_mbigen_alg_a:interrupt-controller@400,d008 {
+ compatible = "hisilicon,mbigen-v2";
+ reg = <0x400 0xd008 0x0 0x1>;
+
+ p1_mbigen_sec_a: intc_sec {
+ msi-parent = <&p1_its_dsa_a 0x44400>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <33>;
+ };
+ p1_mbigen_smmu_alg_a: intc_smmu_alg {
+ msi-parent = <&p1_its_dsa_a 0x44b1b>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <3>;
+ };
+ };
+ p1_mbigen_alg_b:interrupt-controller@408,d008 {
+ compatible = "hisilicon,mbigen-v2";
+ reg = <0x408 0xd008 0x0 0x1>;
+
+ p1_mbigen_sec_b: intc_sec {
+ msi-parent = <&p1_its_dsa_b 0x46400>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <33>;
+ };
+ p1_mbigen_smmu_alg_b: intc_smmu_alg {
+ msi-parent = <&p1_its_dsa_b 0x46b1b>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ num-pins = <3>;
+ };
+ };
p0_mbigen_dsa_a: interrupt-controller@c008 {
compatible = "hisilicon,mbigen-v2";
reg = <0x0 0xc008 0x0 0x1>;
@@ -1107,6 +1174,58 @@
hisilicon,broken-prefetch-cmd;
status = "disabled";
};
+ p0_smmu_alg_a: smmu_alg@d004 {
+ compatible = "arm,smmu-v3";
+ reg = <0x0 0xd004 0x0 0x2>;
+ interrupt-parent = <&p0_mbigen_smmu_alg_a>;
+ interrupts = <733 1>,
+ <734 1>,
+ <735 1>;
+ interrupt-names = "eventq", "gerror", "priq";
+ #iommu-cells = <1>;
+ dma-coherent;
+ hisilicon,broken-prefetch-cmd;
+ /* smmu-cb-memtype = <0x0 0x1>;*/
+ };
+ p0_smmu_alg_b: smmu_alg@8,d004 {
+ compatible = "arm,smmu-v3";
+ reg = <0x8 0xd004 0x0 0x2>;
+ interrupt-parent = <&p0_mbigen_smmu_alg_b>;
+ interrupts = <733 1>,
+ <734 1>,
+ <735 1>;
+ interrupt-names = "eventq", "gerror", "priq";
+ #iommu-cells = <1>;
+ dma-coherent;
+ hisilicon,broken-prefetch-cmd;
+ /* smmu-cb-memtype = <0x0 0x1>;*/
+ };
+ p1_smmu_alg_a: smmu_alg@400,d004 {
+ compatible = "arm,smmu-v3";
+ reg = <0x400 0xd004 0x0 0x2>;
+ interrupt-parent = <&p1_mbigen_smmu_alg_a>;
+ interrupts = <733 1>,
+ <734 1>,
+ <735 1>;
+ interrupt-name
[PATCH 1/3] dt-bindings: Add bindings for Hisilicon SEC crypto accelerators.
The hip06 and hip07 SoCs contain a number of these crypto units which
accelerate AES and DES operations.
Signed-off-by: Jonathan Cameron
---
.../bindings/crypto/hisilicon,hip07-sec.txt| 67 ++
1 file changed, 67 insertions(+)
diff --git a/Documentation/devicetree/bindings/crypto/hisilicon,hip07-sec.txt
b/Documentation/devicetree/bindings/crypto/hisilicon,hip07-sec.txt
new file mode 100644
index ..78d2db9d4de5
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/hisilicon,hip07-sec.txt
@@ -0,0 +1,67 @@
+* Hisilicon hip07 Security Accelerator (SEC)
+
+Required properties:
+- compatible: Must contain one of
+ - "hisilicon,hip06-sec"
+ - "hisilicon,hip07-sec"
+- reg: Memory addresses and lengths of the memory regions through which
+ this device is controlled.
+ Region 0 has registers to control the backend processing engines.
+ Region 1 has registers for functionality common to all queues.
+ Regions 2-18 have registers for the 16 individual queues which are isolated
+ both in hardware and within the driver.
+- interrupts: Interrupt specifiers.
+ Refer to interrupt-controller/interrupts.txt for generic interrupt client
node
+ bindings.
+ Interrupt 0 is for the SEC unit error queue.
+ Interrupt 2N + 1 is the completion interrupt for queue N.
+ Interrupt 2N + 2 is the error interrupt for queue N.
+- dma-coherent: The driver assumes coherent dma is possible.
+
+Optional properties:
+- iommus: The SEC units are behind smmu-v3 iommus.
+ Refer to iommu/arm,smmu-v3.txt for more information.
+
+Example:
+
+p1_sec_a: crypto@400,d200 {
+ compatible = "hisilicon,hip07-sec";
+ reg = <0x400 0xd000 0x0 0x1
+ 0x400 0xd200 0x0 0x1
+ 0x400 0xd201 0x0 0x1
+ 0x400 0xd202 0x0 0x1
+ 0x400 0xd203 0x0 0x1
+ 0x400 0xd204 0x0 0x1
+ 0x400 0xd205 0x0 0x1
+ 0x400 0xd206 0x0 0x1
+ 0x400 0xd207 0x0 0x1
+ 0x400 0xd208 0x0 0x1
+ 0x400 0xd209 0x0 0x1
+ 0x400 0xd20a 0x0 0x1
+ 0x400 0xd20b 0x0 0x1
+ 0x400 0xd20c 0x0 0x1
+ 0x400 0xd20d 0x0 0x1
+ 0x400 0xd20e 0x0 0x1
+ 0x400 0xd20f 0x0 0x1
+ 0x400 0xd210 0x0 0x1>;
+ interrupt-parent = <&p1_mbigen_sec_a>;
+ iommus = <&p1_smmu_alg_a 0x600>;
+ dma-coherent;
+ interrupts = <576 4>,
+<577 1>, <578 4>,
+<579 1>, <580 4>,
+<581 1>, <582 4>,
+<583 1>, <584 4>,
+<585 1>, <586 4>,
+<587 1>, <588 4>,
+<589 1>, <590 4>,
+<591 1>, <592 4>,
+<593 1>, <594 4>,
+<595 1>, <596 4>,
+<597 1>, <598 4>,
+<599 1>, <600 4>,
+<601 1>, <602 4>,
+<603 1>, <604 4>,
+<605 1>, <606 4>,
+<607 1>, <608 4>;
+};
--
2.16.2
[PATCH 2/3] crypto: hisilicon SEC security accelerator driver
This accelerator is found inside hisilicon hip06 and hip07 SoCs.
Each instance provides a number of queues which feed a different number of
backend acceleration units.
The queues are operating in an out of order mode in the interests of
throughput. The silicon does not do tracking of dependencies between
multiple 'messages' or update of the IVs as appropriate for training.
Hence where relevant we need to do this in software.
Signed-off-by: Jonathan Cameron
---
drivers/crypto/Kconfig |2 +
drivers/crypto/Makefile |1 +
drivers/crypto/hisilicon/Kconfig| 14 +
drivers/crypto/hisilicon/Makefile |2 +
drivers/crypto/hisilicon/sec/Makefile |3 +
drivers/crypto/hisilicon/sec/sec_algs.c | 1122 ++
drivers/crypto/hisilicon/sec/sec_drv.c | 1323 +++
drivers/crypto/hisilicon/sec/sec_drv.h | 428 ++
8 files changed, 2895 insertions(+)
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index d1ea1a07cecb..d0b80d0d1f8b 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -750,4 +750,6 @@ config CRYPTO_DEV_CCREE
cryptographic operations on the system REE.
If unsure say Y.
+source "drivers/crypto/hisilicon/Kconfig"
+
endif # CRYPTO_HW
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index 7ae87b4f6c8d..ee43aed8cb69 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/
+obj-y += hisilicon/
diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
new file mode 100644
index ..8ca9c503bcb0
--- /dev/null
+++ b/drivers/crypto/hisilicon/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0
+
+config CRYPTO_DEV_HISI_SEC
+ tristate "Support for Hisilicon SEC crypto block cipher accelerator"
+ select CRYPTO_BLKCIPHER
+ select CRYPTO_ALGAPI
+ select SG_SPLIT
+ depends on ARM64 || COMPILE_TEST
+ depends on HAS_IOMEM
+ help
+ Support for Hisilicon SEC Engine in Hip06 and Hip07
+
+ To compile this as a module, choose M here: the module
+ will be called hisi_sec.
diff --git a/drivers/crypto/hisilicon/Makefile
b/drivers/crypto/hisilicon/Makefile
new file mode 100644
index ..463f46ace182
--- /dev/null
+++ b/drivers/crypto/hisilicon/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_CRYPTO_DEV_HISI_SEC) += sec/
diff --git a/drivers/crypto/hisilicon/sec/Makefile
b/drivers/crypto/hisilicon/sec/Makefile
new file mode 100644
index ..a55b698e0c27
--- /dev/null
+++ b/drivers/crypto/hisilicon/sec/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_CRYPTO_DEV_HISI_SEC) += hisi_sec.o
+hisi_sec-y = sec_algs.o sec_drv.o
diff --git a/drivers/crypto/hisilicon/sec/sec_algs.c
b/drivers/crypto/hisilicon/sec/sec_algs.c
new file mode 100644
index ..d69d3ce358b0
--- /dev/null
+++ b/drivers/crypto/hisilicon/sec/sec_algs.c
@@ -0,0 +1,1122 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2016-2017 Hisilicon Limited. */
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sec_drv.h"
+
+#define SEC_MAX_CIPHER_KEY 64
+#define SEC_REQ_LIMIT SZ_32M
+
+struct sec_c_alg_cfg {
+ unsigned c_alg : 3;
+ unsigned c_mode : 3;
+ unsigned key_len: 2;
+ unsigned c_width: 2;
+};
+
+static const struct sec_c_alg_cfg sec_c_alg_cfgs[] = {
+ [SEC_C_DES_ECB_64] = {
+ .c_alg = SEC_C_ALG_DES,
+ .c_mode = SEC_C_MODE_ECB,
+ .key_len = SEC_KEY_LEN_DES,
+ },
+ [SEC_C_DES_CBC_64] = {
+ .c_alg = SEC_C_ALG_DES,
+ .c_mode = SEC_C_MODE_CBC,
+ .key_len = SEC_KEY_LEN_DES,
+ },
+ [SEC_C_3DES_ECB_192_3KEY] = {
+ .c_alg = SEC_C_ALG_3DES,
+ .c_mode = SEC_C_MODE_ECB,
+ .key_len = SEC_KEY_LEN_3DES_3_KEY,
+ },
+ [SEC_C_3DES_ECB_192_2KEY] = {
+ .c_alg = SEC_C_ALG_3DES,
+ .c_mode = SEC_C_MODE_ECB,
+ .key_len = SEC_KEY_LEN_3DES_2_KEY,
+ },
+ [SEC_C_3DES_CBC_192_3KEY] = {
+ .c_alg = SEC_C_ALG_3DES,
+ .c_mode = SEC_C_MODE_CBC,
+ .key_len = SEC_KEY_LEN_3DES_3_KEY,
+ },
+ [SEC_C_3DES_CBC_192_2KEY] = {
+ .c_alg = SEC_C_ALG_3DES,
+ .c_mode = SEC_C_MODE_CBC,
+ .key_len = SEC_KEY_LEN_3DES_2_KEY,
+ },
+ [SEC_C_AES_ECB_128] = {
+ .c_alg = SEC_C_ALG_AES,
+ .c_mode = SEC_C_MODE_ECB,
+ .key_len =
[PATCH] crypto: skcipher - fix aligning block size in skcipher_copy_iv()
From: Eric Biggers
The ALIGN() macro needs to be passed the alignment, not the alignmask
(which is the alignment minus 1).
Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface")
Cc: # v4.10+
Signed-off-by: Eric Biggers
---
crypto/skcipher.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 7d6a49fe3047..4f6b8dadaceb 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -398,7 +398,7 @@ static int skcipher_copy_iv(struct skcipher_walk *walk)
unsigned size;
u8 *iv;
- aligned_bs = ALIGN(bs, alignmask);
+ aligned_bs = ALIGN(bs, alignmask + 1);
/* Minimum size to align buffer by alignmask. */
size = alignmask & ~a;
--
2.18.0.233.g985f88cf7e-goog
[PATCH] crypto: scatterwalk - remove 'chain' argument from scatterwalk_crypto_chain()
From: Eric Biggers
All callers pass chain=0 to scatterwalk_crypto_chain().
Remove this unneeded parameter.
Signed-off-by: Eric Biggers
---
crypto/lrw.c | 4 ++--
crypto/scatterwalk.c | 2 +-
crypto/xts.c | 4 ++--
include/crypto/scatterwalk.h | 8 +---
net/tls/tls_device_fallback.c | 2 +-
5 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/crypto/lrw.c b/crypto/lrw.c
index 954a7064a179..393a782679c7 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -188,7 +188,7 @@ static int post_crypt(struct skcipher_request *req)
if (rctx->dst != sg) {
rctx->dst[0] = *sg;
sg_unmark_end(rctx->dst);
- scatterwalk_crypto_chain(rctx->dst, sg_next(sg), 0, 2);
+ scatterwalk_crypto_chain(rctx->dst, sg_next(sg), 2);
}
rctx->dst[0].length -= offset - sg->offset;
rctx->dst[0].offset = offset;
@@ -265,7 +265,7 @@ static int pre_crypt(struct skcipher_request *req)
if (rctx->src != sg) {
rctx->src[0] = *sg;
sg_unmark_end(rctx->src);
- scatterwalk_crypto_chain(rctx->src, sg_next(sg), 0, 2);
+ scatterwalk_crypto_chain(rctx->src, sg_next(sg), 2);
}
rctx->src[0].length -= offset - sg->offset;
rctx->src[0].offset = offset;
diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index c16c94f88733..d0b92c1cd6e9 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -91,7 +91,7 @@ struct scatterlist *scatterwalk_ffwd(struct scatterlist
dst[2],
sg_init_table(dst, 2);
sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);
- scatterwalk_crypto_chain(dst, sg_next(src), 0, 2);
+ scatterwalk_crypto_chain(dst, sg_next(src), 2);
return dst;
}
diff --git a/crypto/xts.c b/crypto/xts.c
index 12284183bd20..ccf55fbb8bc2 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -138,7 +138,7 @@ static int post_crypt(struct skcipher_request *req)
if (rctx->dst != sg) {
rctx->dst[0] = *sg;
sg_unmark_end(rctx->dst);
- scatterwalk_crypto_chain(rctx->dst, sg_next(sg), 0, 2);
+ scatterwalk_crypto_chain(rctx->dst, sg_next(sg), 2);
}
rctx->dst[0].length -= offset - sg->offset;
rctx->dst[0].offset = offset;
@@ -204,7 +204,7 @@ static int pre_crypt(struct skcipher_request *req)
if (rctx->src != sg) {
rctx->src[0] = *sg;
sg_unmark_end(rctx->src);
- scatterwalk_crypto_chain(rctx->src, sg_next(sg), 0, 2);
+ scatterwalk_crypto_chain(rctx->src, sg_next(sg), 2);
}
rctx->src[0].length -= offset - sg->offset;
rctx->src[0].offset = offset;
diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index 880e6be9e95e..eac72840a7d2 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -22,14 +22,8 @@
#include
static inline void scatterwalk_crypto_chain(struct scatterlist *head,
- struct scatterlist *sg,
- int chain, int num)
+ struct scatterlist *sg, int num)
{
- if (chain) {
- head->length += sg->length;
- sg = sg_next(sg);
- }
-
if (sg)
sg_chain(head, num, sg);
else
diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
index 748914abdb60..4e1ec12bc0fb 100644
--- a/net/tls/tls_device_fallback.c
+++ b/net/tls/tls_device_fallback.c
@@ -42,7 +42,7 @@ static void chain_to_walk(struct scatterlist *sg, struct
scatter_walk *walk)
sg_set_page(sg, sg_page(src),
src->length - diff, walk->offset);
- scatterwalk_crypto_chain(sg, sg_next(src), 0, 2);
+ scatterwalk_crypto_chain(sg, sg_next(src), 2);
}
static int tls_enc_record(struct aead_request *aead_req,
--
2.18.0.233.g985f88cf7e-goog
[PATCH] crypto: scatterwalk - remove scatterwalk_samebuf()
From: Eric Biggers
scatterwalk_samebuf() is never used. Remove it.
Signed-off-by: Eric Biggers
---
include/crypto/scatterwalk.h | 7 ---
1 file changed, 7 deletions(-)
diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index eac72840a7d2..a66c127a20ed 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -30,13 +30,6 @@ static inline void scatterwalk_crypto_chain(struct
scatterlist *head,
sg_mark_end(head);
}
-static inline unsigned long scatterwalk_samebuf(struct scatter_walk *walk_in,
- struct scatter_walk *walk_out)
-{
- return !(((sg_page(walk_in->sg) - sg_page(walk_out->sg)) << PAGE_SHIFT)
+
-(int)(walk_in->offset - walk_out->offset));
-}
-
static inline unsigned int scatterwalk_pagelen(struct scatter_walk *walk)
{
unsigned int len = walk->sg->offset + walk->sg->length - walk->offset;
--
2.18.0.233.g985f88cf7e-goog
[PATCH] crypto: skcipher - remove unnecessary setting of walk->nbytes
From: Eric Biggers Setting 'walk->nbytes = walk->total' in skcipher_walk_first() doesn't make sense because actually walk->nbytes needs to be set to the length of the first step in the walk, which may be less than walk->total. This is done by skcipher_walk_next() which is called immediately afterwards. Also walk->nbytes was already set to 0 in skcipher_walk_skcipher(), which is a better default value in case it's forgotten to be set later. Therefore, remove the unnecessary assignment to walk->nbytes. Signed-off-by: Eric Biggers --- crypto/skcipher.c | 1 - 1 file changed, 1 deletion(-) diff --git a/crypto/skcipher.c b/crypto/skcipher.c index 7d6a49fe3047..9f7d229827b5 100644 --- a/crypto/skcipher.c +++ b/crypto/skcipher.c @@ -436,7 +436,6 @@ static int skcipher_walk_first(struct skcipher_walk *walk) } walk->page = NULL; - walk->nbytes = walk->total; return skcipher_walk_next(walk); } -- 2.18.0.233.g985f88cf7e-goog
[PATCH 1/3] crypto: skcipher - fix crash flushing dcache in error path
From: Eric Biggers
scatterwalk_done() is only meant to be called after a nonzero number of
bytes have been processed, since scatterwalk_pagedone() will flush the
dcache of the *previous* page. But in the error case of
skcipher_walk_done(), e.g. if the input wasn't an integer number of
blocks, scatterwalk_done() was actually called after advancing 0 bytes.
This caused a crash ("BUG: unable to handle kernel paging request")
during '!PageSlab(page)' on architectures like arm and arm64 that define
ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
page-aligned as in that case walk->offset == 0.
Fix it by reorganizing skcipher_walk_done() to skip the
scatterwalk_advance() and scatterwalk_done() if an error has occurred.
This bug was found by syzkaller fuzzing.
Reproducer, assuming ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE:
#include
#include
#include
int main()
{
struct sockaddr_alg addr = {
.salg_type = "skcipher",
.salg_name = "cbc(aes-generic)",
};
char buffer[4096] __attribute__((aligned(4096))) = { 0 };
int fd;
fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(fd, (void *)&addr, sizeof(addr));
setsockopt(fd, SOL_ALG, ALG_SET_KEY, buffer, 16);
fd = accept(fd, NULL, NULL);
write(fd, buffer, 15);
read(fd, buffer, 15);
}
Reported-by: Liu Chao
Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface")
Cc: # v4.10+
Signed-off-by: Eric Biggers
---
crypto/skcipher.c | 53 ---
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 7d6a49fe3047..5f7017b36d75 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -95,7 +95,7 @@ static inline u8 *skcipher_get_spot(u8 *start, unsigned int
len)
return max(start, end_page);
}
-static int skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize)
+static void skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize)
{
u8 *addr;
@@ -103,23 +103,24 @@ static int skcipher_done_slow(struct skcipher_walk *walk,
unsigned int bsize)
addr = skcipher_get_spot(addr, bsize);
scatterwalk_copychunks(addr, &walk->out, bsize,
(walk->flags & SKCIPHER_WALK_PHYS) ? 2 : 1);
- return 0;
}
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
- unsigned int n = walk->nbytes - err;
- unsigned int nbytes;
-
- nbytes = walk->total - n;
-
- if (unlikely(err < 0)) {
- nbytes = 0;
- n = 0;
- } else if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
- SKCIPHER_WALK_SLOW |
- SKCIPHER_WALK_COPY |
- SKCIPHER_WALK_DIFF {
+ unsigned int n; /* bytes processed */
+ bool more;
+
+ if (unlikely(err < 0))
+ goto finish;
+
+ n = walk->nbytes - err;
+ walk->total -= n;
+ more = (walk->total != 0);
+
+ if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
+ SKCIPHER_WALK_SLOW |
+ SKCIPHER_WALK_COPY |
+ SKCIPHER_WALK_DIFF {
unmap_src:
skcipher_unmap_src(walk);
} else if (walk->flags & SKCIPHER_WALK_DIFF) {
@@ -131,28 +132,28 @@ int skcipher_walk_done(struct skcipher_walk *walk, int
err)
skcipher_unmap_dst(walk);
} else if (unlikely(walk->flags & SKCIPHER_WALK_SLOW)) {
if (WARN_ON(err)) {
+ /* unexpected case; didn't process all bytes */
err = -EINVAL;
- nbytes = 0;
- } else
- n = skcipher_done_slow(walk, n);
+ goto finish;
+ }
+ skcipher_done_slow(walk, n);
+ goto already_advanced;
}
- if (err > 0)
- err = 0;
-
- walk->total = nbytes;
- walk->nbytes = nbytes;
-
scatterwalk_advance(&walk->in, n);
scatterwalk_advance(&walk->out, n);
- scatterwalk_done(&walk->in, 0, nbytes);
- scatterwalk_done(&walk->out, 1, nbytes);
+already_advanced:
+ scatterwalk_done(&walk->in, 0, more);
+ scatterwalk_done(&walk->out, 1, more);
- if (nbytes) {
+ if (more) {
crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ?
CRYPTO_TFM_REQ_MAY_SLEEP : 0);
return skcipher_walk_next(walk);
}
+ err = 0;
+finish:
+ walk->nbytes = 0;
/* Short-circuit for the common/fast path. */
if (!((unsigned long)walk->buffer | (uns
[PATCH 0/3] crypto: fix crash in scatterwalk_pagedone()
From: Eric Biggers This series fixes the bug reported by Liu Chao (found using syzkaller) where a crash occurs in scatterwalk_pagedone() on architectures such as arm and arm64 that implement flush_dcache_page(), due to an invalid page pointer when walk->offset == 0. This series attempts to address the underlying problem which is that scatterwalk_pagedone() shouldn't have been called at all in that case. Eric Biggers (3): crypto: skcipher - fix crash flushing dcache in error path crypto: blkcipher - fix crash flushing dcache in error path crypto: ablkcipher - fix crash flushing dcache in error path crypto/ablkcipher.c | 57 + crypto/blkcipher.c | 54 +- crypto/skcipher.c | 53 - 3 files changed, 79 insertions(+), 85 deletions(-) -- 2.18.0.233.g985f88cf7e-goog
[PATCH 3/3] crypto: ablkcipher - fix crash flushing dcache in error path
From: Eric Biggers
Like the skcipher_walk and blkcipher_walk cases:
scatterwalk_done() is only meant to be called after a nonzero number of
bytes have been processed, since scatterwalk_pagedone() will flush the
dcache of the *previous* page. But in the error case of
ablkcipher_walk_done(), e.g. if the input wasn't an integer number of
blocks, scatterwalk_done() was actually called after advancing 0 bytes.
This caused a crash ("BUG: unable to handle kernel paging request")
during '!PageSlab(page)' on architectures like arm and arm64 that define
ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
page-aligned as in that case walk->offset == 0.
Fix it by reorganizing ablkcipher_walk_done() to skip the
scatterwalk_advance() and scatterwalk_done() if an error has occurred.
Reported-by: Liu Chao
Fixes: bf06099db18a ("crypto: skcipher - Add ablkcipher_walk interfaces")
Cc: # v2.6.35+
Signed-off-by: Eric Biggers
---
crypto/ablkcipher.c | 57 +
1 file changed, 26 insertions(+), 31 deletions(-)
diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
index 1edb5000d783..8882e90e868e 100644
--- a/crypto/ablkcipher.c
+++ b/crypto/ablkcipher.c
@@ -71,11 +71,9 @@ static inline u8 *ablkcipher_get_spot(u8 *start, unsigned
int len)
return max(start, end_page);
}
-static inline unsigned int ablkcipher_done_slow(struct ablkcipher_walk *walk,
- unsigned int bsize)
+static inline void ablkcipher_done_slow(struct ablkcipher_walk *walk,
+ unsigned int n)
{
- unsigned int n = bsize;
-
for (;;) {
unsigned int len_this_page = scatterwalk_pagelen(&walk->out);
@@ -87,17 +85,13 @@ static inline unsigned int ablkcipher_done_slow(struct
ablkcipher_walk *walk,
n -= len_this_page;
scatterwalk_start(&walk->out, sg_next(walk->out.sg));
}
-
- return bsize;
}
-static inline unsigned int ablkcipher_done_fast(struct ablkcipher_walk *walk,
- unsigned int n)
+static inline void ablkcipher_done_fast(struct ablkcipher_walk *walk,
+ unsigned int n)
{
scatterwalk_advance(&walk->in, n);
scatterwalk_advance(&walk->out, n);
-
- return n;
}
static int ablkcipher_walk_next(struct ablkcipher_request *req,
@@ -107,39 +101,40 @@ int ablkcipher_walk_done(struct ablkcipher_request *req,
struct ablkcipher_walk *walk, int err)
{
struct crypto_tfm *tfm = req->base.tfm;
- unsigned int nbytes = 0;
+ unsigned int n; /* bytes processed */
+ bool more;
- if (likely(err >= 0)) {
- unsigned int n = walk->nbytes - err;
+ if (unlikely(err < 0))
+ goto finish;
- if (likely(!(walk->flags & ABLKCIPHER_WALK_SLOW)))
- n = ablkcipher_done_fast(walk, n);
- else if (WARN_ON(err)) {
- err = -EINVAL;
- goto err;
- } else
- n = ablkcipher_done_slow(walk, n);
+ n = walk->nbytes - err;
+ walk->total -= n;
+ more = (walk->total != 0);
- nbytes = walk->total - n;
- err = 0;
+ if (likely(!(walk->flags & ABLKCIPHER_WALK_SLOW))) {
+ ablkcipher_done_fast(walk, n);
+ } else {
+ if (WARN_ON(err)) {
+ /* unexpected case; didn't process all bytes */
+ err = -EINVAL;
+ goto finish;
+ }
+ ablkcipher_done_slow(walk, n);
}
- scatterwalk_done(&walk->in, 0, nbytes);
- scatterwalk_done(&walk->out, 1, nbytes);
-
-err:
- walk->total = nbytes;
- walk->nbytes = nbytes;
+ scatterwalk_done(&walk->in, 0, more);
+ scatterwalk_done(&walk->out, 1, more);
- if (nbytes) {
+ if (more) {
crypto_yield(req->base.flags);
return ablkcipher_walk_next(req, walk);
}
-
+ err = 0;
+finish:
+ walk->nbytes = 0;
if (walk->iv != req->info)
memcpy(req->info, walk->iv, tfm->crt_ablkcipher.ivsize);
kfree(walk->iv_buffer);
-
return err;
}
EXPORT_SYMBOL_GPL(ablkcipher_walk_done);
--
2.18.0.233.g985f88cf7e-goog
[PATCH 2/3] crypto: blkcipher - fix crash flushing dcache in error path
From: Eric Biggers
Like the skcipher_walk case:
scatterwalk_done() is only meant to be called after a nonzero number of
bytes have been processed, since scatterwalk_pagedone() will flush the
dcache of the *previous* page. But in the error case of
blkcipher_walk_done(), e.g. if the input wasn't an integer number of
blocks, scatterwalk_done() was actually called after advancing 0 bytes.
This caused a crash ("BUG: unable to handle kernel paging request")
during '!PageSlab(page)' on architectures like arm and arm64 that define
ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
page-aligned as in that case walk->offset == 0.
Fix it by reorganizing blkcipher_walk_done() to skip the
scatterwalk_advance() and scatterwalk_done() if an error has occurred.
This bug was found by syzkaller fuzzing.
Reproducer, assuming ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE:
#include
#include
#include
int main()
{
struct sockaddr_alg addr = {
.salg_type = "skcipher",
.salg_name = "ecb(aes-generic)",
};
char buffer[4096] __attribute__((aligned(4096))) = { 0 };
int fd;
fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(fd, (void *)&addr, sizeof(addr));
setsockopt(fd, SOL_ALG, ALG_SET_KEY, buffer, 16);
fd = accept(fd, NULL, NULL);
write(fd, buffer, 15);
read(fd, buffer, 15);
}
Reported-by: Liu Chao
Fixes: 5cde0af2a982 ("[CRYPTO] cipher: Added block cipher type")
Cc: # v2.6.19+
Signed-off-by: Eric Biggers
---
crypto/blkcipher.c | 54 ++
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index dd4dcab3766a..f93abf13b5d4 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -70,19 +70,18 @@ static inline u8 *blkcipher_get_spot(u8 *start, unsigned
int len)
return max(start, end_page);
}
-static inline unsigned int blkcipher_done_slow(struct blkcipher_walk *walk,
- unsigned int bsize)
+static inline void blkcipher_done_slow(struct blkcipher_walk *walk,
+ unsigned int bsize)
{
u8 *addr;
addr = (u8 *)ALIGN((unsigned long)walk->buffer, walk->alignmask + 1);
addr = blkcipher_get_spot(addr, bsize);
scatterwalk_copychunks(addr, &walk->out, bsize, 1);
- return bsize;
}
-static inline unsigned int blkcipher_done_fast(struct blkcipher_walk *walk,
- unsigned int n)
+static inline void blkcipher_done_fast(struct blkcipher_walk *walk,
+ unsigned int n)
{
if (walk->flags & BLKCIPHER_WALK_COPY) {
blkcipher_map_dst(walk);
@@ -96,49 +95,48 @@ static inline unsigned int blkcipher_done_fast(struct
blkcipher_walk *walk,
scatterwalk_advance(&walk->in, n);
scatterwalk_advance(&walk->out, n);
-
- return n;
}
int blkcipher_walk_done(struct blkcipher_desc *desc,
struct blkcipher_walk *walk, int err)
{
- unsigned int nbytes = 0;
+ unsigned int n; /* bytes processed */
+ bool more;
- if (likely(err >= 0)) {
- unsigned int n = walk->nbytes - err;
+ if (unlikely(err < 0))
+ goto finish;
- if (likely(!(walk->flags & BLKCIPHER_WALK_SLOW)))
- n = blkcipher_done_fast(walk, n);
- else if (WARN_ON(err)) {
- err = -EINVAL;
- goto err;
- } else
- n = blkcipher_done_slow(walk, n);
+ n = walk->nbytes - err;
+ walk->total -= n;
+ more = (walk->total != 0);
- nbytes = walk->total - n;
- err = 0;
+ if (likely(!(walk->flags & BLKCIPHER_WALK_SLOW))) {
+ blkcipher_done_fast(walk, n);
+ } else {
+ if (WARN_ON(err)) {
+ /* unexpected case; didn't process all bytes */
+ err = -EINVAL;
+ goto finish;
+ }
+ blkcipher_done_slow(walk, n);
}
- scatterwalk_done(&walk->in, 0, nbytes);
- scatterwalk_done(&walk->out, 1, nbytes);
+ scatterwalk_done(&walk->in, 0, more);
+ scatterwalk_done(&walk->out, 1, more);
-err:
- walk->total = nbytes;
- walk->nbytes = nbytes;
-
- if (nbytes) {
+ if (more) {
crypto_yield(desc->flags);
return blkcipher_walk_next(desc, walk);
}
-
+ err = 0;
+finish:
+ walk->nbytes = 0;
if (walk->iv != desc->info)
memcpy(desc->info, walk->iv, walk->ivsize);
if (walk->buffer != walk->page)
kfree(
Re: [PATCH v6 0/6] crypto: Add Qcom PRNG support
On 16-07-18, 11:20, Vinod Koul wrote:
> This series removes the hwrng qcom driver and replaces it with crypto qcom
> driver and then adds support for Execution Environment (EE) found in v2
> version of the hardware and ACPI support for these
Stephan, Herbert
Any chance this could make it for 4.19. It has been here for a while and
I haven't heard any objections.
>
> Changes in v6:
> - Fix a typo in kconfig. Remove of_device.h and add of.h header
> - Add review and tested tags
>
> Changes in v5:
> - Update ACPI check and use generic driver data API
>
> Changes in v4:
> - Use memcpy for data copy
> - Fix trailing bytes copy
> - Fix ACPI ID table name
>
> Timur Tabi (1):
> crypto: qcom: Add ACPI support
>
> Vinod Koul (5):
> hwrng: remove msm hw_random driver
> dt-bindings: crypto: Move prng binding to crypto
> crypto: Add Qcom prng driver
> dt-bindings: crypto: Add new compatible qcom,prng-ee
> crypto: qcom: Add support for prng-ee
>
> .../bindings/{rng => crypto}/qcom,prng.txt | 4 +-
> drivers/char/hw_random/Kconfig | 13 --
> drivers/char/hw_random/Makefile| 1 -
> drivers/char/hw_random/msm-rng.c | 183
> drivers/crypto/Kconfig | 11 +
> drivers/crypto/Makefile| 1 +
> drivers/crypto/qcom-rng.c | 229
> +
> 7 files changed, 244 insertions(+), 198 deletions(-)
> rename Documentation/devicetree/bindings/{rng => crypto}/qcom,prng.txt (73%)
> delete mode 100644 drivers/char/hw_random/msm-rng.c
> create mode 100644 drivers/crypto/qcom-rng.c
>
> --
> 2.14.4
--
~Vinod
Re: [REPORT] Possible unnecessary usages of GFP_ATOMIC in crypto/ablkcipher.c
On 2018/7/23 14:21, Herbert Xu wrote: On Mon, Jul 23, 2018 at 10:39:40AM +0800, Jia-Ju Bai wrote: My tool DCNS reports three unnecessary usages of GFP_ATOMIC in crypto/ablkcipher.c: crypto/ablkcipher.c, 162: kmalloc(GFP_ATOMIC) in ablkcipher_next_slow crypto/ablkcipher.c, 199: kmalloc(GFP_ATOMIC) in ablkcipher_copy_iv crypto/ablkcipher.c, 315: kmalloc(GFP_ATOMIC) in setkey_unaligned I meant to manually check the code, but I find that there are many functions calling ablkcipher_next_slow(), ablkcipher_copy_iv() and setkey_unaligned(), so I am not sure whether the above three reports are true. Could someone help me to validate these reports? Thanks a lot in advance :) They must use GFP_ATOMIC because they can be called from softirq context, e.g., IPsec. Okay, thanks for the reply. Best wishes, Jia-Ju Bai
Re: [PATCH 13/14] arm64: dts: marvell: armada-cp110: update the crypto engine compatible
Hi Olof,
On Sat, Jul 21, 2018 at 02:35:01PM -0700, Olof Johansson wrote:
> On Thu, Jun 28, 2018 at 8:15 AM, Antoine Tenart
> wrote:
> > New compatibles are now supported by the Inside Secure SafeXcel driver.
> > As they are more specific than the old ones, they should be used
> > whenever possible. This patch updates the Marvell cp110 device tree
> > accordingly.
> >
> > Signed-off-by: Antoine Tenart
> > ---
> > arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > index 2bf083272a87..bb2914f90048 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > @@ -427,7 +427,7 @@
> > };
> >
> > CP110_LABEL(crypto): crypto@80 {
> > - compatible = "inside-secure,safexcel-eip197";
> > + compatible = "inside-secure,safexcel-eip197b";
>
> So the device is still compatible with the less-specific binding,
> right? If so, it should probably have both compatible properties in
> there, not just the more specific one.
Using "safexcel-eip197" as a compatible was a mistake as there's no such
thing as an eip197, they all have minor versions (such as 'b').
I've thought about using the compatible for a less specific binding, but
this isn't true for the other patch, using the compatible ending in
"-eip97". The engine it supports is the most specific one (i.e. with the
largest number of algorithms supported). So it would simply not work,
and as we only have a few device trees supporting the engine as of now,
I thought fixing this by removing the wrong compatible was a better
solution (of course the driver is backward compatible, and using the old
compatibles will still work).
Thanks,
Antoine
--
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
[PATCH] crypto: cavium: nitrox: Replace GFP_ATOMIC with GFP_KERNEL in crypto_alloc_context()
crypto_alloc_context() is only called by nitrox_skcipher_init(), which is never called in atomic context. crypto_alloc_context() calls dma_pool_alloc() with GFP_ATOMIC, which is not necessary. GFP_ATOMIC can be replaced with GFP_KERNEL. This is found by a static analysis tool named DCNS written by myself. I also manually check the kernel code before reporting it. Signed-off-by: Jia-Ju Bai --- drivers/crypto/cavium/nitrox/nitrox_lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/crypto/cavium/nitrox/nitrox_lib.c b/drivers/crypto/cavium/nitrox/nitrox_lib.c index 4fdc921ba611..ebe267379ac9 100644 --- a/drivers/crypto/cavium/nitrox/nitrox_lib.c +++ b/drivers/crypto/cavium/nitrox/nitrox_lib.c @@ -148,7 +148,7 @@ void *crypto_alloc_context(struct nitrox_device *ndev) void *vaddr; dma_addr_t dma; - vaddr = dma_pool_alloc(ndev->ctx_pool, (GFP_ATOMIC | __GFP_ZERO), &dma); + vaddr = dma_pool_alloc(ndev->ctx_pool, (GFP_KERNEL | __GFP_ZERO), &dma); if (!vaddr) return NULL; -- 2.17.0
[PATCH] crypto: qat: adf_aer: Replace GFP_ATOMIC with GFP_KERNEL in adf_dev_aer_schedule_reset()
adf_dev_aer_schedule_reset() is never called in atomic context, as it calls wait_for_completion_timeout(). adf_dev_aer_schedule_reset() calls kzalloc() with GFP_ATOMIC, which is not necessary. GFP_ATOMIC can be replaced with GFP_KERNEL. This is found by a static analysis tool named DCNS written by myself. I also manually check the kernel code before reporting it. Signed-off-by: Jia-Ju Bai --- drivers/crypto/qat/qat_common/adf_aer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/crypto/qat/qat_common/adf_aer.c b/drivers/crypto/qat/qat_common/adf_aer.c index da8a2d3b5e9a..9225d060e18f 100644 --- a/drivers/crypto/qat/qat_common/adf_aer.c +++ b/drivers/crypto/qat/qat_common/adf_aer.c @@ -163,7 +163,7 @@ static int adf_dev_aer_schedule_reset(struct adf_accel_dev *accel_dev, return 0; set_bit(ADF_STATUS_RESTARTING, &accel_dev->status); - reset_data = kzalloc(sizeof(*reset_data), GFP_ATOMIC); + reset_data = kzalloc(sizeof(*reset_data), GFP_KERNEL); if (!reset_data) return -ENOMEM; reset_data->accel_dev = accel_dev; -- 2.17.0
[PATCH] crypto: virtio: Replace GFP_ATOMIC with GFP_KERNEL in __virtio_crypto_ablkcipher_do_req()
__virtio_crypto_ablkcipher_do_req() is never called in atomic context.
__virtio_crypto_ablkcipher_do_req() is only called by
virtio_crypto_ablkcipher_crypt_req(), which is only called by
virtcrypto_find_vqs() that is never called in atomic context.
__virtio_crypto_ablkcipher_do_req() calls kzalloc_node() with GFP_ATOMIC,
which is not necessary.
GFP_ATOMIC can be replaced with GFP_KERNEL.
This is found by a static analysis tool named DCNS written by myself.
I also manually check the kernel code before reporting it.
Signed-off-by: Jia-Ju Bai
---
drivers/crypto/virtio/virtio_crypto_algs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
b/drivers/crypto/virtio/virtio_crypto_algs.c
index ba190cfa7aa1..83bcc83dae43 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -371,12 +371,12 @@ __virtio_crypto_ablkcipher_do_req(struct
virtio_crypto_sym_request *vc_sym_req,
/* Why 3? outhdr + iv + inhdr */
sg_total = src_nents + dst_nents + 3;
- sgs = kzalloc_node(sg_total * sizeof(*sgs), GFP_ATOMIC,
+ sgs = kzalloc_node(sg_total * sizeof(*sgs), GFP_KERNEL,
dev_to_node(&vcrypto->vdev->dev));
if (!sgs)
return -ENOMEM;
- req_data = kzalloc_node(sizeof(*req_data), GFP_ATOMIC,
+ req_data = kzalloc_node(sizeof(*req_data), GFP_KERNEL,
dev_to_node(&vcrypto->vdev->dev));
if (!req_data) {
kfree(sgs);
@@ -432,7 +432,7 @@ __virtio_crypto_ablkcipher_do_req(struct
virtio_crypto_sym_request *vc_sym_req,
* Avoid to do DMA from the stack, switch to using
* dynamically-allocated for the IV
*/
- iv = kzalloc_node(ivsize, GFP_ATOMIC,
+ iv = kzalloc_node(ivsize, GFP_KERNEL,
dev_to_node(&vcrypto->vdev->dev));
if (!iv) {
err = -ENOMEM;
--
2.17.0
Re: CCREE performance on R-Car H3 + crash
Hi Gilad, CC linux-crypto for the crash log. On Sun, Jul 22, 2018 at 7:28 AM Gilad Ben-Yossef wrote: > On Thu, Jul 19, 2018 at 3:43 PM, Geert Uytterhoeven > wrote: > > I've noticed CCREE is used with a LUKS-formatted disk, so I did some small > > performance benchmarks. The disk is an old 160 GiB SATA hard drive, > > connected to either Salvator-XS (R-Car H3 ES2.0) or Koelsch (R-Car M2-W). > > > > hdparm -t /dev/sda (unencrypted data): 62 MB/s (both systems) > > > > hdparm -t /dev/dm-0 (encrypted data, LUKS AES): > > > > salvator-xs (CCREE): 15 MB/s > > salvator-xs (SW):62 MB/s > > koelsch (SW):47 MB/s > > > > I'm a bit disappointment by the results when using crypto acceleration. > > Apparently the in-kernel optimized arm64 implementation can decrypt at > > raw read speed, while CCREE can't keep up. > > Interesting. What is the encryption sector size used? crypt_config.sector_size is 512. > If it is he default of 512 bytes, you might want to try setting the > new block_size DM-Crypt option. I believe it will have a large effect > on the result. Where do I specifiy that option? I couldn't find it. Is that compatible with the encrypted media that's already in use? > > Is this expected, and in line with your experiences? > > Well, if you were indeed using the default 512 bytes and taking into > account that sadly the CryptoCell inside the R-Car is not wired to > utilize a coherent bus (the cache invalidation costs a fortune in > performance) it is not unexpected. > > When reasoning about these things it is worth taking into account that > the design target of using CryptoCell is not to get the absolute best > raw performance as such but rather to get good performance / per cycle > / per watt. > > When the Arm AES Crypto Extension is used (I assume you are using that > and not, say, the NEON implementation) I expect the raw performance to > always be better than CryptoCell, certainly when it is not attached to > a coherent bus. However, if you check CPU utilization during the > operation (basically how much encryption is "stealing" computation > power from the CPU) and subsequent power consumption you are supposed > to see that there is a large area of work loads where it makes more > sense to utilize CryptoCell for disk encryption and use the CPU for > something else. ... in theory, at least. :-) Sure, you have to consider raw performance vs. CPU utilization vs. power consumption. Need more ways to tweak the kernel :-) $ cryptsetup benchmark # Tests are approximate using memory only (no storage IO). PBKDF2-sha1 478364 iterations per second for 256-bit key PBKDF2-sha256 927943 iterations per second for 256-bit key PBKDF2-sha512 360583 iterations per second for 256-bit key PBKDF2-ripemd160 266677 iterations per second for 256-bit key PBKDF2-whirlpool 115787 iterations per second for 256-bit key # Algorithm | Key | Encryption | Decryption aes-cbc 128b46.0 MiB/s46.7 MiB/s serpent-cbc 128b N/A N/A twofish-cbc 128b N/A N/A aes-cbc 256b46.5 MiB/s46.4 MiB/s serpent-cbc 256b N/A N/A twofish-cbc 256b N/A N/A Segmentation fault Oops. ccree e6601000.crypto: Unsupported data size 65536. Unable to handle kernel paging request at virtual address ffbf5c3c3c20 Mem abort info: ESR = 0x9605 Exception class = DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 Data abort info: ISV = 0, ISS = 0x0005 CM = 0, WnR = 0 swapper pgtable: 4k pages, 39-bit VAs, pgdp = 0ed05706 [ffbf5c3c3c20] pgd=, pud= Internal error: Oops: 9605 [#1] SMP Modules linked in: CPU: 4 PID: 986 Comm: cryptsetup Not tainted 4.18.0-rc5-salvator-x-00429-ge5b9d1fce24c0b6c-dirty #118 Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT) pstate: 2045 (nzCv daif +PAN -UAO) pc : ksize+0x28/0x48 lr : kzfree+0x1c/0x44 sp : ff800bab3a40 x29: ff800bab3a40 x28: ffea x27: x26: 0001 x25: ffc6f72e0480 x24: 0010 x23: ffc6fa227810 x22: x21: ff800900e000 x20: ffc6f48a5a80 x19: 5a5a5a5a5a5a5a5a x18: 000a x17: x16: x15: bc43 x14: ff8089c9aadf x13: x12: 0030 x11: fffe x10: ff8009c9aae8 x9 : 05f5e0ff x8 : x7 : ff800814c948 x6 : 0001 x5 : x4 : x3 : x2 : 38716e04a5aa3600 x1 : ffbf x0 : ffbf5c3c3c18 Process cryptsetup (pid: 986, stack limit = 0xc7b0be1c) Call trace: ksize+0x28/0x48 cc_cipher_process+0x3d4/0x7cc cc_cipher_encrypt+0x18/0x20 skcipher_recvmsg+0x2e0/0x344 sock_recvmsg+0x1c/0x24 sock_read_iter+0x88/0xd8 __vfs_read+0x108/0x138 vfs_read+0x8c/0x120 ksys_read+0x5c/0x
Re: CCREE performance on R-Car H3 + crash
On Mon, Jul 23, 2018 at 12:55 PM, Geert Uytterhoeven wrote: > Hi Gilad, > > CC linux-crypto for the crash log. > > On Sun, Jul 22, 2018 at 7:28 AM Gilad Ben-Yossef wrote: >> On Thu, Jul 19, 2018 at 3:43 PM, Geert Uytterhoeven >> wrote: >> > I've noticed CCREE is used with a LUKS-formatted disk, so I did some small ... > > $ cryptsetup benchmark > # Tests are approximate using memory only (no storage IO). > PBKDF2-sha1 478364 iterations per second for 256-bit key > PBKDF2-sha256 927943 iterations per second for 256-bit key > PBKDF2-sha512 360583 iterations per second for 256-bit key > PBKDF2-ripemd160 266677 iterations per second for 256-bit key > PBKDF2-whirlpool 115787 iterations per second for 256-bit key > # Algorithm | Key | Encryption | Decryption > aes-cbc 128b46.0 MiB/s46.7 MiB/s > serpent-cbc 128b N/A N/A > twofish-cbc 128b N/A N/A > aes-cbc 256b46.5 MiB/s46.4 MiB/s > serpent-cbc 256b N/A N/A > twofish-cbc 256b N/A N/A > Segmentation fault > > Oops. > > ccree e6601000.crypto: Unsupported data size 65536. > Unable to handle kernel paging request at virtual address ffbf5c3c3c20 > Oy. Thank you for reporting this. I'll take a look at what is going on ASAP. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker values of β will give rise to dom!
Re: [PATCH 0/4][RFC v2] Introduce the in-kernel hibernation encryption
On Fr, 2018-07-20 at 12:25 +0200, Pavel Machek wrote: > Hi! Hello, > > Let me paste the log here: > > > > 1. (This is not to compare with uswsusp but other > > tools) One advantage is: Users do not have to > > encrypt the whole swap partition as other tools. > > Well.. encrypting the partition seems like good idea anyway. Yes, but it is a policy decision the kernel should not force. STD needs to work anyway. > > 2. Ideally kernel memory should be encrypted by the > >kernel itself. We have uswsusp to support user > >space hibernation, however doing the encryption > >in kernel space has more advantages: > >2.1 Not having to transfer plain text kernel memory to > >user space. Per Lee, Chun-Yi, uswsusp is disabled > >when the kernel is locked down: > >https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/ > >linux-fs.git/commit/?h=lockdown-20180410& > >id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 > >due to: > >"There have some functions be locked-down because > >there have no appropriate mechanisms to check the > >integrity of writing data." > >https://patchwork.kernel.org/patch/10476751/ > > So your goal is to make hibernation compatible with kernel > lockdown? Do your patches provide sufficient security that hibernation > can be enabled with kernel lockdown? OK, maybe I am dense, but if the key comes from user space, will that be enough? > >2.2 Not having to copy each page to user space > >one by one not in parallel, which might introduce > >significant amount of copy_to_user() and it might > >not be efficient on servers having large amount of DRAM. > > So how big speedup can be attributed by not doing copy_to_user? That would be an argument for compression in kernel space. Not encrpting would always be faster. > >2.3 Distribution has requirement to do snapshot > >signature for verification, which can be built > >by leveraging this patch set. > > Signatures can be done by uswsusp, too, right? Not if you want to keep the chain of trust intact. User space is not signed. > >2.4 The encryption is in the kernel, so it doesn't > >have to worry too much about bugs in user space > >utilities and similar, for example. > > Answer to bugs in userspace is _not_ to move code from userspace to kernel. Indeed. > > Joey Lee and I had a discussion on his previous work at > > https://patchwork.kernel.org/patch/10476751 > > We collaborate on this task and his snapshot signature > > feature can be based on this patch set. > > Well, his work can also work without your patchset, right? Yes. But you are objecting to encryption in kernel space at all, aren't you? Regards Oliver
Re: [PATCH 0/4][RFC v2] Introduce the in-kernel hibernation encryption
Hi! > > > 2. Ideally kernel memory should be encrypted by the > > >kernel itself. We have uswsusp to support user > > >space hibernation, however doing the encryption > > >in kernel space has more advantages: > > >2.1 Not having to transfer plain text kernel memory to > > >user space. Per Lee, Chun-Yi, uswsusp is disabled > > >when the kernel is locked down: > > >https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/ > > >linux-fs.git/commit/?h=lockdown-20180410& > > >id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 > > >due to: > > >"There have some functions be locked-down because > > >there have no appropriate mechanisms to check the > > >integrity of writing data." > > >https://patchwork.kernel.org/patch/10476751/ > > > > So your goal is to make hibernation compatible with kernel > > lockdown? Do your patches provide sufficient security that hibernation > > can be enabled with kernel lockdown? > > OK, maybe I am dense, but if the key comes from user space, will that > be enough? Yes, that seems to be one of problems of Yu Chen's patchset. > > > Joey Lee and I had a discussion on his previous work at > > > https://patchwork.kernel.org/patch/10476751 > > > We collaborate on this task and his snapshot signature > > > feature can be based on this patch set. > > > > Well, his work can also work without your patchset, right? > > Yes. But you are objecting to encryption in kernel space at all, > aren't you? I don't particulary love the idea of doing hibernation encryption in the kernel, correct. But we have this weird thing called secure boot, some people seem to want. So we may need some crypto in the kernel -- but I'd like something that works with uswsusp, too. Plus, it is mandatory that patch explains what security guarantees they want to provide against what kinds of attacks... Lee, Chun-Yi's patch seemed more promising. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: CCREE performance on R-Car H3 + crash
On Mon, Jul 23, 2018 at 1:31 PM, Gilad Ben-Yossef wrote: > On Mon, Jul 23, 2018 at 12:55 PM, Geert Uytterhoeven > wrote: >> Hi Gilad, >> >> CC linux-crypto for the crash log. >> >> On Sun, Jul 22, 2018 at 7:28 AM Gilad Ben-Yossef wrote: >>> On Thu, Jul 19, 2018 at 3:43 PM, Geert Uytterhoeven >>> wrote: >>> > I've noticed CCREE is used with a LUKS-formatted disk, so I did some small > ... >> >> $ cryptsetup benchmark >> # Tests are approximate using memory only (no storage IO). >> PBKDF2-sha1 478364 iterations per second for 256-bit key >> PBKDF2-sha256 927943 iterations per second for 256-bit key >> PBKDF2-sha512 360583 iterations per second for 256-bit key >> PBKDF2-ripemd160 266677 iterations per second for 256-bit key >> PBKDF2-whirlpool 115787 iterations per second for 256-bit key >> # Algorithm | Key | Encryption | Decryption >> aes-cbc 128b46.0 MiB/s46.7 MiB/s >> serpent-cbc 128b N/A N/A >> twofish-cbc 128b N/A N/A >> aes-cbc 256b46.5 MiB/s46.4 MiB/s >> serpent-cbc 256b N/A N/A >> twofish-cbc 256b N/A N/A >> Segmentation fault >> >> Oops. >> >> ccree e6601000.crypto: Unsupported data size 65536. >> Unable to handle kernel paging request at virtual address ffbf5c3c3c20 >> > > Oy. Thank you for reporting this. I'll take a look at what is going on ASAP. hmm... well, the plot thickens. I was able to recreate the "Unsupported data size 65536" message and now trying to understand why the check that causes it is there but - I wasn't able to get a crash, nor do I understand why this condition would result in a crash (it ends up returning -EINVAL)... :-( I am surely using a different tree though - I'm based on the cryptodev/master tree with cherry picking of just R-Car ccree clocks and enabling. I was thinking maybe it's a fix that is already in upstream cryptodev tree but not in your tree but didn't manage to identify any obvious suspects What tree are you based off? Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker values of β will give rise to dom!
Re: CCREE performance on R-Car H3 + crash
Hi Gilad,
On Mon, Jul 23, 2018 at 4:25 PM Gilad Ben-Yossef wrote:
> On Mon, Jul 23, 2018 at 1:31 PM, Gilad Ben-Yossef wrote:
> > On Mon, Jul 23, 2018 at 12:55 PM, Geert Uytterhoeven
> > wrote:
> >> CC linux-crypto for the crash log.
> >>
> >> On Sun, Jul 22, 2018 at 7:28 AM Gilad Ben-Yossef
> >> wrote:
> >>> On Thu, Jul 19, 2018 at 3:43 PM, Geert Uytterhoeven
> >>> wrote:
> >>> > I've noticed CCREE is used with a LUKS-formatted disk, so I did some
> >>> > small
> > ...
> >>
> >> $ cryptsetup benchmark
> >> # Tests are approximate using memory only (no storage IO).
> >> PBKDF2-sha1 478364 iterations per second for 256-bit key
> >> PBKDF2-sha256 927943 iterations per second for 256-bit key
> >> PBKDF2-sha512 360583 iterations per second for 256-bit key
> >> PBKDF2-ripemd160 266677 iterations per second for 256-bit key
> >> PBKDF2-whirlpool 115787 iterations per second for 256-bit key
> >> # Algorithm | Key | Encryption | Decryption
> >> aes-cbc 128b46.0 MiB/s46.7 MiB/s
> >> serpent-cbc 128b N/A N/A
> >> twofish-cbc 128b N/A N/A
> >> aes-cbc 256b46.5 MiB/s46.4 MiB/s
> >> serpent-cbc 256b N/A N/A
> >> twofish-cbc 256b N/A N/A
> >> Segmentation fault
> >>
> >> Oops.
> >>
> >> ccree e6601000.crypto: Unsupported data size 65536.
> >> Unable to handle kernel paging request at virtual address ffbf5c3c3c20
> >
> > Oy. Thank you for reporting this. I'll take a look at what is going on ASAP.
>
> hmm... well, the plot thickens.
>
> I was able to recreate the "Unsupported data size 65536" message and
> now trying to understand
> why the check that causes it is there but - I wasn't able to get a
> crash, nor do I understand why
> this condition would result in a crash (it ends up returning -EINVAL)... :-(
>
> I am surely using a different tree though - I'm based on the
> cryptodev/master tree with cherry picking of just R-Car ccree clocks
> and enabling.
>
> I was thinking maybe it's a fix that is already in upstream cryptodev
> tree but not in your tree but didn't manage to identify any obvious
> suspects
>
> What tree are you based off?
My tree is based on renesas-drivers-2018-07-17-v4.18-rc5 from
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/
Do you want me to try something different?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: Does /dev/urandom now block until initialised ?
On Mon, Jul 23, 2018 at 04:43:01AM +0100, Ken Moffat wrote: > Ted, > > last week you proposed an rfc patch to gather entropy from the CPU's > hwrng, and I was pleased - until I discovered one of my stalling > desktop machines does not have a hwrng. At that point I thought that > the problem was only from reading /dev/random, so I went away to look > at persuading the immediate consumer (unbound) to use /dev/urandom. > > Did that, no change. Ran strace from the bootscript, confirmed that > only /dev/urandom was being used, and that it seemed to be blocking. > Thought maybe this was the olnl problematic bootscript, tried moving > it to later, but hit the same problem on chronyd (again, seems to use > urandom). And yes, I probably should have started chronyd first > anyway, but that's irrelevant to this problem. Nope, /dev/urandom still doesn't block. Are you sure it isn't caused by something calling getrandom(2) --- which *will* block? We intentionally left /dev/urandom non-blocking, because of backwards compatibility. > BUT: I'm not sure if I've correctly understood what is happening. > It seems to me that the fix for CVE-2018-1108 (4.17-rc1, 4.16.4) > means /dev/urandom will now block until fully initialised. > > Is that correct and intentional ? No, that's not right. What the fix does is more accurately account for the entropy accounting before getrandom(2) would become non-blocking. There were a bunch of things we were doing wrong, including assuming that 100% of the bytes being sent via add_device_entropy() were random --- when some of the things that were feeding into it was the (fixed) information you would get from running dmidecode (e.g., the fixed results from the BIOS configuration data). Some of those bytes might not be known to an external adversary (such as your CPU mainboard's serial number), but it's not exactly *Secret*. > If so, to get the affected desktop machines to boot I seem to have > some choices... Well, this probably isn't going to be popular, but the other thing that might help is you could switch distro's. I'm guessing you run a Red Hat distro, probably Fedora, right? The problem which most people are seeing turns out to be a terrible interaction between dracut-fips, systemd and a Red Hat specific patch to libgcrypt for FIPS/FEDRAMP compliance: https://src.fedoraproject.org/rpms/libgcrypt/blob/master/f/libgcrypt-1.6.2-fips-ctor.patch#_23 Uninstalling dracut-fips and recreating the initramfs might also help. One of the reasons why I didn't see the problem when I was developing the remediation patch for CVE-2018-1108 is because I run Debian testing, which doesn't have this particular Red Hat patch. > The latter certainly lets it boot in a reasonable time, but people > who understand this seem to regard it as untrustworthy. For users > of /dev/urandom that is no big deal, but does it not mean that the > values from /dev/random will be similarly untrustworthy and > therefore I should not use this machine for generating long-lived > secure keys ? This really depends on how paranoid / careful you are. Remember, your keyboard controller was almost certainly built in Shenzhen, China, and Matt Blaze published a paper on the Jitterbug in 2006: http://www.crypto.com/papers/jbug-Usenix06-final.pdf In practice, after 30 minutes of operation, especially if you are using the keyboard, the entropy pool *will* be sufficiently randomized, whether or not it was sufficientl randomized at boot. The real danger of CVE-2018-1108 was always long-term keys generated at first boot. That was the problem that was discussed in the "Mining your p's and q's: Detection of Widespread Weak Keys in Network Devices" (see https://factorable.net). So generating long-lived keys means (a) you need to be sure you trust all of the software on the system --- some very paranoid people such as Bruce Schneier used a freshly installed machine from CD-ROM that was never attached to the network before examining materials from Edward Snowden, and (b) making sure the entropy pool is initialized. Remember we are constantly feeding input from the hardware sources into the entropy pool; it doesn't stop the moment we think the entropy pool is initialized. And you can always mix extra "stuff" into the entropy pool by echoing the results of say, taking series of dice rolls, aond sending it via the "cat" or "echo" command into /dev/urhandom. So it should be possible to use the machine for generated long lived keys; you might just need to be a bit more careful before you do it. It's really keys generated automatically at boot that are most at risk --- and you can always regenerate the host SSH keys after a fresh install. In fact, what I have done in the past when I first login to a freshly created Cloud VM system is to run command like "dd if=/dev/urandom count=1 bs=256 | od -x", then login to VM, and then run "cat > /dev/urandom", and cut and paste the results of the od -x output into the guest VM
Re: Does /dev/urandom now block until initialised ?
On Mon, Jul 23, 2018 at 11:16 AM, Theodore Y. Ts'o wrote: > On Mon, Jul 23, 2018 at 04:43:01AM +0100, Ken Moffat wrote: >> ... > One of the reasons why I didn't see the problem when I was developing > the remediation patch for CVE-2018-1108 is because I run Debian > testing, which doesn't have this particular Red Hat patch. Off-topic, I'm kind of surprised it took that long to fix it (if I am parsing things correctly). I believe Stephan Mueller wrote up the weakness a couple of years ago. He's the one who explained the interactions to me. Mueller was even cited at https://github.com/systemd/systemd/issues/4167. It is too bad he Mueller not receive credit for it in the CVE database. Jeff
Re: [PATCH 0/4][RFC v2] Introduce the in-kernel hibernation encryption
Hi, On Mon, Jul 23, 2018 at 01:42:36PM +0200, Oliver Neukum wrote: > On Fr, 2018-07-20 at 12:25 +0200, Pavel Machek wrote: > > Hi! > > Hello, > > > > Let me paste the log here: > > > > > > 1. (This is not to compare with uswsusp but other > > > tools) One advantage is: Users do not have to > > > encrypt the whole swap partition as other tools. > > > > Well.. encrypting the partition seems like good idea anyway. > > Yes, but it is a policy decision the kernel should not force. > STD needs to work anyway. > If the swap partition is too large, and there's low usage of memory, then it might a little time costly to encrypt the whole partition. You are right, people has choice to choose which mode they like. > > > 2. Ideally kernel memory should be encrypted by the > > >kernel itself. We have uswsusp to support user > > >space hibernation, however doing the encryption > > >in kernel space has more advantages: > > >2.1 Not having to transfer plain text kernel memory to > > >user space. Per Lee, Chun-Yi, uswsusp is disabled > > >when the kernel is locked down: > > >https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/ > > >linux-fs.git/commit/?h=lockdown-20180410& > > >id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 > > >due to: > > >"There have some functions be locked-down because > > >there have no appropriate mechanisms to check the > > >integrity of writing data." > > >https://patchwork.kernel.org/patch/10476751/ > > > > So your goal is to make hibernation compatible with kernel > > lockdown? Do your patches provide sufficient security that hibernation > > can be enabled with kernel lockdown? > > OK, maybe I am dense, but if the key comes from user space, will that > be enough? > Good point, we once tried to generate key in kernel, but people suggest to generate key in userspace and provide it to the kernel, which is what ecryptfs do currently, so it seems this should also be safe for encryption in kernel. https://www.spinics.net/lists/linux-crypto/msg33145.html Thus Chun-Yi's signature can use EFI key and both the key from user space. Best, Yu > > >2.2 Not having to copy each page to user space > > >one by one not in parallel, which might introduce > > >significant amount of copy_to_user() and it might > > >not be efficient on servers having large amount of DRAM. > > > > So how big speedup can be attributed by not doing copy_to_user? > > That would be an argument for compression in kernel space. > Not encrpting would always be faster. > > > >2.3 Distribution has requirement to do snapshot > > >signature for verification, which can be built > > >by leveraging this patch set. > > > > Signatures can be done by uswsusp, too, right? > > Not if you want to keep the chain of trust intact. User space > is not signed. > > > >2.4 The encryption is in the kernel, so it doesn't > > >have to worry too much about bugs in user space > > >utilities and similar, for example. > > > > Answer to bugs in userspace is _not_ to move code from userspace to kernel. > > Indeed. > > > > Joey Lee and I had a discussion on his previous work at > > > https://patchwork.kernel.org/patch/10476751 > > > We collaborate on this task and his snapshot signature > > > feature can be based on this patch set. > > > > Well, his work can also work without your patchset, right? > > Yes. But you are objecting to encryption in kernel space at all, > aren't you? > > Regards > Oliver >
Re: [PATCH 0/4][RFC v2] Introduce the in-kernel hibernation encryption
Hello, On Mon, Jul 23, 2018 at 02:22:27PM +0200, Pavel Machek wrote: > Hi! > > > > > 2. Ideally kernel memory should be encrypted by the > > > >kernel itself. We have uswsusp to support user > > > >space hibernation, however doing the encryption > > > >in kernel space has more advantages: > > > >2.1 Not having to transfer plain text kernel memory to > > > >user space. Per Lee, Chun-Yi, uswsusp is disabled > > > >when the kernel is locked down: > > > >https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/ > > > >linux-fs.git/commit/?h=lockdown-20180410& > > > >id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 > > > >due to: > > > >"There have some functions be locked-down because > > > >there have no appropriate mechanisms to check the > > > >integrity of writing data." > > > >https://patchwork.kernel.org/patch/10476751/ > > > > > > So your goal is to make hibernation compatible with kernel > > > lockdown? Do your patches provide sufficient security that hibernation > > > can be enabled with kernel lockdown? > > > > OK, maybe I am dense, but if the key comes from user space, will that > > be enough? > > Yes, that seems to be one of problems of Yu Chen's patchset. > It is a trade off to derive the key in user space, we once tried to derive the key in user space, and people suggested a better way is to do it in user space. And there is a similar user case of kernel using key from user space is derived from ecryptfs for ext4. > > > > Joey Lee and I had a discussion on his previous work at > > > > https://patchwork.kernel.org/patch/10476751 > > > > We collaborate on this task and his snapshot signature > > > > feature can be based on this patch set. > > > > > > Well, his work can also work without your patchset, right? > > > > Yes. But you are objecting to encryption in kernel space at all, > > aren't you? > > I don't particulary love the idea of doing hibernation encryption in > the kernel, correct. > > But we have this weird thing called secure boot, some people seem to > want. So we may need some crypto in the kernel -- but I'd like > something that works with uswsusp, too. Plus, it is mandatory that > patch explains what security guarantees they want to provide against > what kinds of attacks... > > Lee, Chun-Yi's patch seemed more promising. Pavel > The only difference between Chun-Yi's hibernation encrytion solution and our solution is that his strategy encrypts the snapshot from sratch, and ours encryts each page before them going to block device. The benefit of his solution is that the snapshot can be encrypt in kernel first thus the uswsusp is allowed to read it to user space even kernel is lock down. And I had a discussion with Chun-Yi that we can use his snapshot solution to make uswsusp happy, and we share the crypto help code and he can also use our user provided key for his signature. >From this point of view, our code are actually the same, except that we can help clean up the code and also enhance some encrytion process for his solution. I don't know why you don't like encryption in kernel, because from my point of view, without encryption hibernation in kernel, uswsusp could not be enabled if kernel is lock down : -) Or do I miss something? Best, Yu > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: Does /dev/urandom now block until initialised ?
On 23 July 2018 at 16:16, Theodore Y. Ts'o wrote: > On Mon, Jul 23, 2018 at 04:43:01AM +0100, Ken Moffat wrote: >> >> Did that, no change. Ran strace from the bootscript, confirmed that >> only /dev/urandom was being used, and that it seemed to be blocking. > > Nope, /dev/urandom still doesn't block. Are you sure it isn't caused > by something calling getrandom(2) --- which *will* block? I'm not at all sure, which was why I asked. > > We intentionally left /dev/urandom non-blocking, because of backwards > compatibility. > >> BUT: I'm not sure if I've correctly understood what is happening. >> It seems to me that the fix for CVE-2018-1108 (4.17-rc1, 4.16.4) >> means /dev/urandom will now block until fully initialised. >> >> Is that correct and intentional ? > > No, that's not right. What the fix does is more accurately account > for the entropy accounting before getrandom(2) would become > non-blocking. There were a bunch of things we were doing wrong, > including assuming that 100% of the bytes being sent via > add_device_entropy() were random --- when some of the things that were > feeding into it was the (fixed) information you would get from running > dmidecode (e.g., the fixed results from the BIOS configuration data). > > Some of those bytes might not be known to an external adversary (such > as your CPU mainboard's serial number), but it's not exactly *Secret*. > >> If so, to get the affected desktop machines to boot I seem to have >> some choices... > > Well, this probably isn't going to be popular, but the other thing > that might help is you could switch distro's. I'm guessing you run a > Red Hat distro, probably Fedora, right? > Wrong, linuxfromscratch (sysv version) and beyond linuxfromscratch plus extras such as chronyd. The only initrd is on the haswell, and just for intel microcode. > The problem which most people are seeing turns out to be a terrible > interaction between dracut-fips, systemd and a Red Hat specific patch > to libgcrypt for FIPS/FEDRAMP compliance: > > https://src.fedoraproject.org/rpms/libgcrypt/blob/master/f/libgcrypt-1.6.2-fips-ctor.patch#_23 > > Uninstalling dracut-fips and recreating the initramfs might also help. > > One of the reasons why I didn't see the problem when I was developing > the remediation patch for CVE-2018-1108 is because I run Debian > testing, which doesn't have this particular Red Hat patch. > >> The latter certainly lets it boot in a reasonable time, but people >> who understand this seem to regard it as untrustworthy. For users >> of /dev/urandom that is no big deal, but does it not mean that the >> values from /dev/random will be similarly untrustworthy and >> therefore I should not use this machine for generating long-lived >> secure keys ? > > This really depends on how paranoid / careful you are. Remember, your > keyboard controller was almost certainly built in Shenzhen, China, and > Matt Blaze published a paper on the Jitterbug in 2006: > > http://www.crypto.com/papers/jbug-Usenix06-final.pdf > > In practice, after 30 minutes of operation, especially if you are > using the keyboard, the entropy pool *will* be sufficiently > randomized, whether or not it was sufficientl randomized at boot. The > real danger of CVE-2018-1108 was always long-term keys generated at > first boot. That was the problem that was discussed in the "Mining > your p's and q's: Detection of Widespread Weak Keys in Network > Devices" (see https://factorable.net). > > So generating long-lived keys means (a) you need to be sure you trust > all of the software on the system --- some very paranoid people such > as Bruce Schneier used a freshly installed machine from CD-ROM that > was never attached to the network before examining materials from > Edward Snowden, and (b) making sure the entropy pool is initialized. > > Remember we are constantly feeding input from the hardware sources > into the entropy pool; it doesn't stop the moment we think the entropy > pool is initialized. And you can always mix extra "stuff" into the > entropy pool by echoing the results of say, taking series of dice > rolls, aond sending it via the "cat" or "echo" command into > /dev/urhandom. > > So it should be possible to use the machine for generated long lived > keys; you might just need to be a bit more careful before you do it. > It's really keys generated automatically at boot that are most at risk > --- and you can always regenerate the host SSH keys after a fresh > install. In fact, what I have done in the past when I first login to > a freshly created Cloud VM system is to run command like "dd > if=/dev/urandom count=1 bs=256 | od -x", then login to VM, and then > run "cat > /dev/urandom", and cut and paste the results of the od -x > output into the guest VM, to better initialize the entropy pool on the > VM before regenerating the host SSH keys. > > Cheers, > > - Ted Thanks. In that case I'll go with the simple fix (haveged). ĸe
Re: Does /dev/urandom now block until initialised ?
On Mon, Jul 23, 2018 at 12:11:12PM -0400, Jeffrey Walton wrote: > > I believe Stephan Mueller wrote up the weakness a couple of years ago. > He's the one who explained the interactions to me. Mueller was even > cited at https://github.com/systemd/systemd/issues/4167. Stephan had a lot of complaints about the existing random driver. That's because he has a replacement driver that he has been pushing, and instead of giving explicit complaints with specific patches to fix those specific issues, he have a generalized blast of complaints, plus a "big bang rewrite". I've reviewed his lrng doc, and this specific issue was not among his complaints. Quite a while ago, I had gone through his document, and had specifically addressed each of his complaints. As far as I have been able determine, all of the specific technical complaints (as opposed to personal preference issues) have been addressed. His complaint is a text book complaint about how *not* to file a bug report. That being said, we try to take bug reports from as many sources as possible even if they aren't well formed or submitted in the ideal place. (I'm reminded of Linux's networking scalability limitations which Microsoft filed in the Wall Street Journal 15+ years ago --- and which only applied if you had 4 CPU's and four 10 megabit networking cards; if you had four CPU's and a 100 megabit networking card, Linux would grind Microsoft into the dust; still it was a bug, and we appreciated the report and we fixed it, even if it wasn't filed in the ideal forum. :-) > It is too bad he Mueller not receive credit for it in the CVE database. As near as I can tell, he doesn't deserve it for this particular issue. It's all Jann Horn and Google's Project Zero. (And his writeup is a textbook example of how to report this sort of issue with great specifity and analysis.) - Ted
