[PATCH v2] crypto/fsl: Add support for black key blob
ZHIZHIKIN Andrey
andrey.zhizhikin at leica-geosystems.com
Wed Sep 28 19:45:19 CEST 2022
Hello Gaurav,
> -----Original Message-----
> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Gaurav Jain
> Sent: Wednesday, September 28, 2022 12:40 PM
> To: u-boot at lists.denx.de; Stefano Babic <sbabic at denx.de>
> Cc: Fabio Estevam <festevam at gmail.com>; Peng Fan <peng.fan at nxp.com>; Ye Li
> <ye.li at nxp.com>; NXP i . MX U-Boot Team <uboot-imx at nxp.com>; Horia Geanta
> <horia.geanta at nxp.com>; Varun Sethi <V.Sethi at nxp.com>; Gaurav Jain
> <gaurav.jain at nxp.com>
> Subject: [PATCH v2] crypto/fsl: Add support for black key blob
>
> modified caam descriptor to support black key blob.
A bit more context on this change in appreciated. How shall this
can/be used, what is the benefit, how the "black" blob needs to
be wrapped, and which mechanism shall be used for wrapping?
>
> Signed-off-by: Gaurav Jain <gaurav.jain at nxp.com>
> ---
> changes in v2:
> - rebase to latest
>
> cmd/blob.c | 12 ++++++++----
Nitpick: This file is missing copyright note since initial
Submission, which was done with commit c5de15cbc8a8
("crypto/fsl: Add command for encapsulating/decapsulating
blobs").
Does it make sense that you add it?
> drivers/crypto/fsl/desc.h | 1 +
> drivers/crypto/fsl/fsl_blob.c | 21 +++++++++++++--------
> drivers/crypto/fsl/jobdesc.c | 24 +++++++++++++++++++-----
> drivers/crypto/fsl/jobdesc.h | 8 ++++++--
> 5 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/cmd/blob.c b/cmd/blob.c
> index e2efae7a11..5c459b6f19 100644
> --- a/cmd/blob.c
> +++ b/cmd/blob.c
> @@ -21,10 +21,12 @@
> * @src: - Address of data to be decapsulated
> * @dst: - Address of data to be decapsulated
> * @len: - Size of data to be decapsulated
> + * @keycolor - Determines if the source data is covered (black key) or
> + * plaintext.
> *
> * Returns zero on success,and negative on error.
> */
> -__weak int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> +__weak int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len, u8 keycolor)
Is the only intention to indicate if the blob is "black/red" (wrapped/unwrapped)?
In this case, perhaps the `bool` is sufficient.
> {
> return 0;
> }
> @@ -35,10 +37,12 @@ __weak int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> * @src: - Address of data to be encapsulated
> * @dst: - Address of data to be encapsulated
> * @len: - Size of data to be encapsulated
> + * @keycolor - Determines if the source data is covered (black key) or
> + * plaintext.
> *
> * Returns zero on success,and negative on error.
> */
> -__weak int blob_encap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> +__weak int blob_encap(u8 *key_mod, u8 *src, u8 *dst, u32 len, u8 keycolor)
Ditto.
> {
> return 0;
> }
> @@ -91,9 +95,9 @@ static int do_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> #endif
>
> if (enc)
> - ret = blob_encap(km_ptr, src_ptr, dst_ptr, len);
> + ret = blob_encap(km_ptr, src_ptr, dst_ptr, len, 0);
Key type deserves either its own type, or at least a define. 0 - is just
a "magic number".
This is already done in drivers/crypto/fsl/jobdesc.h below, so shall be
reflected here.
> else
> - ret = blob_decap(km_ptr, src_ptr, dst_ptr, len);
> + ret = blob_decap(km_ptr, src_ptr, dst_ptr, len, 0);
Same as above.
In addition, this implementation suggests that there is no
support to use wrapped keys with this command as there is
no additional parameter to it indicating type, no logic to
check if the key is wrapped, and default value passed
suggests that only plain keys can be used (0 implies it).
Does it make sense that you extend the command here as well
to let the user pass the type?
>
> return ret;
> }
> diff --git a/drivers/crypto/fsl/desc.h b/drivers/crypto/fsl/desc.h
> index 5705c4f944..4c148a2fc4 100644
> --- a/drivers/crypto/fsl/desc.h
> +++ b/drivers/crypto/fsl/desc.h
> @@ -435,6 +435,7 @@
> /* Assuming OP_TYPE = OP_TYPE_UNI_PROTOCOL */
> #define OP_PCLID_SECMEM 0x08
> #define OP_PCLID_BLOB (0x0d << OP_PCLID_SHIFT)
> +#define OP_PCL_BLOB_BLACK 0x0004
BIT() macro?
> #define OP_PCLID_SECRETKEY (0x11 << OP_PCLID_SHIFT)
> #define OP_PCLID_PUBLICKEYPAIR (0x14 << OP_PCLID_SHIFT)
> #define OP_PCLID_DSA_SIGN (0x15 << OP_PCLID_SHIFT)
> diff --git a/drivers/crypto/fsl/fsl_blob.c b/drivers/crypto/fsl/fsl_blob.c
> index 9b6e4bca06..034e6ae5df 100644
> --- a/drivers/crypto/fsl/fsl_blob.c
> +++ b/drivers/crypto/fsl/fsl_blob.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> * Copyright 2014 Freescale Semiconductor, Inc.
> + * Copyright 2022 NXP
> *
> */
>
> @@ -22,13 +23,15 @@
> * @src: - Source address (blob)
> * @dst: - Destination address (data)
> * @len: - Size of decapsulated data
> + * @keycolor - Determines if the source data is covered (black key) or
> + * plaintext.
> *
> * Note: Start and end of the key_mod, src and dst buffers have to be aligned to
> * the cache line size (ARCH_DMA_MINALIGN) for the CAAM operation to succeed.
> *
> * Returns zero on success, negative on error.
> */
> -int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> +int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len, u8 keycolor)
> {
> int ret, size, i = 0;
> u32 *desc;
> @@ -55,7 +58,7 @@ int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> flush_dcache_range((unsigned long)src,
> (unsigned long)src + size);
>
> - inline_cnstr_jobdesc_blob_decap(desc, key_mod, src, dst, len);
> + inline_cnstr_jobdesc_blob_decap(desc, key_mod, src, dst, len, keycolor);
>
> debug("Descriptor dump:\n");
> for (i = 0; i < 14; i++)
> @@ -65,8 +68,8 @@ int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> flush_dcache_range((unsigned long)desc,
> (unsigned long)desc + size);
>
> - flush_dcache_range((unsigned long)dst,
> - (unsigned long)dst + size);
> + size = ALIGN(len, ARCH_DMA_MINALIGN);
> + invalidate_dcache_range((unsigned long)dst, (unsigned long)dst + size);
>
> ret = run_descriptor_jr(desc);
>
> @@ -94,13 +97,15 @@ int blob_decap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> * @src: - Source address (data)
> * @dst: - Destination address (blob)
> * @len: - Size of data to be encapsulated
> + * @keycolor - Determines if the source data is covered (black key) or
> + * plaintext.
> *
> * Note: Start and end of the key_mod, src and dst buffers have to be aligned to
> * the cache line size (ARCH_DMA_MINALIGN) for the CAAM operation to succeed.
> *
> * Returns zero on success, negative on error.
> */
> -int blob_encap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> +int blob_encap(u8 *key_mod, u8 *src, u8 *dst, u32 len, u8 keycolor)
> {
> int ret, size, i = 0;
> u32 *desc;
> @@ -127,7 +132,7 @@ int blob_encap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> flush_dcache_range((unsigned long)src,
> (unsigned long)src + size);
>
> - inline_cnstr_jobdesc_blob_encap(desc, key_mod, src, dst, len);
> + inline_cnstr_jobdesc_blob_encap(desc, key_mod, src, dst, len, keycolor);
>
> debug("Descriptor dump:\n");
> for (i = 0; i < 14; i++)
> @@ -137,8 +142,8 @@ int blob_encap(u8 *key_mod, u8 *src, u8 *dst, u32 len)
> flush_dcache_range((unsigned long)desc,
> (unsigned long)desc + size);
>
> - flush_dcache_range((unsigned long)dst,
> - (unsigned long)dst + size);
> + size = ALIGN(BLOB_SIZE(len), ARCH_DMA_MINALIGN);
> + invalidate_dcache_range((unsigned long)dst, (unsigned long)dst + size);
>
> ret = run_descriptor_jr(desc);
>
> diff --git a/drivers/crypto/fsl/jobdesc.c b/drivers/crypto/fsl/jobdesc.c
> index 542b1652d8..1280e6122e 100644
> --- a/drivers/crypto/fsl/jobdesc.c
> +++ b/drivers/crypto/fsl/jobdesc.c
> @@ -4,7 +4,7 @@
> * Basic job descriptor construction
> *
> * Copyright 2014 Freescale Semiconductor, Inc.
> - * Copyright 2018 NXP
> + * Copyright 2018, 2022 NXP
Do you have a new standard to indicate Copyright? AFAIK, the year
indicated at the first submission is enough to claim copyright, so
this change is not necessary here. Besides, LF recommends [1] that
year is not used in the notice for simplicity reasons.
> *
> */
>
> @@ -210,13 +210,14 @@ void inline_cnstr_jobdesc_hash(uint32_t *desc,
> #ifndef CONFIG_SPL_BUILD
> void inline_cnstr_jobdesc_blob_encap(uint32_t *desc, uint8_t *key_idnfr,
> uint8_t *plain_txt, uint8_t *enc_blob,
> - uint32_t in_sz)
> + uint32_t in_sz, uint8_t keycolor)
> {
> caam_dma_addr_t dma_addr_key_idnfr, dma_addr_in, dma_addr_out;
> uint32_t key_sz = KEY_IDNFR_SZ_BYTES;
> /* output blob will have 32 bytes key blob in beginning and
> * 16 byte HMAC identifier at end of data blob */
> uint32_t out_sz = in_sz + KEY_BLOB_SIZE + MAC_SIZE;
> + uint32_t bk_store;
>
> dma_addr_key_idnfr = virt_to_phys((void *)key_idnfr);
> dma_addr_in = virt_to_phys((void *)plain_txt);
> @@ -230,16 +231,23 @@ void inline_cnstr_jobdesc_blob_encap(uint32_t *desc,
> uint8_t *key_idnfr,
>
> append_seq_out_ptr(desc, dma_addr_out, out_sz, 0);
>
> - append_operation(desc, OP_TYPE_ENCAP_PROTOCOL | OP_PCLID_BLOB);
> + bk_store = OP_PCLID_BLOB;
> +
> + /* An input black key cannot be stored in a red blob */
> + if (keycolor == BLACK_KEY)
> + bk_store |= OP_PCL_BLOB_BLACK;
> +
> + append_operation(desc, OP_TYPE_ENCAP_PROTOCOL | bk_store);
> }
>
> void inline_cnstr_jobdesc_blob_decap(uint32_t *desc, uint8_t *key_idnfr,
> uint8_t *enc_blob, uint8_t *plain_txt,
> - uint32_t out_sz)
> + uint32_t out_sz, uint8_t keycolor)
> {
> caam_dma_addr_t dma_addr_key_idnfr, dma_addr_in, dma_addr_out;
> uint32_t key_sz = KEY_IDNFR_SZ_BYTES;
> uint32_t in_sz = out_sz + KEY_BLOB_SIZE + MAC_SIZE;
> + uint32_t bk_store;
>
> dma_addr_key_idnfr = virt_to_phys((void *)key_idnfr);
> dma_addr_in = virt_to_phys((void *)enc_blob);
> @@ -253,7 +261,13 @@ void inline_cnstr_jobdesc_blob_decap(uint32_t *desc, uint8_t
> *key_idnfr,
>
> append_seq_out_ptr(desc, dma_addr_out, out_sz, 0);
>
> - append_operation(desc, OP_TYPE_DECAP_PROTOCOL | OP_PCLID_BLOB);
> + bk_store = OP_PCLID_BLOB;
> +
> + /* An input black key cannot be stored in a red blob */
> + if (keycolor == BLACK_KEY)
> + bk_store |= OP_PCL_BLOB_BLACK;
> +
> + append_operation(desc, OP_TYPE_DECAP_PROTOCOL | bk_store);
> }
> #endif
> /*
> diff --git a/drivers/crypto/fsl/jobdesc.h b/drivers/crypto/fsl/jobdesc.h
> index c4501abd26..99ac049c3e 100644
> --- a/drivers/crypto/fsl/jobdesc.h
> +++ b/drivers/crypto/fsl/jobdesc.h
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0+ */
> /*
> * Copyright 2014 Freescale Semiconductor, Inc.
> + * Copyright 2022 NXP
> *
> */
>
> @@ -13,6 +14,9 @@
>
> #define KEY_IDNFR_SZ_BYTES 16
>
> +/* Encrypted key */
> +#define BLACK_KEY 1
How about "red" key? Does it need its own define here?
> +
> #ifdef CONFIG_CMD_DEKBLOB
> /* inline_cnstr_jobdesc_blob_dek:
> * Intializes and constructs the job descriptor for DEK encapsulation
> @@ -33,11 +37,11 @@ void inline_cnstr_jobdesc_hash(uint32_t *desc,
>
> void inline_cnstr_jobdesc_blob_encap(uint32_t *desc, uint8_t *key_idnfr,
> uint8_t *plain_txt, uint8_t *enc_blob,
> - uint32_t in_sz);
> + uint32_t in_sz, uint8_t keycolor);
>
> void inline_cnstr_jobdesc_blob_decap(uint32_t *desc, uint8_t *key_idnfr,
> uint8_t *enc_blob, uint8_t *plain_txt,
> - uint32_t out_sz);
> + uint32_t out_sz, uint8_t keycolor);
>
> void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk);
>
> --
> 2.25.1
-- andrey
Link: [1]: https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects
More information about the U-Boot
mailing list