Re: [RESEND PATCH 02/10] crypto: sa2ul: Add crypto driver

[Keerthy]

On 28/06/19 10:37 AM, Eric Biggers wrote:

On Fri, Jun 28, 2019 at 09:57:37AM +0530, Keerthy wrote:

The Security Accelerator (SA2_UL) subsystem provides hardware
cryptographic acceleration for the following use cases:
• Encryption and authentication for secure boot
• Encryption and authentication of content in applications
   requiring DRM (digital rights management) and
   content/asset protection
The device includes one instantiation of SA2_UL named SA2_UL0

SA2_UL supports the following cryptographic industry standards to enable data 
authentication, data
integrity and data confidentiality.

Crypto function library for software acceleration
o AES operation
o 3DES operation
o SHA1 operation
o MD5 operation
o SHA2 – 224, 256, 384, 512 operation

Authentication supported via following hardware cores
o SHA1
o MD5
o SHA2 -224
o SHA2-256
o SHA2-384
o SHA2-512

What about HMAC?

Your actual driver only exposes HMAC-SHA*, not SHA* anything.

What does the hardware actually support?

Hardware supports both SHA and HMAC-SHA

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 603413f28fa3..b9a3fa026c74 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -785,4 +785,21 @@ config CRYPTO_DEV_CCREE

source "drivers/crypto/hisilicon/Kconfig" +config CRYPTO_DEV_SA2UL

+       tristate "Support for TI security accelerator"
+       depends on ARCH_K3 || COMPILE_TEST
+       select ARM64_CRYPTO
+       select CRYPTO_AES
+       select CRYPTO_AES_ARM64
+       select CRYPTO_SHA1
+       select CRYPTO_MD5
+       select CRYPTO_ALGAPI
+       select CRYPTO_AUTHENC
+       select HW_RANDOM
+       default m if ARCH_K3
+       help
+         Keystone devices include a security accelerator engine that may be
+         used for crypto offload.  Select this if you want to use hardware
+         acceleration for cryptographic algorithms on these devices.

This shouldn't be enabled by default.  Note that arm64 defconfig sets ARCH_K3 as
well as lots of other ARCH_* options, so clearly just because ARCH_K3 is set
doesn't mean the kernel is being built specifically for your platform.

okay. I will remove that.

+/*
+ * Mode Control Instructions for various Key lengths 128, 192, 256
+ * For CBC (Cipher Block Chaining) mode for encryption
+ */
+static u8 mci_cbc_enc_array[3][MODE_CONTROL_BYTES] = {
+       {       0x21, 0x00, 0x00, 0x18, 0x88, 0x0a, 0xaa, 0x4b, 0x7e, 0x00,
+               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00        },
+       {       0x21, 0x00, 0x00, 0x18, 0x88, 0x4a, 0xaa, 0x4b, 0x7e, 0x00,
+               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00        },
+       {       0x21, 0x00, 0x00, 0x18, 0x88, 0x8a, 0xaa, 0x4b, 0x7e, 0x00,
+               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00        },
+};

Use 'const' for static constants.

Okay

+static int sa_aes_cbc_setkey(struct crypto_ablkcipher *tfm, const u8 *key,
+                            unsigned int keylen)
+{
+       struct algo_data *ad = kzalloc(sizeof(*ad), GFP_KERNEL);

Need to check from error for all memory allocations.

+static struct sa_alg_tmpl sa_algs[] = {
+       {.type = CRYPTO_ALG_TYPE_ABLKCIPHER,

ablkcipher API is deprecated.  Use skcipher instead.

Okay

(To be clear, these are just a few things I happened to notice from very quickly
skimming through this patch.  I don't have time to do a proper review of random
drivers.)

I will incorporate the comments in v2.

Thanks for your quick review.

- Eric