On 23.02.22 16:41, Jarkko Sakkinen wrote:
> On Tue, Feb 22, 2022 at 08:58:18PM +0100, Ahmad Fatoum wrote:
>> The NXP Cryptographic Acceleration and Assurance Module (CAAM)
>> can be used to protect user-defined data across system reboot:
>>
>> - When the system is fused and boots into secure state, the master
>> key is a unique never-disclosed device-specific key
>> - random key is encrypted by key derived from master key
>> - data is encrypted using the random key
>> - encrypted data and its encrypted random key are stored alongside
>> - This blob can now be safely stored in non-volatile memory
>>
>> On next power-on:
>> - blob is loaded into CAAM
>> - CAAM writes decrypted data either into memory or key register
>>
>> Add functions to realize encrypting and decrypting into memory alongside
>> the CAAM driver.
>>
>> They will be used in a later commit as a source for the trusted key
>> seal/unseal mechanism.
>>
>> Reviewed-by: David Gstir <[email protected]>
>> Reviewed-by: Pankaj Gupta <[email protected]>
>> Tested-By: Tim Harvey <[email protected]>
>> Tested-by: Matthias Schiffer <[email protected]>
>> Signed-off-by: Steffen Trumtrar <[email protected]>
>> Signed-off-by: Ahmad Fatoum <[email protected]>
>> ---
>> To: "Horia Geantă" <[email protected]>
>> To: Aymen Sghaier <[email protected]>
>> To: Herbert Xu <[email protected]>
>> To: "David S. Miller" <[email protected]>
>> Cc: James Bottomley <[email protected]>
>> Cc: Jarkko Sakkinen <[email protected]>
>> Cc: Mimi Zohar <[email protected]>
>> Cc: David Howells <[email protected]>
>> Cc: James Morris <[email protected]>
>> Cc: Eric Biggers <[email protected]>
>> Cc: "Serge E. Hallyn" <[email protected]>
>> Cc: Jan Luebbe <[email protected]>
>> Cc: David Gstir <[email protected]>
>> Cc: Richard Weinberger <[email protected]>
>> Cc: Franck LENORMAND <[email protected]>
>> Cc: Sumit Garg <[email protected]>
>> Cc: Tim Harvey <[email protected]>
>> Cc: Matthias Schiffer <[email protected]>
>> Cc: Pankaj Gupta <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> drivers/crypto/caam/Kconfig | 3 +
>> drivers/crypto/caam/Makefile | 1 +
>> drivers/crypto/caam/blob_gen.c | 230 +++++++++++++++++++++++++++++++++
>> include/soc/fsl/caam-blob.h | 56 ++++++++
>> 4 files changed, 290 insertions(+)
>> create mode 100644 drivers/crypto/caam/blob_gen.c
>> create mode 100644 include/soc/fsl/caam-blob.h
>>
>> diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
>> index 84ea7cba5ee5..ea9f8b1ae981 100644
>> --- a/drivers/crypto/caam/Kconfig
>> +++ b/drivers/crypto/caam/Kconfig
>> @@ -151,6 +151,9 @@ config CRYPTO_DEV_FSL_CAAM_RNG_API
>> Selecting this will register the SEC4 hardware rng to
>> the hw_random API for supplying the kernel entropy pool.
>>
>> +config CRYPTO_DEV_FSL_CAAM_BLOB_GEN
>> + bool
>> +
>> endif # CRYPTO_DEV_FSL_CAAM_JR
>>
>> endif # CRYPTO_DEV_FSL_CAAM
>> diff --git a/drivers/crypto/caam/Makefile b/drivers/crypto/caam/Makefile
>> index 3570286eb9ce..25f7ae5a4642 100644
>> --- a/drivers/crypto/caam/Makefile
>> +++ b/drivers/crypto/caam/Makefile
>> @@ -21,6 +21,7 @@ caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI) += caamalg_qi.o
>> caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_AHASH_API) += caamhash.o
>> caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API) += caamrng.o
>> caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_PKC_API) += caampkc.o pkc_desc.o
>> +caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_BLOB_GEN) += blob_gen.o
>>
>> caam-$(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI) += qi.o
>> ifneq ($(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI),)
>> diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
>> new file mode 100644
>> index 000000000000..513d3f90e438
>> --- /dev/null
>> +++ b/drivers/crypto/caam/blob_gen.c
>> @@ -0,0 +1,230 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2015 Pengutronix, Steffen Trumtrar <[email protected]>
>> + * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <[email protected]>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <soc/fsl/caam-blob.h>
>> +
>> +#include "compat.h"
>> +#include "desc_constr.h"
>> +#include "desc.h"
>> +#include "error.h"
>> +#include "intern.h"
>> +#include "jr.h"
>> +#include "regs.h"
>> +
>> +struct caam_blob_priv {
>> + struct device jrdev;
>> +};
>> +
>> +struct caam_blob_job_result {
>> + int err;
>> + struct completion completion;
>> +};
>> +
>> +static void caam_blob_job_done(struct device *dev, u32 *desc, u32 err, void *context)
>> +{
>> + struct caam_blob_job_result *res = context;
>> + int ecode = 0;
>> +
>> + dev_dbg(dev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
>> +
>> + if (err)
>> + ecode = caam_jr_strstatus(dev, err);
>> +
>> + res->err = ecode;
>> +
>> + /*
>> + * Upon completion, desc points to a buffer containing a CAAM job
>> + * descriptor which encapsulates data into an externally-storable
>> + * blob.
>> + */
>> + complete(&res->completion);
>> +}
>> +
>> +static u32 *caam_blob_alloc_desc(size_t keymod_len)
>> +{
>> + size_t len;
>> +
>> + /* header + (key mod immediate) + 2x pointers + op */
>> + len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + CAAM_PTR_SZ_MAX) + 4;
>
> Nit: the amount of magic numbers is overwhelming here. I neither understand
> the statement nor how that comment should help me to understand it.
header = 4
(key mod immediate) = (4 + ALIGN(keymod_len, 4))
2x pointer = 2 * (4 + 4 + CAAM_PTR_SZ_MAX)
op = 4
I haven't heard back from the CAAM maintainers yet since v1. Perhaps now
is a good occasion to chime in? :-)
@Jarkko, could you take a look at patch 5? That's the gist of the series
and I have yet to get maintainer feedback on that one.
Cheers,
Ahmad
>
> BR, Jarkko
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, 2022-02-23 at 17:20 +0100, Ahmad Fatoum wrote:
> On 23.02.22 16:41, Jarkko Sakkinen wrote:
> > On Tue, Feb 22, 2022 at 08:58:18PM +0100, Ahmad Fatoum wrote:
> > > The NXP Cryptographic Acceleration and Assurance Module (CAAM)
> > > can be used to protect user-defined data across system reboot:
> > >
> > > - When the system is fused and boots into secure state, the master
> > > key is a unique never-disclosed device-specific key
> > > - random key is encrypted by key derived from master key
> > > - data is encrypted using the random key
> > > - encrypted data and its encrypted random key are stored alongside
> > > - This blob can now be safely stored in non-volatile memory
> > >
> > > On next power-on:
> > > - blob is loaded into CAAM
> > > - CAAM writes decrypted data either into memory or key register
> > >
> > > Add functions to realize encrypting and decrypting into memory alongside
> > > the CAAM driver.
> > >
> > > They will be used in a later commit as a source for the trusted key
> > > seal/unseal mechanism.
> > >
> > > Reviewed-by: David Gstir <[email protected]>
> > > Reviewed-by: Pankaj Gupta <[email protected]>
> > > Tested-By: Tim Harvey <[email protected]>
> > > Tested-by: Matthias Schiffer <[email protected]>
> > > Signed-off-by: Steffen Trumtrar <[email protected]>
> > > Signed-off-by: Ahmad Fatoum <[email protected]>
> > > ---
> > > To: "Horia Geantă" <[email protected]>
> > > To: Aymen Sghaier <[email protected]>
> > > To: Herbert Xu <[email protected]>
> > > To: "David S. Miller" <[email protected]>
> > > Cc: James Bottomley <[email protected]>
> > > Cc: Jarkko Sakkinen <[email protected]>
> > > Cc: Mimi Zohar <[email protected]>
> > > Cc: David Howells <[email protected]>
> > > Cc: James Morris <[email protected]>
> > > Cc: Eric Biggers <[email protected]>
> > > Cc: "Serge E. Hallyn" <[email protected]>
> > > Cc: Jan Luebbe <[email protected]>
> > > Cc: David Gstir <[email protected]>
> > > Cc: Richard Weinberger <[email protected]>
> > > Cc: Franck LENORMAND <[email protected]>
> > > Cc: Sumit Garg <[email protected]>
> > > Cc: Tim Harvey <[email protected]>
> > > Cc: Matthias Schiffer <[email protected]>
> > > Cc: Pankaj Gupta <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > ---
> > > drivers/crypto/caam/Kconfig | 3 +
> > > drivers/crypto/caam/Makefile | 1 +
> > > drivers/crypto/caam/blob_gen.c | 230 +++++++++++++++++++++++++++++++++
> > > include/soc/fsl/caam-blob.h | 56 ++++++++
> > > 4 files changed, 290 insertions(+)
> > > create mode 100644 drivers/crypto/caam/blob_gen.c
> > > create mode 100644 include/soc/fsl/caam-blob.h
> > >
> > > diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
> > > index 84ea7cba5ee5..ea9f8b1ae981 100644
> > > --- a/drivers/crypto/caam/Kconfig
> > > +++ b/drivers/crypto/caam/Kconfig
> > > @@ -151,6 +151,9 @@ config CRYPTO_DEV_FSL_CAAM_RNG_API
> > > Selecting this will register the SEC4 hardware rng to
> > > the hw_random API for supplying the kernel entropy pool.
> > >
> > > +config CRYPTO_DEV_FSL_CAAM_BLOB_GEN
> > > + bool
> > > +
> > > endif # CRYPTO_DEV_FSL_CAAM_JR
> > >
> > > endif # CRYPTO_DEV_FSL_CAAM
> > > diff --git a/drivers/crypto/caam/Makefile b/drivers/crypto/caam/Makefile
> > > index 3570286eb9ce..25f7ae5a4642 100644
> > > --- a/drivers/crypto/caam/Makefile
> > > +++ b/drivers/crypto/caam/Makefile
> > > @@ -21,6 +21,7 @@ caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI) += caamalg_qi.o
> > > caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_AHASH_API) += caamhash.o
> > > caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API) += caamrng.o
> > > caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_PKC_API) += caampkc.o pkc_desc.o
> > > +caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_BLOB_GEN) += blob_gen.o
> > >
> > > caam-$(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI) += qi.o
> > > ifneq ($(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI),)
> > > diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
> > > new file mode 100644
> > > index 000000000000..513d3f90e438
> > > --- /dev/null
> > > +++ b/drivers/crypto/caam/blob_gen.c
> > > @@ -0,0 +1,230 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2015 Pengutronix, Steffen Trumtrar <[email protected]>
> > > + * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <[email protected]>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <soc/fsl/caam-blob.h>
> > > +
> > > +#include "compat.h"
> > > +#include "desc_constr.h"
> > > +#include "desc.h"
> > > +#include "error.h"
> > > +#include "intern.h"
> > > +#include "jr.h"
> > > +#include "regs.h"
> > > +
> > > +struct caam_blob_priv {
> > > + struct device jrdev;
> > > +};
> > > +
> > > +struct caam_blob_job_result {
> > > + int err;
> > > + struct completion completion;
> > > +};
> > > +
> > > +static void caam_blob_job_done(struct device *dev, u32 *desc, u32 err, void *context)
> > > +{
> > > + struct caam_blob_job_result *res = context;
> > > + int ecode = 0;
> > > +
> > > + dev_dbg(dev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
> > > +
> > > + if (err)
> > > + ecode = caam_jr_strstatus(dev, err);
> > > +
> > > + res->err = ecode;
> > > +
> > > + /*
> > > + * Upon completion, desc points to a buffer containing a CAAM job
> > > + * descriptor which encapsulates data into an externally-storable
> > > + * blob.
> > > + */
> > > + complete(&res->completion);
> > > +}
> > > +
> > > +static u32 *caam_blob_alloc_desc(size_t keymod_len)
> > > +{
> > > + size_t len;
> > > +
> > > + /* header + (key mod immediate) + 2x pointers + op */
> > > + len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + CAAM_PTR_SZ_MAX) + 4;
> >
> > Nit: the amount of magic numbers is overwhelming here. I neither understand
> > the statement nor how that comment should help me to understand it.
>
> header = 4
> (key mod immediate) = (4 + ALIGN(keymod_len, 4))
> 2x pointer = 2 * (4 + 4 + CAAM_PTR_SZ_MAX)
> op = 4
Please create a struct with the associated fields instead and then
it is just sizeof that.
BR, Jarkko
Hello Jarkko,
On 28.02.22 13:14, Jarkko Sakkinen wrote:
> On Wed, 2022-02-23 at 17:20 +0100, Ahmad Fatoum wrote:
>> On 23.02.22 16:41, Jarkko Sakkinen wrote:
>>> On Tue, Feb 22, 2022 at 08:58:18PM +0100, Ahmad Fatoum wrote:
>>>> + /* header + (key mod immediate) + 2x pointers + op */
>>>> + len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + CAAM_PTR_SZ_MAX) + 4;
>>>
>>> Nit: the amount of magic numbers is overwhelming here. I neither understand
>>> the statement nor how that comment should help me to understand it.
>>
>> header = 4
>> (key mod immediate) = (4 + ALIGN(keymod_len, 4))
>> 2x pointer = 2 * (4 + 4 + CAAM_PTR_SZ_MAX)
>> op = 4
>
> Please create a struct with the associated fields instead and then
> it is just sizeof that.
The CAAM descriptor construction code writes the current length
to the first word and then macros are used to append fields to
the descriptor while incrementing the length. The key modifier
is in the middle of the descriptor, so this isn't representable
as a C struct without using VLAs, which are banned in the kernel.
Even if it worked, it's arguably not good style to define a struct
just to compute the size and then cast it to a u32 * to use the
same macros all other CAAM code uses.
I thought this over a bit and figured that we don't really need to
compute the length dynamically as it's at most 44 or 52 bytes
(depending on DMA address width) anyway, so we can just use a fixed
value. I still keep the addition, because I believe there is a value
in seeing how we arrive at the 44 or 52 bytes instead of just hardcoding
it.
Just sent out v6 with these changes. Let me know what you think.
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |