Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source

[Jarkko Sakkinen]

On Thu Mar 28, 2024 at 10:05 AM EET, David Gstir wrote:
> Jarkko,
>
> > On 27.03.2024, at 16:40, Jarkko Sakkinen  wrote:
> > 
> > On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
> >> Update the documentation for trusted and encrypted KEYS with DCP as new
> >> trust source:
> >> 
> >> - Describe security properties of DCP trust source
> >> - Describe key usage
> >> - Document blob format
> >> 
> >> Co-developed-by: Richard Weinberger 
> >> Signed-off-by: Richard Weinberger 
> >> Co-developed-by: David Oberhollenzer 
> >> Signed-off-by: David Oberhollenzer 
> >> Signed-off-by: David Gstir 
> >> ---
> >> .../security/keys/trusted-encrypted.rst   | 85 +++
> >> 1 file changed, 85 insertions(+)
> >> 
> >> diff --git a/Documentation/security/keys/trusted-encrypted.rst 
> >> b/Documentation/security/keys/trusted-encrypted.rst
> >> index e989b9802f92..81fb3540bb20 100644
> >> --- a/Documentation/security/keys/trusted-encrypted.rst
> >> +++ b/Documentation/security/keys/trusted-encrypted.rst
> >> @@ -42,6 +42,14 @@ safe.
> >>  randomly generated and fused into each SoC at manufacturing time.
> >>  Otherwise, a common fixed test key is used instead.
> >> 
> >> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> >> +
> >> + Rooted to a one-time programmable key (OTP) that is generally 
> >> burnt
> >> + in the on-chip fuses and is accessible to the DCP encryption 
> >> engine only.
> >> + DCP provides two keys that can be used as root of trust: the OTP 
> >> key
> >> + and the UNIQUE key. Default is to use the UNIQUE key, but 
> >> selecting
> >> + the OTP key can be done via a module parameter (dcp_use_otp_key).
> >> +
> >>   *  Execution isolation
> >> 
> >>  (1) TPM
> >> @@ -57,6 +65,12 @@ safe.
> >> 
> >>  Fixed set of operations running in isolated execution environment.
> >> 
> >> + (4) DCP
> >> +
> >> + Fixed set of cryptographic operations running in isolated 
> >> execution
> >> + environment. Only basic blob key encryption is executed there.
> >> + The actual key sealing/unsealing is done on main 
> >> processor/kernel space.
> >> +
> >>   * Optional binding to platform integrity state
> >> 
> >>  (1) TPM
> >> @@ -79,6 +93,11 @@ safe.
> >>  Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs
> >>  for platform integrity.
> >> 
> >> + (4) DCP
> >> +
> >> + Relies on Secure/Trusted boot process (called HAB by vendor) for
> >> + platform integrity.
> >> +
> >>   *  Interfaces and APIs
> >> 
> >>  (1) TPM
> >> @@ -94,6 +113,11 @@ safe.
> >> 
> >>  Interface is specific to silicon vendor.
> >> 
> >> + (4) DCP
> >> +
> >> + Vendor-specific API that is implemented as part of the DCP 
> >> crypto driver in
> >> + ``drivers/crypto/mxs-dcp.c``.
> >> +
> >>   *  Threat model
> >> 
> >>  The strength and appropriateness of a particular trust source for a 
> >> given
> >> @@ -129,6 +153,13 @@ selected trust source:
> >>  CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device
> >>  is probed.
> >> 
> >> +  *  DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> >> +
> >> + The DCP hardware device itself does not provide a dedicated RNG 
> >> interface,
> >> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL 
> >> do have
> >> + a dedicated hardware RNG that is independent from DCP which can be 
> >> enabled
> >> + to back the kernel RNG.
> >> +
> >> Users may override this by specifying ``trusted.rng=kernel`` on the kernel
> >> command-line to override the used RNG with the kernel's random number pool.
> >> 
> >> @@ -231,6 +262,19 @@ Usage::
> >> CAAM-specific format.  The key length for new keys is always in bytes.
> >> Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> >> 
> >> +Trusted Keys usage: DCP
> >> +---
> >> +
> >> +Usage::
> >> +
> >> +keyctl add trusted name "new keylen" ring
> >> +keyctl add trusted name "load hex_blob" ring
> >> +keyctl print keyid
> >> +
> >> +"keyctl print" returns an ASCII hex copy of the sealed key, which is in 
> >> format
> >> +specific to this DCP key-blob implementation.  The key length for new 
> >> keys is
> >> +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> >> +
> >> Encrypted Keys usage
> >> 
> >> 
> >> @@ -426,3 +470,44 @@ string length.
> >> privkey is the binary representation of TPM2B_PUBLIC excluding the
> >> initial TPM2B header which can be reconstructed from the ASN.1 octed
> >> string length.
> >> +
> >> +DCP Blob Format
> >> +---
> >> +
> >> +The Data Co-Processor (DCP) provides hardware-bound AES keys using its
> >> +AES encryption engine only. It does not provide direct key 
> >> sealing/unsealing.
> >> +To make DCP hardware encryption keys usable as trust source, we define

Re: [PATCH 1/2] dt-bindings: crypto: ice: Document sc7280 inline crypto engine

[Herbert Xu]

On Wed, Mar 13, 2024 at 01:53:14PM +0100, Luca Weiss wrote:
> Document the compatible used for the inline crypto engine found on
> SC7280.
> 
> Signed-off-by: Luca Weiss 
> ---
>  Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml | 1 +
>  1 file changed, 1 insertion(+)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH][next] crypto/nx: Avoid -Wflex-array-member-not-at-end warning

[Herbert Xu]

On Tue, Mar 05, 2024 at 01:57:56PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally. So, we are deprecating flexible-array
> members in the middle of another structure.
> 
> There is currently an object (`header`) in `struct nx842_crypto_ctx`
> that contains a flexible structure (`struct nx842_crypto_header`):
> 
> struct nx842_crypto_ctx {
>   ...
> struct nx842_crypto_header header;
> struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
>   ...
> };
> 
> So, in order to avoid ending up with a flexible-array member in the
> middle of another struct, we use the `struct_group_tagged()` helper to
> separate the flexible array from the rest of the members in the flexible
> structure:
> 
> struct nx842_crypto_header {
>   struct_group_tagged(nx842_crypto_header_hdr, hdr,
> 
>   ... the rest of the members
> 
>   );
> struct nx842_crypto_header_group group[];
> } __packed;
> 
> With the change described above, we can now declare an object of the
> type of the tagged struct, without embedding the flexible array in the
> middle of another struct:
> 
> struct nx842_crypto_ctx {
>   ...
> struct nx842_crypto_header_hdr header;
> struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
>   ...
>  } __packed;
> 
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure, through which we can access the flexible
> array if needed.
> 
> So, with these changes, fix the following warning:
> 
> In file included from drivers/crypto/nx/nx-842.c:55:
> drivers/crypto/nx/nx-842.h:174:36: warning: structure containing a flexible 
> array member is not at the end of another structure 
> [-Wflex-array-member-not-at-end]
>   174 | struct nx842_crypto_header header;
>   |^~
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/crypto/nx/nx-842.c |  6 --
>  drivers/crypto/nx/nx-842.h | 10 ++
>  2 files changed, 10 insertions(+), 6 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source

[David Gstir]

Jarkko,

> On 27.03.2024, at 16:40, Jarkko Sakkinen  wrote:
> 
> On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
>> Update the documentation for trusted and encrypted KEYS with DCP as new
>> trust source:
>> 
>> - Describe security properties of DCP trust source
>> - Describe key usage
>> - Document blob format
>> 
>> Co-developed-by: Richard Weinberger 
>> Signed-off-by: Richard Weinberger 
>> Co-developed-by: David Oberhollenzer 
>> Signed-off-by: David Oberhollenzer 
>> Signed-off-by: David Gstir 
>> ---
>> .../security/keys/trusted-encrypted.rst   | 85 +++
>> 1 file changed, 85 insertions(+)
>> 
>> diff --git a/Documentation/security/keys/trusted-encrypted.rst 
>> b/Documentation/security/keys/trusted-encrypted.rst
>> index e989b9802f92..81fb3540bb20 100644
>> --- a/Documentation/security/keys/trusted-encrypted.rst
>> +++ b/Documentation/security/keys/trusted-encrypted.rst
>> @@ -42,6 +42,14 @@ safe.
>>  randomly generated and fused into each SoC at manufacturing time.
>>  Otherwise, a common fixed test key is used instead.
>> 
>> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
>> +
>> + Rooted to a one-time programmable key (OTP) that is generally burnt
>> + in the on-chip fuses and is accessible to the DCP encryption 
>> engine only.
>> + DCP provides two keys that can be used as root of trust: the OTP 
>> key
>> + and the UNIQUE key. Default is to use the UNIQUE key, but selecting
>> + the OTP key can be done via a module parameter (dcp_use_otp_key).
>> +
>>   *  Execution isolation
>> 
>>  (1) TPM
>> @@ -57,6 +65,12 @@ safe.
>> 
>>  Fixed set of operations running in isolated execution environment.
>> 
>> + (4) DCP
>> +
>> + Fixed set of cryptographic operations running in isolated execution
>> + environment. Only basic blob key encryption is executed there.
>> + The actual key sealing/unsealing is done on main processor/kernel 
>> space.
>> +
>>   * Optional binding to platform integrity state
>> 
>>  (1) TPM
>> @@ -79,6 +93,11 @@ safe.
>>  Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs
>>  for platform integrity.
>> 
>> + (4) DCP
>> +
>> + Relies on Secure/Trusted boot process (called HAB by vendor) for
>> + platform integrity.
>> +
>>   *  Interfaces and APIs
>> 
>>  (1) TPM
>> @@ -94,6 +113,11 @@ safe.
>> 
>>  Interface is specific to silicon vendor.
>> 
>> + (4) DCP
>> +
>> + Vendor-specific API that is implemented as part of the DCP crypto 
>> driver in
>> + ``drivers/crypto/mxs-dcp.c``.
>> +
>>   *  Threat model
>> 
>>  The strength and appropriateness of a particular trust source for a 
>> given
>> @@ -129,6 +153,13 @@ selected trust source:
>>  CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device
>>  is probed.
>> 
>> +  *  DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
>> +
>> + The DCP hardware device itself does not provide a dedicated RNG 
>> interface,
>> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL do 
>> have
>> + a dedicated hardware RNG that is independent from DCP which can be 
>> enabled
>> + to back the kernel RNG.
>> +
>> Users may override this by specifying ``trusted.rng=kernel`` on the kernel
>> command-line to override the used RNG with the kernel's random number pool.
>> 
>> @@ -231,6 +262,19 @@ Usage::
>> CAAM-specific format.  The key length for new keys is always in bytes.
>> Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
>> 
>> +Trusted Keys usage: DCP
>> +---
>> +
>> +Usage::
>> +
>> +keyctl add trusted name "new keylen" ring
>> +keyctl add trusted name "load hex_blob" ring
>> +keyctl print keyid
>> +
>> +"keyctl print" returns an ASCII hex copy of the sealed key, which is in 
>> format
>> +specific to this DCP key-blob implementation.  The key length for new keys 
>> is
>> +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
>> +
>> Encrypted Keys usage
>> 
>> 
>> @@ -426,3 +470,44 @@ string length.
>> privkey is the binary representation of TPM2B_PUBLIC excluding the
>> initial TPM2B header which can be reconstructed from the ASN.1 octed
>> string length.
>> +
>> +DCP Blob Format
>> +---
>> +
>> +The Data Co-Processor (DCP) provides hardware-bound AES keys using its
>> +AES encryption engine only. It does not provide direct key 
>> sealing/unsealing.
>> +To make DCP hardware encryption keys usable as trust source, we define
>> +our own custom format that uses a hardware-bound key to secure the sealing
>> +key stored in the key blob.
>> +
>> +Whenever a new trusted key using DCP is generated, we generate a random 
>> 128-bit
>> +blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to
>> +encrypt the trusted key payload using AES-128-GCM.
>> 

Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source

[Jarkko Sakkinen]

On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
> Update the documentation for trusted and encrypted KEYS with DCP as new
> trust source:
>
> - Describe security properties of DCP trust source
> - Describe key usage
> - Document blob format
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> ---
>  .../security/keys/trusted-encrypted.rst   | 85 +++
>  1 file changed, 85 insertions(+)
>
> diff --git a/Documentation/security/keys/trusted-encrypted.rst 
> b/Documentation/security/keys/trusted-encrypted.rst
> index e989b9802f92..81fb3540bb20 100644
> --- a/Documentation/security/keys/trusted-encrypted.rst
> +++ b/Documentation/security/keys/trusted-encrypted.rst
> @@ -42,6 +42,14 @@ safe.
>   randomly generated and fused into each SoC at manufacturing time.
>   Otherwise, a common fixed test key is used instead.
>  
> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> +
> + Rooted to a one-time programmable key (OTP) that is generally burnt
> + in the on-chip fuses and is accessible to the DCP encryption engine 
> only.
> + DCP provides two keys that can be used as root of trust: the OTP key
> + and the UNIQUE key. Default is to use the UNIQUE key, but selecting
> + the OTP key can be done via a module parameter (dcp_use_otp_key).
> +
>*  Execution isolation
>  
>   (1) TPM
> @@ -57,6 +65,12 @@ safe.
>  
>   Fixed set of operations running in isolated execution environment.
>  
> + (4) DCP
> +
> + Fixed set of cryptographic operations running in isolated execution
> + environment. Only basic blob key encryption is executed there.
> + The actual key sealing/unsealing is done on main processor/kernel 
> space.
> +
>* Optional binding to platform integrity state
>  
>   (1) TPM
> @@ -79,6 +93,11 @@ safe.
>   Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs
>   for platform integrity.
>  
> + (4) DCP
> +
> + Relies on Secure/Trusted boot process (called HAB by vendor) for
> + platform integrity.
> +
>*  Interfaces and APIs
>  
>   (1) TPM
> @@ -94,6 +113,11 @@ safe.
>  
>   Interface is specific to silicon vendor.
>  
> + (4) DCP
> +
> + Vendor-specific API that is implemented as part of the DCP crypto 
> driver in
> + ``drivers/crypto/mxs-dcp.c``.
> +
>*  Threat model
>  
>   The strength and appropriateness of a particular trust source for a 
> given
> @@ -129,6 +153,13 @@ selected trust source:
>   CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device
>   is probed.
>  
> +  *  DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs)
> +
> + The DCP hardware device itself does not provide a dedicated RNG 
> interface,
> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL do 
> have
> + a dedicated hardware RNG that is independent from DCP which can be 
> enabled
> + to back the kernel RNG.
> +
>  Users may override this by specifying ``trusted.rng=kernel`` on the kernel
>  command-line to override the used RNG with the kernel's random number pool.
>  
> @@ -231,6 +262,19 @@ Usage::
>  CAAM-specific format.  The key length for new keys is always in bytes.
>  Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
>  
> +Trusted Keys usage: DCP
> +---
> +
> +Usage::
> +
> +keyctl add trusted name "new keylen" ring
> +keyctl add trusted name "load hex_blob" ring
> +keyctl print keyid
> +
> +"keyctl print" returns an ASCII hex copy of the sealed key, which is in 
> format
> +specific to this DCP key-blob implementation.  The key length for new keys is
> +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
> +
>  Encrypted Keys usage
>  
>  
> @@ -426,3 +470,44 @@ string length.
>  privkey is the binary representation of TPM2B_PUBLIC excluding the
>  initial TPM2B header which can be reconstructed from the ASN.1 octed
>  string length.
> +
> +DCP Blob Format
> +---
> +
> +The Data Co-Processor (DCP) provides hardware-bound AES keys using its
> +AES encryption engine only. It does not provide direct key sealing/unsealing.
> +To make DCP hardware encryption keys usable as trust source, we define
> +our own custom format that uses a hardware-bound key to secure the sealing
> +key stored in the key blob.
> +
> +Whenever a new trusted key using DCP is generated, we generate a random 
> 128-bit
> +blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to
> +encrypt the trusted key payload using AES-128-GCM.
> +
> +The BEK itself is encrypted using the hardware-bound key using the DCP's AES
> +encryption engine with AES-128-ECB. The encrypted BEK, generated nonce,
> +BEK-encrypted

Re: [PATCH v7 5/6] docs: document DCP-backed trusted keys kernel params

[Jarkko Sakkinen]

On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
> Document the kernel parameters trusted.dcp_use_otp_key
> and trusted.dcp_skip_zk_test for DCP-backed trusted keys.
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index 24c02c704049..b6944e57768a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6698,6 +6698,7 @@
>   - "tpm"
>   - "tee"
>   - "caam"
> + - "dcp"
>   If not specified then it defaults to iterating through
>   the trust source list starting with TPM and assigns the
>   first trust source as a backend which is initialized
> @@ -6713,6 +6714,18 @@
>   If not specified, "default" is used. In this case,
>   the RNG's choice is left to each individual trust 
> source.
>  
> + trusted.dcp_use_otp_key
> + This is intended to be used in combination with
> + trusted.source=dcp and will select the DCP OTP key
> + instead of the DCP UNIQUE key blob encryption.
> +
> + trusted.dcp_skip_zk_test
> + This is intended to be used in combination with
> + trusted.source=dcp and will disable the check if all
> + the blob key is zero'ed. This is helpful for situations 
> where
> + having this key zero'ed is acceptable. E.g. in testing
> + scenarios.
> +
>   tsc=Disable clocksource stability checks for TSC.
>   Format: 
>   [x86] reliable: mark tsc clocksource as reliable, this

Nicely documented, i.e. even I can understand what is said here :-)

Reviewed-by: Jarkko Sakkinen 

BR, Jarkko

Re: [PATCH v7 2/6] KEYS: trusted: improve scalability of trust source config

[Jarkko Sakkinen]

On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote:
> Enabling trusted keys requires at least one trust source implementation
> (currently TPM, TEE or CAAM) to be enabled. Currently, this is
> done by checking each trust source's config option individually.
> This does not scale when more trust sources like the one for DCP
> are added, because the condition will get long and hard to read.
>
> Add config HAVE_TRUSTED_KEYS which is set to true by each trust source
> once its enabled and adapt the check for having at least one active trust
> source to use this option. Whenever a new trust source is added, it now
> needs to select HAVE_TRUSTED_KEYS.
>
> Signed-off-by: David Gstir 
> ---
>  security/keys/trusted-keys/Kconfig | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/trusted-keys/Kconfig 
> b/security/keys/trusted-keys/Kconfig
> index dbfdd8536468..553dc117f385 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -1,3 +1,6 @@
> +config HAVE_TRUSTED_KEYS
> + bool
> +
>  config TRUSTED_KEYS_TPM
>   bool "TPM-based trusted keys"
>   depends on TCG_TPM >= TRUSTED_KEYS
> @@ -9,6 +12,7 @@ config TRUSTED_KEYS_TPM
>   select ASN1_ENCODER
>   select OID_REGISTRY
>   select ASN1
> + select HAVE_TRUSTED_KEYS
>   help
> Enable use of the Trusted Platform Module (TPM) as trusted key
> backend. Trusted keys are random number symmetric keys,
> @@ -20,6 +24,7 @@ config TRUSTED_KEYS_TEE
>   bool "TEE-based trusted keys"
>   depends on TEE >= TRUSTED_KEYS
>   default y
> + select HAVE_TRUSTED_KEYS
>   help
> Enable use of the Trusted Execution Environment (TEE) as trusted
> key backend.
> @@ -29,10 +34,11 @@ config TRUSTED_KEYS_CAAM
>   depends on CRYPTO_DEV_FSL_CAAM_JR >= TRUSTED_KEYS
>   select CRYPTO_DEV_FSL_CAAM_BLOB_GEN
>   default y
> + select HAVE_TRUSTED_KEYS
>   help
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>  
> -if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM
> -comment "No trust source selected!"
> +if !HAVE_TRUSTED_KEYS
> + comment "No trust source selected!"
>  endif

Tested-by: Jarkko Sakkinen  # for TRUSTED_KEYS_TPM
Reviewed-by: Jarkko Sakkinen 

BR, Jarkko

Re: [PATCH v2] arm64: dts: qcom: sm6350: Add Crypto Engine

[Bjorn Andersson]

On Mon, 19 Feb 2024 11:16:02 +0100, Luca Weiss wrote:
> Add crypto engine (CE) and CE BAM related nodes and definitions for this
> SoC.
> 
> For reference:
> 
>   [2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 5.5.1
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: qcom: sm6350: Add Crypto Engine
  commit: fd5afa5d7e5259cb2320fbe2cf60250f7336f439

Best regards,
-- 
Bjorn Andersson 

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Karel Balej]

#regzbot title: SHA1 support removal breaks iwd's ability to connect to eduroam
#regzbot monitor: 
https://lore.kernel.org/all/20240313233227.56391-1-ebigg...@kernel.org/
#regzbot monitor: 
https://lore.kernel.org/all/czshruij4rkl.34t4easv5d...@matfyz.cz/
#regzbot link: 
https://lore.kernel.org/iwd/njvxKaPo_CBxsQGaNSRHj8xOSxzk1_j_K-minIe4GCKUMB1qxJT8nPk9SGmfqg7Aepm_5dO7FEofYIYP1g15R9V5dJ0F8bN6O4VthSjzu1g=@yartys.no/

Sorry for the tracking mess...

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Ard Biesheuvel]

On Thu, 14 Mar 2024 at 21:20, Eric Biggers  wrote:
>
> On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote:
> > IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs.
> > Anything that wifi requires as far as crypto goes IWD uses the kernel,
> > except ECC is the only exception. The entire list of crypto requirements
> > (for full support at least) for IWD is here:
> >
> > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config
>
> That's quite an extensive list, and it's not documented in the iwd README.
> Don't you get bug reports from users who are running a kernel that's missing 
> one
> of those options?
>
> > For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto
> > operations, (query), encrypt, decrypt, sign, verify.
> >
> > I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time
> > I started working on IWD so I was not aware the documentation was so poor.
> > That is an entirely separate issue than this IMO, and I'm happy to help with
> > getting docs updated to include a proper list of supported features. In
> > addition maybe some automated testing that gets run on kernel builds which
> > actually exercises this API so it doesn't get accidentally get broken in the
> > future? Docs/tests IMO are the proper "fix" here, not telling someone to
> > stop using an API that has existed a long time.
>
> I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a
> collaboration between the iwd developers and the kernel keyrings maintainer.
> So, as far as I can tell, it's not that the kernel had an existing API that 
> iwd
> started using.  It's that iwd got some APIs added to the kernel for 
> themselves.
> KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search
> doesn't return any notable results.  keyctl does provide a command-line
> interface to them, but I can't find any users of the keyctl commands either.
>
> Then, everyone disappeared and it got dumped on the next generation of kernel
> developers, who often don't know that this API even exists.  And since the API
> is also poorly specified and difficult to maintain (e.g., changing a seemingly
> unrelated part of the kernel can break it), the results are predictable...  
> And
> of course the only thing that breaks is iwd, since it's the only user.
>
> It would be worth taking a step back and looking at the overall system
> architecture here.  Is this the best way to ensure a reliable wireless
> experience for Linux users?
>
> Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a
> different direction (e.g. using OpenSSL) should be taken...
>
> (Another issue with the kernel keyrings stuff is that provides a significant
> attack surface for the kernel to be exploited.)
>
> If you do decide to continue with the status quo, it may be necessary for the
> iwd developers to take a more active role in maintaining this API in order to
> ensure it continues working properly for you.
>
> AF_ALG is on *slightly* firmer ground since it's been around for longer, is
> properly part of the crypto subsystem, and has a few other users.  
> Unfortunately
> it still suffers from the same issues though, just to a slightly lesser 
> degree.
>

We dropped MD4 because there are no users in the kernel. It is not the
kernel's job to run code on behalf of user space if it does not
require any privileges and can therefore execute in user space
directly.

The fact that AF_ALG permits this is a huge oversight on the part of
the kernel community, and a major maintenance burden. The point of
AF_ALG was to expose hardware crypto accelerators (which are shared
resources that /need/ to be managed by the kernel) to user space, and
we inadvertently ended up allowing the kernel's pure-software
algorithms to be used in the same way.

The fact that we even added APIs to the kernel to accommodate iwd is
even worse. It means system call overhead (which has become worse due
to all the speculation mitigations) to execute some code that could
execute in user space just as well, which is a bad idea for other
reasons too (for instance, accelerated crypto that uses SIMD in the
kernel disables preemption on many architectures, resulting in
scheduling jitter)

Note that in the case of iwd, it is unlikely that the use of AF_ALG
could ever result in meaningful use of hardware accelerators: today's
wireless interfaces don't use software crypto for the bulk of the data
(i.e., the packets themselves) and the wireless key exchange protocols
etc are unlikely to be supported in generic crypto accelerators, and
even if they were, the latency would likely result in worse
performance overall than a software implementation.

So iwd's deliberate choice to use the kernel as a crypto library is
severely misguided. I have made the same point 4 years ago when I
replaced iwd's use of the kernel's ecb(arc4) code with a suitable
software implementati

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote:
> IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs.
> Anything that wifi requires as far as crypto goes IWD uses the kernel,
> except ECC is the only exception. The entire list of crypto requirements
> (for full support at least) for IWD is here:
> 
> https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config

That's quite an extensive list, and it's not documented in the iwd README.
Don't you get bug reports from users who are running a kernel that's missing one
of those options?

> For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto
> operations, (query), encrypt, decrypt, sign, verify.
> 
> I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time
> I started working on IWD so I was not aware the documentation was so poor.
> That is an entirely separate issue than this IMO, and I'm happy to help with
> getting docs updated to include a proper list of supported features. In
> addition maybe some automated testing that gets run on kernel builds which
> actually exercises this API so it doesn't get accidentally get broken in the
> future? Docs/tests IMO are the proper "fix" here, not telling someone to
> stop using an API that has existed a long time.

I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a
collaboration between the iwd developers and the kernel keyrings maintainer.
So, as far as I can tell, it's not that the kernel had an existing API that iwd
started using.  It's that iwd got some APIs added to the kernel for themselves.
KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search
doesn't return any notable results.  keyctl does provide a command-line
interface to them, but I can't find any users of the keyctl commands either.

Then, everyone disappeared and it got dumped on the next generation of kernel
developers, who often don't know that this API even exists.  And since the API
is also poorly specified and difficult to maintain (e.g., changing a seemingly
unrelated part of the kernel can break it), the results are predictable...  And
of course the only thing that breaks is iwd, since it's the only user.

It would be worth taking a step back and looking at the overall system
architecture here.  Is this the best way to ensure a reliable wireless
experience for Linux users?

Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a
different direction (e.g. using OpenSSL) should be taken...

(Another issue with the kernel keyrings stuff is that provides a significant
attack surface for the kernel to be exploited.)

If you do decide to continue with the status quo, it may be necessary for the
iwd developers to take a more active role in maintaining this API in order to
ensure it continues working properly for you.

AF_ALG is on *slightly* firmer ground since it's been around for longer, is
properly part of the crypto subsystem, and has a few other users.  Unfortunately
it still suffers from the same issues though, just to a slightly lesser degree.

> I'm also not entirely sure why this stuff continues to be removed from the
> kernel. First MD4, then it got reverted, then this (now reverted, thanks).
> Both cases there was not clear justification of why it was being removed.

These algorithms are insecure, and it's likely that the author of these commits
thought that there were no remaining users and nothing would break.  Removing
them is a worthy goal for code maintenance purposes and to avoid providing
insecure options that could accidentally be used.  The AF_ALG and KEYCTL_PKEY_*
APIs are very easy to overlook and I suspect that the author of these commits
did not know about them.  These APIs are rarely used, not well specified, the
availability of them and specific algorithms varies by kernel configuration, and
userspace only uses a subset of the algorithms in the kernel's museum of crypto
primitives anyway.  So it's plausible that there are algorithms that no one is
using or that at least there is a fallback for, so can be safely removed...

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Bottomley]

On Thu, 2024-03-14 at 04:52 -0700, James Prestwood wrote:
> I'm also not entirely sure why this stuff continues to be removed
> from the kernel. First MD4, then it got reverted, then this (now
> reverted, thanks). Both cases there was not clear justification of
> why it was  being removed.

I think this is some misunderstanding of the NIST and FIPS requirements
with regards to hashes, ciphers and bits of security.  The bottom line
is that neither NIST nor FIPS requires the removal of the sha1
algorithm at all.  Both of them still support it for HMAC (for now). 
In addition, the FIPS requirement is only that you not *issue* sha1
hashed signatures.  FIPS still allows you to verify legacy signatures
with sha1 as the signing hash (for backwards compatibility reasons). 
Enterprises with no legacy and no HMAC requirements *may* remove the
hash, but it's not mandated.

So *removing* sha1 from the certificate code was the wrong thing to do.
We should have configurably prevented using sha1 as the algorithm for
new signatures but kept it for signature verification.

Can we please get this sorted out before 2025, because next up is the
FIPS requirement to move to at least 128 bits of security which will
see RSA2048 deprecated in a similar way: We should refuse to issue
RSA2048 signatures, but will still be allowed to verify them for legacy
reasons.

James

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 4:06 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:

On 3/13/2024 3:10 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:

Hi,

On 3/13/24 1:22 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:

Hi,

On 3/13/24 12:44 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:

Hi,

On 3/13/24 1:56 AM, Johannes Berg wrote:

Not sure why you're CC'ing the world, but I guess adding a few more
doesn't hurt ...

On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:

 and I use iwd

This is your problem, the wireless stack in the kernel doesn't use any
kernel crypto code for 802.1X.

Yes, the wireless stack has zero bearing on the issue. I think that's what
you meant by "problem".

IWD has used the kernel crypto API forever which was abruptly broken, that
is the problem.

The original commit says it was to remove support for sha1 signed kernel
modules, but it did more than that and broke the keyctl API.


Which specific API is iwd using that is relevant here?
I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
and grepped for keyctl and AF_ALG, but there are no matches.

IWD uses ELL for its crypto, which uses the AF_ALG API:

https://git.kernel.org/pub/scm/libs/ell/ell.git/

Thanks for pointing out that the relevant code is really in that separate
repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
blamed commit didn't change anything for AF_ALG.


I believe the failure is when calling:

KEYCTL_PKEY_QUERY enc="x962" hash="sha1"

  From logs Michael posted on the IWD list, the ELL API that fails is:

l_key_get_info (ell.git/ell/key.c:416)

Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
weird set of APIs where userspace can ask the kernel to do asymmetric key
operations.  It's unclear why they exist, as the same functionality is available
in userspace crypto libraries.

I suppose that the blamed commit, or at least part of it, will need to be
reverted to keep these weird keyctls working.

For the future, why doesn't iwd just use a userspace crypto library such as
OpenSSL?

I was not around when the original decision was made, but a few reasons I
know we don't use openSSL:

  - IWD has virtually zero dependencies.

Depending on something in the kernel does not eliminate a dependency; it just
adds that particular kernel UAPI to your list of dependencies.  The reason that
we're having this discussion in the first place is because iwd is depending on
an obscure kernel UAPI that is not well defined.  Historically it's been hard to
avoid "breaking" changes in these crypto-related UAPIs because of the poor
design where a huge number of algorithms are potentially supported, but the list
is undocumented and it varies from one system to another based on configuration.
Also due to their obscurity many kernel developers don't know that these UAPIs
even exist.  (The reaction when someone finds out is usually "Why!?")

It may be worth looking at if iwd should make a different choice for this
dependency.  It's understandable to blame dependencies when things go wrong, but
at the same time the choice of dependency is very much a choice, and some
choices can be more technically sound and cause fewer problems than others...


  - OpenSSL + friends are rather large libraries.

The Linux kernel is also large, and it's made larger by having to support
obsolete crypto algorithms for backwards compatibility with iwd.


  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
too).

OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.


Another consideration is once you support openSSL someone wants wolfSSL,
then boringSSL etc. Even if users implement support it just becomes a huge
burden to carry for the project. Just look at wpa_supplicant's src/crypto/
folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
~5k. You have to sort out all the nitty gritty details of each library, and
provide a common driver/API for the core code, differences between openssl
versions, the list goes on.

What is the specific functionality that you're actually relying on that you
think would need 40K lines of code to replace, even using OpenSSL?  I see you
are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
operations are being performed, and with which algorithms and key formats?
Also, is the kernel behavior that you're relying on documented anywhere?  There
are man pages for those keyctls, but they don't say anything about any
particular hash algorithm, SHA-1 or otherwise, being supported.


"And we simply do not break user space."
-Linus Torvalds

Is this no longer applicable?


As I said, the commit, or at least the part of it that 

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 04:06:11PM -0700, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:
> > On 3/13/2024 3:10 PM, Eric Biggers wrote:
> > > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> > >> Hi,
> > >>
> > >> On 3/13/24 1:22 PM, Eric Biggers wrote:
> > >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> >  Hi,
> > 
> >  On 3/13/24 12:44 PM, Eric Biggers wrote:
> > > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> > >> Hi,
> > >>
> > >> On 3/13/24 1:56 AM, Johannes Berg wrote:
> > >>> Not sure why you're CC'ing the world, but I guess adding a few more
> > >>> doesn't hurt ...
> > >>>
> > >>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> >  and I use iwd
> > >>> This is your problem, the wireless stack in the kernel doesn't use 
> > >>> any
> > >>> kernel crypto code for 802.1X.
> > >> Yes, the wireless stack has zero bearing on the issue. I think 
> > >> that's what
> > >> you meant by "problem".
> > >>
> > >> IWD has used the kernel crypto API forever which was abruptly 
> > >> broken, that
> > >> is the problem.
> > >>
> > >> The original commit says it was to remove support for sha1 signed 
> > >> kernel
> > >> modules, but it did more than that and broke the keyctl API.
> > >>
> > > Which specific API is iwd using that is relevant here?
> > > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > > and grepped for keyctl and AF_ALG, but there are no matches.
> >  IWD uses ELL for its crypto, which uses the AF_ALG API:
> > 
> >  https://git.kernel.org/pub/scm/libs/ell/ell.git/
> > >>> Thanks for pointing out that the relevant code is really in that 
> > >>> separate
> > >>> repository.  Note, it seems that keyctl() is the problem here, not 
> > >>> AF_ALG.  The
> > >>> blamed commit didn't change anything for AF_ALG.
> > >>>
> >  I believe the failure is when calling:
> > 
> >  KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> > 
> >   From logs Michael posted on the IWD list, the ELL API that fails is:
> > 
> >  l_key_get_info (ell.git/ell/key.c:416)
> > >>> Okay, I guess that's what's actually causing the problem.  
> > >>> KEYCTL_PKEY_* are a
> > >>> weird set of APIs where userspace can ask the kernel to do asymmetric 
> > >>> key
> > >>> operations.  It's unclear why they exist, as the same functionality is 
> > >>> available
> > >>> in userspace crypto libraries.
> > >>>
> > >>> I suppose that the blamed commit, or at least part of it, will need to 
> > >>> be
> > >>> reverted to keep these weird keyctls working.
> > >>>
> > >>> For the future, why doesn't iwd just use a userspace crypto library 
> > >>> such as
> > >>> OpenSSL?
> > >>
> > >> I was not around when the original decision was made, but a few reasons I
> > >> know we don't use openSSL:
> > >>
> > >>  - IWD has virtually zero dependencies.
> > > 
> > > Depending on something in the kernel does not eliminate a dependency; it 
> > > just
> > > adds that particular kernel UAPI to your list of dependencies.  The 
> > > reason that
> > > we're having this discussion in the first place is because iwd is 
> > > depending on
> > > an obscure kernel UAPI that is not well defined.  Historically it's been 
> > > hard to
> > > avoid "breaking" changes in these crypto-related UAPIs because of the poor
> > > design where a huge number of algorithms are potentially supported, but 
> > > the list
> > > is undocumented and it varies from one system to another based on 
> > > configuration.
> > > Also due to their obscurity many kernel developers don't know that these 
> > > UAPIs
> > > even exist.  (The reaction when someone finds out is usually "Why!?")
> > > 
> > > It may be worth looking at if iwd should make a different choice for this
> > > dependency.  It's understandable to blame dependencies when things go 
> > > wrong, but
> > > at the same time the choice of dependency is very much a choice, and some
> > > choices can be more technically sound and cause fewer problems than 
> > > others...
> > > 
> > >>  - OpenSSL + friends are rather large libraries.
> > > 
> > > The Linux kernel is also large, and it's made larger by having to support
> > > obsolete crypto algorithms for backwards compatibility with iwd.
> > > 
> > >>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> > >> too).
> > > 
> > > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> > > 
> > >> Another consideration is once you support openSSL someone wants wolfSSL,
> > >> then boringSSL etc. Even if users implement support it just becomes a 
> > >> huge
> > >> burden to carry for the project. Just look at wpa_supplicant's 
> > >> src/crypto/
> > >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which 
> > >> is
> > >> ~5k. You have t

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:
> On 3/13/2024 3:10 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> >> Hi,
> >>
> >> On 3/13/24 1:22 PM, Eric Biggers wrote:
> >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
>  Hi,
> 
>  On 3/13/24 12:44 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> >> Hi,
> >>
> >> On 3/13/24 1:56 AM, Johannes Berg wrote:
> >>> Not sure why you're CC'ing the world, but I guess adding a few more
> >>> doesn't hurt ...
> >>>
> >>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>  and I use iwd
> >>> This is your problem, the wireless stack in the kernel doesn't use any
> >>> kernel crypto code for 802.1X.
> >> Yes, the wireless stack has zero bearing on the issue. I think that's 
> >> what
> >> you meant by "problem".
> >>
> >> IWD has used the kernel crypto API forever which was abruptly broken, 
> >> that
> >> is the problem.
> >>
> >> The original commit says it was to remove support for sha1 signed 
> >> kernel
> >> modules, but it did more than that and broke the keyctl API.
> >>
> > Which specific API is iwd using that is relevant here?
> > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > and grepped for keyctl and AF_ALG, but there are no matches.
>  IWD uses ELL for its crypto, which uses the AF_ALG API:
> 
>  https://git.kernel.org/pub/scm/libs/ell/ell.git/
> >>> Thanks for pointing out that the relevant code is really in that separate
> >>> repository.  Note, it seems that keyctl() is the problem here, not 
> >>> AF_ALG.  The
> >>> blamed commit didn't change anything for AF_ALG.
> >>>
>  I believe the failure is when calling:
> 
>  KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> 
>   From logs Michael posted on the IWD list, the ELL API that fails is:
> 
>  l_key_get_info (ell.git/ell/key.c:416)
> >>> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* 
> >>> are a
> >>> weird set of APIs where userspace can ask the kernel to do asymmetric key
> >>> operations.  It's unclear why they exist, as the same functionality is 
> >>> available
> >>> in userspace crypto libraries.
> >>>
> >>> I suppose that the blamed commit, or at least part of it, will need to be
> >>> reverted to keep these weird keyctls working.
> >>>
> >>> For the future, why doesn't iwd just use a userspace crypto library such 
> >>> as
> >>> OpenSSL?
> >>
> >> I was not around when the original decision was made, but a few reasons I
> >> know we don't use openSSL:
> >>
> >>  - IWD has virtually zero dependencies.
> > 
> > Depending on something in the kernel does not eliminate a dependency; it 
> > just
> > adds that particular kernel UAPI to your list of dependencies.  The reason 
> > that
> > we're having this discussion in the first place is because iwd is depending 
> > on
> > an obscure kernel UAPI that is not well defined.  Historically it's been 
> > hard to
> > avoid "breaking" changes in these crypto-related UAPIs because of the poor
> > design where a huge number of algorithms are potentially supported, but the 
> > list
> > is undocumented and it varies from one system to another based on 
> > configuration.
> > Also due to their obscurity many kernel developers don't know that these 
> > UAPIs
> > even exist.  (The reaction when someone finds out is usually "Why!?")
> > 
> > It may be worth looking at if iwd should make a different choice for this
> > dependency.  It's understandable to blame dependencies when things go 
> > wrong, but
> > at the same time the choice of dependency is very much a choice, and some
> > choices can be more technically sound and cause fewer problems than 
> > others...
> > 
> >>  - OpenSSL + friends are rather large libraries.
> > 
> > The Linux kernel is also large, and it's made larger by having to support
> > obsolete crypto algorithms for backwards compatibility with iwd.
> > 
> >>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> >> too).
> > 
> > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> > 
> >> Another consideration is once you support openSSL someone wants wolfSSL,
> >> then boringSSL etc. Even if users implement support it just becomes a huge
> >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
> >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
> >> ~5k. You have to sort out all the nitty gritty details of each library, and
> >> provide a common driver/API for the core code, differences between openssl
> >> versions, the list goes on.
> > 
> > What is the specific functionality that you're actually relying on that you
> > think would need 40K lines of code to replace, even using OpenSSL?  I see 
> > yo

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Jeff Johnson]

On 3/13/2024 3:10 PM, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
>> Hi,
>>
>> On 3/13/24 1:22 PM, Eric Biggers wrote:
>>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
 Hi,

 On 3/13/24 12:44 PM, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
>> Hi,
>>
>> On 3/13/24 1:56 AM, Johannes Berg wrote:
>>> Not sure why you're CC'ing the world, but I guess adding a few more
>>> doesn't hurt ...
>>>
>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
 and I use iwd
>>> This is your problem, the wireless stack in the kernel doesn't use any
>>> kernel crypto code for 802.1X.
>> Yes, the wireless stack has zero bearing on the issue. I think that's 
>> what
>> you meant by "problem".
>>
>> IWD has used the kernel crypto API forever which was abruptly broken, 
>> that
>> is the problem.
>>
>> The original commit says it was to remove support for sha1 signed kernel
>> modules, but it did more than that and broke the keyctl API.
>>
> Which specific API is iwd using that is relevant here?
> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> and grepped for keyctl and AF_ALG, but there are no matches.
 IWD uses ELL for its crypto, which uses the AF_ALG API:

 https://git.kernel.org/pub/scm/libs/ell/ell.git/
>>> Thanks for pointing out that the relevant code is really in that separate
>>> repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  
>>> The
>>> blamed commit didn't change anything for AF_ALG.
>>>
 I believe the failure is when calling:

 KEYCTL_PKEY_QUERY enc="x962" hash="sha1"

  From logs Michael posted on the IWD list, the ELL API that fails is:

 l_key_get_info (ell.git/ell/key.c:416)
>>> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* 
>>> are a
>>> weird set of APIs where userspace can ask the kernel to do asymmetric key
>>> operations.  It's unclear why they exist, as the same functionality is 
>>> available
>>> in userspace crypto libraries.
>>>
>>> I suppose that the blamed commit, or at least part of it, will need to be
>>> reverted to keep these weird keyctls working.
>>>
>>> For the future, why doesn't iwd just use a userspace crypto library such as
>>> OpenSSL?
>>
>> I was not around when the original decision was made, but a few reasons I
>> know we don't use openSSL:
>>
>>  - IWD has virtually zero dependencies.
> 
> Depending on something in the kernel does not eliminate a dependency; it just
> adds that particular kernel UAPI to your list of dependencies.  The reason 
> that
> we're having this discussion in the first place is because iwd is depending on
> an obscure kernel UAPI that is not well defined.  Historically it's been hard 
> to
> avoid "breaking" changes in these crypto-related UAPIs because of the poor
> design where a huge number of algorithms are potentially supported, but the 
> list
> is undocumented and it varies from one system to another based on 
> configuration.
> Also due to their obscurity many kernel developers don't know that these UAPIs
> even exist.  (The reaction when someone finds out is usually "Why!?")
> 
> It may be worth looking at if iwd should make a different choice for this
> dependency.  It's understandable to blame dependencies when things go wrong, 
> but
> at the same time the choice of dependency is very much a choice, and some
> choices can be more technically sound and cause fewer problems than others...
> 
>>  - OpenSSL + friends are rather large libraries.
> 
> The Linux kernel is also large, and it's made larger by having to support
> obsolete crypto algorithms for backwards compatibility with iwd.
> 
>>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
>> too).
> 
> OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> 
>> Another consideration is once you support openSSL someone wants wolfSSL,
>> then boringSSL etc. Even if users implement support it just becomes a huge
>> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
>> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
>> ~5k. You have to sort out all the nitty gritty details of each library, and
>> provide a common driver/API for the core code, differences between openssl
>> versions, the list goes on.
> 
> What is the specific functionality that you're actually relying on that you
> think would need 40K lines of code to replace, even using OpenSSL?  I see you
> are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
> operations are being performed, and with which algorithms and key formats?
> Also, is the kernel behavior that you're relying on documented anywhere?  
> There
> are man pages for those keyctls, but they don't say anything about any
> 

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> Hi,
> 
> On 3/13/24 1:22 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> > > Hi,
> > > 
> > > On 3/13/24 12:44 PM, Eric Biggers wrote:
> > > > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> > > > > Hi,
> > > > > 
> > > > > On 3/13/24 1:56 AM, Johannes Berg wrote:
> > > > > > Not sure why you're CC'ing the world, but I guess adding a few more
> > > > > > doesn't hurt ...
> > > > > > 
> > > > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > > > > > > and I use iwd
> > > > > > This is your problem, the wireless stack in the kernel doesn't use 
> > > > > > any
> > > > > > kernel crypto code for 802.1X.
> > > > > Yes, the wireless stack has zero bearing on the issue. I think that's 
> > > > > what
> > > > > you meant by "problem".
> > > > > 
> > > > > IWD has used the kernel crypto API forever which was abruptly broken, 
> > > > > that
> > > > > is the problem.
> > > > > 
> > > > > The original commit says it was to remove support for sha1 signed 
> > > > > kernel
> > > > > modules, but it did more than that and broke the keyctl API.
> > > > > 
> > > > Which specific API is iwd using that is relevant here?
> > > > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > > > and grepped for keyctl and AF_ALG, but there are no matches.
> > > IWD uses ELL for its crypto, which uses the AF_ALG API:
> > > 
> > > https://git.kernel.org/pub/scm/libs/ell/ell.git/
> > Thanks for pointing out that the relevant code is really in that separate
> > repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  
> > The
> > blamed commit didn't change anything for AF_ALG.
> > 
> > > I believe the failure is when calling:
> > > 
> > > KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> > > 
> > >  From logs Michael posted on the IWD list, the ELL API that fails is:
> > > 
> > > l_key_get_info (ell.git/ell/key.c:416)
> > Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* 
> > are a
> > weird set of APIs where userspace can ask the kernel to do asymmetric key
> > operations.  It's unclear why they exist, as the same functionality is 
> > available
> > in userspace crypto libraries.
> > 
> > I suppose that the blamed commit, or at least part of it, will need to be
> > reverted to keep these weird keyctls working.
> > 
> > For the future, why doesn't iwd just use a userspace crypto library such as
> > OpenSSL?
> 
> I was not around when the original decision was made, but a few reasons I
> know we don't use openSSL:
> 
>  - IWD has virtually zero dependencies.

Depending on something in the kernel does not eliminate a dependency; it just
adds that particular kernel UAPI to your list of dependencies.  The reason that
we're having this discussion in the first place is because iwd is depending on
an obscure kernel UAPI that is not well defined.  Historically it's been hard to
avoid "breaking" changes in these crypto-related UAPIs because of the poor
design where a huge number of algorithms are potentially supported, but the list
is undocumented and it varies from one system to another based on configuration.
Also due to their obscurity many kernel developers don't know that these UAPIs
even exist.  (The reaction when someone finds out is usually "Why!?")

It may be worth looking at if iwd should make a different choice for this
dependency.  It's understandable to blame dependencies when things go wrong, but
at the same time the choice of dependency is very much a choice, and some
choices can be more technically sound and cause fewer problems than others...

>  - OpenSSL + friends are rather large libraries.

The Linux kernel is also large, and it's made larger by having to support
obsolete crypto algorithms for backwards compatibility with iwd.

>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> too).

OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.

> Another consideration is once you support openSSL someone wants wolfSSL,
> then boringSSL etc. Even if users implement support it just becomes a huge
> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
> ~5k. You have to sort out all the nitty gritty details of each library, and
> provide a common driver/API for the core code, differences between openssl
> versions, the list goes on.

What is the specific functionality that you're actually relying on that you
think would need 40K lines of code to replace, even using OpenSSL?  I see you
are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
operations are being performed, and with which algorithms and key formats?
Also, is the kernel behavior that you're relying on documented anywhere?  There
are man pages for those keyctls, but they don't say anything about any
particular

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 1:22 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:

Hi,

On 3/13/24 12:44 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:

Hi,

On 3/13/24 1:56 AM, Johannes Berg wrote:

Not sure why you're CC'ing the world, but I guess adding a few more
doesn't hurt ...

On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:

and I use iwd

This is your problem, the wireless stack in the kernel doesn't use any
kernel crypto code for 802.1X.

Yes, the wireless stack has zero bearing on the issue. I think that's what
you meant by "problem".

IWD has used the kernel crypto API forever which was abruptly broken, that
is the problem.

The original commit says it was to remove support for sha1 signed kernel
modules, but it did more than that and broke the keyctl API.


Which specific API is iwd using that is relevant here?
I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
and grepped for keyctl and AF_ALG, but there are no matches.

IWD uses ELL for its crypto, which uses the AF_ALG API:

https://git.kernel.org/pub/scm/libs/ell/ell.git/

Thanks for pointing out that the relevant code is really in that separate
repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
blamed commit didn't change anything for AF_ALG.


I believe the failure is when calling:

KEYCTL_PKEY_QUERY enc="x962" hash="sha1"

 From logs Michael posted on the IWD list, the ELL API that fails is:

l_key_get_info (ell.git/ell/key.c:416)

Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
weird set of APIs where userspace can ask the kernel to do asymmetric key
operations.  It's unclear why they exist, as the same functionality is available
in userspace crypto libraries.

I suppose that the blamed commit, or at least part of it, will need to be
reverted to keep these weird keyctls working.

For the future, why doesn't iwd just use a userspace crypto library such as
OpenSSL?


I was not around when the original decision was made, but a few reasons 
I know we don't use openSSL:


 - IWD has virtually zero dependencies.

 - OpenSSL + friends are rather large libraries.

 - AF_ALG has transparent hardware acceleration (not sure if openSSL 
does too).


Another consideration is once you support openSSL someone wants wolfSSL, 
then boringSSL etc. Even if users implement support it just becomes a 
huge burden to carry for the project. Just look at wpa_supplicant's 
src/crypto/ folder, nearly 40k LOC in there, compared to ELL's crypto 
modules which is ~5k. You have to sort out all the nitty gritty details 
of each library, and provide a common driver/API for the core code, 
differences between openssl versions, the list goes on.


Thanks,

James




- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> Hi,
> 
> On 3/13/24 12:44 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> > > Hi,
> > > 
> > > On 3/13/24 1:56 AM, Johannes Berg wrote:
> > > > Not sure why you're CC'ing the world, but I guess adding a few more
> > > > doesn't hurt ...
> > > > 
> > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > > > >and I use iwd
> > > > This is your problem, the wireless stack in the kernel doesn't use any
> > > > kernel crypto code for 802.1X.
> > > Yes, the wireless stack has zero bearing on the issue. I think that's what
> > > you meant by "problem".
> > > 
> > > IWD has used the kernel crypto API forever which was abruptly broken, that
> > > is the problem.
> > > 
> > > The original commit says it was to remove support for sha1 signed kernel
> > > modules, but it did more than that and broke the keyctl API.
> > > 
> > Which specific API is iwd using that is relevant here?
> > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > and grepped for keyctl and AF_ALG, but there are no matches.
> 
> IWD uses ELL for its crypto, which uses the AF_ALG API:
> 
> https://git.kernel.org/pub/scm/libs/ell/ell.git/

Thanks for pointing out that the relevant code is really in that separate
repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
blamed commit didn't change anything for AF_ALG.

> I believe the failure is when calling:
> 
> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> 
> From logs Michael posted on the IWD list, the ELL API that fails is:
> 
> l_key_get_info (ell.git/ell/key.c:416)

Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
weird set of APIs where userspace can ask the kernel to do asymmetric key
operations.  It's unclear why they exist, as the same functionality is available
in userspace crypto libraries.

I suppose that the blamed commit, or at least part of it, will need to be
reverted to keep these weird keyctls working.

For the future, why doesn't iwd just use a userspace crypto library such as
OpenSSL?

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 12:44 PM, Eric Biggers wrote:

On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:

Hi,

On 3/13/24 1:56 AM, Johannes Berg wrote:

Not sure why you're CC'ing the world, but I guess adding a few more
doesn't hurt ...

On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:

   and I use iwd

This is your problem, the wireless stack in the kernel doesn't use any
kernel crypto code for 802.1X.

Yes, the wireless stack has zero bearing on the issue. I think that's what
you meant by "problem".

IWD has used the kernel crypto API forever which was abruptly broken, that
is the problem.

The original commit says it was to remove support for sha1 signed kernel
modules, but it did more than that and broke the keyctl API.


Which specific API is iwd using that is relevant here?
I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
and grepped for keyctl and AF_ALG, but there are no matches.


IWD uses ELL for its crypto, which uses the AF_ALG API:

https://git.kernel.org/pub/scm/libs/ell/ell.git/

I believe the failure is when calling:

KEYCTL_PKEY_QUERY enc="x962" hash="sha1"

From logs Michael posted on the IWD list, the ELL API that fails is:

l_key_get_info (ell.git/ell/key.c:416)

Thanks,

James



- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Karel Balej]

Thank you all for your feedback so far.

Since it seems that this really is a regression on the kernel side, let
me add the appropriate list to Cc and tag this:

#regzbot introduced: 16ab7cb5825f

Best regards,
K. B.

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> Hi,
> 
> On 3/13/24 1:56 AM, Johannes Berg wrote:
> > Not sure why you're CC'ing the world, but I guess adding a few more
> > doesn't hurt ...
> > 
> > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > >   and I use iwd
> > This is your problem, the wireless stack in the kernel doesn't use any
> > kernel crypto code for 802.1X.
> 
> Yes, the wireless stack has zero bearing on the issue. I think that's what
> you meant by "problem".
> 
> IWD has used the kernel crypto API forever which was abruptly broken, that
> is the problem.
> 
> The original commit says it was to remove support for sha1 signed kernel
> modules, but it did more than that and broke the keyctl API.
> 

Which specific API is iwd using that is relevant here?
I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
and grepped for keyctl and AF_ALG, but there are no matches.

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Michael Yartys]

Hi

This came in via the iwd mailing list, and I would like to add some small a 
detail as I also experience this issue on my university eduroam network. I've 
verified that the certificate chain doesn't contain SHA-1 signed certificates, 
so the update breaks more than just SHA-1.

Michael




On Wednesday, March 13th, 2024 at 09:56, Johannes Berg 
 wrote:

> 
> 
> Not sure why you're CC'ing the world, but I guess adding a few more
> doesn't hurt ...
> 
> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> 
> > and I use iwd
> 
> 
> This is your problem, the wireless stack in the kernel doesn't use any
> kernel crypto code for 802.1X.
> 
> I suppose iwd wants to use the kernel infrastructure but has no
> fallbacks to other implementations.
> 
> johannes

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 1:56 AM, Johannes Berg wrote:

Not sure why you're CC'ing the world, but I guess adding a few more
doesn't hurt ...

On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:

  and I use iwd

This is your problem, the wireless stack in the kernel doesn't use any
kernel crypto code for 802.1X.


Yes, the wireless stack has zero bearing on the issue. I think that's 
what you meant by "problem".


IWD has used the kernel crypto API forever which was abruptly broken, 
that is the problem.


The original commit says it was to remove support for sha1 signed kernel 
modules, but it did more than that and broke the keyctl API.




I suppose iwd wants to use the kernel infrastructure but has no
fallbacks to other implementations.
johannes

Re: [PATCH 1/2] dt-bindings: crypto: ice: Document sc7280 inline crypto engine

[Krzysztof Kozlowski]

On 13/03/2024 13:53, Luca Weiss wrote:
> Document the compatible used for the inline crypto engine found on
> SC7280.
> 
> Signed-off-by: Luca Weiss 
> ---

Acked-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add inline crypto engine

[Konrad Dybcio]

On 3/13/24 13:53, Luca Weiss wrote:

Add the ICE found on sc7280 and link it to the UFS node.

For reference:

   [0.261424] qcom-ice 1d88000.crypto: Found QC Inline Crypto Engine (ICE) 
v3.2.0

Signed-off-by: Luca Weiss 
---


Reviewed-by: Konrad Dybcio 

Konrad

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Johannes Berg]

Not sure why you're CC'ing the world, but I guess adding a few more
doesn't hurt ...

On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> 
>  and I use iwd

This is your problem, the wireless stack in the kernel doesn't use any
kernel crypto code for 802.1X.

I suppose iwd wants to use the kernel infrastructure but has no
fallbacks to other implementations.

johannes

[REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Karel Balej]

Dimitri, Johannes,

ever since upgrading to Linux v6.7 I am unable to connect to a 802.1X
wireless network (specifically, eduroam). In my dmesg, the following
messages appear:

[   68.161621] wlan0: authenticate with xx:xx:xx:xx:xx:xx (local 
address=xx:xx:xx:xx:xx:xx)
[   68.163733] wlan0: send auth to xx:xx:xx:xx:xx:xx (try 1/3)
[   68.165773] wlan0: authenticated
[   68.166785] wlan0: associate with xx:xx:xx:xx:xx:xx (try 1/3)
[   68.168498] wlan0: RX AssocResp from xx:xx:xx:xx:xx:xx (capab=0x1411 
status=0 aid=4)
[   68.172445] wlan0: associated
[   68.204956] wlan0: Limiting TX power to 23 (23 - 0) dBm as 
advertised by xx:xx:xx:xx:xx:xx
[   70.262032] wlan0: deauthenticated from xx:xx:xx:xx:xx:xx (Reason: 
23=IEEE8021X_FAILED)
[   73.065966] wlan0: authenticate with xx:xx:xx:xx:xx:xx (local 
address=xx:xx:xx:xx:xx:xx)
[   73.068006] wlan0: send auth to xx:xx:xx:xx:xx:xx (try 1/3)
[   73.070166] wlan0: authenticated
[   73.070756] wlan0: associate with xx:xx:xx:xx:xx:xx (try 1/3)
[   73.072807] wlan0: RX AssocResp from xx:xx:xx:xx:xx:xx (capab=0x1411 
status=0 aid=4)
[   73.076676] wlan0: associated
[   73.120396] wlan0: Limiting TX power to 23 (23 - 0) dBm as 
advertised by xx:xx:xx:xx:xx:xx
[   75.148376] wlan0: deauthenticating from xx:xx:xx:xx:xx:xx by local 
choice (Reason: 23=IEEE8021X_FAILED)
[   77.718016] wlan0: authenticate with xx:xx:xx:xx:xx:xx (local 
address=xx:xx:xx:xx:xx:xx)
[   77.720137] wlan0: send auth to xx:xx:xx:xx:xx:xx (try 1/3)
[   77.722670] wlan0: authenticated
[   77.724737] wlan0: associate with xx:xx:xx:xx:xx:xx (try 1/3)
[   77.726172] wlan0: RX AssocResp from xx:xx:xx:xx:xx:xx (capab=0x1411 
status=0 aid=4)
[   77.730822] wlan0: associated
[   77.830763] wlan0: Limiting TX power to 23 (23 - 0) dBm as 
advertised by xx:xx:xx:xx:xx:xx
[   79.784199] wlan0: deauthenticating from xx:xx:xx:xx:xx:xx by local 
choice (Reason: 23=IEEE8021X_FAILED)

The connection works fine with v6.6 and I have bisected the problem to
the revision introduced by this patch (16ab7cb5825f mainline).

My wireless kernel driver is iwlwifi and I use iwd. I started the bisect
with a config copied from my distribution package [1].

Would you please help me with this? Please let me know if I forgot to
mention something which could be helpful in resolving this.

[1] 
https://raw.githubusercontent.com/void-linux/void-packages/master/srcpkgs/linux6.6/files/x86_64-dotconfig

Thank you very much, kind regards,
K. B.

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 11:31 PM EET, Eric Snowberg wrote:
>
>
> > On Mar 11, 2024, at 1:18 PM, Jarkko Sakkinen  wrote:
> > 
> > On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> >> + return -1;
> > 
> > Missed this one: why a magic number?
>
> Good point, I'll change this to return -ENOKEY.  Thanks.

Either that or a boolean function, which ever fits to the overall
flow better... The upside of error code is less branching in the call
sites. The upside of boolean is that caller exactly knows all the
values that ever should come out as a result.

Your choice ofc.

BR, Jarkko

Re: [PATCH RFC 8/8] clavis: Introduce new LSM called clavis

[Eric Snowberg]

> On Mar 11, 2024, at 8:45 PM, Randy Dunlap  wrote:
> 
> On 3/11/24 09:11, Eric Snowberg wrote:
>> In the future it is envisioned this LSM could be enhanced to provide
>> access control for UEFI Secure Boot Advanced Targeting (SBAT).  Using
>> the same clavis= boot param and storing the additional contents within
>> the new RT UEFI var, SBAT restrictions could be maintained across kexec.
> 
> What does "RT" mean here?

I will define it in the next round.  It stands for Run-Time.

>> +
>> +Clavis is a Linux Security Module that provides mandatory access control to
>> +system kernel keys (i.e. builtin, secondary, machine and platform). These
>> +restrictions will prohit keys from being used for validation. Upon boot, the
> 
> prohibit

I will fix this spelling error and all the others you identified below.

> 
>> +Clavis LSM is provided a key id as a boot param.  This single key is then
>> +used as the root of trust for any access control modifications made going
>> +forward. Access control updates must be signed and validated by this key.
>> +
>> +Clavis has its own keyring.  All ACL updates are applied through this 
>> keyring.
>> +The update must be signed by the single root of trust key.
>> +
>> +When enabled, all system keys are prohibited from being used until an ACL is
>> +added for it. There is two exceptions to this rule, builtin keys may be used
> 
> What is "it"?  The predecessor seems to be "all system keys" (plural).

The word "for" should be "to" above.  The sentence should be:

When enabled, all system keys are prohibited from being used until an ACL is
added to it.

>> +
>> +The Clavis LSM contains a system keyring call .clavis.  It contains a single
>> +asymmetric key that is use to validate anything added to it.  This key can 
>> only
>> +be added during boot and must be a preexisting system kernel key.  If the
>> +clavis= boot param is not used, the keyring does not exist and the feature
>> +can not be used until the next power on reset.
> 
> So just a reboot won't cause it to be used?  Must be power off/on?

A reboot would too.  I will rework this sentence.  I just want to show that 
doing a kexec 
can not be used to make changes to the root of trust.

>> diff --git a/security/security.c b/security/security.c
>> index 4cb832b00c40..d1da60a1b7a4 100644
>> --- a/security/security.c
>> +++ b/security/security.c
> 
>> @@ -5313,6 +5314,19 @@ void security_key_post_create_or_update(struct key 
>> *keyring, struct key *key,
>> call_void_hook(key_post_create_or_update, keyring, key, payload,
>>   payload_len, flags, create);
>> }
>> +
>> +/**
>> + * security_key_verify_signature - verify signature
>> + * @key: key
>> + * @public_key_signature: signature
> 
> Above should be "@sig:".

I will fix that too.  Thanks for your review.

RE: [EXTERNAL] [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Bharat Bhushan]

> -Original Message-
> From: Eric Snowberg 
> Sent: Monday, March 11, 2024 9:41 PM
> To: linux-security-mod...@vger.kernel.org
> Cc: dhowe...@redhat.com; dw...@infradead.org;
> herb...@gondor.apana.org.au; da...@davemloft.net; a...@kernel.org;
> jar...@kernel.org; p...@paul-moore.com; jmor...@namei.org;
> se...@hallyn.com; zo...@linux.ibm.com; roberto.sa...@huawei.com;
> dmitry.kasat...@gmail.com; m...@digikod.net; ca...@schaufler-ca.com;
> stef...@linux.ibm.com; eric.snowb...@oracle.com; linux-
> ker...@vger.kernel.org; keyri...@vger.kernel.org; linux-
> cry...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> integr...@vger.kernel.org
> Subject: [EXTERNAL] [PATCH RFC 1/8] certs: Introduce ability to link to a
> system key
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> --
> Introduce a new function to allow a keyring to link to a key contained
> within one of the system keyrings (builtin, secondary, or platform).
> Depending on how the kernel is built, if the machine keyring is
> available, it will be checked as well, since it is linked to the secondary
> keyring. If the asymmetric key id matches a key within one of these
> system keyrings, the matching key is linked into the passed in
> keyring.
> 
> Signed-off-by: Eric Snowberg 
> ---
>  certs/system_keyring.c| 29 +
>  include/keys/system_keyring.h |  7 ++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..b647be49f6e0 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
>   platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +/**
> + * system_key_link - Link to a system key
> + * @keyring: The keyring to link into
> + * @id: The asymmetric key id to look for in the system keyring
> + */
> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> + struct key *tkey;
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL,
> false);
> +#else
> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL,
> false);
> +#endif
> + if (!IS_ERR(tkey))
> + goto found;
> +
> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL,
> false);
> +
> + if (!IS_ERR(tkey))
> + goto found;
> +
> + return -1;

Please use -ENOKEY.

Thanks
-Bharat

> +
> +found:
> + key_link(keyring, tkey);
> + return 0;
> +}
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 8365adf842ef..b47ac8e2001a 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -9,6 +9,7 @@
>  #define _KEYS_SYSTEM_KEYRING_H
> 
>  #include 
> +struct asymmetric_key_id;
> 
>  enum blacklist_hash_type {
>   /* TBSCertificate hash */
> @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key
> *dest_keyring,
>   const union key_payload *payload,
>   struct key *restriction_key);
>  extern __init int load_module_cert(struct key *keyring);
> -
> +extern int system_key_link(struct key *keyring, struct asymmetric_key_id
> *id);
>  #else
>  #define restrict_link_by_builtin_trusted restrict_link_reject
>  #define restrict_link_by_digsig_builtin restrict_link_reject
> @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key
> *keyring)
>   return 0;
>  }
> 
> +static inline int system_key_link(struct key *keyring, struct 
> asymmetric_key_id
> *id)
> +{
> + return 0;
> +}
>  #endif
> 
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> --
> 2.39.3
> 

Re: [PATCH RFC 8/8] clavis: Introduce new LSM called clavis

[Randy Dunlap]

On 3/11/24 09:11, Eric Snowberg wrote:
> In the future it is envisioned this LSM could be enhanced to provide
> access control for UEFI Secure Boot Advanced Targeting (SBAT).  Using
> the same clavis= boot param and storing the additional contents within
> the new RT UEFI var, SBAT restrictions could be maintained across kexec.

What does "RT" mean here?

(more below)

> 
> Signed-off-by: Eric Snowberg 
> ---
>  Documentation/admin-guide/LSM/clavis.rst | 190 +++
>  MAINTAINERS  |   7 +
>  crypto/asymmetric_keys/signature.c   |   4 +
>  include/linux/lsm_hook_defs.h|   2 +
>  include/linux/security.h |   7 +
>  include/uapi/linux/lsm.h |   1 +
>  security/Kconfig |  10 +-
>  security/clavis/Makefile |   1 +
>  security/clavis/clavis.c |  25 +++
>  security/clavis/clavis.h |   4 +
>  security/clavis/clavis_keyring.c |  83 ++
>  security/security.c  |  16 +-
>  12 files changed, 344 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/admin-guide/LSM/clavis.rst
>  create mode 100644 security/clavis/clavis.c
> 


> diff --git a/Documentation/admin-guide/LSM/clavis.rst 
> b/Documentation/admin-guide/LSM/clavis.rst
> new file mode 100644
> index ..b0a73defb4fc
> --- /dev/null
> +++ b/Documentation/admin-guide/LSM/clavis.rst
> @@ -0,0 +1,190 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=
> +Clavis
> +=
> +
> +Clavis is a Linux Security Module that provides mandatory access control to
> +system kernel keys (i.e. builtin, secondary, machine and platform). These
> +restrictions will prohit keys from being used for validation. Upon boot, the

 prohibit

> +Clavis LSM is provided a key id as a boot param.  This single key is then
> +used as the root of trust for any access control modifications made going
> +forward. Access control updates must be signed and validated by this key.
> +
> +Clavis has its own keyring.  All ACL updates are applied through this 
> keyring.
> +The update must be signed by the single root of trust key.
> +
> +When enabled, all system keys are prohibited from being used until an ACL is
> +added for it. There is two exceptions to this rule, builtin keys may be used

What is "it"?  The predecessor seems to be "all system keys" (plural).


> +to validate both signed kernels and modules.
> +
> +Adding system kernel keys can only be performed by the machine owner, this

  owner;

> +could be through the Machine Owner Key (MOK) or the UEFI Secure Boot DB. It
> +is possible the machine owner and system administrator may be different
> +people. The system administrator will not be able to make ACL updates without
> +them being signed by the machine owner.
> +
> +On UEFI platforms, the root of trust key shall survive a kexec. Trying to
> +defeat or change it from the command line is not allowed.  The original boot
> +param is stored in UEFI and will always be referenced following a kexec.
> +
> +The Clavis LSM contains a system keyring call .clavis.  It contains a single
> +asymmetric key that is use to validate anything added to it.  This key can 
> only
> +be added during boot and must be a preexisting system kernel key.  If the
> +clavis= boot param is not used, the keyring does not exist and the feature
> +can not be used until the next power on reset.

So just a reboot won't cause it to be used?  Must be power off/on?

> +
> +The only user space components are OpenSSL and the keyctl utility. A new
> +key type call clavis_key_acl is used for ACL updates. Any number of signed
> +clavis_key_acl entries may be added to the .clavis keyring. The 
> clavis_key_acl
> +contains the subject key identifer along with the allowed usage type for

identifier

> +the key.
> +
> +The format is as follows:
> +::
> +
> +  XX:YYY
> +
> +  XX - Single byte of the key type
> + VERIFYING_MODULE_SIGNATURE00
> + VERIFYING_FIRMWARE_SIGNATURE  01
> + VERIFYING_KEXEC_PE_SIGNATURE  02
> + VERIFYING_KEY_SIGNATURE   03
> + VERIFYING_KEY_SELF_SIGNATURE  04
> + VERIFYING_UNSPECIFIED_SIGNATURE   05
> +  :  - ASCII colon
> +  YY - Even number of hexadecimal characters representing the key id
> +
> +The clavis_key_acl must be S/MIME signed by the sole asymmetric key contained
> +within the .clavis keyring.
> +
> +In the future if new features are added, new key types could be created.
> +
> +Usage Examples
> +==
> +
> +How to create a signing key:
> +
> +
> +::
> +
> +  cat < clavis-lsm.genkey
> +  [ req ]
> +  default_bits = 4096
> +  distinguished_name = req_distinguished_name
> +  prompt = no
> +  string_mask = utf8only
> +  x509_extensions = v3_ca
> +  

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Eric Snowberg]

> On Mar 11, 2024, at 1:18 PM, Jarkko Sakkinen  wrote:
> 
> On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
>> + return -1;
> 
> Missed this one: why a magic number?

Good point, I'll change this to return -ENOKEY.  Thanks.

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Eric Snowberg]

> On Mar 11, 2024, at 1:16 PM, Jarkko Sakkinen  wrote:
> 
> On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
>> Introduce a new function to allow a keyring to link to a key contained
>> within one of the system keyrings (builtin, secondary, or platform).
>> Depending on how the kernel is built, if the machine keyring is
>> available, it will be checked as well, since it is linked to the secondary
>> keyring. If the asymmetric key id matches a key within one of these
>> system keyrings, the matching key is linked into the passed in
>> keyring.
>> 
>> Signed-off-by: Eric Snowberg 
>> ---
>> certs/system_keyring.c| 29 +
>> include/keys/system_keyring.h |  7 ++-
>> 2 files changed, 35 insertions(+), 1 deletion(-)
>> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 9de610bf1f4b..b647be49f6e0 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key 
>> *keyring)
>> platform_trusted_keys = keyring;
>> }
>> #endif
>> +
>> +/**
>> + * system_key_link - Link to a system key
>> + * @keyring: The keyring to link into
>> + * @id: The asymmetric key id to look for in the system keyring
>> + */
>> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
>> +{
>> + struct key *tkey;
> 
> I'd suggest to replace this with just 'tkey'. Single obscure character
> does not bring any readability value.

I assume you mean replace it with "key"?  If so, yes, I'll change that in the 
next round.

>> +
>> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false);
>> +#else
>> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
>> +#endif
> 
> I'd have just single call site here and inside ifdef-shenanigans i'd
> just set helper "keyring" to point to the appropriate keyring.

I'll also make this change

>> + if (!IS_ERR(tkey))
>> + goto found;
>> +
>> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false);
>> +
> 
> Please remove this empty line as the check below is associated with it.

and remove the empty line.Thanks.

Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys

[Jarkko Sakkinen]

On Fri Mar 8, 2024 at 9:17 AM EET, David Gstir wrote:
> Hi Jarkko,
>
> > On 07.03.2024, at 20:30, Jarkko Sakkinen  wrote:
>
> [...]
>
> >> +
> >> +static int trusted_dcp_init(void)
> >> +{
> >> + int ret;
> >> +
> >> + if (use_otp_key)
> >> + pr_info("Using DCP OTP key\n");
> >> +
> >> + ret = test_for_zero_key();
> >> + if (ret) {
> >> + pr_err("Test for zero'ed keys failed: %i\n", ret);
> > 
> > I'm not sure whether this should err or warn.
> > 
> > What sort of situations can cause the test the fail (e.g.
> > adversary/interposer, bad configuration etc.).
>
> This occurs when the hardware is not in "secure mode". I.e. it’s a bad 
> configuration issue.
> Once the board is properly configured, this will never trigger again.
> Do you think a warning is better for this then?

Bad configuration is not unexpected configuration so it cannot possibly
be an error situation as far as Linux is considered. So warning is 
appropriate here I'd figure.

BR, Jarkko

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> + return -1;

Missed this one: why a magic number?

BR, Jarkko

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> Introduce a new function to allow a keyring to link to a key contained
> within one of the system keyrings (builtin, secondary, or platform).
> Depending on how the kernel is built, if the machine keyring is
> available, it will be checked as well, since it is linked to the secondary
> keyring. If the asymmetric key id matches a key within one of these
> system keyrings, the matching key is linked into the passed in
> keyring.
>
> Signed-off-by: Eric Snowberg 
> ---
>  certs/system_keyring.c| 29 +
>  include/keys/system_keyring.h |  7 ++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..b647be49f6e0 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key 
> *keyring)
>   platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +/**
> + * system_key_link - Link to a system key
> + * @keyring: The keyring to link into
> + * @id: The asymmetric key id to look for in the system keyring
> + */
> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> + struct key *tkey;

I'd suggest to replace this with just 'tkey'. Single obscure character
does not bring any readability value.

> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, 
> false);
> +#else
> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
> +#endif

I'd have just single call site here and inside ifdef-shenanigans i'd
just set helper "keyring" to point to the appropriate keyring.

> + if (!IS_ERR(tkey))
> + goto found;
> +
> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, 
> false);
> +

Please remove this empty line as the check below is associated with it.

> + if (!IS_ERR(tkey))
> + goto found;
> +
> + return -1;
> +
> +found:
> + key_link(keyring, tkey);
> + return 0;
> +}
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 8365adf842ef..b47ac8e2001a 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -9,6 +9,7 @@
>  #define _KEYS_SYSTEM_KEYRING_H
>  
>  #include 
> +struct asymmetric_key_id;
>  
>  enum blacklist_hash_type {
>   /* TBSCertificate hash */
> @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key 
> *dest_keyring,
>   const union key_payload *payload,
>   struct key *restriction_key);
>  extern __init int load_module_cert(struct key *keyring);
> -
> +extern int system_key_link(struct key *keyring, struct asymmetric_key_id 
> *id);
>  #else
>  #define restrict_link_by_builtin_trusted restrict_link_reject
>  #define restrict_link_by_digsig_builtin restrict_link_reject
> @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key 
> *keyring)
>   return 0;
>  }
>  
> +static inline int system_key_link(struct key *keyring, struct 
> asymmetric_key_id *id)
> +{
> + return 0;
> +}
>  #endif
>  
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING

BR, Jarkko

Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys

[David Gstir]

Hi Jarkko,

> On 07.03.2024, at 20:30, Jarkko Sakkinen  wrote:

[...]

>> +
>> +static int trusted_dcp_init(void)
>> +{
>> + int ret;
>> +
>> + if (use_otp_key)
>> + pr_info("Using DCP OTP key\n");
>> +
>> + ret = test_for_zero_key();
>> + if (ret) {
>> + pr_err("Test for zero'ed keys failed: %i\n", ret);
> 
> I'm not sure whether this should err or warn.
> 
> What sort of situations can cause the test the fail (e.g.
> adversary/interposer, bad configuration etc.).

This occurs when the hardware is not in "secure mode". I.e. it’s a bad 
configuration issue.
Once the board is properly configured, this will never trigger again.
Do you think a warning is better for this then?

Thanks,
- David

Re: [PATCH v6 5/6] docs: document DCP-backed trusted keys kernel params

[Jarkko Sakkinen]

On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote:
> Document the kernel parameters trusted.dcp_use_otp_key
> and trusted.dcp_skip_zk_test for DCP-backed trusted keys.
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index 24c02c704049..b6944e57768a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6698,6 +6698,7 @@
>   - "tpm"
>   - "tee"
>   - "caam"
> + - "dcp"
>   If not specified then it defaults to iterating through
>   the trust source list starting with TPM and assigns the
>   first trust source as a backend which is initialized
> @@ -6713,6 +6714,18 @@
>   If not specified, "default" is used. In this case,
>   the RNG's choice is left to each individual trust 
> source.
>  
> + trusted.dcp_use_otp_key
> + This is intended to be used in combination with
> + trusted.source=dcp and will select the DCP OTP key
> + instead of the DCP UNIQUE key blob encryption.
> +
> + trusted.dcp_skip_zk_test
> + This is intended to be used in combination with
> + trusted.source=dcp and will disable the check if all
> + the blob key is zero'ed. This is helpful for situations 
> where
> + having this key zero'ed is acceptable. E.g. in testing
> + scenarios.
> +
>   tsc=Disable clocksource stability checks for TSC.
>   Format: 
>   [x86] reliable: mark tsc clocksource as reliable, this

I don't disagree with the API part.

Mimi?

BR, Jarkko

Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys

[Jarkko Sakkinen]

On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
> Beside of accelerated crypto operations, it also offers support for
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism similar to what CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software (i.e. the kernel).
>
> The software-based blob format used by DCP trusted keys encrypts
> the payload using AES-128-GCM with a freshly generated random key and nonce.
> The random key itself is AES-128-ECB encrypted using the DCP unique
> or OTP key.
>
> The DCP trusted key blob format is:
> /*
>  * struct dcp_blob_fmt - DCP BLOB format.
>  *
>  * @fmt_version: Format version, currently being %1
>  * @blob_key: Random AES 128 key which is used to encrypt @payload,
>  *@blob_key itself is encrypted with OTP or UNIQUE device key in
>  *AES-128-ECB mode by DCP.
>  * @nonce: Random nonce used for @payload encryption.
>  * @payload_len: Length of the plain text @payload.
>  * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
>  *   GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
>  *
>  * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len 
> +
>  * AES_BLOCK_SIZE.
>  */
> struct dcp_blob_fmt {
>   __u8 fmt_version;
>   __u8 blob_key[AES_KEYSIZE_128];
>   __u8 nonce[AES_KEYSIZE_128];
>   __le32 payload_len;
>   __u8 payload[];
> } __packed;
>
> By default the unique key is used. It is also possible to use the
> OTP key. While the unique key should be unique it is not documented how
> this key is derived. Therefore selection the OTP key is supported as
> well via the use_otp_key module parameter.
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> ---
>  include/keys/trusted_dcp.h|  11 +
>  security/keys/trusted-keys/Kconfig|   8 +
>  security/keys/trusted-keys/Makefile   |   2 +
>  security/keys/trusted-keys/trusted_core.c |   6 +-
>  security/keys/trusted-keys/trusted_dcp.c  | 309 ++
>  5 files changed, 335 insertions(+), 1 deletion(-)
>  create mode 100644 include/keys/trusted_dcp.h
>  create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..9aaa42075b40
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + */
> +
> +#ifndef TRUSTED_DCP_H
> +#define TRUSTED_DCP_H
> +
> +extern struct trusted_key_ops dcp_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Kconfig 
> b/security/keys/trusted-keys/Kconfig
> index 553dc117f385..1fb8aa001995 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -39,6 +39,14 @@ config TRUSTED_KEYS_CAAM
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>  
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + select HAVE_TRUSTED_KEYS
> + help
> +   Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
>  if !HAVE_TRUSTED_KEYS
>   comment "No trust source selected!"
>  endif
> diff --git a/security/keys/trusted-keys/Makefile 
> b/security/keys/trusted-keys/Makefile
> index 735aa0bc08ef..f0f3b27f688b 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -14,3 +14,5 @@ trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>  trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>  
>  trusted-$(CONFIG_TRUSTED_KEYS_CAAM) += trusted_caam.o
> +
> +trusted-$(CONFIG_TRUSTED_KEYS_DCP) += trusted_dcp.o
> diff --git a/security/keys/trusted-keys/trusted_core.c 
> b/security/keys/trusted-keys/trusted_core.c
> index fee1ab2c734d..5113aeae5628 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -30,7 +31,7 @@ MODULE_PARM_DESC(rng, "Select trusted key RNG");
>  
>  static char *trusted_key_source;
>  module_param_named(source, trusted_key_source, charp, 0);
> -MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee or caam)");
> +MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee, caam or 
> dcp)");
>  
>  static const struct trusted_key_source trusted_key_sources[] = {
>  #if defined(CONFIG_TRUSTED_KEYS_TPM)
> @@ -42,6 +43,9 @@ static const struct trusted_key_source 
> trusted_key_sources[] = {
>  #if defined(CONFIG_T

Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys

[Jarkko Sakkinen]

On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote:> DCP (Data Co-Processor) 
is the little brother of NXP's CAAM IP.
> Beside of accelerated crypto operations, it also offers support for
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism similar to what CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software (i.e. the kernel).
>
> The software-based blob format used by DCP trusted keys encrypts
> the payload using AES-128-GCM with a freshly generated random key and nonce.
> The random key itself is AES-128-ECB encrypted using the DCP unique
> or OTP key.
>
> The DCP trusted key blob format is:
> /*
>  * struct dcp_blob_fmt - DCP BLOB format.
>  *
>  * @fmt_version: Format version, currently being %1
>  * @blob_key: Random AES 128 key which is used to encrypt @payload,
>  *@blob_key itself is encrypted with OTP or UNIQUE device key in
>  *AES-128-ECB mode by DCP.
>  * @nonce: Random nonce used for @payload encryption.
>  * @payload_len: Length of the plain text @payload.
>  * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
>  *   GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
>  *
>  * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len 
> +
>  * AES_BLOCK_SIZE.
>  */
> struct dcp_blob_fmt {
>   __u8 fmt_version;
>   __u8 blob_key[AES_KEYSIZE_128];
>   __u8 nonce[AES_KEYSIZE_128];
>   __le32 payload_len;
>   __u8 payload[];
> } __packed;
>
> By default the unique key is used. It is also possible to use the
> OTP key. While the unique key should be unique it is not documented how
> this key is derived. Therefore selection the OTP key is supported as
> well via the use_otp_key module parameter.
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> ---
>  include/keys/trusted_dcp.h|  11 +
>  security/keys/trusted-keys/Kconfig|   8 +
>  security/keys/trusted-keys/Makefile   |   2 +
>  security/keys/trusted-keys/trusted_core.c |   6 +-
>  security/keys/trusted-keys/trusted_dcp.c  | 309 ++
>  5 files changed, 335 insertions(+), 1 deletion(-)
>  create mode 100644 include/keys/trusted_dcp.h
>  create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..9aaa42075b40
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + */
> +
> +#ifndef TRUSTED_DCP_H
> +#define TRUSTED_DCP_H
> +
> +extern struct trusted_key_ops dcp_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Kconfig 
> b/security/keys/trusted-keys/Kconfig
> index 553dc117f385..1fb8aa001995 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -39,6 +39,14 @@ config TRUSTED_KEYS_CAAM
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>  
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + select HAVE_TRUSTED_KEYS
> + help
> +   Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
>  if !HAVE_TRUSTED_KEYS
>   comment "No trust source selected!"
>  endif
> diff --git a/security/keys/trusted-keys/Makefile 
> b/security/keys/trusted-keys/Makefile
> index 735aa0bc08ef..f0f3b27f688b 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -14,3 +14,5 @@ trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>  trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>  
>  trusted-$(CONFIG_TRUSTED_KEYS_CAAM) += trusted_caam.o
> +
> +trusted-$(CONFIG_TRUSTED_KEYS_DCP) += trusted_dcp.o
> diff --git a/security/keys/trusted-keys/trusted_core.c 
> b/security/keys/trusted-keys/trusted_core.c
> index fee1ab2c734d..5113aeae5628 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -30,7 +31,7 @@ MODULE_PARM_DESC(rng, "Select trusted key RNG");
>  
>  static char *trusted_key_source;
>  module_param_named(source, trusted_key_source, charp, 0);
> -MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee or caam)");
> +MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee, caam or 
> dcp)");
>  
>  static const struct trusted_key_source trusted_key_sources[] = {
>  #if defined(CONFIG_TRUSTED_KEYS_TPM)
> @@ -42,6 +43,9 @@ static const struct trusted_key_source 
> trusted_key_sources[] = {
>  #if defined(CONFIG_T

Re: [PATCH v6 2/6] KEYS: trusted: improve scalability of trust source config

[Jarkko Sakkinen]

On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote:
> Enabling trusted keys requires at least one trust source implementation
> (currently TPM, TEE or CAAM) to be enabled. Currently, this is
> done by checking each trust source's config option individually.
> This does not scale when more trust sources like the one for DCP
> are added.

nit: just to complete sentence and tie up the story "added because ..."

>
> Add config HAVE_TRUSTED_KEYS which is set to true by each trust source
> once its enabled and adapt the check for having at least one active trust
> source to use this option. Whenever a new trust source is added, it now
> needs to select HAVE_TRUSTED_KEYS.
>
> Signed-off-by: David Gstir 
> ---
>  security/keys/trusted-keys/Kconfig | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/trusted-keys/Kconfig 
> b/security/keys/trusted-keys/Kconfig
> index dbfdd8536468..553dc117f385 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -1,3 +1,6 @@
> +config HAVE_TRUSTED_KEYS
> + bool
> +
>  config TRUSTED_KEYS_TPM
>   bool "TPM-based trusted keys"
>   depends on TCG_TPM >= TRUSTED_KEYS
> @@ -9,6 +12,7 @@ config TRUSTED_KEYS_TPM
>   select ASN1_ENCODER
>   select OID_REGISTRY
>   select ASN1
> + select HAVE_TRUSTED_KEYS
>   help
> Enable use of the Trusted Platform Module (TPM) as trusted key
> backend. Trusted keys are random number symmetric keys,
> @@ -20,6 +24,7 @@ config TRUSTED_KEYS_TEE
>   bool "TEE-based trusted keys"
>   depends on TEE >= TRUSTED_KEYS
>   default y
> + select HAVE_TRUSTED_KEYS
>   help
> Enable use of the Trusted Execution Environment (TEE) as trusted
> key backend.
> @@ -29,10 +34,11 @@ config TRUSTED_KEYS_CAAM
>   depends on CRYPTO_DEV_FSL_CAAM_JR >= TRUSTED_KEYS
>   select CRYPTO_DEV_FSL_CAAM_BLOB_GEN
>   default y
> + select HAVE_TRUSTED_KEYS
>   help
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>  
> -if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM
> -comment "No trust source selected!"
> +if !HAVE_TRUSTED_KEYS
> + comment "No trust source selected!"
>  endif

otherwise, it is in reasonable shape and form.

BR, Jarkko

Re: [PATCH v6 1/6] crypto: mxs-dcp: Add support for hardware-bound keys

[Jarkko Sakkinen]

On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote:
> DCP (Data Co-Processor) is able to derive private keys for a fused
> random seed, which can be referenced by handle but not accessed by
> the CPU. Similarly, DCP is able to store arbitrary keys in four
> dedicated key slots located in its secure memory area (internal SRAM).
> These keys can be used to perform AES encryption.
>
> Expose these derived keys and key slots through the crypto API via their
> handle. The main purpose is to add DCP-backed trusted keys. Other
> use cases are possible too (see similar existing paes implementations),
> but these should carefully be evaluated as e.g. enabling AF_ALG will
> give userspace full access to use keys. In scenarios with untrustworthy
> userspace, this will enable en-/decryption oracles.
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> Acked-by: Herbert Xu 
> ---
>  drivers/crypto/mxs-dcp.c | 104 ++-
>  include/soc/fsl/dcp.h|  20 
>  2 files changed, 113 insertions(+), 11 deletions(-)
>  create mode 100644 include/soc/fsl/dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index 2b3ebe0db3a6..057d73c370b7 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
>   struct crypto_skcipher  *fallback;
>   unsigned intkey_len;
>   uint8_t key[AES_KEYSIZE_128];
> + boolkey_referenced;
>  };
>  
>  struct dcp_aes_req_ctx {
> @@ -155,6 +157,7 @@ static struct dcp *global_sdcp;
>  #define MXS_DCP_CONTROL0_HASH_TERM   (1 << 13)
>  #define MXS_DCP_CONTROL0_HASH_INIT   (1 << 12)
>  #define MXS_DCP_CONTROL0_PAYLOAD_KEY (1 << 11)
> +#define MXS_DCP_CONTROL0_OTP_KEY (1 << 10)
>  #define MXS_DCP_CONTROL0_CIPHER_ENCRYPT  (1 << 8)
>  #define MXS_DCP_CONTROL0_CIPHER_INIT (1 << 9)
>  #define MXS_DCP_CONTROL0_ENABLE_HASH (1 << 6)
> @@ -168,6 +171,8 @@ static struct dcp *global_sdcp;
>  #define MXS_DCP_CONTROL1_CIPHER_MODE_ECB (0 << 4)
>  #define MXS_DCP_CONTROL1_CIPHER_SELECT_AES128(0 << 0)
>  
> +#define MXS_DCP_CONTROL1_KEY_SELECT_SHIFT8
> +
>  static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
>  {
>   int dma_err;
> @@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>   struct dcp *sdcp = global_sdcp;
>   struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan];
>   struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + bool key_referenced = actx->key_referenced;
>   int ret;
>  
> - key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> -   2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> - ret = dma_mapping_error(sdcp->dev, key_phys);
> - if (ret)
> - return ret;
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> +   2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + ret = dma_mapping_error(sdcp->dev, key_phys);
> + if (ret)
> + return ret;
> + }
>  
>   src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> DCP_BUF_SZ, DMA_TO_DEVICE);
> @@ -255,8 +263,12 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>   MXS_DCP_CONTROL0_INTERRUPT |
>   MXS_DCP_CONTROL0_ENABLE_CIPHER;
>  
> - /* Payload contains the key. */
> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + if (key_referenced)
> + /* Set OTP key bit to select the key via KEY_SELECT. */
> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
> + else
> + /* Payload contains the key. */
> + desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
>  
>   if (rctx->enc)
>   desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -270,6 +282,9 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>   else
>   desc->control1 |= MXS_DCP_CONTROL1_CIPHER_MODE_CBC;
>  
> + if (key_referenced)
> + desc->control1 |= sdcp->coh->aes_key[0] << 
> MXS_DCP_CONTROL1_KEY_SELECT_SHIFT;
> +
>   desc->next_cmd_addr = 0;
>   desc->source = src_phys;
>   desc->destination = dst_phys;
> @@ -284,9 +299,9 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>  err_dst:
>   dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE);
>  err_src:
> - dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> -  DMA_TO_DEVICE);
> -
> + if (!key_referenced)
> + dma_unm

Re: [PATCH v5 4/6] MAINTAINERS: add entry for DCP-based trusted keys

[David Gstir]

Jarkko,

> On 04.03.2024, at 23:48, Jarkko Sakkinen  wrote:
> 
> On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
>> This covers trusted keys backed by NXP's DCP (Data Co-Processor) chip
>> found in smaller i.MX SoCs.
>> 
>> Signed-off-by: David Gstir 
>> ---
>> MAINTAINERS | 9 +
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 90f13281d297..988d01226131 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11647,6 +11647,15 @@ S: Maintained
>> F: include/keys/trusted_caam.h
>> F: security/keys/trusted-keys/trusted_caam.c
>> 
>> +KEYS-TRUSTED-DCP
>> +M: David Gstir 
>> +R: sigma star Kernel Team 
>> +L: linux-integr...@vger.kernel.org
>> +L: keyri...@vger.kernel.org
>> +S: Supported
>> +F: include/keys/trusted_dcp.h
>> +F: security/keys/trusted-keys/trusted_dcp.c
>> +
>> KEYS-TRUSTED-TEE
>> M: Sumit Garg 
>> L: linux-integr...@vger.kernel.org
> 
> Acked-by: Jarkko Sakkinen 
> 
> I can for sure put this. The code quality is *not* bad :-) However, your
> backing story really needs rework. It is otherwise impossible to
> understand the code changes later on because amount of information is
> vast, and you tend to forget details of stuff that you are not actively
> working on. That is why we care so deeply about them.

got it! :) I’ve tried to rework the commit messages as good as possible
for v6 and will send that series momentarily.

Thanks!
- David

Re: [PATCH v5 0/6] DCP as trusted keys backend

[Jarkko Sakkinen]

On Tue Dec 19, 2023 at 2:45 AM EET, Paul Moore wrote:
> On Fri, Dec 15, 2023 at 6:07 AM David Gstir  wrote:
> >
> > This is a revival of the previous patch set submitted by Richard Weinberger:
> > https://lore.kernel.org/linux-integrity/20210614201620.30451-1-rich...@nod.at/
> >
> > v4 is here:
> > https://lore.kernel.org/keyrings/20231024162024.51260-1-da...@sigma-star.at/
> >
> > v4 -> v5:
> > - Make Kconfig for trust source check scalable as suggested by Jarkko 
> > Sakkinen
> > - Add Acked-By from Herbert Xu to patch #1 - thanks!
> > v3 -> v4:
> > - Split changes on MAINTAINERS and documentation into dedicated patches
> > - Use more concise wording in commit messages as suggested by Jarkko 
> > Sakkinen
> > v2 -> v3:
> > - Addressed review comments from Jarkko Sakkinen
> > v1 -> v2:
> > - Revive and rebase to latest version
> > - Include review comments from Ahmad Fatoum
> >
> > The Data CoProcessor (DCP) is an IP core built into many NXP SoCs such
> > as i.mx6ull.
> >
> > Similar to the CAAM engine used in more powerful SoCs, DCP can AES-
> > encrypt/decrypt user data using a unique, never-disclosed,
> > device-specific key. Unlike CAAM though, it cannot directly wrap and
> > unwrap blobs in hardware. As DCP offers only the bare minimum feature
> > set and a blob mechanism needs aid from software. A blob in this case
> > is a piece of sensitive data (e.g. a key) that is encrypted and
> > authenticated using the device-specific key so that unwrapping can only
> > be done on the hardware where the blob was wrapped.
> >
> > This patch series adds a DCP based, trusted-key backend and is similar
> > in spirit to the one by Ahmad Fatoum [0] that does the same for CAAM.
> > It is of interest for similar use cases as the CAAM patch set, but for
> > lower end devices, where CAAM is not available.
> >
> > Because constructing and parsing the blob has to happen in software,
> > we needed to decide on a blob format and chose the following:
> >
> > struct dcp_blob_fmt {
> > __u8 fmt_version;
> > __u8 blob_key[AES_KEYSIZE_128];
> > __u8 nonce[AES_KEYSIZE_128];
> > __le32 payload_len;
> > __u8 payload[];
> > } __packed;
> >
> > The `fmt_version` is currently 1.
> >
> > The encrypted key is stored in the payload area. It is AES-128-GCM
> > encrypted using `blob_key` and `nonce`, GCM auth tag is attached at
> > the end of the payload (`payload_len` does not include the size of
> > the auth tag).
> >
> > The `blob_key` itself is encrypted in AES-128-ECB mode by DCP using
> > the OTP or UNIQUE device key. A new `blob_key` and `nonce` are generated
> > randomly, when sealing/exporting the DCP blob.
> >
> > This patchset was tested with dm-crypt on an i.MX6ULL board.
> >
> > [0] 
> > https://lore.kernel.org/keyrings/20220513145705.2080323-1-a.fat...@pengutronix.de/
> >
> > David Gstir (6):
> >   crypto: mxs-dcp: Add support for hardware-bound keys
> >   KEYS: trusted: improve scalability of trust source config
> >   KEYS: trusted: Introduce NXP DCP-backed trusted keys
> >   MAINTAINERS: add entry for DCP-based trusted keys
> >   docs: document DCP-backed trusted keys kernel params
> >   docs: trusted-encrypted: add DCP as new trust source
> >
> >  .../admin-guide/kernel-parameters.txt |  13 +
> >  .../security/keys/trusted-encrypted.rst   |  85 +
> >  MAINTAINERS   |   9 +
> >  drivers/crypto/mxs-dcp.c  | 104 +-
> >  include/keys/trusted_dcp.h|  11 +
> >  include/soc/fsl/dcp.h |  17 +
> >  security/keys/trusted-keys/Kconfig|  18 +-
> >  security/keys/trusted-keys/Makefile   |   2 +
> >  security/keys/trusted-keys/trusted_core.c |   6 +-
> >  security/keys/trusted-keys/trusted_dcp.c  | 311 ++
> >  10 files changed, 562 insertions(+), 14 deletions(-)
> >  create mode 100644 include/keys/trusted_dcp.h
> >  create mode 100644 include/soc/fsl/dcp.h
> >  create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> Jarkko, Mimi, David - if this patchset isn't already in your review
> queue, can you take a look at it from a security/keys perspective?
>
> Thanks.

I gave my 5 cents. I had no intention not to review it, somehow just
slipped. I try to do my best but sometimes this still does happen :-) So
please ping me if there is radio silence. 

BR, Jarkko

Re: [PATCH v5 4/6] MAINTAINERS: add entry for DCP-based trusted keys

[Jarkko Sakkinen]

On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> This covers trusted keys backed by NXP's DCP (Data Co-Processor) chip
> found in smaller i.MX SoCs.
>
> Signed-off-by: David Gstir 
> ---
>  MAINTAINERS | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..988d01226131 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11647,6 +11647,15 @@ S:   Maintained
>  F:   include/keys/trusted_caam.h
>  F:   security/keys/trusted-keys/trusted_caam.c
>  
> +KEYS-TRUSTED-DCP
> +M:   David Gstir 
> +R:   sigma star Kernel Team 
> +L:   linux-integr...@vger.kernel.org
> +L:   keyri...@vger.kernel.org
> +S:   Supported
> +F:   include/keys/trusted_dcp.h
> +F:   security/keys/trusted-keys/trusted_dcp.c
> +
>  KEYS-TRUSTED-TEE
>  M:   Sumit Garg 
>  L:   linux-integr...@vger.kernel.org

Acked-by: Jarkko Sakkinen 

I can for sure put this. The code quality is *not* bad :-) However, your
backing story really needs rework. It is otherwise impossible to
understand the code changes later on because amount of information is
vast, and you tend to forget details of stuff that you are not actively
working on. That is why we care so deeply about them.

BR, Jarkko

Re: [PATCH v5 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys

[Jarkko Sakkinen]

On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
> Beside of accelerated crypto operations, it also offers support for

Why acronym is not opened already in the first patch? Also, that does
not mean it could not be opened also here. Sometimes redundancy is for
better...

> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism similar to what CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software (i.e. the kernel).
>
> The software-based blob format used by DCP trusted keys encrypts
> the payload using AES-128-GCM with a freshly generated random key and nonce.
> The random key itself is AES-128-ECB encrypted using the DCP unique
> or OTP key.
>
> The DCP trusted key blob format is:
> /*
>  * struct dcp_blob_fmt - DCP BLOB format.
>  *
>  * @fmt_version: Format version, currently being %1
>  * @blob_key: Random AES 128 key which is used to encrypt @payload,
>  *@blob_key itself is encrypted with OTP or UNIQUE device key in
>  *AES-128-ECB mode by DCP.
>  * @nonce: Random nonce used for @payload encryption.
>  * @payload_len: Length of the plain text @payload.
>  * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
>  *   GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
>  *
>  * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len 
> +
>  * AES_BLOCK_SIZE.
>  */
> struct dcp_blob_fmt {
>   __u8 fmt_version;
>   __u8 blob_key[AES_KEYSIZE_128];
>   __u8 nonce[AES_KEYSIZE_128];
>   __le32 payload_len;
>   __u8 payload[];
> } __packed;
>
> By default the unique key is used. It is also possible to use the
> OTP key. While the unique key should be unique it is not documented how
> this key is derived. Therefore selection the OTP key is supported as
> well via the use_otp_key module parameter.

This is pretty good but I'll look it in more detail in the next
iteration of the patch set.

>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> ---
>  include/keys/trusted_dcp.h|  11 +
>  security/keys/trusted-keys/Kconfig|   8 +
>  security/keys/trusted-keys/Makefile   |   2 +
>  security/keys/trusted-keys/trusted_core.c |   6 +-
>  security/keys/trusted-keys/trusted_dcp.c  | 311 ++
>  5 files changed, 337 insertions(+), 1 deletion(-)
>  create mode 100644 include/keys/trusted_dcp.h
>  create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..9aaa42075b40
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + */
> +
> +#ifndef TRUSTED_DCP_H
> +#define TRUSTED_DCP_H
> +
> +extern struct trusted_key_ops dcp_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Kconfig 
> b/security/keys/trusted-keys/Kconfig
> index 553dc117f385..1fb8aa001995 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -39,6 +39,14 @@ config TRUSTED_KEYS_CAAM
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>  
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + select HAVE_TRUSTED_KEYS
> + help
> +   Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
>  if !HAVE_TRUSTED_KEYS
>   comment "No trust source selected!"
>  endif
> diff --git a/security/keys/trusted-keys/Makefile 
> b/security/keys/trusted-keys/Makefile
> index 735aa0bc08ef..f0f3b27f688b 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -14,3 +14,5 @@ trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>  trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>  
>  trusted-$(CONFIG_TRUSTED_KEYS_CAAM) += trusted_caam.o
> +
> +trusted-$(CONFIG_TRUSTED_KEYS_DCP) += trusted_dcp.o
> diff --git a/security/keys/trusted-keys/trusted_core.c 
> b/security/keys/trusted-keys/trusted_core.c
> index c6fc50d67214..8af0988be850 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -30,7 +31,7 @@ MODULE_PARM_DESC(rng, "Select trusted key RNG");
>  
>  static char *trusted_key_source;
>  module_param_named(source, trusted_key_source, charp, 0);
> -MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee or caam)");
> +MODULE_PARM_DESC(source, "Select trusted keys source (tpm,

Re: [PATCH v5 2/6] KEYS: trusted: improve scalability of trust source config

[Jarkko Sakkinen]

On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> Checking if at least one valid trust source is selected does not scale
> and becomes hard to read. This improves this in preparation for the DCP
> trust source.

This commit needs a complete rewrite and I do not have time and
energy to propose one but here's what it should contain:

1. Add HAVE_TRUSTED_KEYS to th trusted-keys/Kconfig.
2. The use and purpose of HAVE_TRUSTED_KEYS.

If you read your commit message, do you see anything at all concerning
the code change? It only tells a story about something that is not
properly being defined to be "hard to read", which is no rationale to
change anything at all in the kernel.

If you put factors more focus on being as straight and easy to get
in the commit messages, it will also improve the round-trip time
between sending the patch set and getting reviewed, because people
with limited time at their hands tend to pick the low-hanging fruit
first.

BR, Jarkko

Re: [PATCH v5 1/6] crypto: mxs-dcp: Add support for hardware-bound keys

[Jarkko Sakkinen]

Further remarks.

On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> DCP is capable of performing AES with two hardware-bound keys:
>
> - The one-time programmable (OTP) key which is burnt via on-chip fuses
> - The unique key (UK) which is derived from the OTP key
>
> In addition to the two hardware-bound keys, DCP also supports
> storing keys in 4 dedicated key slots within its secure memory area
> (internal SRAM).
>
> These keys are not stored in main memory and are therefore
> not directly accessible by the operating system. To use them
> for AES operations, a one-byte key reference has to supplied
> with the DCP operation descriptor in the control register.
>
> This adds support for using any of these 6 keys through the crypto API
> via their key reference after they have been set up. The main purpose

Please write actions always in imperative form. E.g. instead of "This
adds" you could just as well simply write "Add", right?

Also, "adding support" is somewhat abstract expression tbh. You should
rather point out the driver exactly you are modifying (completely
missing BTW) and what sort of new functionalities this mysetery word
"support" maps into.

More cocrete and dumbed you can ever be, the better is the commit
message and more likely we also get the code changes you are doing.

> is to add support for DCP-backed trusted keys. Other use cases are
> possible too (see similar existing paes implementations), but these
> should carefully be evaluated as e.g. enabling AF_ALG will give
> userspace full access to use keys. In scenarios with untrustworthy
> userspace, this will enable en-/decryption oracles.
>
> Co-developed-by: Richard Weinberger 
> Signed-off-by: Richard Weinberger 
> Co-developed-by: David Oberhollenzer 
> Signed-off-by: David Oberhollenzer 
> Signed-off-by: David Gstir 
> Acked-by: Herbert Xu 
> ---
>  drivers/crypto/mxs-dcp.c | 104 ++-
>  include/soc/fsl/dcp.h|  17 +++
>  2 files changed, 110 insertions(+), 11 deletions(-)
>  create mode 100644 include/soc/fsl/dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index f6b7bce0e656..2dc664fb2faf 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
>   struct crypto_skcipher  *fallback;
>   unsigned intkey_len;
>   uint8_t key[AES_KEYSIZE_128];
> + boolkey_referenced;
>  };
>  
>  struct dcp_aes_req_ctx {
> @@ -155,6 +157,7 @@ static struct dcp *global_sdcp;
>  #define MXS_DCP_CONTROL0_HASH_TERM   (1 << 13)
>  #define MXS_DCP_CONTROL0_HASH_INIT   (1 << 12)
>  #define MXS_DCP_CONTROL0_PAYLOAD_KEY (1 << 11)
> +#define MXS_DCP_CONTROL0_OTP_KEY (1 << 10)
>  #define MXS_DCP_CONTROL0_CIPHER_ENCRYPT  (1 << 8)
>  #define MXS_DCP_CONTROL0_CIPHER_INIT (1 << 9)
>  #define MXS_DCP_CONTROL0_ENABLE_HASH (1 << 6)
> @@ -168,6 +171,8 @@ static struct dcp *global_sdcp;
>  #define MXS_DCP_CONTROL1_CIPHER_MODE_ECB (0 << 4)
>  #define MXS_DCP_CONTROL1_CIPHER_SELECT_AES128(0 << 0)
>  
> +#define MXS_DCP_CONTROL1_KEY_SELECT_SHIFT8
> +
>  static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
>  {
>   int dma_err;
> @@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>   struct dcp *sdcp = global_sdcp;
>   struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan];
>   struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + bool key_referenced = actx->key_referenced;
>   int ret;
>  
> - key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> -   2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> - ret = dma_mapping_error(sdcp->dev, key_phys);
> - if (ret)
> - return ret;
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> +   2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + ret = dma_mapping_error(sdcp->dev, key_phys);
> + if (ret)
> + return ret;
> + }
>  
>   src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> DCP_BUF_SZ, DMA_TO_DEVICE);
> @@ -255,8 +263,12 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>   MXS_DCP_CONTROL0_INTERRUPT |
>   MXS_DCP_CONTROL0_ENABLE_CIPHER;
>  
> - /* Payload contains the key. */
> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + if (key_referenced)
> + /* Set OTP key bit to select the key via KEY_SELECT. */
> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
> + else
> + /* Payload contains the key. */
> + desc->control0 |= MXS_DCP_CONTROL0_P

Re: [PATCH v5 1/6] crypto: mxs-dcp: Add support for hardware-bound keys

[Jarkko Sakkinen]

On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> DCP is capable of performing AES with two hardware-bound keys:
>
> - The one-time programmable (OTP) key which is burnt via on-chip fuses
> - The unique key (UK) which is derived from the OTP key

This is somewhat cryptic explanation for the commmon and reoccuring
theme of having a fused random seed and a key derivation function.

I'd just write it is all about.

"DCP is able to derive private keys for a fused random seed, which
can be referenced by handle but not accessed by the CPU. These keys
can be used to perform AES encryption."

My explanation neither includes acronyms OTP and UK and still
delivers the message so much better. That actually further makes
it better because less crappy standard consortium terminology is
always better :-)

BR, Jarkko

Re: [PATCH v5 0/6] DCP as trusted keys backend

[Jarkko Sakkinen]

On Mon Feb 26, 2024 at 12:20 AM EET, Richard Weinberger wrote:
> Mimi, James, Jarkko, David,
>
> you remained silent for a whole release cycle.
> Is there anything we can do to get this forward?
>
> Thanks,
> //richard

Thanks for reminding.

>From my side, I've had pretty busy month as I've adapted to a new work
project: https://sochub.fi/

I exported the thread [1] and will look into it within this or next week
in detail (thus the large'ish time window).

[1] 
https://lore.kernel.org/linux-integrity/1733761.uacIGzncQW@somecomputer/t.mbox.gz

BR, Jarkko

Re: [PATCH v5 0/6] DCP as trusted keys backend

[Richard Weinberger]

Mimi, James, Jarkko, David,

you remained silent for a whole release cycle.
Is there anything we can do to get this forward?

Thanks,
//richard

Am Dienstag, 13. Februar 2024, 10:59:56 CET schrieb Richard Weinberger:
> Am Montag, 5. Februar 2024, 09:39:07 CET schrieb David Gstir:
> > Hi,
> > 
> > > On 15.12.2023, at 12:06, David Gstir  wrote:
> > > 
> > > This is a revival of the previous patch set submitted by Richard 
> > > Weinberger:
> > > https://lore.kernel.org/linux-integrity/20210614201620.30451-1-rich...@nod.at/
> > > 
> > > v4 is here:
> > > https://lore.kernel.org/keyrings/20231024162024.51260-1-da...@sigma-star.at/
> > > 
> > > v4 -> v5:
> > > - Make Kconfig for trust source check scalable as suggested by Jarkko 
> > > Sakkinen
> > > - Add Acked-By from Herbert Xu to patch #1 - thanks!
> > > v3 -> v4:
> > > - Split changes on MAINTAINERS and documentation into dedicated patches
> > > - Use more concise wording in commit messages as suggested by Jarkko 
> > > Sakkinen
> > > v2 -> v3:
> > > - Addressed review comments from Jarkko Sakkinen
> > > v1 -> v2:
> > > - Revive and rebase to latest version
> > > - Include review comments from Ahmad Fatoum
> > > 
> > > The Data CoProcessor (DCP) is an IP core built into many NXP SoCs such
> > > as i.mx6ull.
> > > 
> > > Similar to the CAAM engine used in more powerful SoCs, DCP can AES-
> > > encrypt/decrypt user data using a unique, never-disclosed,
> > > device-specific key. Unlike CAAM though, it cannot directly wrap and
> > > unwrap blobs in hardware. As DCP offers only the bare minimum feature
> > > set and a blob mechanism needs aid from software. A blob in this case
> > > is a piece of sensitive data (e.g. a key) that is encrypted and
> > > authenticated using the device-specific key so that unwrapping can only
> > > be done on the hardware where the blob was wrapped.
> > > 
> > > This patch series adds a DCP based, trusted-key backend and is similar
> > > in spirit to the one by Ahmad Fatoum [0] that does the same for CAAM.
> > > It is of interest for similar use cases as the CAAM patch set, but for
> > > lower end devices, where CAAM is not available.
> > > 
> > > Because constructing and parsing the blob has to happen in software,
> > > we needed to decide on a blob format and chose the following:
> > > 
> > > struct dcp_blob_fmt {
> > > __u8 fmt_version;
> > > __u8 blob_key[AES_KEYSIZE_128];
> > > __u8 nonce[AES_KEYSIZE_128];
> > > __le32 payload_len;
> > > __u8 payload[];
> > > } __packed;
> > > 
> > > The `fmt_version` is currently 1.
> > > 
> > > The encrypted key is stored in the payload area. It is AES-128-GCM
> > > encrypted using `blob_key` and `nonce`, GCM auth tag is attached at
> > > the end of the payload (`payload_len` does not include the size of
> > > the auth tag).
> > > 
> > > The `blob_key` itself is encrypted in AES-128-ECB mode by DCP using
> > > the OTP or UNIQUE device key. A new `blob_key` and `nonce` are generated
> > > randomly, when sealing/exporting the DCP blob.
> > > 
> > > This patchset was tested with dm-crypt on an i.MX6ULL board.
> > > 
> > > [0] 
> > > https://lore.kernel.org/keyrings/20220513145705.2080323-1-a.fat...@pengutronix.de/
> > > 
> > > David Gstir (6):
> > >  crypto: mxs-dcp: Add support for hardware-bound keys
> > >  KEYS: trusted: improve scalability of trust source config
> > >  KEYS: trusted: Introduce NXP DCP-backed trusted keys
> > >  MAINTAINERS: add entry for DCP-based trusted keys
> > >  docs: document DCP-backed trusted keys kernel params
> > >  docs: trusted-encrypted: add DCP as new trust source
> > > 
> > > .../admin-guide/kernel-parameters.txt |  13 +
> > > .../security/keys/trusted-encrypted.rst   |  85 +
> > > MAINTAINERS   |   9 +
> > > drivers/crypto/mxs-dcp.c  | 104 +-
> > > include/keys/trusted_dcp.h|  11 +
> > > include/soc/fsl/dcp.h |  17 +
> > > security/keys/trusted-keys/Kconfig|  18 +-
> > > security/keys/trusted-keys/Makefile   |   2 +
> > > security/keys/trusted-keys/trusted_core.c |   6 +-
> > > security/keys/trusted-keys/trusted_dcp.c  | 311 ++
> > > 10 files changed, 562 insertions(+), 14 deletions(-)
> > > create mode 100644 include/keys/trusted_dcp.h
> > > create mode 100644 include/soc/fsl/dcp.h
> > > create mode 100644 security/keys/trusted-keys/trusted_dcp.c
> > 
> > Jarkko, Mimi, David do you need anything from my side for these patches to 
> > get them merged?
> 
> Friendly ping also from my side. :-)
> 
> Thanks,
> //richard
> 
> -- 
> ​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
> UID/VAT Nr: ATU 66964118 | FN: 374287y
> 


-- 
​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: virtcrypto_dataq_callback calls crypto_finalize_request() from irq context

[Michael S. Tsirkin]

On Thu, Nov 02, 2023 at 01:25:31PM +, Gonglei (Arei) wrote:
> 
> 
> > -Original Message-
> > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > Sent: Thursday, November 2, 2023 9:17 PM
> > To: Gonglei (Arei) 
> > Cc: Halil Pasic ; Herbert Xu
> > ; Jason Wang ;
> > virtualizat...@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
> > linux-crypto@vger.kernel.org; Marc Hartmayer 
> > Subject: Re: virtcrypto_dataq_callback calls crypto_finalize_request() from 
> > irq
> > context
> > 
> > On Thu, Nov 02, 2023 at 01:04:07PM +, Gonglei (Arei) wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > > Sent: Wednesday, November 1, 2023 9:26 PM
> > > > To: Halil Pasic 
> > > > Cc: Gonglei (Arei) ; Herbert Xu
> > > > ; Jason Wang ;
> > > > virtualizat...@lists.linux-foundation.org;
> > > > linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org; Marc
> > > > Hartmayer 
> > > > Subject: Re: virtcrypto_dataq_callback calls
> > > > crypto_finalize_request() from irq context
> > > >
> > > > On Sun, Sep 24, 2023 at 07:39:41PM +0200, Halil Pasic wrote:
> > > > > On Sun, 24 Sep 2023 11:56:25 + "Gonglei (Arei)"
> > > > >  wrote:
> > > > >
> > > > > > Hi Halil,
> > > > > >
> > > > > > Commit 4058cf08945 introduced a check for detecting crypto
> > > > > > completion function called with enable BH, and indeed the
> > > > > > virtio-crypto driver didn't disable BH, which needs a patch to fix 
> > > > > > it.
> > > > > >
> > > > > > P.S.:
> > > > > > https://lore.kernel.org/lkml/20220221120833.2618733-5-clabbe@bay
> > > > > > libr
> > > > > > e.com/T/
> > > > > >
> > > > > > Regards,
> > > > > > -Gonglei
> > > > >
> > > > > Thanks Gonglei!
> > > > >
> > > > > Thanks! I would be glad to test that fix on s390x. Are you about
> > > > > to send one?
> > > > >
> > > > > Regards,
> > > > > Halil
> > > >
> > > >
> > > > Gonglei did you intend to send a fix?
> > >
> > > Actually I sent a patch a month ago, pls see another thread.
> > >
> > >
> > > Regards,
> > > -Gonglei
> > 
> > And I think there was an issue with that patch that you wanted to fix?
> > config changed callback got fixed but this still didn't.
> > 
> Now my concern is whether or not the judgement (commit 4058cf08945c1) is 
> reasonable.
> 
> Regards,
> -Gonglei

So what is the plan to deal with the issue?

-- 
MST

Re: [PATCH 2/2] arm64: dts: qcom: sm6350: Add Crypto Engine

[Luca Weiss]

On Fri Feb 16, 2024 at 7:09 PM CET, Bjorn Andersson wrote:
> On Fri, Feb 16, 2024 at 11:46:49AM +0100, Luca Weiss wrote:
> > On Fri Jan 5, 2024 at 5:30 PM CET, Stephan Gerhold wrote:
> > > On Fri, Jan 05, 2024 at 05:15:44PM +0100, Luca Weiss wrote:
> > > > Add crypto engine (CE) and CE BAM related nodes and definitions for this
> > > > SoC.
> > > > 
> > > > For reference:
> > > > 
> > > >   [2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 
> > > > 5.5.1
> > > > 
> > > > Signed-off-by: Luca Weiss 
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 
> > > > +++
> > > >  1 file changed, 31 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi 
> > > > b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > > index 8fd6f4d03490..516aadbb16bb 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > > @@ -1212,6 +1212,37 @@ ufs_mem_phy_lanes: phy@1d87400 {
> > > > };
> > > > };
> > > >  
> > > > +   cryptobam: dma-controller@1dc4000 {
> > > > +   compatible = "qcom,bam-v1.7.4", 
> > > > "qcom,bam-v1.7.0";
> > > > +   reg = <0 0x01dc4000 0 0x24000>;
> > > > +   interrupts = ;
> > > > +   #dma-cells = <1>;
> > > > +   qcom,ee = <0>;
> > > > +   qcom,controlled-remotely;
> > > > +   num-channels = <16>;
> > > > +   qcom,num-ees = <4>;
> > > > +   iommus = <&apps_smmu 0x432 0x>,
> > > > +<&apps_smmu 0x438 0x0001>,
> > > > +<&apps_smmu 0x43f 0x>,
> > > > +<&apps_smmu 0x426 0x0011>,
> > > > +<&apps_smmu 0x436 0x0011>;
> > >
> > > The last two lines look equivalent to me: 0x436 & ~0x0011 = 0x426.
> > 
> > I don't understand the IOMMU SID + mask really, but I think I've seen
> > somewhere before like here that TZ can be a bit picky with the SIDs?
> > 
> > https://lore.kernel.org/linux-arm-msm/opqdrmyj3y64nqqqmakjydn5rkspizufyeavm7ec7c7ufqz4wk@ey2a7bq3shfj/
> > https://lore.kernel.org/linux-arm-msm/11b5db69-49f5-4d7b-81c9-687d66a5c...@linaro.org/
> > 
> > I don't quite want to risk having some obscure use case breaking because
> > we cleaned up the dts ;)
> > 
> > But if you're more sure than me that it won't break, let me know!
> > 
> > >
> > > It's also a bit weird that the mask has one more digit than the stream
> > > ID. And ordered numerically (by stream ID, first number) it would be a
> > > bit easier to read. :-)
> > 
> > Sorting them is no problem, can do that for v2.
> > 
>
> Where you able to do this? I don't see a v2 in my inbox, am I just
> searching poorly?

Only sent v2 some minutes ago, didn't have any more time on Friday.

Regards
Luca

>
> Regards,
> Bjorn
>
> > >
> > > Thanks,
> > > Stephan
> > 

Re: [PATCH 2/2] arm64: dts: qcom: sm6350: Add Crypto Engine

[Bjorn Andersson]

On Fri, Feb 16, 2024 at 11:46:49AM +0100, Luca Weiss wrote:
> On Fri Jan 5, 2024 at 5:30 PM CET, Stephan Gerhold wrote:
> > On Fri, Jan 05, 2024 at 05:15:44PM +0100, Luca Weiss wrote:
> > > Add crypto engine (CE) and CE BAM related nodes and definitions for this
> > > SoC.
> > > 
> > > For reference:
> > > 
> > >   [2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 
> > > 5.5.1
> > > 
> > > Signed-off-by: Luca Weiss 
> > > ---
> > >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 +++
> > >  1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi 
> > > b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > index 8fd6f4d03490..516aadbb16bb 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > @@ -1212,6 +1212,37 @@ ufs_mem_phy_lanes: phy@1d87400 {
> > >   };
> > >   };
> > >  
> > > + cryptobam: dma-controller@1dc4000 {
> > > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
> > > + reg = <0 0x01dc4000 0 0x24000>;
> > > + interrupts = ;
> > > + #dma-cells = <1>;
> > > + qcom,ee = <0>;
> > > + qcom,controlled-remotely;
> > > + num-channels = <16>;
> > > + qcom,num-ees = <4>;
> > > + iommus = <&apps_smmu 0x432 0x>,
> > > +  <&apps_smmu 0x438 0x0001>,
> > > +  <&apps_smmu 0x43f 0x>,
> > > +  <&apps_smmu 0x426 0x0011>,
> > > +  <&apps_smmu 0x436 0x0011>;
> >
> > The last two lines look equivalent to me: 0x436 & ~0x0011 = 0x426.
> 
> I don't understand the IOMMU SID + mask really, but I think I've seen
> somewhere before like here that TZ can be a bit picky with the SIDs?
> 
> https://lore.kernel.org/linux-arm-msm/opqdrmyj3y64nqqqmakjydn5rkspizufyeavm7ec7c7ufqz4wk@ey2a7bq3shfj/
> https://lore.kernel.org/linux-arm-msm/11b5db69-49f5-4d7b-81c9-687d66a5c...@linaro.org/
> 
> I don't quite want to risk having some obscure use case breaking because
> we cleaned up the dts ;)
> 
> But if you're more sure than me that it won't break, let me know!
> 
> >
> > It's also a bit weird that the mask has one more digit than the stream
> > ID. And ordered numerically (by stream ID, first number) it would be a
> > bit easier to read. :-)
> 
> Sorting them is no problem, can do that for v2.
> 

Where you able to do this? I don't see a v2 in my inbox, am I just
searching poorly?

Regards,
Bjorn

> >
> > Thanks,
> > Stephan
> 

Re: [PATCH 2/2] arm64: dts: qcom: sm6350: Add Crypto Engine

[Stephan Gerhold]

On Fri, Feb 16, 2024 at 11:46:49AM +0100, Luca Weiss wrote:
> On Fri Jan 5, 2024 at 5:30 PM CET, Stephan Gerhold wrote:
> > On Fri, Jan 05, 2024 at 05:15:44PM +0100, Luca Weiss wrote:
> > > Add crypto engine (CE) and CE BAM related nodes and definitions for this
> > > SoC.
> > > 
> > > For reference:
> > > 
> > >   [2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 
> > > 5.5.1
> > > 
> > > Signed-off-by: Luca Weiss 
> > > ---
> > >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 +++
> > >  1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi 
> > > b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > index 8fd6f4d03490..516aadbb16bb 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > @@ -1212,6 +1212,37 @@ ufs_mem_phy_lanes: phy@1d87400 {
> > >   };
> > >   };
> > >  
> > > + cryptobam: dma-controller@1dc4000 {
> > > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
> > > + reg = <0 0x01dc4000 0 0x24000>;
> > > + interrupts = ;
> > > + #dma-cells = <1>;
> > > + qcom,ee = <0>;
> > > + qcom,controlled-remotely;
> > > + num-channels = <16>;
> > > + qcom,num-ees = <4>;
> > > + iommus = <&apps_smmu 0x432 0x>,
> > > +  <&apps_smmu 0x438 0x0001>,
> > > +  <&apps_smmu 0x43f 0x>,
> > > +  <&apps_smmu 0x426 0x0011>,
> > > +  <&apps_smmu 0x436 0x0011>;
> >
> > The last two lines look equivalent to me: 0x436 & ~0x0011 = 0x426.
> 
> I don't understand the IOMMU SID + mask really, but I think I've seen
> somewhere before like here that TZ can be a bit picky with the SIDs?
> 
> https://lore.kernel.org/linux-arm-msm/opqdrmyj3y64nqqqmakjydn5rkspizufyeavm7ec7c7ufqz4wk@ey2a7bq3shfj/
> https://lore.kernel.org/linux-arm-msm/11b5db69-49f5-4d7b-81c9-687d66a5c...@linaro.org/
> 
> I don't quite want to risk having some obscure use case breaking because
> we cleaned up the dts ;)
> 
> But if you're more sure than me that it won't break, let me know!
> 

I'm afraid I can't really help with this kind of certainty. My knowledge
about proprietary Qualcomm firmware is probably even more limited than
yours. However, my personal feeling is that the "TZ wants X" arguments
are most often just as badly based on superficial knowledge.

In simplified terms, the SMMU has a number of components connected to it
(the crypto BAM, USB controller, UFS, ...). When the components make
memory requests they are identified by a number of Stream IDs (SIDs).
The purpose of "iommus" in the device tree is to describe all SIDs that
belong to a particular device. These SIDs are then all assigned to a
context bank that allows the device to access selected regions in RAM.

It shouldn't matter *how* the SIDs are matched inside the SMMU, as long
as they end up at the correct context bank. The SMMU will look through
the configured Stream Match Registers (SMRs = SID + Mask) to find the
context bank that is assigned to the SID. The docs say "If MASK[i]==1,
ID[i] is ignored.". This means a SMR with ID=0x426 MASK=0x0011 is
definitely identical to ID=0x436 MASK=0x0011. Having the extra entry
will make absolutely no difference to the SMMU aside from wasting a
pointless SMR.

The links you posted suggest "TZ" looks at the SMRs allocated by Linux.
If that is really the case then I would expect that to be fundamentally
broken. In my opinion there is absolutely no guarantee how or in which
order Linux allocates the SMRs. Such functionality would either be
extremely complex or broken in tons of edge cases.

TL;DR: I cannot provide proof that removing this entry makes a
difference. I can just say that I doubt it does, and if it does, then we
have far more serious problems. The device tree is supposed to describe
the hardware ("This device makes memory requests with the following
SIDs") and not fundamentally broken peculiarities of the proprietary TZ
firmware ("registers must be programmed exactly with these values").

Thanks,
Stephan

Re: [PATCH 2/2] arm64: dts: qcom: sm6350: Add Crypto Engine

[Luca Weiss]

On Fri Jan 5, 2024 at 5:30 PM CET, Stephan Gerhold wrote:
> On Fri, Jan 05, 2024 at 05:15:44PM +0100, Luca Weiss wrote:
> > Add crypto engine (CE) and CE BAM related nodes and definitions for this
> > SoC.
> > 
> > For reference:
> > 
> >   [2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 5.5.1
> > 
> > Signed-off-by: Luca Weiss 
> > ---
> >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 +++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi 
> > b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > index 8fd6f4d03490..516aadbb16bb 100644
> > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > @@ -1212,6 +1212,37 @@ ufs_mem_phy_lanes: phy@1d87400 {
> > };
> > };
> >  
> > +   cryptobam: dma-controller@1dc4000 {
> > +   compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
> > +   reg = <0 0x01dc4000 0 0x24000>;
> > +   interrupts = ;
> > +   #dma-cells = <1>;
> > +   qcom,ee = <0>;
> > +   qcom,controlled-remotely;
> > +   num-channels = <16>;
> > +   qcom,num-ees = <4>;
> > +   iommus = <&apps_smmu 0x432 0x>,
> > +<&apps_smmu 0x438 0x0001>,
> > +<&apps_smmu 0x43f 0x>,
> > +<&apps_smmu 0x426 0x0011>,
> > +<&apps_smmu 0x436 0x0011>;
>
> The last two lines look equivalent to me: 0x436 & ~0x0011 = 0x426.

I don't understand the IOMMU SID + mask really, but I think I've seen
somewhere before like here that TZ can be a bit picky with the SIDs?

https://lore.kernel.org/linux-arm-msm/opqdrmyj3y64nqqqmakjydn5rkspizufyeavm7ec7c7ufqz4wk@ey2a7bq3shfj/
https://lore.kernel.org/linux-arm-msm/11b5db69-49f5-4d7b-81c9-687d66a5c...@linaro.org/

I don't quite want to risk having some obscure use case breaking because
we cleaned up the dts ;)

But if you're more sure than me that it won't break, let me know!

>
> It's also a bit weird that the mask has one more digit than the stream
> ID. And ordered numerically (by stream ID, first number) it would be a
> bit easier to read. :-)

Sorting them is no problem, can do that for v2.

>
> Thanks,
> Stephan

Re: [PATCH v5 0/6] DCP as trusted keys backend

[Richard Weinberger]

Am Montag, 5. Februar 2024, 09:39:07 CET schrieb David Gstir:
> Hi,
> 
> > On 15.12.2023, at 12:06, David Gstir  wrote:
> > 
> > This is a revival of the previous patch set submitted by Richard Weinberger:
> > https://lore.kernel.org/linux-integrity/20210614201620.30451-1-rich...@nod.at/
> > 
> > v4 is here:
> > https://lore.kernel.org/keyrings/20231024162024.51260-1-da...@sigma-star.at/
> > 
> > v4 -> v5:
> > - Make Kconfig for trust source check scalable as suggested by Jarkko 
> > Sakkinen
> > - Add Acked-By from Herbert Xu to patch #1 - thanks!
> > v3 -> v4:
> > - Split changes on MAINTAINERS and documentation into dedicated patches
> > - Use more concise wording in commit messages as suggested by Jarkko 
> > Sakkinen
> > v2 -> v3:
> > - Addressed review comments from Jarkko Sakkinen
> > v1 -> v2:
> > - Revive and rebase to latest version
> > - Include review comments from Ahmad Fatoum
> > 
> > The Data CoProcessor (DCP) is an IP core built into many NXP SoCs such
> > as i.mx6ull.
> > 
> > Similar to the CAAM engine used in more powerful SoCs, DCP can AES-
> > encrypt/decrypt user data using a unique, never-disclosed,
> > device-specific key. Unlike CAAM though, it cannot directly wrap and
> > unwrap blobs in hardware. As DCP offers only the bare minimum feature
> > set and a blob mechanism needs aid from software. A blob in this case
> > is a piece of sensitive data (e.g. a key) that is encrypted and
> > authenticated using the device-specific key so that unwrapping can only
> > be done on the hardware where the blob was wrapped.
> > 
> > This patch series adds a DCP based, trusted-key backend and is similar
> > in spirit to the one by Ahmad Fatoum [0] that does the same for CAAM.
> > It is of interest for similar use cases as the CAAM patch set, but for
> > lower end devices, where CAAM is not available.
> > 
> > Because constructing and parsing the blob has to happen in software,
> > we needed to decide on a blob format and chose the following:
> > 
> > struct dcp_blob_fmt {
> > __u8 fmt_version;
> > __u8 blob_key[AES_KEYSIZE_128];
> > __u8 nonce[AES_KEYSIZE_128];
> > __le32 payload_len;
> > __u8 payload[];
> > } __packed;
> > 
> > The `fmt_version` is currently 1.
> > 
> > The encrypted key is stored in the payload area. It is AES-128-GCM
> > encrypted using `blob_key` and `nonce`, GCM auth tag is attached at
> > the end of the payload (`payload_len` does not include the size of
> > the auth tag).
> > 
> > The `blob_key` itself is encrypted in AES-128-ECB mode by DCP using
> > the OTP or UNIQUE device key. A new `blob_key` and `nonce` are generated
> > randomly, when sealing/exporting the DCP blob.
> > 
> > This patchset was tested with dm-crypt on an i.MX6ULL board.
> > 
> > [0] 
> > https://lore.kernel.org/keyrings/20220513145705.2080323-1-a.fat...@pengutronix.de/
> > 
> > David Gstir (6):
> >  crypto: mxs-dcp: Add support for hardware-bound keys
> >  KEYS: trusted: improve scalability of trust source config
> >  KEYS: trusted: Introduce NXP DCP-backed trusted keys
> >  MAINTAINERS: add entry for DCP-based trusted keys
> >  docs: document DCP-backed trusted keys kernel params
> >  docs: trusted-encrypted: add DCP as new trust source
> > 
> > .../admin-guide/kernel-parameters.txt |  13 +
> > .../security/keys/trusted-encrypted.rst   |  85 +
> > MAINTAINERS   |   9 +
> > drivers/crypto/mxs-dcp.c  | 104 +-
> > include/keys/trusted_dcp.h|  11 +
> > include/soc/fsl/dcp.h |  17 +
> > security/keys/trusted-keys/Kconfig|  18 +-
> > security/keys/trusted-keys/Makefile   |   2 +
> > security/keys/trusted-keys/trusted_core.c |   6 +-
> > security/keys/trusted-keys/trusted_dcp.c  | 311 ++
> > 10 files changed, 562 insertions(+), 14 deletions(-)
> > create mode 100644 include/keys/trusted_dcp.h
> > create mode 100644 include/soc/fsl/dcp.h
> > create mode 100644 security/keys/trusted-keys/trusted_dcp.c
> 
> Jarkko, Mimi, David do you need anything from my side for these patches to 
> get them merged?

Friendly ping also from my side. :-)

Thanks,
//richard

-- 
​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: [PATCH] crypto: virtio/akcipher - Fix stack overflow on memcpy

[Herbert Xu]

On Tue, Jan 30, 2024 at 07:27:40PM +0800, zhenwei pi wrote:
> sizeof(struct virtio_crypto_akcipher_session_para) is less than
> sizeof(struct virtio_crypto_op_ctrl_req::u), copying more bytes from
> stack variable leads stack overflow. Clang reports this issue by
> commands:
> make -j CC=clang-14 mrproper >/dev/null 2>&1
> make -j O=/tmp/crypto-build CC=clang-14 allmodconfig >/dev/null 2>&1
> make -j O=/tmp/crypto-build W=1 CC=clang-14 drivers/crypto/virtio/
>   virtio_crypto_akcipher_algs.o
> 
> Fixes: 59ca6c93387d ("virtio-crypto: implement RSA algorithm")
> Link: 
> https://lore.kernel.org/all/0a194a79-e3a3-45e7-be98-83abd3e1c...@roeck-us.net/
> Signed-off-by: zhenwei pi 
> ---
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: qat - use kcalloc() instead of kzalloc()

[Cabiddu, Giovanni]

On Mon, Jan 29, 2024 at 06:57:28AM -0600, Lenko Donchev wrote:
> We are trying to get rid of all multiplications from allocation
> functions to prevent integer overflows[1]. Here the multiplication is
> obviously safe, but using kcalloc() is more appropriate and improves
> readability. This patch has no effect on runtime behavior.
> 
> Link: https://github.com/KSPP/linux/issues/162 [1]
> Link: 
> https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>  [2]
> 
> Signed-off-by: Lenko Donchev 
Thanks for your patch.
The same change was already applied to the cryptodev tree about two
weeks ago. See 4da3bc65d218 ("crypto: qat - use kcalloc_node() instead of
kzalloc_node()") [1].

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=4da3bc65d218605557696109e42cfeee666d601f

Regards,

-- 
Giovanni

Re: [PATCH v5 0/6] DCP as trusted keys backend

[David Gstir]

Hi,

> On 15.12.2023, at 12:06, David Gstir  wrote:
> 
> This is a revival of the previous patch set submitted by Richard Weinberger:
> https://lore.kernel.org/linux-integrity/20210614201620.30451-1-rich...@nod.at/
> 
> v4 is here:
> https://lore.kernel.org/keyrings/20231024162024.51260-1-da...@sigma-star.at/
> 
> v4 -> v5:
> - Make Kconfig for trust source check scalable as suggested by Jarkko Sakkinen
> - Add Acked-By from Herbert Xu to patch #1 - thanks!
> v3 -> v4:
> - Split changes on MAINTAINERS and documentation into dedicated patches
> - Use more concise wording in commit messages as suggested by Jarkko Sakkinen
> v2 -> v3:
> - Addressed review comments from Jarkko Sakkinen
> v1 -> v2:
> - Revive and rebase to latest version
> - Include review comments from Ahmad Fatoum
> 
> The Data CoProcessor (DCP) is an IP core built into many NXP SoCs such
> as i.mx6ull.
> 
> Similar to the CAAM engine used in more powerful SoCs, DCP can AES-
> encrypt/decrypt user data using a unique, never-disclosed,
> device-specific key. Unlike CAAM though, it cannot directly wrap and
> unwrap blobs in hardware. As DCP offers only the bare minimum feature
> set and a blob mechanism needs aid from software. A blob in this case
> is a piece of sensitive data (e.g. a key) that is encrypted and
> authenticated using the device-specific key so that unwrapping can only
> be done on the hardware where the blob was wrapped.
> 
> This patch series adds a DCP based, trusted-key backend and is similar
> in spirit to the one by Ahmad Fatoum [0] that does the same for CAAM.
> It is of interest for similar use cases as the CAAM patch set, but for
> lower end devices, where CAAM is not available.
> 
> Because constructing and parsing the blob has to happen in software,
> we needed to decide on a blob format and chose the following:
> 
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
> 
> The `fmt_version` is currently 1.
> 
> The encrypted key is stored in the payload area. It is AES-128-GCM
> encrypted using `blob_key` and `nonce`, GCM auth tag is attached at
> the end of the payload (`payload_len` does not include the size of
> the auth tag).
> 
> The `blob_key` itself is encrypted in AES-128-ECB mode by DCP using
> the OTP or UNIQUE device key. A new `blob_key` and `nonce` are generated
> randomly, when sealing/exporting the DCP blob.
> 
> This patchset was tested with dm-crypt on an i.MX6ULL board.
> 
> [0] 
> https://lore.kernel.org/keyrings/20220513145705.2080323-1-a.fat...@pengutronix.de/
> 
> David Gstir (6):
>  crypto: mxs-dcp: Add support for hardware-bound keys
>  KEYS: trusted: improve scalability of trust source config
>  KEYS: trusted: Introduce NXP DCP-backed trusted keys
>  MAINTAINERS: add entry for DCP-based trusted keys
>  docs: document DCP-backed trusted keys kernel params
>  docs: trusted-encrypted: add DCP as new trust source
> 
> .../admin-guide/kernel-parameters.txt |  13 +
> .../security/keys/trusted-encrypted.rst   |  85 +
> MAINTAINERS   |   9 +
> drivers/crypto/mxs-dcp.c  | 104 +-
> include/keys/trusted_dcp.h|  11 +
> include/soc/fsl/dcp.h |  17 +
> security/keys/trusted-keys/Kconfig|  18 +-
> security/keys/trusted-keys/Makefile   |   2 +
> security/keys/trusted-keys/trusted_core.c |   6 +-
> security/keys/trusted-keys/trusted_dcp.c  | 311 ++
> 10 files changed, 562 insertions(+), 14 deletions(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 include/soc/fsl/dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c

Jarkko, Mimi, David do you need anything from my side for these patches to get 
them merged?

Thanks,
- David

Re: [PATCH v2 2/6] ubsan: Reintroduce signed and unsigned overflow sanitizers

[Miguel Ojeda]

On Fri, Feb 2, 2024 at 1:17 PM Kees Cook  wrote:
>
> Perhaps I should hold off on bringing the unsigned sanitizer back? I was
> hoping to work in parallel with the signed sanitizer, but maybe this
> isn't the right approach?

If you can do anything to keep it in-tree, I think it would be nice so
that others can easily use it to test the tooling and to start to
clean up cases. A per-subsystem opt-in like Marco says could be a way,
and you could perhaps do one very small subsystem or similar to see
how it would look like.

Something that could also help would be to split the cases even
further (say, only overflows and not underflows), but is that a
possibility with the current tooling?

Thanks for working on this, Kees!

Cheers,
Miguel

Re: [PATCH v2 2/6] ubsan: Reintroduce signed and unsigned overflow sanitizers

[Marco Elver]

On Fri, 2 Feb 2024 at 13:17, Kees Cook  wrote:
>
> On Fri, Feb 02, 2024 at 12:01:55PM +0100, Marco Elver wrote:
> > On Fri, 2 Feb 2024 at 11:16, Kees Cook  wrote:
> > > [...]
> > > +config UBSAN_UNSIGNED_WRAP
> > > +   bool "Perform checking for unsigned arithmetic wrap-around"
> > > +   depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> > > +   depends on !X86_32 # avoid excessive stack usage on x86-32/clang
> > > +   depends on !COMPILE_TEST
> > > +   help
> > > + This option enables -fsanitize=unsigned-integer-overflow which 
> > > checks
> > > + for wrap-around of any arithmetic operations with unsigned 
> > > integers. This
> > > + currently causes x86 to fail to boot.
> >
> > My hypothesis is that these options will quickly be enabled by various
> > test and fuzzing setups, to the detriment of kernel developers. While
> > the commit message states that these are for experimentation, I do not
> > think it is at all clear from the Kconfig options.
>
> I can certainly rephrase it more strongly. I would hope that anyone
> enabling the unsigned sanitizer would quickly realize how extremely
> noisy it is.
>
> > Unsigned integer wrap-around is relatively common (it is _not_ UB
> > after all). While I can appreciate that in some cases wrap around is a
> > genuine semantic bug, and that's what we want to find with these
> > changes, ultimately marking all semantically valid wrap arounds to
> > catch the unmarked ones. Given these patterns are so common, and C
> > programmers are used to them, it will take a lot of effort to mark all
> > the intentional cases. But I fear that even if we get to that place,
> > _unmarked_  but semantically valid unsigned wrap around will keep
> > popping up again and again.
>
> I agree -- it's going to be quite a challenge. My short-term goal is to
> see how far the sanitizer itself can get with identifying intentional
> uses. For example, I found two more extremely common code patterns that
> trip it now:
>
> unsigned int i = ...;
> ...
> while (i--) { ... }
>
> This trips the sanitizer at loop exit. :P It seems like churn to
> refactor all of these into "for (; i; i--)". The compiler should be able
> to identify this by looking for later uses of "i", etc.
>
> The other is negative constants: -1UL, -3ULL, etc. These are all over
> the place and very very obviously intentional and should be ignored by
> the compiler.

Yeah, banning technically valid code like this is going to be a very hard sell.

> > What is the long-term vision to minimize the additional churn this may
> > introduce?
>
> My hope is that we can evolve the coverage over time. Solving it all at
> once won't be possible, but I think we can get pretty far with the
> signed overflow sanitizer, which runs relatively cleanly already.
>
> If we can't make meaningful progress in unsigned annotations, I think
> we'll have to work on gaining type-based operator overloading so we can
> grow type-aware arithmetic. That will serve as a much cleaner
> annotation. E.g. introduce jiffie_t, which wraps.
>
> > I think the problem reminds me a little of the data race problem,
> > although I suspect unsigned integer wraparound is much more common
> > than data races (which unlike unsigned wrap around is actually UB) -
> > so chasing all intentional unsigned integer wrap arounds and marking
> > will take even more effort than marking all intentional data races
> > (which we're still slowly, but steadily, making progress towards).
> >
> > At the very least, these options should 'depends on EXPERT' or even
> > 'depends on BROKEN' while the story is still being worked out.
>
> Perhaps I should hold off on bringing the unsigned sanitizer back? I was
> hoping to work in parallel with the signed sanitizer, but maybe this
> isn't the right approach?

I leave that to you - to me any of these options would be ok:

1. Remove completely for now.

2. Make it 'depends on BROKEN' (because I think even 'depends on
EXPERT' won't help avoid the inevitable spam from test robots).

3. Make it a purely opt-in sanitizer: rather than having subsystems
opt out with UBSAN_WRAP_UNSIGNED:=n, do the opposite and say that for
subsystems that want to opt in, they have to specify
UBSAN_WRAP_UNSIGNED:=y to explicitly opt in.

I can see there being value in explicitly marking semantically
intended unsigned integer wrap, and catch unintended cases, so option
#3 seems appealing. At least that way, if a maintainer chooses to opt
in, they are committed to sorting out their code. Hypothetically, if I
was the maintainer of some smaller subsystem and have had wrap around
bugs in the past, I would certainly consider opting in. It feels a lot
nicer than having it forced upon me.

Thanks,
-- Marco

Re: [PATCH v2 2/6] ubsan: Reintroduce signed and unsigned overflow sanitizers

[Kees Cook]

On Fri, Feb 02, 2024 at 12:01:55PM +0100, Marco Elver wrote:
> On Fri, 2 Feb 2024 at 11:16, Kees Cook  wrote:
> > [...]
> > +config UBSAN_UNSIGNED_WRAP
> > +   bool "Perform checking for unsigned arithmetic wrap-around"
> > +   depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> > +   depends on !X86_32 # avoid excessive stack usage on x86-32/clang
> > +   depends on !COMPILE_TEST
> > +   help
> > + This option enables -fsanitize=unsigned-integer-overflow which 
> > checks
> > + for wrap-around of any arithmetic operations with unsigned 
> > integers. This
> > + currently causes x86 to fail to boot.
> 
> My hypothesis is that these options will quickly be enabled by various
> test and fuzzing setups, to the detriment of kernel developers. While
> the commit message states that these are for experimentation, I do not
> think it is at all clear from the Kconfig options.

I can certainly rephrase it more strongly. I would hope that anyone
enabling the unsigned sanitizer would quickly realize how extremely
noisy it is.

> Unsigned integer wrap-around is relatively common (it is _not_ UB
> after all). While I can appreciate that in some cases wrap around is a
> genuine semantic bug, and that's what we want to find with these
> changes, ultimately marking all semantically valid wrap arounds to
> catch the unmarked ones. Given these patterns are so common, and C
> programmers are used to them, it will take a lot of effort to mark all
> the intentional cases. But I fear that even if we get to that place,
> _unmarked_  but semantically valid unsigned wrap around will keep
> popping up again and again.

I agree -- it's going to be quite a challenge. My short-term goal is to
see how far the sanitizer itself can get with identifying intentional
uses. For example, I found two more extremely common code patterns that
trip it now:

unsigned int i = ...;
...
while (i--) { ... }

This trips the sanitizer at loop exit. :P It seems like churn to
refactor all of these into "for (; i; i--)". The compiler should be able
to identify this by looking for later uses of "i", etc.

The other is negative constants: -1UL, -3ULL, etc. These are all over
the place and very very obviously intentional and should be ignored by
the compiler.

> What is the long-term vision to minimize the additional churn this may
> introduce?

My hope is that we can evolve the coverage over time. Solving it all at
once won't be possible, but I think we can get pretty far with the
signed overflow sanitizer, which runs relatively cleanly already.

If we can't make meaningful progress in unsigned annotations, I think
we'll have to work on gaining type-based operator overloading so we can
grow type-aware arithmetic. That will serve as a much cleaner
annotation. E.g. introduce jiffie_t, which wraps.

> I think the problem reminds me a little of the data race problem,
> although I suspect unsigned integer wraparound is much more common
> than data races (which unlike unsigned wrap around is actually UB) -
> so chasing all intentional unsigned integer wrap arounds and marking
> will take even more effort than marking all intentional data races
> (which we're still slowly, but steadily, making progress towards).
> 
> At the very least, these options should 'depends on EXPERT' or even
> 'depends on BROKEN' while the story is still being worked out.

Perhaps I should hold off on bringing the unsigned sanitizer back? I was
hoping to work in parallel with the signed sanitizer, but maybe this
isn't the right approach?

-- 
Kees Cook

Re: [PATCH v2 2/6] ubsan: Reintroduce signed and unsigned overflow sanitizers

[Marco Elver]

On Fri, 2 Feb 2024 at 11:16, Kees Cook  wrote:
>
> Effectively revert commit 6aaa31aeb9cf ("ubsan: remove overflow
> checks"), to allow the kernel to be built with the "overflow"
> sanitizers again. This gives developers a chance to experiment[1][2][3]
> with the instrumentation again, while compilers adjust their sanitizers
> to deal with the impact of -fno-strict-oveflow (i.e. moving from
> "overflow" checking to "wrap-around" checking).
>
> Notably, the naming of the options is adjusted to use the name "WRAP"
> instead of "OVERFLOW". In the strictest sense, arithmetic "overflow"
> happens when a result exceeds the storage of the type, and is considered
> by the C standard and compilers to be undefined behavior for signed
> and pointer types (without -fno-strict-overflow). Unsigned arithmetic
> overflow is defined as always wrapping around.
>
> Because the kernel is built with -fno-strict-overflow, signed and pointer
> arithmetic is defined to always wrap around instead of "overflowing"
> (which could either be elided due to being undefined behavior or would
> wrap around, which led to very weird bugs in the kernel).
>
> So, the config options are added back as CONFIG_UBSAN_SIGNED_WRAP and
> CONFIG_UBSAN_UNSIGNED_WRAP. Since the kernel has several places that
> explicitly depend on wrap-around behavior (e.g. counters, atomics, crypto,
> etc), also introduce the __signed_wrap and __unsigned_wrap function
> attributes for annotating functions where wrapping is expected and should
> not be instrumented. This will allow us to distinguish in the kernel
> between intentional and unintentional cases of arithmetic wrap-around.
>
> Additionally keep these disabled under CONFIG_COMPILE_TEST for now.
>
> Link: https://github.com/KSPP/linux/issues/26 [1]
> Link: https://github.com/KSPP/linux/issues/27 [2]
> Link: https://github.com/KSPP/linux/issues/344 [3]
> Cc: Justin Stitt 
> Cc: Miguel Ojeda 
> Cc: Nathan Chancellor 
> Cc: Nick Desaulniers 
> Cc: Peter Zijlstra 
> Cc: Marco Elver 
> Cc: Hao Luo 
> Cc: Przemek Kitszel 
> Signed-off-by: Kees Cook 
> ---
>  include/linux/compiler_types.h | 14 ++-
>  lib/Kconfig.ubsan  | 19 ++
>  lib/test_ubsan.c   | 49 
>  lib/ubsan.c| 68 ++
>  lib/ubsan.h|  4 ++
>  scripts/Makefile.ubsan |  2 +
>  6 files changed, 155 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6f1ca49306d2..e585614f3152 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -282,11 +282,23 @@ struct ftrace_likely_data {
>  #define __no_sanitize_or_inline __always_inline
>  #endif
>
> +/* Allow wrapping arithmetic within an annotated function. */
> +#ifdef CONFIG_UBSAN_SIGNED_WRAP
> +# define __signed_wrap 
> __attribute__((no_sanitize("signed-integer-overflow")))
> +#else
> +# define __signed_wrap
> +#endif
> +#ifdef CONFIG_UBSAN_UNSIGNED_WRAP
> +# define __unsigned_wrap 
> __attribute__((no_sanitize("unsigned-integer-overflow")))
> +#else
> +# define __unsigned_wrap
> +#endif
> +
>  /* Section for code which can't be instrumented at all */
>  #define __noinstr_section(section) \
> noinline notrace __attribute((__section__(section)))\
> __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
> -   __no_sanitize_memory
> +   __no_sanitize_memory __signed_wrap __unsigned_wrap
>
>  #define noinstr __noinstr_section(".noinstr.text")
>
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 59e21bfec188..a7003e5bd2a1 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -116,6 +116,25 @@ config UBSAN_UNREACHABLE
>   This option enables -fsanitize=unreachable which checks for control
>   flow reaching an expected-to-be-unreachable position.
>
> +config UBSAN_SIGNED_WRAP
> +   bool "Perform checking for signed arithmetic wrap-around"
> +   default UBSAN
> +   depends on !COMPILE_TEST
> +   depends on $(cc-option,-fsanitize=signed-integer-overflow)
> +   help
> + This option enables -fsanitize=signed-integer-overflow which checks
> + for wrap-around of any arithmetic operations with signed integers.
> +
> +config UBSAN_UNSIGNED_WRAP
> +   bool "Perform checking for unsigned arithmetic wrap-around"
> +   depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> +   depends on !X86_32 # avoid excessive stack usage on x86-32/clang
> +   depends on !COMPILE_TEST
> +   help
> + This option enables -fsanitize=unsigned-integer-overflow which 
> checks
> + for wrap-around of any arithmetic operations with unsigned 
> integers. This
> + currently causes x86 to fail to boot.

My hypothesis is that these options will quickly be enabled by various
test and fuzzing setups, to the detriment 

Re: [PATCH] crypto: virtio/akcipher - Fix stack overflow on memcpy

[Jason Wang]

On Tue, Jan 30, 2024 at 7:28 PM zhenwei pi  wrote:
>
> sizeof(struct virtio_crypto_akcipher_session_para) is less than
> sizeof(struct virtio_crypto_op_ctrl_req::u), copying more bytes from
> stack variable leads stack overflow. Clang reports this issue by
> commands:
> make -j CC=clang-14 mrproper >/dev/null 2>&1
> make -j O=/tmp/crypto-build CC=clang-14 allmodconfig >/dev/null 2>&1
> make -j O=/tmp/crypto-build W=1 CC=clang-14 drivers/crypto/virtio/
>   virtio_crypto_akcipher_algs.o
>
> Fixes: 59ca6c93387d ("virtio-crypto: implement RSA algorithm")
> Link: 
> https://lore.kernel.org/all/0a194a79-e3a3-45e7-be98-83abd3e1c...@roeck-us.net/
> Signed-off-by: zhenwei pi 

Acked-by: Jason Wang 

Thanks

Re: [PATCH] crypto: virtio/akcipher - Fix stack overflow on memcpy

[Michael S. Tsirkin]

On Tue, Jan 30, 2024 at 07:27:40PM +0800, zhenwei pi wrote:
> sizeof(struct virtio_crypto_akcipher_session_para) is less than
> sizeof(struct virtio_crypto_op_ctrl_req::u), copying more bytes from
> stack variable leads stack overflow. Clang reports this issue by
> commands:
> make -j CC=clang-14 mrproper >/dev/null 2>&1
> make -j O=/tmp/crypto-build CC=clang-14 allmodconfig >/dev/null 2>&1
> make -j O=/tmp/crypto-build W=1 CC=clang-14 drivers/crypto/virtio/
>   virtio_crypto_akcipher_algs.o
> 
> Fixes: 59ca6c93387d ("virtio-crypto: implement RSA algorithm")
> Link: 
> https://lore.kernel.org/all/0a194a79-e3a3-45e7-be98-83abd3e1c...@roeck-us.net/
> Signed-off-by: zhenwei pi 

Cc: sta...@vger.kernel.org
Acked-by: Michael S. Tsirkin 



> ---
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c 
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 2621ff8a9376..de53eddf6796 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -104,7 +104,8 @@ static void virtio_crypto_dataq_akcipher_callback(struct 
> virtio_crypto_request *
>  }
>  
>  static int virtio_crypto_alg_akcipher_init_session(struct 
> virtio_crypto_akcipher_ctx *ctx,
> - struct virtio_crypto_ctrl_header *header, void *para,
> + struct virtio_crypto_ctrl_header *header,
> + struct virtio_crypto_akcipher_session_para *para,
>   const uint8_t *key, unsigned int keylen)
>  {
>   struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
> @@ -128,7 +129,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct 
> virtio_crypto_akcipher
>  
>   ctrl = &vc_ctrl_req->ctrl;
>   memcpy(&ctrl->header, header, sizeof(ctrl->header));
> - memcpy(&ctrl->u, para, sizeof(ctrl->u));
> + memcpy(&ctrl->u.akcipher_create_session.para, para, sizeof(*para));
>   input = &vc_ctrl_req->input;
>   input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>  
> -- 
> 2.34.1

Re: [PATCH] crypto: virtio/akcipher - Fix stack overflow on memcpy

[Nathan Chancellor]

On Tue, Jan 30, 2024 at 07:27:40PM +0800, zhenwei pi wrote:
> sizeof(struct virtio_crypto_akcipher_session_para) is less than
> sizeof(struct virtio_crypto_op_ctrl_req::u), copying more bytes from
> stack variable leads stack overflow. Clang reports this issue by
> commands:
> make -j CC=clang-14 mrproper >/dev/null 2>&1
> make -j O=/tmp/crypto-build CC=clang-14 allmodconfig >/dev/null 2>&1
> make -j O=/tmp/crypto-build W=1 CC=clang-14 drivers/crypto/virtio/
>   virtio_crypto_akcipher_algs.o
> 
> Fixes: 59ca6c93387d ("virtio-crypto: implement RSA algorithm")
> Link: 
> https://lore.kernel.org/all/0a194a79-e3a3-45e7-be98-83abd3e1c...@roeck-us.net/
> Signed-off-by: zhenwei pi 

I can confirm this resolves the warning for me as well.

Tested-by: Nathan Chancellor  # build

I suspect Guenter should have a formal Reported-by tag. Should this be
CC'd for stable@ as well?

Cheers,
Nathan

> ---
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c 
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 2621ff8a9376..de53eddf6796 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -104,7 +104,8 @@ static void virtio_crypto_dataq_akcipher_callback(struct 
> virtio_crypto_request *
>  }
>  
>  static int virtio_crypto_alg_akcipher_init_session(struct 
> virtio_crypto_akcipher_ctx *ctx,
> - struct virtio_crypto_ctrl_header *header, void *para,
> + struct virtio_crypto_ctrl_header *header,
> + struct virtio_crypto_akcipher_session_para *para,
>   const uint8_t *key, unsigned int keylen)
>  {
>   struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
> @@ -128,7 +129,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct 
> virtio_crypto_akcipher
>  
>   ctrl = &vc_ctrl_req->ctrl;
>   memcpy(&ctrl->header, header, sizeof(ctrl->header));
> - memcpy(&ctrl->u, para, sizeof(ctrl->u));
> + memcpy(&ctrl->u.akcipher_create_session.para, para, sizeof(*para));
>   input = &vc_ctrl_req->input;
>   input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>  
> -- 
> 2.34.1
> 

Re: [PATCH] crypto: qat - use kcalloc() instead of kzalloc()

[Gustavo A. R. Silva]

On 1/29/24 06:57, Lenko Donchev wrote:

We are trying to get rid of all multiplications from allocation
functions to prevent integer overflows[1]. Here the multiplication is
obviously safe, but using kcalloc() is more appropriate and improves
readability. This patch has no effect on runtime behavior.

Link: https://github.com/KSPP/linux/issues/162 [1]
Link: 
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
 [2]

Signed-off-by: Lenko Donchev 


Reviewed-by: Gustavo A. R. Silva 

Thanks!
--
Gustavo


---
  drivers/crypto/intel/qat/qat_common/adf_isr.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_isr.c 
b/drivers/crypto/intel/qat/qat_common/adf_isr.c
index 3557a0d6dea2..a13d9885d60f 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_isr.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_isr.c
@@ -272,7 +272,7 @@ static int adf_isr_alloc_msix_vectors_data(struct 
adf_accel_dev *accel_dev)
if (!accel_dev->pf.vf_info)
msix_num_entries += hw_data->num_banks;
  
-	irqs = kzalloc_node(msix_num_entries * sizeof(*irqs),

+   irqs = kcalloc_node(msix_num_entries, sizeof(*irqs),
GFP_KERNEL, dev_to_node(&GET_DEV(accel_dev)));
if (!irqs)
return -ENOMEM;

Re: Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[zhenwei pi]

On 1/29/24 16:46, Michael S. Tsirkin wrote:

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:

On 1/26/24 14:35, Nathan Chancellor wrote:

(slimming up the CC list, I don't think this is too relevant to the
wider stable community)

On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:

On 1/26/24 12:34, Nathan Chancellor wrote:

On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:

On 1/26/24 09:51, Greg Kroah-Hartman wrote:

On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:

On 1/22/24 15:55, Greg Kroah-Hartman wrote:

This is the start of the stable review cycle for the 5.10.209 release.
There are 286 patches in this series, all will be posted as a response
to this one.  If anyone has any issues with these being applied, please
let me know.

Responses should be made by Wed, 24 Jan 2024 23:56:49 +.
Anything received after that time might be too late.


[ ... ]


zhenwei pi 
 virtio-crypto: implement RSA algorithm



Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
It is quite beyond a bug fix. Also, unless I am really missing something,
the series (or at least this patch) was not applied to v5.15.y, so we now
have functionality in v5.10.y which is not in v5.15.y.


See the commit text, it was a dependency of a later fix and documented
as such.

Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
gladly accepted :)



We reverted the entire series from the merge because it results in a build
failure for us.

In file included from 
/home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
In file included from 
/home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
In file included from 
/home/groeck/src/linux-chromeos/include/linux/string.h:293:
/home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: 
call to __read_overflow2_field declared with 'warning' attribute: detected read 
beyond size of field (2nd parameter); maybe use struct_group()? 
[-Werror,-Wattribute-warning]
   __read_overflow2_field(q_size_field, size);


For what it's worth, this is likely self inflicted for chromeos-5.10,
which carries a revert of commit eaafc590053b ("fortify: Explicitly
disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
"fortify: Explicitly disable Clang support""). I don't see the series
that added proper support for clang to fortify in 5.18 that ended with
commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
branch, so this seems somewhat expected.



That explains that ;-). I don't mind if the patches stay in v5.10.y,
we have them reverted anyway.

The revert was a pure process issue, as you may see when looking into
commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
Still, that doesn't explain why the problem exists in 5.18+.


I also see that upstream (starting with 6.1) when trying to build it with clang,
so I guess it is one of those bug-for-bug compatibility things. I really have
no idea what causes it, or why we don't see the problem when building
chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
merging 5.10.209 into it. Making things worse, the problem isn't _always_
seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
I have no idea what triggers the problem.


Have a .config that reproduces it on upstream? I have not personally
seen this warning in my build matrix nor has our continuous-integration
matrix (I don't see it in the warning output at the bottom but that
could have missed something for some reason) in 6.1:



The following command sequence reproduces the problem for me with all stable
branches starting with 5.18.y (plus mainline).

rm -rf /tmp/crypto-build
mkdir /tmp/crypto-build
make -j CC=clang-15 mrproper >/dev/null 2>&1
make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
make -j O=/tmp/crypto-build W=1 CC=clang-15 
drivers/crypto/virtio/virtio_crypto_akcipher_algs.o

I tried clang versions 14, 15, and 16. This is with my home system running
Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
the version is

Ubuntu clang version 15.0.7
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin


Okay interesting, this warning is hidden behind W=1, which our CI does
not test with. Looks like it has been that way since the introduction of
these checks in f68f2ff91512 ("fortify: Detect struct member overflows
in memcpy() at compile-time").



Interestingly the warning is seen in chromeos-5.10, without this patch,
and without W=1. I guess that must have to do with the revert which is
finally biting us.


I think this is a legitimate warning though. It is complaining about the
second memcpy() in virtio_crypto_alg_akcipher_init_session():

memcpy(&ctrl->u, para, sizeof(ctrl->u));

wh

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Michael S. Tsirkin]

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
> On 1/26/24 14:35, Nathan Chancellor wrote:
> > (slimming up the CC list, I don't think this is too relevant to the
> > wider stable community)
> > 
> > On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> > > On 1/26/24 12:34, Nathan Chancellor wrote:
> > > > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > > > This is the start of the stable review cycle for the 5.10.209 
> > > > > > > > release.
> > > > > > > > There are 286 patches in this series, all will be posted as a 
> > > > > > > > response
> > > > > > > > to this one.  If anyone has any issues with these being 
> > > > > > > > applied, please
> > > > > > > > let me know.
> > > > > > > > 
> > > > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +.
> > > > > > > > Anything received after that time might be too late.
> > > > > > > > 
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > zhenwei pi 
> > > > > > > > virtio-crypto: implement RSA algorithm
> > > > > > > > 
> > > > > > > 
> > > > > > > Curious: Why was this (and its subsequent fixes) backported to 
> > > > > > > v5.10.y ?
> > > > > > > It is quite beyond a bug fix. Also, unless I am really missing 
> > > > > > > something,
> > > > > > > the series (or at least this patch) was not applied to v5.15.y, 
> > > > > > > so we now
> > > > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > > > 
> > > > > > See the commit text, it was a dependency of a later fix and 
> > > > > > documented
> > > > > > as such.
> > > > > > 
> > > > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > > > gladly accepted :)
> > > > > > 
> > > > > 
> > > > > We reverted the entire series from the merge because it results in a 
> > > > > build
> > > > > failure for us.
> > > > > 
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: 
> > > > > error: call to __read_overflow2_field declared with 'warning' 
> > > > > attribute: detected read beyond size of field (2nd parameter); maybe 
> > > > > use struct_group()? [-Werror,-Wattribute-warning]
> > > > >   __read_overflow2_field(q_size_field, size);
> > > > 
> > > > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > > > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > > > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > > > "fortify: Explicitly disable Clang support""). I don't see the series
> > > > that added proper support for clang to fortify in 5.18 that ended with
> > > > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > > > branch, so this seems somewhat expected.
> > > > 
> > > 
> > > That explains that ;-). I don't mind if the patches stay in v5.10.y,
> > > we have them reverted anyway.
> > > 
> > > The revert was a pure process issue, as you may see when looking into
> > > commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
> > > Still, that doesn't explain why the problem exists in 5.18+.
> > > 
> > > > > I also see that upstream (starting with 6.1) when trying to build it 
> > > > > with clang,
> > > > > so I guess it is one of those bug-for-bug compatibility things. I 
> > > > > really have
> > > > > no idea what causes it, or why we don't see the problem when building
> > > > > chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 
> > > > > after
> > > > > merging 5.10.209 into it. Making things worse, the problem isn't 
> > > > > _always_
> > > > > seen. Sometimes I can compile the file in 6.1.y without error, 
> > > > > sometimes not.
> > > > > I have no idea what triggers the problem.
> > > > 
> > > > Have a .config that reproduces it on upstream? I have not personally
> > > > seen this warning in my build matrix nor has our continuous-integration
> > > > matrix (I don't see it in the warning output at the bottom but that
> > > > could have missed something for some reason) in 6.1:
> > > > 
> > > 
> > > The following command sequence reproduces the problem for me with all 
> > > stable
> > > branches starting with 5.18.y (plus mainline).
> > > 
> > > rm -rf /tmp/crypto-build
> > > mkdir /tmp/crypto-build
> > > make -j CC=clang-15 mrproper >/dev/null 2>&1
> > > make -j O=/tmp/crypto-build CC=clang-

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Michael S. Tsirkin]

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
> On 1/26/24 14:35, Nathan Chancellor wrote:
> > (slimming up the CC list, I don't think this is too relevant to the
> > wider stable community)
> > 
> > On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> > > On 1/26/24 12:34, Nathan Chancellor wrote:
> > > > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > > > This is the start of the stable review cycle for the 5.10.209 
> > > > > > > > release.
> > > > > > > > There are 286 patches in this series, all will be posted as a 
> > > > > > > > response
> > > > > > > > to this one.  If anyone has any issues with these being 
> > > > > > > > applied, please
> > > > > > > > let me know.
> > > > > > > > 
> > > > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +.
> > > > > > > > Anything received after that time might be too late.
> > > > > > > > 
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > zhenwei pi 
> > > > > > > > virtio-crypto: implement RSA algorithm
> > > > > > > > 
> > > > > > > 
> > > > > > > Curious: Why was this (and its subsequent fixes) backported to 
> > > > > > > v5.10.y ?
> > > > > > > It is quite beyond a bug fix. Also, unless I am really missing 
> > > > > > > something,
> > > > > > > the series (or at least this patch) was not applied to v5.15.y, 
> > > > > > > so we now
> > > > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > > > 
> > > > > > See the commit text, it was a dependency of a later fix and 
> > > > > > documented
> > > > > > as such.
> > > > > > 
> > > > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > > > gladly accepted :)
> > > > > > 
> > > > > 
> > > > > We reverted the entire series from the merge because it results in a 
> > > > > build
> > > > > failure for us.
> > > > > 
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > > > In file included from 
> > > > > /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: 
> > > > > error: call to __read_overflow2_field declared with 'warning' 
> > > > > attribute: detected read beyond size of field (2nd parameter); maybe 
> > > > > use struct_group()? [-Werror,-Wattribute-warning]
> > > > >   __read_overflow2_field(q_size_field, size);
> > > > 
> > > > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > > > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > > > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > > > "fortify: Explicitly disable Clang support""). I don't see the series
> > > > that added proper support for clang to fortify in 5.18 that ended with
> > > > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > > > branch, so this seems somewhat expected.
> > > > 
> > > 
> > > That explains that ;-). I don't mind if the patches stay in v5.10.y,
> > > we have them reverted anyway.
> > > 
> > > The revert was a pure process issue, as you may see when looking into
> > > commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
> > > Still, that doesn't explain why the problem exists in 5.18+.
> > > 
> > > > > I also see that upstream (starting with 6.1) when trying to build it 
> > > > > with clang,
> > > > > so I guess it is one of those bug-for-bug compatibility things. I 
> > > > > really have
> > > > > no idea what causes it, or why we don't see the problem when building
> > > > > chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 
> > > > > after
> > > > > merging 5.10.209 into it. Making things worse, the problem isn't 
> > > > > _always_
> > > > > seen. Sometimes I can compile the file in 6.1.y without error, 
> > > > > sometimes not.
> > > > > I have no idea what triggers the problem.
> > > > 
> > > > Have a .config that reproduces it on upstream? I have not personally
> > > > seen this warning in my build matrix nor has our continuous-integration
> > > > matrix (I don't see it in the warning output at the bottom but that
> > > > could have missed something for some reason) in 6.1:
> > > > 
> > > 
> > > The following command sequence reproduces the problem for me with all 
> > > stable
> > > branches starting with 5.18.y (plus mainline).
> > > 
> > > rm -rf /tmp/crypto-build
> > > mkdir /tmp/crypto-build
> > > make -j CC=clang-15 mrproper >/dev/null 2>&1
> > > make -j O=/tmp/crypto-build CC=clang-

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Nathan Chancellor]

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
> Anyway, how did you find that ? Is there a magic trick to find the
> actual code causing the warning ? I am asking because we had seen
> similar warnings before, and it would help to know how to find the
> problematic code.

The easiest way I have found is figuring out what primitive is causing
the warning (memset, memcpy) then just commenting out the uses in the
particular file until the warning goes away. Sometimes it is quick like
in this case since there were only two instances of memcpy() in that
file but other cases it can definitely take time. There could be
potential issues with that approach if the problematic use is in a
header, at which point you could generate a preprocessed ('.i') file and
see where fortify_memcpy_chk() or fortify_memset_chk() come from in that
file.

Cheers,
Nathan

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Guenter Roeck]

On 1/26/24 14:35, Nathan Chancellor wrote:

(slimming up the CC list, I don't think this is too relevant to the
wider stable community)

On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:

On 1/26/24 12:34, Nathan Chancellor wrote:

On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:

On 1/26/24 09:51, Greg Kroah-Hartman wrote:

On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:

On 1/22/24 15:55, Greg Kroah-Hartman wrote:

This is the start of the stable review cycle for the 5.10.209 release.
There are 286 patches in this series, all will be posted as a response
to this one.  If anyone has any issues with these being applied, please
let me know.

Responses should be made by Wed, 24 Jan 2024 23:56:49 +.
Anything received after that time might be too late.


[ ... ]


zhenwei pi 
virtio-crypto: implement RSA algorithm



Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
It is quite beyond a bug fix. Also, unless I am really missing something,
the series (or at least this patch) was not applied to v5.15.y, so we now
have functionality in v5.10.y which is not in v5.15.y.


See the commit text, it was a dependency of a later fix and documented
as such.

Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
gladly accepted :)



We reverted the entire series from the merge because it results in a build
failure for us.

In file included from 
/home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
In file included from 
/home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
In file included from 
/home/groeck/src/linux-chromeos/include/linux/string.h:293:
/home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: 
call to __read_overflow2_field declared with 'warning' attribute: detected read 
beyond size of field (2nd parameter); maybe use struct_group()? 
[-Werror,-Wattribute-warning]
  __read_overflow2_field(q_size_field, size);


For what it's worth, this is likely self inflicted for chromeos-5.10,
which carries a revert of commit eaafc590053b ("fortify: Explicitly
disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
"fortify: Explicitly disable Clang support""). I don't see the series
that added proper support for clang to fortify in 5.18 that ended with
commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
branch, so this seems somewhat expected.



That explains that ;-). I don't mind if the patches stay in v5.10.y,
we have them reverted anyway.

The revert was a pure process issue, as you may see when looking into
commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
Still, that doesn't explain why the problem exists in 5.18+.


I also see that upstream (starting with 6.1) when trying to build it with clang,
so I guess it is one of those bug-for-bug compatibility things. I really have
no idea what causes it, or why we don't see the problem when building
chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
merging 5.10.209 into it. Making things worse, the problem isn't _always_
seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
I have no idea what triggers the problem.


Have a .config that reproduces it on upstream? I have not personally
seen this warning in my build matrix nor has our continuous-integration
matrix (I don't see it in the warning output at the bottom but that
could have missed something for some reason) in 6.1:



The following command sequence reproduces the problem for me with all stable
branches starting with 5.18.y (plus mainline).

rm -rf /tmp/crypto-build
mkdir /tmp/crypto-build
make -j CC=clang-15 mrproper >/dev/null 2>&1
make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
make -j O=/tmp/crypto-build W=1 CC=clang-15 
drivers/crypto/virtio/virtio_crypto_akcipher_algs.o

I tried clang versions 14, 15, and 16. This is with my home system running
Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
the version is

Ubuntu clang version 15.0.7
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin


Okay interesting, this warning is hidden behind W=1, which our CI does
not test with. Looks like it has been that way since the introduction of
these checks in f68f2ff91512 ("fortify: Detect struct member overflows
in memcpy() at compile-time").



Interestingly the warning is seen in chromeos-5.10, without this patch,
and without W=1. I guess that must have to do with the revert which is
finally biting us.


I think this is a legitimate warning though. It is complaining about the
second memcpy() in virtio_crypto_alg_akcipher_init_session():

   memcpy(&ctrl->u, para, sizeof(ctrl->u));

where ctrl is:

   struct virtio_crypto_op_ctrl_req {
   struct virtio_crypto_ctrl_header header; 

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Nathan Chancellor]

(slimming up the CC list, I don't think this is too relevant to the
wider stable community)

On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> On 1/26/24 12:34, Nathan Chancellor wrote:
> > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > This is the start of the stable review cycle for the 5.10.209 
> > > > > > release.
> > > > > > There are 286 patches in this series, all will be posted as a 
> > > > > > response
> > > > > > to this one.  If anyone has any issues with these being applied, 
> > > > > > please
> > > > > > let me know.
> > > > > > 
> > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +.
> > > > > > Anything received after that time might be too late.
> > > > > > 
> > > > > [ ... ]
> > > > > 
> > > > > > zhenwei pi 
> > > > > >virtio-crypto: implement RSA algorithm
> > > > > > 
> > > > > 
> > > > > Curious: Why was this (and its subsequent fixes) backported to 
> > > > > v5.10.y ?
> > > > > It is quite beyond a bug fix. Also, unless I am really missing 
> > > > > something,
> > > > > the series (or at least this patch) was not applied to v5.15.y, so we 
> > > > > now
> > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > 
> > > > See the commit text, it was a dependency of a later fix and documented
> > > > as such.
> > > > 
> > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > gladly accepted :)
> > > > 
> > > 
> > > We reverted the entire series from the merge because it results in a build
> > > failure for us.
> > > 
> > > In file included from 
> > > /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > In file included from 
> > > /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > In file included from 
> > > /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > In file included from 
> > > /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: 
> > > error: call to __read_overflow2_field declared with 'warning' attribute: 
> > > detected read beyond size of field (2nd parameter); maybe use 
> > > struct_group()? [-Werror,-Wattribute-warning]
> > >  __read_overflow2_field(q_size_field, size);
> > 
> > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > "fortify: Explicitly disable Clang support""). I don't see the series
> > that added proper support for clang to fortify in 5.18 that ended with
> > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > branch, so this seems somewhat expected.
> > 
> 
> That explains that ;-). I don't mind if the patches stay in v5.10.y,
> we have them reverted anyway.
> 
> The revert was a pure process issue, as you may see when looking into
> commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
> Still, that doesn't explain why the problem exists in 5.18+.
> 
> > > I also see that upstream (starting with 6.1) when trying to build it with 
> > > clang,
> > > so I guess it is one of those bug-for-bug compatibility things. I really 
> > > have
> > > no idea what causes it, or why we don't see the problem when building
> > > chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
> > > merging 5.10.209 into it. Making things worse, the problem isn't _always_
> > > seen. Sometimes I can compile the file in 6.1.y without error, sometimes 
> > > not.
> > > I have no idea what triggers the problem.
> > 
> > Have a .config that reproduces it on upstream? I have not personally
> > seen this warning in my build matrix nor has our continuous-integration
> > matrix (I don't see it in the warning output at the bottom but that
> > could have missed something for some reason) in 6.1:
> > 
> 
> The following command sequence reproduces the problem for me with all stable
> branches starting with 5.18.y (plus mainline).
> 
> rm -rf /tmp/crypto-build
> mkdir /tmp/crypto-build
> make -j CC=clang-15 mrproper >/dev/null 2>&1
> make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
> make -j O=/tmp/crypto-build W=1 CC=clang-15 
> drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
> 
> I tried clang versions 14, 15, and 16. This is with my home system running
> Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For 
> clang-15,
> the version is
> 
> Ubuntu clang version 15.0.7
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin

Okay interesting, this warning is hidden behind W=1, which our CI does
not test with. Looks like it has been that way since the i

Re: [PATCH] crypto: sun8i-ce - Use kcalloc() instead of kzalloc()

[Herbert Xu]

On Sun, Jan 21, 2024 at 04:34:07PM +0100, Erick Archer wrote:
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
> 
> So, use the purpose specific kcalloc() function instead of the argument
> size * count in the kzalloc() function.
> 
> Link: 
> https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>  [1]
> Link: https://github.com/KSPP/linux/issues/162
> Signed-off-by: Erick Archer 
> ---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: qat - use kcalloc_node() instead of kzalloc_node()

[Herbert Xu]

On Sun, Jan 21, 2024 at 05:40:43PM +0100, Erick Archer wrote:
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
> 
> So, use the purpose specific kcalloc_node() function instead of the
> argument count * size in the kzalloc_node() function.
> 
> Link: 
> https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>  [1]
> Link: https://github.com/KSPP/linux/issues/162
> Signed-off-by: Erick Archer 
> ---
>  drivers/crypto/intel/qat/qat_common/adf_isr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] dt-bindings: qcom-qce: Add compatible for SM6350

[Herbert Xu]

On Fri, Jan 05, 2024 at 05:15:43PM +0100, Luca Weiss wrote:
> Add a compatible for the crypto block found on the SM6350 SoC.
> 
> Signed-off-by: Luca Weiss 
> ---
>  Documentation/devicetree/bindings/crypto/qcom-qce.yaml | 1 +
>  1 file changed, 1 insertion(+)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection

[Herbert Xu]

On Tue, Dec 26, 2023 at 11:12:23AM +0100, Markus Elfring wrote:
> From: Markus Elfring 
> Date: Tue, 26 Dec 2023 11:00:20 +0100
> 
> The kfree() function was called in up to two cases by the
> __virtio_crypto_akcipher_do_req() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> * Adjust jump targets.
> 
> * Delete two initialisations which became unnecessary
>   with this refactoring.
> 
> Signed-off-by: Markus Elfring 
> ---
> 
> v2:
> A typo was fixed for the delimiter of a label.
> 
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] virtio_crypto: remove duplicate check if queue is broken

[Stefan Hajnoczi]

On Wed, 24 Jan 2024 at 07:27, Li RongQing  wrote:
>
> virtqueue_enable_cb() will call virtqueue_poll() which will check if
> queue is broken at beginning, so remove the virtqueue_is_broken() call
>
> Signed-off-by: Li RongQing 
> ---
>  drivers/crypto/virtio/virtio_crypto_core.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Stefan Hajnoczi 

>
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c 
> b/drivers/crypto/virtio/virtio_crypto_core.c
> index 43a0838..3607b6f 100644
> --- a/drivers/crypto/virtio/virtio_crypto_core.c
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -42,8 +42,6 @@ static void virtcrypto_ctrlq_callback(struct virtqueue *vq)
> virtio_crypto_ctrlq_callback(vc_ctrl_req);
> spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> }
> -   if (unlikely(virtqueue_is_broken(vq)))
> -   break;
> } while (!virtqueue_enable_cb(vq));
> spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
>  }
> --
> 2.9.4
>
>

Re: [PATCH] crypto: sun8i-ce - Use kcalloc() instead of kzalloc()

[Jernej Škrabec]

Dne nedelja, 21. januar 2024 ob 16:34:07 CET je Erick Archer napisal(a):
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
> 
> So, use the purpose specific kcalloc() function instead of the argument
> size * count in the kzalloc() function.
> 
> Link: 
> https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>  [1]
> Link: https://github.com/KSPP/linux/issues/162
> Signed-off-by: Erick Archer 

Acked-by: Jernej Skrabec 

Best regards,
Jernej

Re: [PATCH] crypto: qat - use kcalloc_node() instead of kzalloc_node()

[Cabiddu, Giovanni]

On Sun, Jan 21, 2024 at 05:40:43PM +0100, Erick Archer wrote:
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
> 
> So, use the purpose specific kcalloc_node() function instead of the
> argument count * size in the kzalloc_node() function.
> 
> Link: 
> https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>  [1]
> Link: https://github.com/KSPP/linux/issues/162
> Signed-off-by: Erick Archer 
Acked-by: Giovanni Cabiddu 

Re: [PATCH 46/82] crypto: Refactor intentional wrap-around test

[Kees Cook]

On January 22, 2024 7:07:45 PM PST, Eric Biggers  wrote:
>Just to double check, you really intend to forbid *unsigned* integer 
>wraparound?
>This patch's commit message focuses on signed, and barely mentions unsigned.
>The actual code changes in this patch only deals with unsigned.

I don't mean to forbid wrap-around; we just need to annotate it. I can see how 
this commit log didn't do a great job explaining this. I hope the cover letter 
is more sensible:
https://lore.kernel.org/linux-hardening/20240122235208.work.748-k...@kernel.org/

>Also, what's the motivation for addressing the 'x + y < x' case but not other
>cases in the same file?

It's a code pattern we could find easily. It's working from the instances found 
via Coccinelle earlier in the series:
https://lore.kernel.org/linux-hardening/20240123002814.1396804-5-keesc...@chromium.org/

> For example, the le128_add() function which this patch
>modifies has two other intentional wraparounds, which this patch doesn't touch.

For dedicated wrapping functions we can mark them with __unsigned_wrap:
https://lore.kernel.org/linux-hardening/20240123002814.1396804-6-keesc...@chromium.org/

>Also, the le128_sub() function just below le128_add() is very similar but does
>wraparound in the other direction.  That's 6 cases in 20 lines of code, but 
>this
>patch only addresses 1.  And of course, lots of other crypto code relies on
>unsigned wraparounds too, which this patch overlooks.  

Right -- finding these kinds of things is where a lot of time will be spent in 
the future, I suspect. :)

> So I'm a bit confused
>about the point of this patch.  If we really wanted to explicitly annotate all
>the intentional wraparounds in a particular piece of code, so that the code can
>be run with the corresponding sanitizer enabled, wouldn't it be necessary to
>actually test the code with that sanitizer enabled to find all the cases?

Yes, but there's a lot of code to test -- I'm trying to get the first steps 
done. And then once the sanitizers are in good shape, the fuzzers can grind. 
(I'm trying to add some parallelism to this project; this code pattern was 
known so I figured we could address it now.)

-Kees

-- 
Kees Cook

Re: [PATCH 46/82] crypto: Refactor intentional wrap-around test

[Eric Biggers]

On Mon, Jan 22, 2024 at 04:27:21PM -0800, Kees Cook wrote:
> In an effort to separate intentional arithmetic wrap-around from
> unexpected wrap-around, we need to refactor places that depend on this
> kind of math. One of the most common code patterns of this is:
> 
>   VAR + value < VAR
> 
> Notably, this is considered "undefined behavior" for signed and pointer
> types, which the kernel works around by using the -fno-strict-overflow
> option in the build[1] (which used to just be -fwrapv). Regardless, we
> want to get the kernel source to the position where we can meaningfully
> instrument arithmetic wrap-around conditions and catch them when they
> are unexpected, regardless of whether they are signed[2], unsigned[3],
> or pointer[4] types.
> 
> Refactor open-coded wrap-around addition test to use add_would_overflow().
> This paves the way to enabling the wrap-around sanitizers in the future.
> 
> Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 
> [1]
> Link: https://github.com/KSPP/linux/issues/26 [2]
> Link: https://github.com/KSPP/linux/issues/27 [3]
> Link: https://github.com/KSPP/linux/issues/344 [4]
> Cc: Herbert Xu 
> Cc: "David S. Miller" 
> Cc: Aditya Srivastava 
> Cc: Randy Dunlap 
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Kees Cook 
> ---
>  crypto/adiantum.c   | 2 +-
>  drivers/crypto/amcc/crypto4xx_alg.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/adiantum.c b/crypto/adiantum.c
> index 60f3883b736a..c2f62ca455af 100644
> --- a/crypto/adiantum.c
> +++ b/crypto/adiantum.c
> @@ -190,7 +190,7 @@ static inline void le128_add(le128 *r, const le128 *v1, 
> const le128 *v2)
>  
>   r->b = cpu_to_le64(x + y);
>   r->a = cpu_to_le64(le64_to_cpu(v1->a) + le64_to_cpu(v2->a) +
> -(x + y < x));
> +(add_would_overflow(x, y)));
>  }
>  
>  /* Subtraction in Z/(2^{128}Z) */
> diff --git a/drivers/crypto/amcc/crypto4xx_alg.c 
> b/drivers/crypto/amcc/crypto4xx_alg.c
> index e0af611a95d8..33f73234ddd9 100644
> --- a/drivers/crypto/amcc/crypto4xx_alg.c
> +++ b/drivers/crypto/amcc/crypto4xx_alg.c
> @@ -251,7 +251,7 @@ crypto4xx_ctr_crypt(struct skcipher_request *req, bool 
> encrypt)
>* the whole IV is a counter.  So fallback if the counter is going to
>* overlow.
>*/
> - if (counter + nblks < counter) {
> + if (add_would_overflow(counter, nblks)) {
>   SYNC_SKCIPHER_REQUEST_ON_STACK(subreq, ctx->sw_cipher.cipher);
>   int ret;

Just to double check, you really intend to forbid *unsigned* integer wraparound?
This patch's commit message focuses on signed, and barely mentions unsigned.
The actual code changes in this patch only deals with unsigned.

Also, what's the motivation for addressing the 'x + y < x' case but not other
cases in the same file?  For example, the le128_add() function which this patch
modifies has two other intentional wraparounds, which this patch doesn't touch.
Also, the le128_sub() function just below le128_add() is very similar but does
wraparound in the other direction.  That's 6 cases in 20 lines of code, but this
patch only addresses 1.  And of course, lots of other crypto code relies on
unsigned wraparounds too, which this patch overlooks.  So I'm a bit confused
about the point of this patch.  If we really wanted to explicitly annotate all
the intentional wraparounds in a particular piece of code, so that the code can
be run with the corresponding sanitizer enabled, wouldn't it be necessary to
actually test the code with that sanitizer enabled to find all the cases?

- Eric

Re: [PATCH] crypto: sun8i-ce - Use kcalloc() instead of kzalloc()

[Gustavo A. R. Silva]

On 1/21/24 09:34, Erick Archer wrote:

As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.

So, use the purpose specific kcalloc() function instead of the argument
size * count in the kzalloc() function.

Link: 
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
 [1]
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer 


Reviewed-by: Gustavo A. R. Silva 

Thanks!
--
Gustavo


---
  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c 
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
index d358334e5981..ee2a28c906ed 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
@@ -362,7 +362,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void 
*breq)
digestsize = SHA512_DIGEST_SIZE;

/* the padding could be up to two block. */
-   buf = kzalloc(bs * 2, GFP_KERNEL | GFP_DMA);
+   buf = kcalloc(2, bs, GFP_KERNEL | GFP_DMA);
if (!buf) {
err = -ENOMEM;
goto theend;
--
2.25.1

Re: [PATCH] crypto: qat - use kcalloc_node() instead of kzalloc_node()

[Gustavo A. R. Silva]

On 1/21/24 10:40, Erick Archer wrote:

As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.

So, use the purpose specific kcalloc_node() function instead of the
argument count * size in the kzalloc_node() function.

Link: 
https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
 [1]
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer 


Reviewed-by: Gustavo A. R. Silva 

Thanks!
--
Gustavo


---
  drivers/crypto/intel/qat/qat_common/adf_isr.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_isr.c 
b/drivers/crypto/intel/qat/qat_common/adf_isr.c
index 3557a0d6dea2..a13d9885d60f 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_isr.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_isr.c
@@ -272,7 +272,7 @@ static int adf_isr_alloc_msix_vectors_data(struct 
adf_accel_dev *accel_dev)
if (!accel_dev->pf.vf_info)
msix_num_entries += hw_data->num_banks;

-   irqs = kzalloc_node(msix_num_entries * sizeof(*irqs),
+   irqs = kcalloc_node(msix_num_entries, sizeof(*irqs),
GFP_KERNEL, dev_to_node(&GET_DEV(accel_dev)));
if (!irqs)
return -ENOMEM;
--
2.25.1

Re: [PATCH 0/2] Add Crypto Engine support for SM6350

[Konrad Dybcio]

On 1/9/24 12:27, Luca Weiss wrote:

On Mon Jan 8, 2024 at 1:40 PM CET, Konrad Dybcio wrote:

On 5.01.2024 17:15, Luca Weiss wrote:

Add the compatible and nodes for the QCE found on SM6350 SoC.

Not completely sure how to fully test it but "kcapi-speed --all" shows
no issues. Let me know if I can/should test this more.


I think I used `cryptsetup benchmark` with and without the ICE enabled
a couple years back. IIRC the CPU should be faaar faster but also chug
power while at it.


Are you sure you mean QCE here (which this patch is about) and not ICE?


I.. think I do. It's been a while.



I'm not aware of them working together somehow but I wouldn't be
surprised if there's something since I don't know much of this area at
all.


No idea

Konrad

Re: crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection

[Markus Elfring]

>> The kfree() function was called in up to two cases by the
>> __virtio_crypto_akcipher_do_req() function during error handling
>> even if the passed variable contained a null pointer.
>> This issue was detected by using the Coccinelle software.
>
> If the script is short and simple would you mind, in the future,
> including it below the fold -- this may help others do similar work down
> the line -- Or …

Would you like to take another look at the clarification approach
“Reconsidering kfree() calls for null pointers (with SmPL)”?
https://lore.kernel.org/cocci/6cbcf640-55e5-2f11-4a09-716fe681c...@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00096.html

Regards,
Markus

Re: [PATCH v2] crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection

[Justin Stitt]

Hi,

On Tue, Dec 26, 2023 at 11:12:23AM +0100, Markus Elfring wrote:
> From: Markus Elfring 
> Date: Tue, 26 Dec 2023 11:00:20 +0100
>
> The kfree() function was called in up to two cases by the
> __virtio_crypto_akcipher_do_req() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.

If the script is short and simple would you mind, in the future,
including it below the fold -- this may help others do similar work down
the line -- Or you could also link to a git-managed version like what
Kees has been doing with his __counted_by patches [1].

>
> * Adjust jump targets.
>
> * Delete two initialisations which became unnecessary
>   with this refactoring.
>
> Signed-off-by: Markus Elfring 
> ---

Nonetheless,

Reviewed-by: Justin Stitt 
>
> v2:
> A typo was fixed for the delimiter of a label.
>
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c 
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 2621ff8a9376..057da5bd8d30 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -224,11 +224,11 @@ static int __virtio_crypto_akcipher_do_req(struct 
> virtio_crypto_akcipher_request
>   struct virtio_crypto *vcrypto = ctx->vcrypto;
>   struct virtio_crypto_op_data_req *req_data = vc_req->req_data;
>   struct scatterlist *sgs[4], outhdr_sg, inhdr_sg, srcdata_sg, dstdata_sg;
> - void *src_buf = NULL, *dst_buf = NULL;
> + void *src_buf, *dst_buf = NULL;
>   unsigned int num_out = 0, num_in = 0;
>   int node = dev_to_node(&vcrypto->vdev->dev);
>   unsigned long flags;
> - int ret = -ENOMEM;
> + int ret;
>   bool verify = vc_akcipher_req->opcode == VIRTIO_CRYPTO_AKCIPHER_VERIFY;
>   unsigned int src_len = verify ? req->src_len + req->dst_len : 
> req->src_len;
>
> @@ -239,7 +239,7 @@ static int __virtio_crypto_akcipher_do_req(struct 
> virtio_crypto_akcipher_request
>   /* src data */
>   src_buf = kcalloc_node(src_len, 1, GFP_KERNEL, node);
>   if (!src_buf)
> - goto err;
> + return -ENOMEM;
>
>   if (verify) {
>   /* for verify operation, both src and dst data work as OUT 
> direction */
> @@ -254,7 +254,7 @@ static int __virtio_crypto_akcipher_do_req(struct 
> virtio_crypto_akcipher_request
>   /* dst data */
>   dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, node);
>   if (!dst_buf)
> - goto err;
> + goto free_src;
>
>   sg_init_one(&dstdata_sg, dst_buf, req->dst_len);
>   sgs[num_out + num_in++] = &dstdata_sg;
> @@ -277,9 +277,9 @@ static int __virtio_crypto_akcipher_do_req(struct 
> virtio_crypto_akcipher_request
>   return 0;
>
>  err:
> - kfree(src_buf);
>   kfree(dst_buf);
> -
> +free_src:
> + kfree(src_buf);
>   return -ENOMEM;
>  }
>
> --
> 2.43.0
>

[1]: https://lore.kernel.org/all/20230922175023.work.239-k...@kernel.org/

Thanks
Justin

Re: [PATCH 0/2] Add Crypto Engine support for SM6350

[Luca Weiss]

On Mon Jan 8, 2024 at 1:40 PM CET, Konrad Dybcio wrote:
> On 5.01.2024 17:15, Luca Weiss wrote:
> > Add the compatible and nodes for the QCE found on SM6350 SoC.
> > 
> > Not completely sure how to fully test it but "kcapi-speed --all" shows
> > no issues. Let me know if I can/should test this more.
>
> I think I used `cryptsetup benchmark` with and without the ICE enabled
> a couple years back. IIRC the CPU should be faaar faster but also chug
> power while at it.

Are you sure you mean QCE here (which this patch is about) and not ICE?

I'm not aware of them working together somehow but I wouldn't be
surprised if there's something since I don't know much of this area at
all.

Regards
Luca

>
> Konrad

Re: [PATCH 0/2] Add Crypto Engine support for SM6350

[Konrad Dybcio]

On 5.01.2024 17:15, Luca Weiss wrote:
> Add the compatible and nodes for the QCE found on SM6350 SoC.
> 
> Not completely sure how to fully test it but "kcapi-speed --all" shows
> no issues. Let me know if I can/should test this more.

I think I used `cryptsetup benchmark` with and without the ICE enabled
a couple years back. IIRC the CPU should be faaar faster but also chug
power while at it.

Konrad

Re: [PATCH 1/2] dt-bindings: qcom-qce: Add compatible for SM6350

[Krzysztof Kozlowski]

On 05/01/2024 17:15, Luca Weiss wrote:
> Add a compatible for the crypto block found on the SM6350 SoC.
> 
> Signed-off-by: Luca Weiss 
> ---

Acked-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

Re: [PATCH 2/2] arm64: dts: qcom: sm6350: Add Crypto Engine

[Stephan Gerhold]

On Fri, Jan 05, 2024 at 05:15:44PM +0100, Luca Weiss wrote:
> Add crypto engine (CE) and CE BAM related nodes and definitions for this
> SoC.
> 
> For reference:
> 
>   [2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 5.5.1
> 
> Signed-off-by: Luca Weiss 
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 +++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi 
> b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 8fd6f4d03490..516aadbb16bb 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -1212,6 +1212,37 @@ ufs_mem_phy_lanes: phy@1d87400 {
>   };
>   };
>  
> + cryptobam: dma-controller@1dc4000 {
> + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
> + reg = <0 0x01dc4000 0 0x24000>;
> + interrupts = ;
> + #dma-cells = <1>;
> + qcom,ee = <0>;
> + qcom,controlled-remotely;
> + num-channels = <16>;
> + qcom,num-ees = <4>;
> + iommus = <&apps_smmu 0x432 0x>,
> +  <&apps_smmu 0x438 0x0001>,
> +  <&apps_smmu 0x43f 0x>,
> +  <&apps_smmu 0x426 0x0011>,
> +  <&apps_smmu 0x436 0x0011>;

The last two lines look equivalent to me: 0x436 & ~0x0011 = 0x426.

It's also a bit weird that the mask has one more digit than the stream
ID. And ordered numerically (by stream ID, first number) it would be a
bit easier to read. :-)

Thanks,
Stephan

RE: [PATCH v2] crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection

[Gonglei (Arei)]

> -Original Message-
> From: Markus Elfring [mailto:markus.elfr...@web.de]
> Sent: Tuesday, December 26, 2023 6:12 PM
> To: kernel test robot ; virtualizat...@lists.linux.dev;
> linux-crypto@vger.kernel.org; kernel-janit...@vger.kernel.org; David S. Miller
> ; Gonglei (Arei) ; Herbert
> Xu ; Jason Wang ;
> Michael S. Tsirkin ; Xuan Zhuo
> 
> Cc: l...@lists.linux.dev; oe-kbuild-...@lists.linux.dev; 
> net...@vger.kernel.org;
> LKML ; co...@inria.fr
> Subject: [PATCH v2] crypto: virtio - Less function calls in
> __virtio_crypto_akcipher_do_req() after error detection
> 
> From: Markus Elfring 
> Date: Tue, 26 Dec 2023 11:00:20 +0100
> 
> The kfree() function was called in up to two cases by the
> __virtio_crypto_akcipher_do_req() function during error handling even if the
> passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> * Adjust jump targets.
> 
> * Delete two initialisations which became unnecessary
>   with this refactoring.
> 
> Signed-off-by: Markus Elfring 
> ---
> 
> v2:
> A typo was fixed for the delimiter of a label.
> 
>  drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

Reviewed-by: Gonglei 


Regards,
-Gonglei

Re: [PATCH] crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection

[kernel test robot]

Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on herbert-cryptodev-2.6/master]
[also build test ERROR on herbert-crypto-2.6/master linus/master v6.7-rc7 
next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/crypto-virtio-Less-function-calls-in-__virtio_crypto_akcipher_do_req-after-error-detection/20231225-154431
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:
https://lore.kernel.org/r/2413f22f-f0c3-45e0-9f6b-a551bdf0f54c%40web.de
patch subject: [PATCH] crypto: virtio - Less function calls in 
__virtio_crypto_akcipher_do_req() after error detection
config: x86_64-rhel-8.3-bpf 
(https://download.01.org/0day-ci/archive/20231226/202312261008.7aherlax-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): 
(https://download.01.org/0day-ci/archive/20231226/202312261008.7aherlax-...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-kbuild-all/202312261008.7aherlax-...@intel.com/

All errors (new ones prefixed by >>):

   drivers/crypto/virtio/virtio_crypto_akcipher_algs.c: In function 
'__virtio_crypto_akcipher_do_req':
>> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:281:1: error: 'free_src' 
>> undeclared (first use in this function)
 281 | free_src;
 | ^~~~
   drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:281:1: note: each 
undeclared identifier is reported only once for each function it appears in
>> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:257:25: error: label 
>> 'free_src' used but not defined
 257 | goto free_src;
 | ^~~~


vim +/free_src +281 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c

   218  
   219  static int __virtio_crypto_akcipher_do_req(struct 
virtio_crypto_akcipher_request *vc_akcipher_req,
   220  struct akcipher_request *req, struct data_queue 
*data_vq)
   221  {
   222  struct virtio_crypto_akcipher_ctx *ctx = 
vc_akcipher_req->akcipher_ctx;
   223  struct virtio_crypto_request *vc_req = &vc_akcipher_req->base;
   224  struct virtio_crypto *vcrypto = ctx->vcrypto;
   225  struct virtio_crypto_op_data_req *req_data = vc_req->req_data;
   226  struct scatterlist *sgs[4], outhdr_sg, inhdr_sg, srcdata_sg, 
dstdata_sg;
   227  void *src_buf, *dst_buf = NULL;
   228  unsigned int num_out = 0, num_in = 0;
   229  int node = dev_to_node(&vcrypto->vdev->dev);
   230  unsigned long flags;
   231  int ret;
   232  bool verify = vc_akcipher_req->opcode == 
VIRTIO_CRYPTO_AKCIPHER_VERIFY;
   233  unsigned int src_len = verify ? req->src_len + req->dst_len : 
req->src_len;
   234  
   235  /* out header */
   236  sg_init_one(&outhdr_sg, req_data, sizeof(*req_data));
   237  sgs[num_out++] = &outhdr_sg;
   238  
   239  /* src data */
   240  src_buf = kcalloc_node(src_len, 1, GFP_KERNEL, node);
   241  if (!src_buf)
   242  return -ENOMEM;
   243  
   244  if (verify) {
   245  /* for verify operation, both src and dst data work as 
OUT direction */
   246  sg_copy_to_buffer(req->src, sg_nents(req->src), 
src_buf, src_len);
   247  sg_init_one(&srcdata_sg, src_buf, src_len);
   248  sgs[num_out++] = &srcdata_sg;
   249  } else {
   250  sg_copy_to_buffer(req->src, sg_nents(req->src), 
src_buf, src_len);
   251  sg_init_one(&srcdata_sg, src_buf, src_len);
   252  sgs[num_out++] = &srcdata_sg;
   253  
   254  /* dst data */
   255  dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, 
node);
   256  if (!dst_buf)
 > 257  goto free_src;
   258  
   259  sg_init_one(&dstdata_sg, dst_buf, req->dst_len);
   260  sgs[num_out + num_in++] = &dstdata_sg;
   261  }
   262  
   263  vc_akcipher_req->src_buf = src_buf;
   264  vc_akcipher_req->dst_buf = dst_buf;
   265  
   266  /* in header */
   267  sg_init_one(&inhdr_sg, &vc_req->status, sizeof(vc_req->status));
   268  sgs[num_out + num_in++] = &inhdr_sg;
   269  
   270  spin_lock_irqsave(&data_vq->lock, flags);
   271  ret = virtqueue_add_sgs(data_vq->vq, sgs, num_out, num_in, 
vc_req, GFP_ATOMIC);
   272  virtqueue_kick(data_vq->vq

Re: [PATCH] crypto: virtio - Less function calls in __virtio_crypto_akcipher_do_req() after error detection

[kernel test robot]

Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on herbert-cryptodev-2.6/master]
[also build test ERROR on herbert-crypto-2.6/master linus/master v6.7-rc7 
next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/crypto-virtio-Less-function-calls-in-__virtio_crypto_akcipher_do_req-after-error-detection/20231225-154431
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:
https://lore.kernel.org/r/2413f22f-f0c3-45e0-9f6b-a551bdf0f54c%40web.de
patch subject: [PATCH] crypto: virtio - Less function calls in 
__virtio_crypto_akcipher_do_req() after error detection
config: arm-randconfig-001-20231225 
(https://download.01.org/0day-ci/archive/20231226/202312260852.0ge5o8il-...@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git 
ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): 
(https://download.01.org/0day-ci/archive/20231226/202312260852.0ge5o8il-...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-kbuild-all/202312260852.0ge5o8il-...@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:281:1: error: use of 
>> undeclared identifier 'free_src'; did you mean 'free_irq'?
   free_src;
   ^~~~
   free_irq
   include/linux/interrupt.h:196:20: note: 'free_irq' declared here
   extern const void *free_irq(unsigned int, void *);
  ^
>> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:257:9: error: use of 
>> undeclared label 'free_src'
   goto free_src;
^
>> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:281:1: warning: 
>> expression result unused [-Wunused-value]
   free_src;
   ^~~~
   1 warning and 2 errors generated.


vim +281 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c

   218  
   219  static int __virtio_crypto_akcipher_do_req(struct 
virtio_crypto_akcipher_request *vc_akcipher_req,
   220  struct akcipher_request *req, struct data_queue 
*data_vq)
   221  {
   222  struct virtio_crypto_akcipher_ctx *ctx = 
vc_akcipher_req->akcipher_ctx;
   223  struct virtio_crypto_request *vc_req = &vc_akcipher_req->base;
   224  struct virtio_crypto *vcrypto = ctx->vcrypto;
   225  struct virtio_crypto_op_data_req *req_data = vc_req->req_data;
   226  struct scatterlist *sgs[4], outhdr_sg, inhdr_sg, srcdata_sg, 
dstdata_sg;
   227  void *src_buf, *dst_buf = NULL;
   228  unsigned int num_out = 0, num_in = 0;
   229  int node = dev_to_node(&vcrypto->vdev->dev);
   230  unsigned long flags;
   231  int ret;
   232  bool verify = vc_akcipher_req->opcode == 
VIRTIO_CRYPTO_AKCIPHER_VERIFY;
   233  unsigned int src_len = verify ? req->src_len + req->dst_len : 
req->src_len;
   234  
   235  /* out header */
   236  sg_init_one(&outhdr_sg, req_data, sizeof(*req_data));
   237  sgs[num_out++] = &outhdr_sg;
   238  
   239  /* src data */
   240  src_buf = kcalloc_node(src_len, 1, GFP_KERNEL, node);
   241  if (!src_buf)
   242  return -ENOMEM;
   243  
   244  if (verify) {
   245  /* for verify operation, both src and dst data work as 
OUT direction */
   246  sg_copy_to_buffer(req->src, sg_nents(req->src), 
src_buf, src_len);
   247  sg_init_one(&srcdata_sg, src_buf, src_len);
   248  sgs[num_out++] = &srcdata_sg;
   249  } else {
   250  sg_copy_to_buffer(req->src, sg_nents(req->src), 
src_buf, src_len);
   251  sg_init_one(&srcdata_sg, src_buf, src_len);
   252  sgs[num_out++] = &srcdata_sg;
   253  
   254  /* dst data */
   255  dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, 
node);
   256  if (!dst_buf)
 > 257  goto free_src;
   258  
   259  sg_init_one(&dstdata_sg, dst_buf, req->dst_len);
   260  sgs[num_out + num_in++] = &dstdata_sg;
   261  }
   262  
   263  vc_akcipher_req->src_buf = src_buf;
   264  vc_akcipher_req->dst_buf = dst_buf;
   265  
   266  /* in header */
   267  sg_init_one(&inhdr_sg, &vc_req->status, sizeof(vc_req->status));
   268  sgs[num_out + num_in++] = &inhdr_sg;
   269  
   270  spin_lock_irqsave(&data_vq->lock, flags

Re: [PATCH v3 5/8] phy: qcom: qmp-ufs: Add SC7180 support

[Dmitry Baryshkov]

On Mon, 25 Dec 2023 at 14:05, David Wronek  wrote:
>
> The SC7180 UFS PHY is identical to the one found on SM7150. Add a
> compatible for it.
>
> Signed-off-by: David Wronek 
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Dmitry Baryshkov 

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] crypto: virtio-crypto: Wait for tasklet to complete on device remove

[Herbert Xu]

On Mon, Dec 11, 2023 at 07:42:15PM +0800, Gonglei wrote:
> From: wangyangxin 
> 
> The scheduled tasklet needs to be executed on device remove.
> 
> Fixes: fed93fb62e05 ("crypto: virtio - Handle dataq logic with tasklet")
> Signed-off-by: wangyangxin 
> Signed-off-by: Gonglei 
> ---
>  drivers/crypto/virtio/virtio_crypto_core.c | 3 +++
>  1 file changed, 3 insertions(+)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2/2] crypto: virtio-crypto: Fix gcc check warnings

[Herbert Xu]

On Mon, Dec 11, 2023 at 07:42:16PM +0800, Gonglei wrote:
>
>  static inline int virtio_crypto_get_current_node(void)
>  {
> - int cpu, node;
> + int node;
>  
> - cpu = get_cpu();
> - node = topology_physical_package_id(cpu);
> + node = topology_physical_package_id(get_cpu());

This looks like a bogus warning.  I think we should do something
like this instead:

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index ae81a7191c1c..0cb43986061b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -191,7 +191,7 @@ DECLARE_PER_CPU_READ_MOSTLY(struct cpuinfo_x86, cpu_info);
 #define cpu_data(cpu)  per_cpu(cpu_info, cpu)
 #else
 #define cpu_info   boot_cpu_data
-#define cpu_data(cpu)  boot_cpu_data
+#define cpu_data(cpu)  ((void)cpu, boot_cpu_data)
 #endif
 
 extern const struct seq_operations cpuinfo_op;

Please send this patch to the x86 people.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v5 0/6] DCP as trusted keys backend

[Paul Moore]

On Fri, Dec 15, 2023 at 6:07 AM David Gstir  wrote:
>
> This is a revival of the previous patch set submitted by Richard Weinberger:
> https://lore.kernel.org/linux-integrity/20210614201620.30451-1-rich...@nod.at/
>
> v4 is here:
> https://lore.kernel.org/keyrings/20231024162024.51260-1-da...@sigma-star.at/
>
> v4 -> v5:
> - Make Kconfig for trust source check scalable as suggested by Jarkko Sakkinen
> - Add Acked-By from Herbert Xu to patch #1 - thanks!
> v3 -> v4:
> - Split changes on MAINTAINERS and documentation into dedicated patches
> - Use more concise wording in commit messages as suggested by Jarkko Sakkinen
> v2 -> v3:
> - Addressed review comments from Jarkko Sakkinen
> v1 -> v2:
> - Revive and rebase to latest version
> - Include review comments from Ahmad Fatoum
>
> The Data CoProcessor (DCP) is an IP core built into many NXP SoCs such
> as i.mx6ull.
>
> Similar to the CAAM engine used in more powerful SoCs, DCP can AES-
> encrypt/decrypt user data using a unique, never-disclosed,
> device-specific key. Unlike CAAM though, it cannot directly wrap and
> unwrap blobs in hardware. As DCP offers only the bare minimum feature
> set and a blob mechanism needs aid from software. A blob in this case
> is a piece of sensitive data (e.g. a key) that is encrypted and
> authenticated using the device-specific key so that unwrapping can only
> be done on the hardware where the blob was wrapped.
>
> This patch series adds a DCP based, trusted-key backend and is similar
> in spirit to the one by Ahmad Fatoum [0] that does the same for CAAM.
> It is of interest for similar use cases as the CAAM patch set, but for
> lower end devices, where CAAM is not available.
>
> Because constructing and parsing the blob has to happen in software,
> we needed to decide on a blob format and chose the following:
>
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
>
> The `fmt_version` is currently 1.
>
> The encrypted key is stored in the payload area. It is AES-128-GCM
> encrypted using `blob_key` and `nonce`, GCM auth tag is attached at
> the end of the payload (`payload_len` does not include the size of
> the auth tag).
>
> The `blob_key` itself is encrypted in AES-128-ECB mode by DCP using
> the OTP or UNIQUE device key. A new `blob_key` and `nonce` are generated
> randomly, when sealing/exporting the DCP blob.
>
> This patchset was tested with dm-crypt on an i.MX6ULL board.
>
> [0] 
> https://lore.kernel.org/keyrings/20220513145705.2080323-1-a.fat...@pengutronix.de/
>
> David Gstir (6):
>   crypto: mxs-dcp: Add support for hardware-bound keys
>   KEYS: trusted: improve scalability of trust source config
>   KEYS: trusted: Introduce NXP DCP-backed trusted keys
>   MAINTAINERS: add entry for DCP-based trusted keys
>   docs: document DCP-backed trusted keys kernel params
>   docs: trusted-encrypted: add DCP as new trust source
>
>  .../admin-guide/kernel-parameters.txt |  13 +
>  .../security/keys/trusted-encrypted.rst   |  85 +
>  MAINTAINERS   |   9 +
>  drivers/crypto/mxs-dcp.c  | 104 +-
>  include/keys/trusted_dcp.h|  11 +
>  include/soc/fsl/dcp.h |  17 +
>  security/keys/trusted-keys/Kconfig|  18 +-
>  security/keys/trusted-keys/Makefile   |   2 +
>  security/keys/trusted-keys/trusted_core.c |   6 +-
>  security/keys/trusted-keys/trusted_dcp.c  | 311 ++
>  10 files changed, 562 insertions(+), 14 deletions(-)
>  create mode 100644 include/keys/trusted_dcp.h
>  create mode 100644 include/soc/fsl/dcp.h
>  create mode 100644 security/keys/trusted-keys/trusted_dcp.c

Jarkko, Mimi, David - if this patchset isn't already in your review
queue, can you take a look at it from a security/keys perspective?

Thanks.

-- 
paul-moore.com