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