RE: [PATCH v2] crypto/fsl: Add support for black key blob

Hello Gaurav,

-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Gaurav Jain Sent: Wednesday, September 28, 2022 12:40 PM To: u-boot@lists.denx.de; Stefano Babic sbabic@denx.de Cc: Fabio Estevam festevam@gmail.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com; NXP i . MX U-Boot Team uboot-imx@nxp.com; Horia Geanta horia.geanta@nxp.com; Varun Sethi V.Sethi@nxp.com; Gaurav Jain gaurav.jain@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@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-s...