Re: [PATCH v6 4/4] crypto: Add Allwinner Security System crypto accelerator
Le 26/03/2015 19:31, Boris Brezillon a écrit :
> Hi Corentin,
>
> Here is a quick review, there surely are a lot of other things I didn't
> spot.
>
> On Mon, 16 Mar 2015 20:01:22 +0100
> LABBE Corentin wrote:
>
>> Add support for the Security System included in Allwinner SoC A20.
>> The Security System is a hardware cryptographic accelerator that support:
>> - MD5 and SHA1 hash algorithms
>> - AES block cipher in CBC mode with 128/196/256bits keys.
>> - DES and 3DES block cipher in CBC mode
>>
>> Signed-off-by: LABBE Corentin
>> ---
>
> [...]
>
>> +static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
>> +{
>> +struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
>> +struct sunxi_tfm_ctx *op = crypto_ablkcipher_ctx(tfm);
>> +const char *cipher_type;
>> +struct sunxi_ss_ctx *ss = op->ss;
>> +
>> +if (areq->nbytes == 0)
>> +return 0;
>> +
>> +if (areq->info == NULL) {
>> +dev_err(ss->dev, "ERROR: Empty IV\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (areq->src == NULL || areq->dst == NULL) {
>> +dev_err(ss->dev, "ERROR: Some SGs are NULL\n");
>> +return -EINVAL;
>> +}
>> +
>> +cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
>> +
>> +if (strcmp("cbc(aes)", cipher_type) == 0) {
>> +mode |= SS_OP_AES | SS_CBC | SS_ENABLED | op->keymode;
>> +return sunxi_ss_aes_poll(areq, mode);
>> +}
>> +
>> +if (strcmp("cbc(des)", cipher_type) == 0) {
>> +mode |= SS_OP_DES | SS_CBC | SS_ENABLED | op->keymode;
>> +return sunxi_ss_des_poll(areq, mode);
>> +}
>> +
>> +if (strcmp("cbc(des3_ede)", cipher_type) == 0) {
>> +mode |= SS_OP_3DES | SS_CBC | SS_ENABLED | op->keymode;
>> +return sunxi_ss_des_poll(areq, mode);
>> +}
>
> Hm, I'm not sure doing these string comparisons in the crypto operation
> path is a good idea.
> Moreover, you're doing 3 string comparisons, even though only one can
> be valid at a time (using 'else if' would have been a bit better).
>
> Anyway, IMHO this function should be split into 3 functions, and
> referenced by your alg template definitions.
> Something like:
>
> int sunxi_ss_xxx_encrypt(struct ablkcipher_request *areq)
> {
> /* put your cipher specific intialization here */
>
> return sunxi_ss_xxx_poll(areq, SS_ENCRYPTION);
> }
>
> int sunxi_ss_xxx_decrypt(struct ablkcipher_request *areq)
> {
> /* put your cipher specific intialization here */
>
> return sunxi_ss_xxx_poll(areq, SS_DECRYPTION);
> }
>
You are right, I have added more block mode, and that was added too much strcmp.
So splitting in simplier functions is good.
>
>> +
>> +int sunxi_ss_cipher_init(struct crypto_tfm *tfm)
>> +{
>> +const char *name = crypto_tfm_alg_name(tfm);
>> +struct sunxi_tfm_ctx *op = crypto_tfm_ctx(tfm);
>> +struct crypto_alg *alg = tfm->__crt_alg;
>> +struct sunxi_ss_alg_template *algt;
>> +struct sunxi_ss_ctx *ss;
>> +
>> +memset(op, 0, sizeof(struct sunxi_tfm_ctx));
>> +
>> +algt = container_of(alg, struct sunxi_ss_alg_template, alg.crypto);
>> +ss = algt->ss;
>> +op->ss = algt->ss;
>> +
>> +/* fallback is needed only for DES/3DES */
>> +if (strcmp("cbc(des)", name) == 0 ||
>> +strcmp("cbc(des3_ede)", name) == 0) {
>> +op->fallback = crypto_alloc_ablkcipher(name, 0,
>> +CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK);
>> +if (IS_ERR(op->fallback)) {
>> +dev_err(ss->dev, "ERROR: allocating fallback algo %s\n",
>> +name);
>> +return PTR_ERR(op->fallback);
>> +}
>> +}
>
> Ditto: just create a specific init function for the des case:
>
> int sunxi_ss_cbc_des_init(struct crypto_tfm *tfm)
> {
> sunxi_ss_cipher_init(tfm);
>
> op->fallback = crypto_alloc_ablkcipher(name, 0,
> CRYPTO_ALG_ASYNC |
> CRYPTO_ALG_NEED_FALLBACK);
> if (IS_ERR(op->fallback)) {
> dev_err(ss->dev, "ERROR: allocating fallback algo %s\n",
> name);
> return PTR_ERR(op->fallback);
> }
>
> return 0;
> }
>
Ok
>
> [..]
>
>> +/*
>> + * Optimized function for the case where we have only one SG,
>> + * so we can use kmap_atomic
>> + */
>> +static int sunxi_ss_aes_poll_atomic(struct ablkcipher_request *areq)
>> +{
>> +u32 spaces;
>> +struct scatterlist *in_sg = areq->src;
>> +struct scatterlist *out_sg = areq->dst;
>> +void *src_addr;
>> +void *dst_addr;
>> +unsigned int ileft = areq->nbytes;
>> +unsigned int oleft = areq->nbytes;
>> +unsigned int todo;
>> +u32 *src32;
>> +u32 *dst32;
>> +u32 rx_cnt = 32;
>> +u32 tx_cnt = 0;
>> +int i;
>> +struct crypto_ablkcipher *tfm = crypto_ablkciph
Re: [PATCH v6 4/4] crypto: Add Allwinner Security System crypto accelerator
Hi Corentin,
Here is a quick review, there surely are a lot of other things I didn't
spot.
On Mon, 16 Mar 2015 20:01:22 +0100
LABBE Corentin wrote:
> Add support for the Security System included in Allwinner SoC A20.
> The Security System is a hardware cryptographic accelerator that support:
> - MD5 and SHA1 hash algorithms
> - AES block cipher in CBC mode with 128/196/256bits keys.
> - DES and 3DES block cipher in CBC mode
>
> Signed-off-by: LABBE Corentin
> ---
[...]
> +static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
> +{
> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
> + struct sunxi_tfm_ctx *op = crypto_ablkcipher_ctx(tfm);
> + const char *cipher_type;
> + struct sunxi_ss_ctx *ss = op->ss;
> +
> + if (areq->nbytes == 0)
> + return 0;
> +
> + if (areq->info == NULL) {
> + dev_err(ss->dev, "ERROR: Empty IV\n");
> + return -EINVAL;
> + }
> +
> + if (areq->src == NULL || areq->dst == NULL) {
> + dev_err(ss->dev, "ERROR: Some SGs are NULL\n");
> + return -EINVAL;
> + }
> +
> + cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
> +
> + if (strcmp("cbc(aes)", cipher_type) == 0) {
> + mode |= SS_OP_AES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_aes_poll(areq, mode);
> + }
> +
> + if (strcmp("cbc(des)", cipher_type) == 0) {
> + mode |= SS_OP_DES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_des_poll(areq, mode);
> + }
> +
> + if (strcmp("cbc(des3_ede)", cipher_type) == 0) {
> + mode |= SS_OP_3DES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_des_poll(areq, mode);
> + }
Hm, I'm not sure doing these string comparisons in the crypto operation
path is a good idea.
Moreover, you're doing 3 string comparisons, even though only one can
be valid at a time (using 'else if' would have been a bit better).
Anyway, IMHO this function should be split into 3 functions, and
referenced by your alg template definitions.
Something like:
int sunxi_ss_xxx_encrypt(struct ablkcipher_request *areq)
{
/* put your cipher specific intialization here */
return sunxi_ss_xxx_poll(areq, SS_ENCRYPTION);
}
int sunxi_ss_xxx_decrypt(struct ablkcipher_request *areq)
{
/* put your cipher specific intialization here */
return sunxi_ss_xxx_poll(areq, SS_DECRYPTION);
}
> +
> +int sunxi_ss_cipher_init(struct crypto_tfm *tfm)
> +{
> + const char *name = crypto_tfm_alg_name(tfm);
> + struct sunxi_tfm_ctx *op = crypto_tfm_ctx(tfm);
> + struct crypto_alg *alg = tfm->__crt_alg;
> + struct sunxi_ss_alg_template *algt;
> + struct sunxi_ss_ctx *ss;
> +
> + memset(op, 0, sizeof(struct sunxi_tfm_ctx));
> +
> + algt = container_of(alg, struct sunxi_ss_alg_template, alg.crypto);
> + ss = algt->ss;
> + op->ss = algt->ss;
> +
> + /* fallback is needed only for DES/3DES */
> + if (strcmp("cbc(des)", name) == 0 ||
> + strcmp("cbc(des3_ede)", name) == 0) {
> + op->fallback = crypto_alloc_ablkcipher(name, 0,
> + CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK);
> + if (IS_ERR(op->fallback)) {
> + dev_err(ss->dev, "ERROR: allocating fallback algo %s\n",
> + name);
> + return PTR_ERR(op->fallback);
> + }
> + }
Ditto: just create a specific init function for the des case:
int sunxi_ss_cbc_des_init(struct crypto_tfm *tfm)
{
sunxi_ss_cipher_init(tfm);
op->fallback = crypto_alloc_ablkcipher(name, 0,
CRYPTO_ALG_ASYNC |
CRYPTO_ALG_NEED_FALLBACK);
if (IS_ERR(op->fallback)) {
dev_err(ss->dev, "ERROR: allocating fallback algo %s\n",
name);
return PTR_ERR(op->fallback);
}
return 0;
}
[..]
> +/*
> + * Optimized function for the case where we have only one SG,
> + * so we can use kmap_atomic
> + */
> +static int sunxi_ss_aes_poll_atomic(struct ablkcipher_request *areq)
> +{
> + u32 spaces;
> + struct scatterlist *in_sg = areq->src;
> + struct scatterlist *out_sg = areq->dst;
> + void *src_addr;
> + void *dst_addr;
> + unsigned int ileft = areq->nbytes;
> + unsigned int oleft = areq->nbytes;
> + unsigned int todo;
> + u32 *src32;
> + u32 *dst32;
> + u32 rx_cnt = 32;
> + u32 tx_cnt = 0;
> + int i;
> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
> + struct sunxi_tfm_ctx *op = crypto_ablkcipher_ctx(tfm);
> + struct sunxi_ss_ctx *ss = op->ss;
> +
> + src_addr = kmap_atomic(sg_page(in_sg)) + in_sg->offset;
> + if (src_addr == NULL) {
> + dev_err(ss->dev, "kmap_atomic error for src SG\n");
> +
[PATCH v6 4/4] crypto: Add Allwinner Security System crypto accelerator
Add support for the Security System included in Allwinner SoC A20.
The Security System is a hardware cryptographic accelerator that support:
- MD5 and SHA1 hash algorithms
- AES block cipher in CBC mode with 128/196/256bits keys.
- DES and 3DES block cipher in CBC mode
Signed-off-by: LABBE Corentin
---
drivers/crypto/Kconfig| 17 ++
drivers/crypto/Makefile | 1 +
drivers/crypto/sunxi-ss/Makefile | 2 +
drivers/crypto/sunxi-ss/sunxi-ss-cipher.c | 408 +
drivers/crypto/sunxi-ss/sunxi-ss-core.c | 339 +
drivers/crypto/sunxi-ss/sunxi-ss-hash.c | 475 ++
drivers/crypto/sunxi-ss/sunxi-ss.h| 200 +
7 files changed, 1442 insertions(+)
create mode 100644 drivers/crypto/sunxi-ss/Makefile
create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-core.c
create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-hash.c
create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss.h
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 2fb0fdf..9ba9759 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -436,4 +436,21 @@ config CRYPTO_DEV_QCE
hardware. To compile this driver as a module, choose M here. The
module will be called qcrypto.
+config CRYPTO_DEV_SUNXI_SS
+ tristate "Support for Allwinner Security System cryptographic
accelerator"
+ depends on ARCH_SUNXI
+ select CRYPTO_MD5
+ select CRYPTO_SHA1
+ select CRYPTO_AES
+ select CRYPTO_DES
+ select CRYPTO_BLKCIPHER
+ help
+ Some Allwinner SoC have a crypto accelerator named
+ Security System. Select this if you want to use it.
+ The Security System handle AES/DES/3DES ciphers in CBC mode
+ and SHA1 and MD5 hash algorithms.
+
+ To compile this driver as a module, choose M here: the module
+ will be called sunxi-ss.
+
endif # CRYPTO_HW
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index 3924f93..856545c 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -25,3 +25,4 @@ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
+obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss/
diff --git a/drivers/crypto/sunxi-ss/Makefile b/drivers/crypto/sunxi-ss/Makefile
new file mode 100644
index 000..8bb287d
--- /dev/null
+++ b/drivers/crypto/sunxi-ss/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss.o
+sunxi-ss-y += sunxi-ss-core.o sunxi-ss-hash.o sunxi-ss-cipher.o
diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
new file mode 100644
index 000..3ed0ad0
--- /dev/null
+++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
@@ -0,0 +1,408 @@
+/*
+ * sunxi-ss-cipher.c - hardware cryptographic accelerator for Allwinner A20 SoC
+ *
+ * Copyright (C) 2013-2015 Corentin LABBE
+ *
+ * This file add support for AES cipher with 128,192,256 bits
+ * keysize in CBC mode.
+ * Add support also for DES and 3DES in CBC mode.
+ *
+ * You could find the datasheet in Documentation/arm/sunxi/README
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include "sunxi-ss.h"
+
+static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
+{
+ struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
+ struct sunxi_tfm_ctx *op = crypto_ablkcipher_ctx(tfm);
+ const char *cipher_type;
+ struct sunxi_ss_ctx *ss = op->ss;
+
+ if (areq->nbytes == 0)
+ return 0;
+
+ if (areq->info == NULL) {
+ dev_err(ss->dev, "ERROR: Empty IV\n");
+ return -EINVAL;
+ }
+
+ if (areq->src == NULL || areq->dst == NULL) {
+ dev_err(ss->dev, "ERROR: Some SGs are NULL\n");
+ return -EINVAL;
+ }
+
+ cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
+
+ if (strcmp("cbc(aes)", cipher_type) == 0) {
+ mode |= SS_OP_AES | SS_CBC | SS_ENABLED | op->keymode;
+ return sunxi_ss_aes_poll(areq, mode);
+ }
+
+ if (strcmp("cbc(des)", cipher_type) == 0) {
+ mode |= SS_OP_DES | SS_CBC | SS_ENABLED | op->keymode;
+ return sunxi_ss_des_poll(areq, mode);
+ }
+
+ if (strcmp("cbc(des3_ede)", cipher_type) == 0) {
+ mode |= SS_OP_3DES | SS_CBC | SS_ENABLED | op->keymode;
+ return sunxi_ss_des_poll(areq, mode);
+ }
+
+ dev_err(ss->dev, "ERROR: Cipher %s not handled\n", cipher_type);
+ return -EINVAL;
+}
+
+int sunxi_ss
