Re: [PATCH] tpm2: Export tpm2_get_cc_attrs_tbl for ibmvtpm driver as module
On Tue, Mar 17, 2020 at 09:08:19AM -0400, Stefan Berger wrote: > From: Stefan Berger > > This patch fixes the following problem when the ibmvtpm driver > is built as a module: > > ERROR: modpost: "tpm2_get_cc_attrs_tbl" [drivers/char/tpm/tpm_ibmvtpm.ko] > undefined! > make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1 > make: *** [Makefile:1298: modules] Error 2 > > Signed-off-by: Stefan Berger Hi, wrong tag (we use "tpm:"), missing fixes tag and please cc stable. Thanks. /Jarkko
Re: [PATCH] tpm2: Export tpm2_get_cc_attrs_tbl for ibmvtpm driver as module
On Wed, Mar 18, 2020 at 03:53:54PM -0400, Stefan Berger wrote: > On 3/18/20 3:42 PM, Jarkko Sakkinen wrote: > > On Tue, Mar 17, 2020 at 09:08:19AM -0400, Stefan Berger wrote: > > > From: Stefan Berger > > > > > > This patch fixes the following problem when the ibmvtpm driver > > > is built as a module: > > > > > > ERROR: modpost: "tpm2_get_cc_attrs_tbl" [drivers/char/tpm/tpm_ibmvtpm.ko] > > > undefined! > > > make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1 > > > make: *** [Makefile:1298: modules] Error 2 > > > > > > Signed-off-by: Stefan Berger > > Hi, wrong tag (we use "tpm:"), missing fixes tag and please cc stable. > > Thanks. > > I did not add the fixes tag because I do not know the final commit hash, or > is it the final commit hash once it is in linux-next? I doubt it with all > the merging that can occur. Can you send me a new version after rc1 is out? /Jarkko
Re: [PATCH v2] qtpm2: Export tpm2_get_cc_attrs_tbl for ibmvtpm driver as module
On Wed, Mar 18, 2020 at 09:00:17PM -0400, Stefan Berger wrote:
> From: Stefan Berger
>
> This patch fixes the following problem when the ibmvtpm driver
> is built as a module:
>
> ERROR: modpost: "tpm2_get_cc_attrs_tbl" [drivers/char/tpm/tpm_ibmvtpm.ko]
> undefined!
> make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1
> make: *** [Makefile:1298: modules] Error 2
>
> Fixes: 18b3670d79ae ("tpm: ibmvtpm: Add support for TPM2")
> Signed-off-by: Stefan Berger
> Reported-by: Sachin Sant
> Tested-by: Sachin Sant
Should have "tpm:" tag in the short summary.
/Jarkko
Re: [PATCH] tpm2: Export tpm2_get_cc_attrs_tbl for ibmvtpm driver as module
On Thu, Mar 19, 2020 at 11:56:11AM -0400, Stefan Berger wrote: > On 3/19/20 10:27 AM, Jarkko Sakkinen wrote: > > On Wed, Mar 18, 2020 at 03:53:54PM -0400, Stefan Berger wrote: > > > On 3/18/20 3:42 PM, Jarkko Sakkinen wrote: > > > > On Tue, Mar 17, 2020 at 09:08:19AM -0400, Stefan Berger wrote: > > > > > From: Stefan Berger > > > > > > > > > > This patch fixes the following problem when the ibmvtpm driver > > > > > is built as a module: > > > > > > > > > > ERROR: modpost: "tpm2_get_cc_attrs_tbl" > > > > > [drivers/char/tpm/tpm_ibmvtpm.ko] undefined! > > > > > make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1 > > > > > make: *** [Makefile:1298: modules] Error 2 > > > > > > > > > > Signed-off-by: Stefan Berger > > > > Hi, wrong tag (we use "tpm:"), missing fixes tag and please cc stable. > > > > Thanks. > > > I did not add the fixes tag because I do not know the final commit hash, > > > or > > > is it the final commit hash once it is in linux-next? I doubt it with all > > > the merging that can occur. > > Can you send me a new version after rc1 is out? > > Michael Ellerman (cc'ed) told me that the fixes tag should 'work' once the > bad patch is in linux-next. So I reposted yesterday (with a stray 'q' in the > title :-( ): OK, cool, I'll correct it and apply thanks. Reviewed-by: Jarkko Sakkinen /Jarkko
Re: [PATCH v2] qtpm2: Export tpm2_get_cc_attrs_tbl for ibmvtpm driver as module
On Wed, Apr 01, 2020 at 02:40:30PM +0530, Sachin Sant wrote:
>
>
> > On 20-Mar-2020, at 1:27 AM, Jarkko Sakkinen
> > wrote:
> >
> > On Wed, Mar 18, 2020 at 09:00:17PM -0400, Stefan Berger wrote:
> >> From: Stefan Berger
> >>
> >> This patch fixes the following problem when the ibmvtpm driver
> >> is built as a module:
> >>
> >> ERROR: modpost: "tpm2_get_cc_attrs_tbl" [drivers/char/tpm/tpm_ibmvtpm.ko]
> >> undefined!
> >> make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1
> >> make: *** [Makefile:1298: modules] Error 2
> >>
> >> Fixes: 18b3670d79ae ("tpm: ibmvtpm: Add support for TPM2")
> >> Signed-off-by: Stefan Berger
> >> Reported-by: Sachin Sant
> >> Tested-by: Sachin Sant
> >
>
> Ping. This failure can now be seen in mainline (cad18da0af) as well.
It is in my tree
/Jarkko
Re: [PATCH v2] qtpm2: Export tpm2_get_cc_attrs_tbl for ibmvtpm driver as module
On Tue, Apr 14, 2020 at 10:51:37AM +1000, Michael Ellerman wrote:
> Jarkko Sakkinen writes:
> > On Wed, Apr 01, 2020 at 02:40:30PM +0530, Sachin Sant wrote:
> >> > On 20-Mar-2020, at 1:27 AM, Jarkko Sakkinen
> >> > wrote:
> >> >
> >> > On Wed, Mar 18, 2020 at 09:00:17PM -0400, Stefan Berger wrote:
> >> >> From: Stefan Berger
> >> >>
> >> >> This patch fixes the following problem when the ibmvtpm driver
> >> >> is built as a module:
> >> >>
> >> >> ERROR: modpost: "tpm2_get_cc_attrs_tbl"
> >> >> [drivers/char/tpm/tpm_ibmvtpm.ko] undefined!
> >> >> make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1
> >> >> make: *** [Makefile:1298: modules] Error 2
> >> >>
> >> >> Fixes: 18b3670d79ae ("tpm: ibmvtpm: Add support for TPM2")
> >> >> Signed-off-by: Stefan Berger
> >> >> Reported-by: Sachin Sant
> >> >> Tested-by: Sachin Sant
> >> >
> >>
> >> Ping. This failure can now be seen in mainline (cad18da0af) as well.
> >
> > It is in my tree
>
> Can you please send it to Linus?
>
> cheers
Yes.
/Jarkko
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:
> The nr_allocated_banks and allocated banks are initialized as part of
> tpm_chip_register. Currently, this is done as part of auto startup
> function. However, some drivers, like the ibm vtpm driver, do not run
> auto startup during initialization. This results in uninitialized memory
> issue and causes a kernel panic during boot.
>
> This patch moves the pcr allocation outside the auto startup function
> into tpm_chip_register. This ensures that allocated banks are initialized
> in any case.
>
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
> PCR read")
> Signed-off-by: Nayna Jain
Please add
Reported-by: Michal Suchanek
It is missing. Michal is there a chance you could try this out once
Nayna send a new version?
> ---
> drivers/char/tpm/tpm-chip.c | 37 +
> drivers/char/tpm/tpm.h | 1 +
> drivers/char/tpm/tpm1-cmd.c | 12
> drivers/char/tpm/tpm2-cmd.c | 6 +-
> 4 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 8804c9e916fd..958508bb8379 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
> return hwrng_register(&chip->hwrng);
> }
>
> +/*
> + * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs
> + */
> +static int tpm_pcr_allocation(struct tpm_chip *chip)
Why that name and not tpm_get_pcr_allocation()? Do not get why
"get_" has been dropped. Please add it back.
Would be senseful to create tpm1_get_pcr_allocation() to tpm1-cmd.c
now that a new function needs to be introduced anyway. Please do
it for that for TPM 1.x part.
/Jarkko
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On Fri, 2019-07-05 at 13:42 +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:
> > The nr_allocated_banks and allocated banks are initialized as part of
> > tpm_chip_register. Currently, this is done as part of auto startup
> > function. However, some drivers, like the ibm vtpm driver, do not run
> > auto startup during initialization. This results in uninitialized memory
> > issue and causes a kernel panic during boot.
> >
> > This patch moves the pcr allocation outside the auto startup function
> > into tpm_chip_register. This ensures that allocated banks are initialized
> > in any case.
> >
> > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
> > PCR read")
> > Signed-off-by: Nayna Jain
>
> Please add
>
> Reported-by: Michal Suchanek
>
> It is missing. Michal is there a chance you could try this out once
> Nayna send a new version?
Hey, I saw Michal's tested-by. I can do that minor reorg cosmetic
bits myeslf and add Micha's tag.
Some issue with the network but I'll push a commit soonish.
/Jarkko
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On Fri, 2019-07-05 at 11:32 -0400, Nayna wrote:
> I am not sure of the purpose of tpm_stop_chip(), so I have left it as it
> is. Jarkko, what do you think about the change ?
Stefan right. Your does not work, or will randomly work or not work
depending on the chip.
You need to turn the TPM on with tpm_chip_start() and turn it off with
tpm_chip_stop() once you are done. This is done in tpm_chip_register()
before calling tpm_auto_startup().
TPM power management was once in tpm_transmit() but not anymore after my
patch set that removed nested tpm_transmit() calls.
While you're on it please take into account my earlier feedback.
Also, short summary could be "tpm: tpm_ibm_vtpm: Fix unallocated banks"
Some oddballs in your patch that I have to ask.
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
rc = tpm2_get_pcr_allocation(chip);
if (rc)
goto out;
}
chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
GFP_KERNEL);
if (!chip->allocated_banks) {
rc = -ENOMEM;
goto out;
}
Why you don't return on site and instead jump somewhere? Also the
2nd line for kcalloc() is misaligned.
out:
if (rc < 0)
rc = -ENODEV;
This will cause a new regression i.e. you let TPM error codes
through.
To summarize this patch fixes one regression and introduces two
completely new ones...
/Jarkko`
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On Fri, 2019-07-05 at 20:50 +0300, Jarkko Sakkinen wrote: > To summarize this patch fixes one regression and introduces two > completely new ones... Anyway, take the time and update it. The principle is right anyway. I'll merge it once it is in a better shape... /Jarkko
Re: [PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
On Sat, 2019-07-06 at 20:18 -0400, Nayna Jain wrote:
> +/*
> + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs
> + * @chip: TPM chip to use.
> + */
> +static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> +{
> + int rc;
> +
> + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + rc = tpm2_get_pcr_allocation(chip);
> + else
> + rc = tpm1_get_pcr_allocation(chip);
> +
> + return rc;
> +}
It is just a trivial static function, which means that kdoc comment is
not required and neither it is useful. Please remove that. I would
rewrite the function like:
static int tpm_get_pcr_allocation(struct tpm_chip *chip)
{
int rc;
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
tpm2_get_pcr_allocation(chip) :
tpm1_get_pcr_allocation(chip);
return rc > 0 ? -ENODEV : rc;
}
This addresses the issue that Stefan also pointed out. You have to
deal with the TPM error codes.
/Jarkko
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On Sat, 2019-07-06 at 20:25 -0400, Nayna wrote: > Thanks Jarkko. I just now posted the v2 version that includes your and > Stefan's feedbacks. Looks almost legit :-) /Jarkko
Re: [PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote:
> > static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> > {
> > int rc;
> >
> > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
> > tpm2_get_pcr_allocation(chip) :
> > tpm1_get_pcr_allocation(chip);
>
> >
> > return rc > 0 ? -ENODEV : rc;
> > }
> >
> > This addresses the issue that Stefan also pointed out. You have to
> > deal with the TPM error codes.
>
> Hm, in the past I was told by Christoph not to use the ternary
> operator. Have things changed? Other than removing the comment, the
> only other difference is the return.
Lets purge the snippet:
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
tpm2_get_pcr_allocation(chip) :
tpm1_get_pcr_allocation(chip);
In this statement ternary operator does make sense because it is the
most readable way to express what is going on. We assign something
selecting one of the two options as the value of rc based on a
condition.
It is like a natural fit for a ternary-operation and less messy than two
assigment statements.
On the other hand:
return rc > 0 ? -ENODEV : rc;
Here a better form would definitely be:
if (rc > 0)
return - ENODEV;
return rc;
It is just two hard to grasp the logic when ternary operation is used.
Total ban of any language construct would be just utterly stupid. I
would instead use common sense here.
/Jarkko
Re: [PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
On Mon, Jul 08, 2019 at 03:43:04PM -0700, Christoph Hellwig wrote:
> On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote:
> > > static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> > > {
> > > int rc;
> > >
> > > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
> > >tpm2_get_pcr_allocation(chip) :
> > >tpm1_get_pcr_allocation(chip);
> >
> > >
> > > return rc > 0 ? -ENODEV : rc;
> > > }
> > >
> > > This addresses the issue that Stefan also pointed out. You have to
> > > deal with the TPM error codes.
> >
> > Hm, in the past I was told by Christoph not to use the ternary
> > operator. Have things changed? Other than removing the comment, the
> > only other difference is the return.
>
> In the end it is a matter of personal preference, but I find the
> quote version above using the ternary horribly obsfucated.
I fully agree that the return statement is an obsfucated mess and
not a good place at all for using ternary operator.
/Jarkko
Re: [PATCH v3] tpm: tpm_ibm_vtpm: Fix unallocated banks
On Thu, Jul 11, 2019 at 12:13:35PM -0400, Nayna Jain wrote:
> The nr_allocated_banks and allocated banks are initialized as part of
> tpm_chip_register. Currently, this is done as part of auto startup
> function. However, some drivers, like the ibm vtpm driver, do not run
> auto startup during initialization. This results in uninitialized memory
> issue and causes a kernel panic during boot.
>
> This patch moves the pcr allocation outside the auto startup function
> into tpm_chip_register. This ensures that allocated banks are initialized
> in any case.
>
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
> PCR read")
> Reported-by: Michal Suchanek
> Signed-off-by: Nayna Jain
> Reviewed-by: Mimi Zohar
> Tested-by: Sachin Sant
> Tested-by: Michal Suchánek
Reviewed-by: Jarkko Sakkinen
/Jarkko
Re: [PATCH v3] tpm: tpm_ibm_vtpm: Fix unallocated banks
On Thu, Jul 11, 2019 at 11:28:24PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 12:13:35PM -0400, Nayna Jain wrote:
> > The nr_allocated_banks and allocated banks are initialized as part of
> > tpm_chip_register. Currently, this is done as part of auto startup
> > function. However, some drivers, like the ibm vtpm driver, do not run
> > auto startup during initialization. This results in uninitialized memory
> > issue and causes a kernel panic during boot.
> >
> > This patch moves the pcr allocation outside the auto startup function
> > into tpm_chip_register. This ensures that allocated banks are initialized
> > in any case.
> >
> > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
> > PCR read")
> > Reported-by: Michal Suchanek
> > Signed-off-by: Nayna Jain
> > Reviewed-by: Mimi Zohar
> > Tested-by: Sachin Sant
> > Tested-by: Michal Suchánek
>
> Reviewed-by: Jarkko Sakkinen
Thanks a lot! It is applied now.
/Jarkko
Re: [PATCH] drivers: char: tpm: remove unneeded MODULE_VERSION() usage
On Mon, 2019-12-16 at 09:42 +0100, Enrico Weigelt, metux IT consult wrote: > Remove MODULE_VERSION(), as it isn't needed at all: the only version > making sense is the kernel version. Take the following line away: > See also: https://lkml.org/lkml/2017/11/22/480 And just before SOB: Link: https://lkml.org/lkml/2017/11/22/480 > Signed-off-by: Enrico Weigelt, metux IT consult You have some extra cruft there. It should be: Signed-off-by: Enrico Weigelt /Jarkko
Re: [PATCH] drivers: char: tpm: remove unneeded MODULE_VERSION() usage
On Tue, 2019-12-17 at 13:16 +0200, Jarkko Sakkinen wrote: > On Mon, 2019-12-16 at 09:42 +0100, Enrico Weigelt, metux IT consult wrote: > > Remove MODULE_VERSION(), as it isn't needed at all: the only version > > making sense is the kernel version. > > Take the following line away: > > > See also: https://lkml.org/lkml/2017/11/22/480 > > And just before SOB: > > Link: https://lkml.org/lkml/2017/11/22/480 > > Signed-off-by: Enrico Weigelt, metux IT consult > > You have some extra cruft there. It should be: > > Signed-off-by: Enrico Weigelt Also, the email that you are sending this patch from incorrectly formatted email address. Please configure your email client to have just Firstname Lastname as the email. /Jarkko
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > Adding more CC's from the original patch, looks like get_maintainers is > > > not that great for this file. > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > This is policy that should be decided by the administrator, not Kconfig > > > > dependencies. > > > > We certainly agree that flexibility is important. However, in this case, > > this also implies that we are expecting system admins to be security > > experts. As per our understanding, CA based infrastructure(PKI) is the > > standard to be followed and not the policy decision. And we can only speak > > for Power. > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed leaf > > certs. > > And that's the problem. > > From a distribution point of view there are two types of leaf certs: > > - leaf certs signed by the distribution CA which need not be imported >because the distribution CA cert is enrolled one way or another > - user generated ad-hoc certificates that are not signed in any way, >and enrolled by the user > > The latter are vouched for by the user by enrolling the certificate, and > confirming that they really want to trust this certificate. Enrolling > user certificates is vital for usability or secure boot. Adding extra > step of creating a CA certificate stored on the same system only > complicates things with no added benefit. This all comes down to the generic fact that kernel should not proactively define what it *expects* sysadmins. CA based infrastructure like anything is a policy decision not a decision to be enforced by kernel. BR, Jarkko
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue Sep 12, 2023 at 6:39 AM EEST, Nayna wrote: > > On 9/7/23 13:32, Michal Suchánek wrote: > > Adding more CC's from the original patch, looks like get_maintainers is > > not that great for this file. > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > >> No other platform needs CA_MACHINE_KEYRING, either. > >> > >> This is policy that should be decided by the administrator, not Kconfig > >> dependencies. > > We certainly agree that flexibility is important. However, in this case, > this also implies that we are expecting system admins to be security > experts. As per our understanding, CA based infrastructure(PKI) is the > standard to be followed and not the policy decision. And we can only > speak for Power. In the end this is dictating policy for no compelling reason, and that is the bottom line here, not playing a mind game what type of expertise a sysadmin might or might not have. BR, Jarkko
Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
> DCP is capable to performing AES with hardware-bound keys.
> These keys are not stored in main memory and are therefore not directly
> accessible by the operating system.
>
> So instead of feeding the key into DCP, we need to place a
> reference to such a key before initiating the crypto operation.
> Keys are referenced by a one byte identifiers.
>
> DCP supports 6 different keys: 4 slots in the secure memory area,
> a one time programmable key which can be burnt via on-chip fuses
> and an unique device key.
>
> Using these keys is restricted to in-kernel users that use them as building
> block for other crypto tools such as trusted keys. Allowing userspace
> (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security
> risk, because there is no access control mechanism.
>
> 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
> ---
> drivers/crypto/mxs-dcp.c | 107 +++
> include/soc/fsl/dcp.h| 19 +++
> 2 files changed, 115 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..d525cb41f2ca 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];
> + boolrefkey;
> };
>
> 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->refkey;
> 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,13 @@ 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;
> + }
Remove curly braces (coding style).
>
> if (rctx->enc)
> desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -270,6 +283,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 +300,10 @@ 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_
Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolrefkey;
> };
s/refkey/key_referenced/
No reason to obfuscate it, especially since there is no supporting
documentation.
BR, Jarkko
Re: [PATCH v2 2/3] KEYS: trusted: Introduce support for NXP DCP-based trusted keys
On Tue Sep 12, 2023 at 2:11 PM EEST, 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 just like CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software.
>
> We chose the following format for the blob:
> /*
> * 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;
>
> @payload is the key provided by trusted_key_ops->seal().
>
> By default the UNIQUE device key is used, it is also possible to use
> the OTP key. While the UNIQUE device key should be unique it is not
> entirely clear whether this is the case due to unclear documentation.
> If someone wants to be sure they can burn their own unique key
> into the OTP fuse and set 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
> ---
> .../admin-guide/kernel-parameters.txt | 13 +
> MAINTAINERS | 9 +
> include/keys/trusted_dcp.h| 13 +
> security/keys/trusted-keys/Kconfig| 9 +-
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 313 ++
> 7 files changed, 363 insertions(+), 2 deletions(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..c11eda8b38e0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6566,6 +6566,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
> @@ -6581,6 +6582,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
> 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
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..7b2a1275c527
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
>
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue Sep 12, 2023 at 10:22 PM EEST, Mimi Zohar wrote: > On Tue, 2023-09-12 at 12:49 +0300, Jarkko Sakkinen wrote: > > On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > > > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > > > Adding more CC's from the original patch, looks like get_maintainers > > > > > is > > > > > not that great for this file. > > > > > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > > > > > This is policy that should be decided by the administrator, not > > > > > > Kconfig > > > > > > dependencies. > > > > > > > > We certainly agree that flexibility is important. However, in this case, > > > > this also implies that we are expecting system admins to be security > > > > experts. As per our understanding, CA based infrastructure(PKI) is the > > > > standard to be followed and not the policy decision. And we can only > > > > speak > > > > for Power. > > > > > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed leaf > > > > certs. > > > > > > And that's the problem. > > > > > > From a distribution point of view there are two types of leaf certs: > > > > > > - leaf certs signed by the distribution CA which need not be imported > > >because the distribution CA cert is enrolled one way or another > > > - user generated ad-hoc certificates that are not signed in any way, > > >and enrolled by the user > > > > > > The latter are vouched for by the user by enrolling the certificate, and > > > confirming that they really want to trust this certificate. Enrolling > > > user certificates is vital for usability or secure boot. Adding extra > > > step of creating a CA certificate stored on the same system only > > > complicates things with no added benefit. > > > > This all comes down to the generic fact that kernel should not > > proactively define what it *expects* sysadmins. > > > > CA based infrastructure like anything is a policy decision not > > a decision to be enforced by kernel. > > Secure boot requires a signature chain of trust. IMA extends the > secure and trusted boot concepts to the kernel. Missing from that > signature chain of trust is the ability of allowing the end > machine/system owner to load other certificates without recompiling the > kernel. The introduction of the machine keyring was to address this. > > I'm not questioning the end user's intent on loading local or third > party keys via the normal mechanisms. If the existing mechanism(s) for > loading local or third party keys were full-proof, then loading a > single certificate, self-signed or not, would be fine. However, that > isn't the reality. The security of the two-stage approach is simply > not equivalent to loading a single certificate. Documentation could > help the end user/system owner to safely create (and manage) separate > certificate signing and code signing certs. > > Unlike UEFI based systems, PowerVM defines two variables trustedcadb > and moduledb, for storing certificate signing and code signing > certificates respectively. First the certs on the trustedcadb are > loaded and then the ones on moduledb are loaded. There's pragmatic reasons to make things more open than they should be in production. As a hardware example I still possess Raspberry Pi 3B for test workloads because it has a broken TZ implementation. The world is really bigger than production workloads. It would be better to document what you said rather than enforce the right choice IMHO (e.g. extend Kconfig documentation). BR, Jarkko
Re: [PATCH v3 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Mon Sep 18, 2023 at 5:18 PM EEST, David Gstir wrote: > DCP is capable to performing AES with hardware-bound keys. > These keys are not stored in main memory and are therefore not directly > accessible by the operating system. > > So instead of feeding the key into DCP, we need to place a > reference to such a key before initiating the crypto operation. > Keys are referenced by a one byte identifiers. Not sure what the action of feeding key into DCP even means if such action does not exists. What you probably would want to describe here is how keys get created and how they are referenced by the kernel. For the "use" part please try to avoid academic paper style long expression starting with "we" pronomine. So the above paragraph would normalize into "The keys inside DCP are referenced by one byte identifier". Here of course would be for the context nice to know what is this set of DCP keys. E.g. are total 256 keys or some subset? When using too much prose there can be surprsingly little digestable information, thus this nitpicking. > DCP supports 6 different keys: 4 slots in the secure memory area, > a one time programmable key which can be burnt via on-chip fuses > and an unique device key. > > Using these keys is restricted to in-kernel users that use them as building > block for other crypto tools such as trusted keys. Allowing userspace > (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security > risk, because there is no access control mechanism. Unless this patch has anything else than trusted keys this should not be an open-ended sentence. You want to say roughly that DCP hardware keys are implemented for the sake to implement trusted keys support, and exactly and only that. This description also lacks actions taken by the code changes below, which is really the beef of any commit description. BR, Jarkko
Re: [PATCH v3 2/3] KEYS: trusted: Introduce support for NXP DCP-based trusted keys
On Mon Sep 18, 2023 at 5:18 PM EEST, 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 just like CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software.
>
> We chose the following format for the blob:
Who is we?
And there is no choosing anything if the below structure if hardware
defined (not software defined):
> /*
> * 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;
>
> @payload is the key provided by trusted_key_ops->seal().
>
> By default the UNIQUE device key is used, it is also possible to use
> the OTP key. While the UNIQUE device key should be unique it is not
> entirely clear whether this is the case due to unclear documentation.
> If someone wants to be sure they can burn their own unique key
> into the OTP fuse and set 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
> ---
> .../admin-guide/kernel-parameters.txt | 13 +
Separate commit for this.
> MAINTAINERS | 9 +
Ditto (i.e. total two additional patches).
> include/keys/trusted_dcp.h| 11 +
> security/keys/trusted-keys/Kconfig| 9 +-
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 311 ++
> 7 files changed, 359 insertions(+), 2 deletions(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..c11eda8b38e0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6566,6 +6566,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
> @@ -6581,6 +6582,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
> 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
> diff --git a/include/keys/trusted_dcp.h b/include/keys/tr
Re: [PATCH v3 3/3] doc: trusted-encrypted: add DCP as new trust source
On Mon Sep 18, 2023 at 5:18 PM EEST, 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 9bc9db8ec651..4452070afbe9 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. "When a new trusted key using DCP is created, a random 128-bit blob encryption key (BEK) and 128-bit nonce are generated." ... or along the lines. BR, Jarkko
Re: [PATCH v3 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Wed Sep 27, 2023 at 9:25 AM EEST, David Gstir wrote: > Jarkko, > > > On 25.09.2023, at 17:22, Jarkko Sakkinen wrote: > > > > On Mon Sep 18, 2023 at 5:18 PM EEST, David Gstir wrote: > >> DCP is capable to performing AES with hardware-bound keys. > >> These keys are not stored in main memory and are therefore not directly > >> accessible by the operating system. > >> > >> So instead of feeding the key into DCP, we need to place a > >> reference to such a key before initiating the crypto operation. > >> Keys are referenced by a one byte identifiers. > > > > Not sure what the action of feeding key into DCP even means if such > > action does not exists. > > > > What you probably would want to describe here is how keys get created > > and how they are referenced by the kernel. > > > > For the "use" part please try to avoid academic paper style long > > expression starting with "we" pronomine. > > > > So the above paragraph would normalize into "The keys inside DCP > > are referenced by one byte identifier". Here of course would be > > for the context nice to know what is this set of DCP keys. E.g. > > are total 256 keys or some subset? > > > > When using too much prose there can be surprsingly little digestable > > information, thus this nitpicking. > > Thanks for reviewing that in detail! I’ll rephrase the commit > messages on all patches to get rid of the academic paper style. > > > > > >> DCP supports 6 different keys: 4 slots in the secure memory area, > >> a one time programmable key which can be burnt via on-chip fuses > >> and an unique device key. > >> > >> Using these keys is restricted to in-kernel users that use them as building > >> block for other crypto tools such as trusted keys. Allowing userspace > >> (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security > >> risk, because there is no access control mechanism. > > > > Unless this patch has anything else than trusted keys this should not > > be an open-ended sentence. You want to say roughly that DCP hardware > > keys are implemented for the sake to implement trusted keys support, > > and exactly and only that. > > > > This description also lacks actions taken by the code changes below, > > which is really the beef of any commit description. > > You’re right. I’ll add that. Yup, I'm just doing my part of the job, as I'm expected to do it :-) Thanks for understanding. > Thanks, > - David BR, Jarkko
Re: [PATCH v4 1/5] crypto: mxs-dcp: Add support for hardware-bound keys
On Tue Oct 24, 2023 at 7:20 PM EEST, 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
> 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
> ---
> 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_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 in
Re: [PATCH v4 2/5] KEYS: trusted: Introduce NXP DCP-backed trusted keys
On Tue Oct 24, 2023 at 7:20 PM EEST, 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| 9 +-
> 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(+), 2 deletions(-)
> 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 dbfdd8536468..c6b80b7e5c78 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -33,6 +33,13 @@ config TRUSTED_KEYS_CAAM
> 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
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + help
> + Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
> +if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM &&
> !TRUSTED_KEYS_DCP
This does not scale tbh.
I'd suggest to add additional patch before adding the new key type,
which clears this up a little bit.
First:
config HAVE_TRUSTED_KEYS
bool
And then following this pattern to all trusted key types:
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.
And finally:
if !HAVE_TRUSTED_KEYS
comment "No trust source selected!"
endif
BR, Jarkko
Re: [PATCH v9 2/4] tpm: of: Make of-tree specific function commonly available
On Fri, 2023-06-09 at 14:49 -0400, Stefan Berger wrote: > > On 6/9/23 14:18, Jarkko Sakkinen wrote: > > On Thu May 25, 2023 at 1:56 AM EEST, Jerry Snitselaar wrote: > > > On Tue, Apr 18, 2023 at 09:44:07AM -0400, Stefan Berger wrote: > > > > Simplify tpm_read_log_of() by moving reusable parts of the code into > > > > an inline function that makes it commonly available so it can be > > > > used also for kexec support. Call the new of_tpm_get_sml_parameters() > > > > function from the TPM Open Firmware driver. > > > > > > > > Signed-off-by: Stefan Berger > > > > Cc: Jarkko Sakkinen > > > > Cc: Jason Gunthorpe > > > > Cc: Rob Herring > > > > Cc: Frank Rowand > > > > Reviewed-by: Mimi Zohar > > > > Tested-by: Nageswara R Sastry > > > > Tested-by: Coiby Xu > > > > Acked-by: Jarkko Sakkinen > > > > > > > > > > Reviewed-by: Jerry Snitselaar > > > > If I just pick tpm only patches they won't apply so maybe TPM changes > > should be better separated if that is by any means possible. > > Per the comment here I am putting this series here on hold. > https://lore.kernel.org/linux-integrity/20230418134409.177485-1-stef...@linux.ibm.com/T/#m03745c2af2c46f19f329522fcb6ccb2bf2eaedc7 Hi, sorry for late response. The Midsummer weekend really messed my schedules (it is a big thing Finland). This year the timing with the kernel cycle has been conflicting. OK cool. BR, Jarkko
Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> Measurement Log) from OF to Linux, before Linux shuts down Open
> Firmware.
>
> This involves creating a buffer to hold the SML, and creating two device
> tree properties to record its base address and size. The kernel then
> later reads those properties from the device tree to find the SML.
>
> When the code was initially added in commit 4a727429abec ("PPC64: Add
> support for instantiating SML from Open Firmware") the powerpc kernel
> was always built big endian, so the properties were created big endian
> by default.
>
> However since then little endian support was added to powerpc, and now
> the code lacks conversions to big endian when creating the properties.
>
> This means on little endian kernels the device tree properties are
> little endian, which is contrary to the device tree spec, and in
> contrast to all other device tree properties.
>
> To cope with that a workaround was added in tpm_read_log_of() to skip
> the endian conversion if the properties were created via the SML
> handover.
>
> A better solution is to encode the properties as big endian as they
> should be, and remove the workaround.
>
> Typically changing the encoding of a property like this would present
> problems for kexec. However the SML is not propagated across kexec, so
> changing the encoding of the properties is a non-issue.
>
> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> event log")
> Signed-off-by: Michael Ellerman
> Reviewed-by: Stefan Berger
> ---
> arch/powerpc/kernel/prom_init.c | 8 ++--
> drivers/char/tpm/eventlog/of.c | 23 ---
> 2 files changed, 10 insertions(+), 21 deletions(-)
Split into two patches (producer and consumer).
BR, Jarkko
>
> v2: Add Stefan's reviewed-by.
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index d464ba412084..72fe306b6820 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
> u32 entry = 0, size = 0, succ = 0;
> u64 base;
> __be32 val;
> + __be64 val64;
>
> prom_debug("prom_instantiate_sml: start...\n");
>
> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
>
> reserve_mem(base, size);
>
> + val64 = cpu_to_be64(base);
> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> - &base, sizeof(base));
> + &val64, sizeof(val64));
> +
> + val = cpu_to_be32(size);
> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> - &size, sizeof(size));
> + &val, sizeof(val));
>
> prom_debug("sml base = 0x%llx\n", base);
> prom_debug("sml size = 0x%x\n", size);
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..0bc0cb6333c6 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> + const __be32 *sizep;
> + const __be64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (sizep == NULL || basep == NULL)
> return -EIO;
>
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + size = be32_to_cpup(sizep);
> + base = be64_to_cpup(basep);
>
> if (size == 0) {
> dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
>
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> > > Firmware.
> > >
> > > This involves creating a buffer to hold the SML, and creating two device
> > > tree properties to record its base address and size. The kernel then
> > > later reads those properties from the device tree to find the SML.
> > >
> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> > > support for instantiating SML from Open Firmware") the powerpc kernel
> > > was always built big endian, so the properties were created big endian
> > > by default.
> > >
> > > However since then little endian support was added to powerpc, and now
> > > the code lacks conversions to big endian when creating the properties.
> > >
> > > This means on little endian kernels the device tree properties are
> > > little endian, which is contrary to the device tree spec, and in
> > > contrast to all other device tree properties.
> > >
> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> > > the endian conversion if the properties were created via the SML
> > > handover.
> > >
> > > A better solution is to encode the properties as big endian as they
> > > should be, and remove the workaround.
> > >
> > > Typically changing the encoding of a property like this would present
> > > problems for kexec. However the SML is not propagated across kexec, so
> > > changing the encoding of the properties is a non-issue.
> > >
> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> > > event log")
> > > Signed-off-by: Michael Ellerman
> > > Reviewed-by: Stefan Berger
> > > ---
> > > arch/powerpc/kernel/prom_init.c | 8 ++--
> > > drivers/char/tpm/eventlog/of.c | 23 ---
> > > 2 files changed, 10 insertions(+), 21 deletions(-)
> >
> > Split into two patches (producer and consumer).
>
> I think this wouldn't be right since it would break the system when only one
> patch is applied since it would be reading the fields in the wrong endianess.
I think it would help if the commit message would better explain
what is going on. It is somewhat difficult to decipher, if you
don't have deep knowledge of the powerpc architecture.
BR, Jarkko
Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Wed Jul 12, 2023 at 12:39 PM UTC, Michael Ellerman wrote:
> Jarkko Sakkinen writes:
> > On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
> >> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> >> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> >> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> >> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> >> > > Firmware.
> >> > >
> >> > > This involves creating a buffer to hold the SML, and creating two
> >> > > device
> >> > > tree properties to record its base address and size. The kernel then
> >> > > later reads those properties from the device tree to find the SML.
> >> > >
> >> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> >> > > support for instantiating SML from Open Firmware") the powerpc kernel
> >> > > was always built big endian, so the properties were created big endian
> >> > > by default.
> >> > >
> >> > > However since then little endian support was added to powerpc, and now
> >> > > the code lacks conversions to big endian when creating the properties.
> >> > >
> >> > > This means on little endian kernels the device tree properties are
> >> > > little endian, which is contrary to the device tree spec, and in
> >> > > contrast to all other device tree properties.
> >> > >
> >> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> >> > > the endian conversion if the properties were created via the SML
> >> > > handover.
> >> > >
> >> > > A better solution is to encode the properties as big endian as they
> >> > > should be, and remove the workaround.
> >> > >
> >> > > Typically changing the encoding of a property like this would present
> >> > > problems for kexec. However the SML is not propagated across kexec, so
> >> > > changing the encoding of the properties is a non-issue.
> >> > >
> >> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical
> >> > > TPM event log")
> >> > > Signed-off-by: Michael Ellerman
> >> > > Reviewed-by: Stefan Berger
> >> > > ---
> >> > > arch/powerpc/kernel/prom_init.c | 8 ++--
> >> > > drivers/char/tpm/eventlog/of.c | 23 ---
> >> > > 2 files changed, 10 insertions(+), 21 deletions(-)
> >> >
> >> > Split into two patches (producer and consumer).
> >>
> >> I think this wouldn't be right since it would break the system when only
> >> one patch is applied since it would be reading the fields in the wrong
> >> endianess.
> >
> > I think it would help if the commit message would better explain
> > what is going on. It is somewhat difficult to decipher, if you
> > don't have deep knowledge of the powerpc architecture.
>
> I mean, it's already 8 paragraphs ¯\_(ツ)_/¯
>
> But I'm happy to expand it. I just don't really know what extra detail
> is needed to make it clearer.
Adding more text is not the right way to clarify things. I'd start
by explaining shortly SML and then move to the handover. It can't
be that hard, right?
Just adding new paragraphs would probably just make it even more
confusing.
BR, Jarkko
Re: [PATCH] char: Explicitly include correct DT includes
; #include > #include > diff --git a/drivers/char/hw_random/omap3-rom-rng.c > b/drivers/char/hw_random/omap3-rom-rng.c > index f06e4f95114f..18dc46b1b58e 100644 > --- a/drivers/char/hw_random/omap3-rom-rng.c > +++ b/drivers/char/hw_random/omap3-rom-rng.c > @@ -20,7 +20,6 @@ > #include > #include > #include > -#include > #include > #include > > diff --git a/drivers/char/hw_random/pasemi-rng.c > b/drivers/char/hw_random/pasemi-rng.c > index 2498d4ef9fe2..6959d6edd44c 100644 > --- a/drivers/char/hw_random/pasemi-rng.c > +++ b/drivers/char/hw_random/pasemi-rng.c > @@ -9,11 +9,10 @@ > > #include > #include > +#include > #include > #include > #include > -#include > -#include > #include > > #define SDCRNG_CTL_REG 0x00 > diff --git a/drivers/char/hw_random/pic32-rng.c > b/drivers/char/hw_random/pic32-rng.c > index 99c8bd0859a1..728b68b1a496 100644 > --- a/drivers/char/hw_random/pic32-rng.c > +++ b/drivers/char/hw_random/pic32-rng.c > @@ -12,9 +12,8 @@ > #include > #include > #include > +#include > #include > -#include > -#include > #include > #include > > diff --git a/drivers/char/hw_random/stm32-rng.c > b/drivers/char/hw_random/stm32-rng.c > index a6731cf0627a..efb6a9f9a11b 100644 > --- a/drivers/char/hw_random/stm32-rng.c > +++ b/drivers/char/hw_random/stm32-rng.c > @@ -10,8 +10,9 @@ > #include > #include > #include > +#include > #include > -#include > +#include > #include > #include > #include > diff --git a/drivers/char/hw_random/xgene-rng.c > b/drivers/char/hw_random/xgene-rng.c > index 7c8f3cb7c6af..c25bb169563d 100644 > --- a/drivers/char/hw_random/xgene-rng.c > +++ b/drivers/char/hw_random/xgene-rng.c > @@ -15,9 +15,8 @@ > #include > #include > #include > -#include > -#include > -#include > +#include > +#include > #include > > #define RNG_MAX_DATUM4 > diff --git a/drivers/char/hw_random/xiphera-trng.c > b/drivers/char/hw_random/xiphera-trng.c > index 2a9fea72b2e0..2c586d1fe8a9 100644 > --- a/drivers/char/hw_random/xiphera-trng.c > +++ b/drivers/char/hw_random/xiphera-trng.c > @@ -7,7 +7,6 @@ > #include > #include > #include > -#include > #include > #include > > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c > b/drivers/char/ipmi/kcs_bmc_aspeed.c > index 2dea8cd5a09a..72640da55380 100644 > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > @@ -14,7 +14,6 @@ > #include > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c > index 528f35b14fb6..76adb108076c 100644 > --- a/drivers/char/tpm/tpm_ftpm_tee.c > +++ b/drivers/char/tpm/tpm_ftpm_tee.c > @@ -11,7 +11,6 @@ > > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 7db3593941ea..e4030404c64e 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -25,7 +25,6 @@ > #include > #include > #include > -#include > #include > #include > #include "tpm.h" > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c > b/drivers/char/tpm/tpm_tis_spi_main.c > index 1f5207974a17..c6101914629d 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -28,7 +28,7 @@ > #include > #include > > -#include > +#include > #include > #include > > diff --git a/drivers/char/tpm/tpm_tis_synquacer.c > b/drivers/char/tpm/tpm_tis_synquacer.c > index 49278746b0e2..7f9b4bfceb6e 100644 > --- a/drivers/char/tpm/tpm_tis_synquacer.c > +++ b/drivers/char/tpm/tpm_tis_synquacer.c > @@ -9,7 +9,6 @@ > #include > #include > #include > -#include > #include > #include "tpm.h" > #include "tpm_tis_core.h" > -- > 2.40.1 drivers/char/tpm/** Acked-by: Jarkko Sakkinen BR, Jarkko
Re: [RFC PATCH v11 01/29] KVM: Wrap kvm_gfn_range.pte in a per-action union
On Wed Jul 19, 2023 at 2:44 AM EEST, Sean Christopherson wrote:
> /* Huge pages aren't expected to be modified without first being
> zapped. */
> - WARN_ON(pte_huge(range->pte) || range->start + 1 != range->end);
> + WARN_ON(pte_huge(range->arg.pte) || range->start + 1 != range->end);
Not familiar with this code. Just checking whether whether instead
pr_{warn,err}() combined with return false would be a more graceful
option?
BR, Jarkko
Re: [PATCH v2 3/6] integrity: remove global variable from machine_keyring.c
On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote:
> trust_mok variable is accessed within a single function locally.
>
> Change trust_mok from global to local static variable.
>
> Signed-off-by: Nayna Jain
> Reviewed-and-tested-by: Mimi Zohar
> ---
> security/integrity/platform_certs/machine_keyring.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/platform_certs/machine_keyring.c
> b/security/integrity/platform_certs/machine_keyring.c
> index 389a6e7c9245..9482e16cb2ca 100644
> --- a/security/integrity/platform_certs/machine_keyring.c
> +++ b/security/integrity/platform_certs/machine_keyring.c
> @@ -8,8 +8,6 @@
> #include
> #include "../integrity.h"
>
> -static bool trust_mok;
> -
> static __init int machine_keyring_init(void)
> {
> int rc;
> @@ -65,9 +63,11 @@ static __init bool uefi_check_trust_mok_keys(void)
> bool __init trust_moklist(void)
> {
> static bool initialized;
> + static bool trust_mok;
>
> if (!initialized) {
> initialized = true;
> + trust_mok = false;
>
> if (uefi_check_trust_mok_keys())
> trust_mok = true;
Nice catch.
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v2 5/6] integrity: PowerVM machine keyring enablement
On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote: > Update Kconfig to enable machine keyring and limit to CA certificates > on PowerVM. Only key signing CA keys are allowed. > > Signed-off-by: Nayna Jain > Reviewed-and-tested-by: Mimi Zohar > > --- > security/integrity/Kconfig | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig > index ec6e0d789da1..232191ee09e3 100644 > --- a/security/integrity/Kconfig > +++ b/security/integrity/Kconfig > @@ -67,7 +67,9 @@ config INTEGRITY_MACHINE_KEYRING > depends on SECONDARY_TRUSTED_KEYRING > depends on INTEGRITY_ASYMMETRIC_KEYS > depends on SYSTEM_BLACKLIST_KEYRING > - depends on LOAD_UEFI_KEYS > + depends on LOAD_UEFI_KEYS || LOAD_PPC_KEYS > + select INTEGRITY_CA_MACHINE_KEYRING if LOAD_PPC_KEYS > + select INTEGRITY_CA_MACHINE_KEYRING_MAX if LOAD_PPC_KEYS > help >If set, provide a keyring to which Machine Owner Keys (MOK) may >be added. This keyring shall contain just MOK keys. Unlike keys > -- > 2.31.1 Reviewed-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v2 6/6] integrity: PowerVM support for loading third party code signing keys
On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote:
> On secure boot enabled PowerVM LPAR, third party code signing keys are
> needed during early boot to verify signed third party modules. These
> third party keys are stored in moduledb object in the Platform
> KeyStore(PKS).
^ space
>
> Load third party code signing keys onto .secondary_trusted_keys keyring.
>
> Signed-off-by: Nayna Jain
> ---
> certs/system_keyring.c| 23 +++
> include/keys/system_keyring.h | 7 ++
> security/integrity/integrity.h| 1 +
> .../platform_certs/keyring_handler.c | 8 +++
> .../platform_certs/keyring_handler.h | 5
> .../integrity/platform_certs/load_powerpc.c | 18 ++-
> 6 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index b348e0898d34..3435d4936fb2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -396,3 +396,26 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
> platform_trusted_keys = keyring;
> }
> #endif
> +
spurious newline character
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> + size_t len)
Documentation is lacking, and should be in a single line, as it totals
less than 100 characters.
> +{
> + key_ref_t key;
> + key_perm_t perm; the following structure
> + int rc = 0;
int rc;
> +
> + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> +
> + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +"asymmetric",
> +NULL, data, len, perm,
> +KEY_ALLOC_NOT_IN_QUOTA);
> + if (IS_ERR(key)) {
> + rc = PTR_ERR(key);
This helper variable is not very useful.
> + pr_err("Problem loading X.509 certificate %d\n", rc);
Why pr_err()? What kind of object is "a problem"?
Also X.509 certificates are everywhere. If these are printed to the
klog, how can e.g. an admin deduce the problem over here?
Even without having these log messages at all I could trace the called
function and be informed that some X.509 cert has an issues. Actually
then I could even deduce the location, thanks to call backtrace.
These have a potential to lead into wrong conclusions.
> + } else {
> + pr_notice("Loaded X.509 cert '%s'\n",
> + key_ref_to_ptr(key)->description);
single line
> + key_ref_put(key);
> + }
I'd suggest instead the following structure:
if (IS_ERR(key)) {
pr_err("Problem loading X.509 certificate %d\n", PTR_ERR(key));
return;
}
pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
key_ref_put(key);
}
BR, Jarkko
Re: [PATCH v3 1/6] integrity: PowerVM support for loading CA keys on machine keyring
On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> Keys that derive their trust from an entity such as a security officer,
> administrator, system owner, or machine owner are said to have "imputed
> trust". CA keys with imputed trust can be loaded onto the machine keyring.
> The mechanism for loading these keys onto the machine keyring is platform
> dependent.
>
> Load keys stored in the variable trustedcadb onto the .machine keyring
> on PowerVM platform.
>
> Signed-off-by: Nayna Jain
> Reviewed-and-tested-by: Mimi Zohar
> ---
> .../integrity/platform_certs/keyring_handler.c | 8
> .../integrity/platform_certs/keyring_handler.h | 5 +
> .../integrity/platform_certs/load_powerpc.c | 17 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/security/integrity/platform_certs/keyring_handler.c
> b/security/integrity/platform_certs/keyring_handler.c
> index 8a1124e4d769..1649d047e3b8 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -69,6 +69,14 @@ __init efi_element_handler_t get_handler_for_mok(const
> efi_guid_t *sig_type)
> return NULL;
> }
>
> +__init efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t
> *sig_type)
> +{
> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> + return add_to_machine_keyring;
> +
> + return NULL;
> +}
> +
> /*
> * Return the appropriate handler for particular signature list types found
> in
> * the UEFI dbx and MokListXRT tables.
> diff --git a/security/integrity/platform_certs/keyring_handler.h
> b/security/integrity/platform_certs/keyring_handler.h
> index 212d894a8c0c..6f15bb4cc8dc 100644
> --- a/security/integrity/platform_certs/keyring_handler.h
> +++ b/security/integrity/platform_certs/keyring_handler.h
> @@ -29,6 +29,11 @@ efi_element_handler_t get_handler_for_db(const efi_guid_t
> *sig_type);
> */
> efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
>
> +/*
> + * Return the handler for particular signature list types for CA keys.
> + */
> +efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
> +
> /*
> * Return the handler for particular signature list types found in the dbx.
> */
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index 170789dc63d2..6263ce3b3f1e 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -59,6 +59,7 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> static int __init load_powerpc_certs(void)
> {
> void *db = NULL, *dbx = NULL, *data = NULL;
> + void *trustedca = NULL;
Could this be just "void *trustedca;" ?
> u64 dsize = 0;
> u64 offset = 0;
> int rc = 0;
> @@ -120,6 +121,22 @@ static int __init load_powerpc_certs(void)
> kfree(data);
> }
>
> + data = get_cert_list("trustedcadb", 12, &dsize);
> + if (!data) {
> + pr_info("Couldn't get trustedcadb list from firmware\n");
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> + pr_err("Error reading trustedcadb from firmware: %d\n", rc);
> + } else {
> + extract_esl(trustedca, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:trustedca", trustedca,
> dsize,
> + get_handler_for_ca_keys);
> + if (rc)
> + pr_err("Couldn't parse trustedcadb signatures: %d\n",
> rc);
> + kfree(data);
> + }
> +
> return rc;
> }
> late_initcall(load_powerpc_certs);
> --
> 2.31.1
BR, Jarkko
Re: [PATCH v3 2/6] integrity: ignore keys failing CA restrictions on non-UEFI platform
On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote: > On non-UEFI platforms, handle restrict_link_by_ca failures differently. > > Certificates which do not satisfy CA restrictions on non-UEFI platforms > are ignored. > > Signed-off-by: Nayna Jain > Reviewed-and-tested-by: Mimi Zohar > --- > security/integrity/platform_certs/machine_keyring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/platform_certs/machine_keyring.c > b/security/integrity/platform_certs/machine_keyring.c > index 7aaed7950b6e..389a6e7c9245 100644 > --- a/security/integrity/platform_certs/machine_keyring.c > +++ b/security/integrity/platform_certs/machine_keyring.c > @@ -36,7 +36,7 @@ void __init add_to_machine_keyring(const char *source, > const void *data, size_t >* If the restriction check does not pass and the platform keyring >* is configured, try to add it into that keyring instead. >*/ > - if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) > + if (rc && efi_enabled(EFI_BOOT) && > IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) > rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source, > data, len, perm); > > -- > 2.31.1 Acked-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v3 6/6] integrity: PowerVM support for loading third party code signing keys
On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> On secure boot enabled PowerVM LPAR, third party code signing keys are
> needed during early boot to verify signed third party modules. These
> third party keys are stored in moduledb object in the Platform
> KeyStore(PKS).
>
> Load third party code signing keys onto .secondary_trusted_keys keyring.
>
> Signed-off-by: Nayna Jain
> ---
> certs/system_keyring.c| 30 +++
> include/keys/system_keyring.h | 7 +
> security/integrity/integrity.h| 1 +
> .../platform_certs/keyring_handler.c | 8 +
> .../platform_certs/keyring_handler.h | 5
> .../integrity/platform_certs/load_powerpc.c | 18 ++-
> 6 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index b348e0898d34..e458d414918d 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -396,3 +396,33 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
> platform_trusted_keys = keyring;
> }
> #endif
> +
> +/**
> + * add_to_secondary_keyring - Add to secondary keyring.
> + * @source: Source of key
> + * @data: The blob holding the key
> + * @len: The length of the data blob
> + *
> + * Add a key to the secondary keyring. The key must be vouched for by a key
> in the builtin,
> + * machine or secondary keyring itself.
> + */
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> size_t len)
> +{
> + key_ref_t key;
> + key_perm_t perm;
> +
> + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> +
> + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +"asymmetric",
> +NULL, data, len, perm,
> +KEY_ALLOC_NOT_IN_QUOTA);
> + if (IS_ERR(key)) {
> + pr_err("Problem loading X.509 certificate from %s to secondary
> keyring %ld\n",
> +source, PTR_ERR(key));
> + return;
> + }
> +
> + pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
> + key_ref_put(key);
> +}
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 7e2583208820..4188f75d1bac 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -50,9 +50,16 @@ int restrict_link_by_digsig_builtin_and_secondary(struct
> key *keyring,
> const struct key_type *type,
> const union key_payload
> *payload,
> struct key *restriction_key);
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> + size_t len);
> +
> #else
> #define restrict_link_by_builtin_and_secondary_trusted
> restrict_link_by_builtin_trusted
> #define restrict_link_by_digsig_builtin_and_secondary
> restrict_link_by_digsig_builtin
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> + size_t len)
> +{
> +}
> #endif
>
> #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d7553c93f5c0..efaa2eb789ad 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -228,6 +228,7 @@ static inline int __init integrity_load_cert(const
> unsigned int id,
> {
> return 0;
> }
> +
> #endif /* CONFIG_INTEGRITY_SIGNATURE */
>
> #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> diff --git a/security/integrity/platform_certs/keyring_handler.c
> b/security/integrity/platform_certs/keyring_handler.c
> index 586027b9a3f5..13ea17207902 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -78,6 +78,14 @@ __init efi_element_handler_t get_handler_for_ca_keys(const
> efi_guid_t *sig_type)
> return NULL;
> }
>
> +__init efi_element_handler_t get_handler_for_code_signing_keys(const
> efi_guid_t *sig_type)
> +{
> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> + return add_to_secondary_keyring;
> +
> + return NULL;
> +}
> +
> /*
> * Return the appropriate handler for particular signature list types found
> in
> * the UEFI dbx and MokListXRT tables.
> diff --git a/security/integrity/platform_certs/keyring_handler.h
> b/security/integrity/platform_certs/keyring_handler.h
> index 6f15bb4cc8dc..f92895cc50f6 100644
> --- a/security/integrity/platform_certs/keyring_handler.h
> +++ b/security/integrity/platform_certs/keyring_handler.h
> @@ -34,6 +34,11 @@ efi_element_handler_t get_handler_for_mok(const efi_guid_t
> *sig_type);
> */
> efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
>
> +/*
> + * R
Re: [PATCH v4 1/6] integrity: PowerVM support for loading CA keys on machine keyring
On Tue Aug 15, 2023 at 2:27 PM EEST, Nayna Jain wrote:
> Keys that derive their trust from an entity such as a security officer,
> administrator, system owner, or machine owner are said to have "imputed
> trust". CA keys with imputed trust can be loaded onto the machine keyring.
> The mechanism for loading these keys onto the machine keyring is platform
> dependent.
>
> Load keys stored in the variable trustedcadb onto the .machine keyring
> on PowerVM platform.
>
> Signed-off-by: Nayna Jain
> Reviewed-and-tested-by: Mimi Zohar
> ---
> .../integrity/platform_certs/keyring_handler.c | 8
> .../integrity/platform_certs/keyring_handler.h | 5 +
> .../integrity/platform_certs/load_powerpc.c | 17 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/security/integrity/platform_certs/keyring_handler.c
> b/security/integrity/platform_certs/keyring_handler.c
> index 8a1124e4d769..1649d047e3b8 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -69,6 +69,14 @@ __init efi_element_handler_t get_handler_for_mok(const
> efi_guid_t *sig_type)
> return NULL;
> }
>
> +__init efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t
> *sig_type)
> +{
> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> + return add_to_machine_keyring;
> +
> + return NULL;
> +}
> +
> /*
> * Return the appropriate handler for particular signature list types found
> in
> * the UEFI dbx and MokListXRT tables.
> diff --git a/security/integrity/platform_certs/keyring_handler.h
> b/security/integrity/platform_certs/keyring_handler.h
> index 212d894a8c0c..6f15bb4cc8dc 100644
> --- a/security/integrity/platform_certs/keyring_handler.h
> +++ b/security/integrity/platform_certs/keyring_handler.h
> @@ -29,6 +29,11 @@ efi_element_handler_t get_handler_for_db(const efi_guid_t
> *sig_type);
> */
> efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
>
> +/*
> + * Return the handler for particular signature list types for CA keys.
> + */
> +efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
> +
> /*
> * Return the handler for particular signature list types found in the dbx.
> */
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index 170789dc63d2..339053d9726d 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -59,6 +59,7 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> static int __init load_powerpc_certs(void)
> {
> void *db = NULL, *dbx = NULL, *data = NULL;
> + void *trustedca;
> u64 dsize = 0;
> u64 offset = 0;
> int rc = 0;
> @@ -120,6 +121,22 @@ static int __init load_powerpc_certs(void)
> kfree(data);
> }
>
> + data = get_cert_list("trustedcadb", 12, &dsize);
> + if (!data) {
> + pr_info("Couldn't get trustedcadb list from firmware\n");
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> + pr_err("Error reading trustedcadb from firmware: %d\n", rc);
> + } else {
> + extract_esl(trustedca, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:trustedca", trustedca,
> dsize,
> + get_handler_for_ca_keys);
> + if (rc)
> + pr_err("Couldn't parse trustedcadb signatures: %d\n",
> rc);
> + kfree(data);
> + }
> +
> return rc;
> }
> late_initcall(load_powerpc_certs);
> --
> 2.31.1
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v4 6/6] integrity: PowerVM support for loading third party code signing keys
or particular signature list types for code signing
> keys.
> + */
> +efi_element_handler_t get_handler_for_code_signing_keys(const efi_guid_t
> *sig_type);
> +
> /*
> * Return the handler for particular signature list types found in the dbx.
> */
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index 339053d9726d..c85febca3343 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -60,6 +60,7 @@ static int __init load_powerpc_certs(void)
> {
> void *db = NULL, *dbx = NULL, *data = NULL;
> void *trustedca;
> + void *moduledb;
> u64 dsize = 0;
> u64 offset = 0;
> int rc = 0;
> @@ -137,6 +138,22 @@ static int __init load_powerpc_certs(void)
> kfree(data);
> }
>
> + data = get_cert_list("moduledb", 9, &dsize);
> + if (!data) {
> + pr_info("Couldn't get moduledb list from firmware\n");
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> + pr_err("Error reading moduledb from firmware: %d\n", rc);
> + } else {
> + extract_esl(moduledb, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:moduledb", moduledb,
> dsize,
> +
> get_handler_for_code_signing_keys);
> + if (rc)
> + pr_err("Couldn't parse moduledb signatures: %d\n", rc);
> + kfree(data);
> + }
> +
> return rc;
> }
> late_initcall(load_powerpc_certs);
> --
> 2.31.1
Reviewed-by: Jarkko Sakkinen
I can pick this. My last PR did not went too great partly because of
mess with tpm_tis but now things are calmer.
BR, Jarkko
Re: [PATCH v5 0/3 RESEND] sed-opal: keyrings, discovery, revert, key store
On Wed Aug 16, 2023 at 10:45 PM EEST, Greg Joyce wrote: > It's been almost 4 weeks since the last resend and there haven't been > any comments. Is there anything that needs to be changed for > acceptance? > > Thanks for your input. > > Greg > > On Fri, 2023-07-21 at 16:15 -0500, gjo...@linux.vnet.ibm.com wrote: > > From: Greg Joyce > > > > This patchset has gone through numerous rounds of review and > > all comments/suggetions have been addressed. The reviews have > > covered all relevant areas including reviews by block and keyring > > developers as well as the SED Opal maintainer. The last > > patchset submission has not solicited any responses in the > > six weeks since it was last distributed. The changes are > > generally useful and ready for inclusion. > > > > TCG SED Opal is a specification from The Trusted Computing Group > > that allows self encrypting storage devices (SED) to be locked at > > power on and require an authentication key to unlock the drive. > > > > The current SED Opal implementation in the block driver > > requires that authentication keys be provided in an ioctl > > so that they can be presented to the underlying SED > > capable drive. Currently, the key is typically entered by > > a user with an application like sedutil or sedcli. While > > this process works, it does not lend itself to automation > > like unlock by a udev rule. > > > > The SED block driver has been extended so it can alternatively > > obtain a key from a sed-opal kernel keyring. The SED ioctls > > will indicate the source of the key, either directly in the > > ioctl data or from the keyring. > > > > Two new SED ioctls have also been added. These are: > > 1) IOC_OPAL_REVERT_LSP to revert LSP state > > 2) IOC_OPAL_DISCOVERY to discover drive capabilities/state > > > > change log v5: > > - rebase to for-6.5/block > > > > change log v4: > > - rebase to 6.3-rc7 > > - replaced "255" magic number with U8_MAX > > > > change log: > > - rebase to 6.x > > - added latest reviews > > - removed platform functions for persistent key storage > > - replaced key update logic with key_create_or_update() > > - minor bracing and padding changes > > - add error returns > > - opal_key structure is application provided but kernel > > verified > > - added brief description of TCG SED Opal > > > > > > Greg Joyce (3): > > block: sed-opal: Implement IOC_OPAL_DISCOVERY > > block: sed-opal: Implement IOC_OPAL_REVERT_LSP > > block: sed-opal: keyring support for SED keys > > > > block/Kconfig | 2 + > > block/opal_proto.h | 4 + > > block/sed-opal.c | 252 > > +- > > include/linux/sed-opal.h | 5 + > > include/uapi/linux/sed-opal.h | 25 +++- > > 5 files changed, 282 insertions(+), 6 deletions(-) > > > > > > base-commit: 1341c7d2ccf42ed91aea80b8579d35bc1ea381e2 I can give because it looks good to me to all patches: Acked-by: Jarkko Sakkinen ... but should not probably go to my tree. BR, Jarkko
Re: [PATCH v3 2/6] integrity: ignore keys failing CA restrictions on non-UEFI platform
On Wed Aug 16, 2023 at 3:58 PM EEST, Mimi Zohar wrote:
> On Mon, 2023-08-14 at 20:38 +0300, Jarkko Sakkinen wrote:
> > On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> > > On non-UEFI platforms, handle restrict_link_by_ca failures differently.
> > >
> > > Certificates which do not satisfy CA restrictions on non-UEFI platforms
> > > are ignored.
> > >
> > > Signed-off-by: Nayna Jain
> > > Reviewed-and-tested-by: Mimi Zohar
> > > ---
> > > security/integrity/platform_certs/machine_keyring.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/security/integrity/platform_certs/machine_keyring.c
> > > b/security/integrity/platform_certs/machine_keyring.c
> > > index 7aaed7950b6e..389a6e7c9245 100644
> > > --- a/security/integrity/platform_certs/machine_keyring.c
> > > +++ b/security/integrity/platform_certs/machine_keyring.c
> > > @@ -36,7 +36,7 @@ void __init add_to_machine_keyring(const char *source,
> > > const void *data, size_t
> > >* If the restriction check does not pass and the platform keyring
> > >* is configured, try to add it into that keyring instead.
> > >*/
> > > - if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > > + if (rc && efi_enabled(EFI_BOOT) &&
> > > IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > > rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
> > >data, len, perm);
> > >
> > > --
> > > 2.31.1
> >
> > Acked-by: Jarkko Sakkinen
>
> Hi Jarkko,
>
> Without the following two commits in your master branch, the last patch
> in this series "[PATCH v4 6/6] integrity: PowerVM support for loading
> third party code signing keys" doesn't apply cleanly.
>
> - commit 409b465f8a83 ("integrity: Enforce digitalSignature usage in
> the ima and evm keyrings")
> - commit e34a6c7dd192 ("KEYS: DigitalSignature link restriction")
>
> If you're not planning on upstreaming this patch set, I'd appreciate
> your creating a topic branch with these two commits.
They reside now in my -next. I'll send PR for the next release Fri.
> --
> thanks,
>
> Mimi
BR, Jarkko
Re: [PATCH v4 6/6] integrity: PowerVM support for loading third party code signing keys
On Thu Aug 17, 2023 at 12:06 AM EEST, Mimi Zohar wrote:
> On Wed, 2023-08-16 at 23:36 +0300, Jarkko Sakkinen wrote:
> > On Tue Aug 15, 2023 at 2:27 PM EEST, Nayna Jain wrote:
> > > On secure boot enabled PowerVM LPAR, third party code signing keys are
> > > needed during early boot to verify signed third party modules. These
> > > third party keys are stored in moduledb object in the Platform
> > > KeyStore (PKS).
> > >
> > > Load third party code signing keys onto .secondary_trusted_keys keyring.
> > >
> > > Signed-off-by: Nayna Jain
> > > ---
> > > certs/system_keyring.c| 30 +++
> > > include/keys/system_keyring.h | 4 +++
> > > .../platform_certs/keyring_handler.c | 8 +
> > > .../platform_certs/keyring_handler.h | 5
> > > .../integrity/platform_certs/load_powerpc.c | 17 +++
> > > 5 files changed, 64 insertions(+)
> > >
> > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > index b348e0898d34..33841c91f12c 100644
> > > --- a/certs/system_keyring.c
> > > +++ b/certs/system_keyring.c
> > > @@ -152,6 +152,36 @@ static __init struct key_restriction
> > > *get_builtin_and_secondary_restriction(void
> > >
> > > return restriction;
> > > }
> > > +
> > > +/**
> > > + * add_to_secondary_keyring - Add to secondary keyring.
> > > + * @source: Source of key
> > > + * @data: The blob holding the key
> > > + * @len: The length of the data blob
> > > + *
> > > + * Add a key to the secondary keyring. The key must be vouched for by a
> > > key in the builtin,
> > > + * machine or secondary keyring itself.
> > > + */
> > > +void __init add_to_secondary_keyring(const char *source, const void
> > > *data, size_t len)
> > > +{
> > > + key_ref_t key;
> > > + key_perm_t perm;
> > > +
> > > + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> > > +
> > > + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> > > +"asymmetric",
> > > +NULL, data, len, perm,
> > > +KEY_ALLOC_NOT_IN_QUOTA);
> > > + if (IS_ERR(key)) {
> > > + pr_err("Problem loading X.509 certificate from %s to secondary
> > > keyring %ld\n",
> > > +source, PTR_ERR(key));
> > > + return;
> > > + }
> > > +
> > > + pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
> > > + key_ref_put(key);
> > > +}
> > > #endif
> > > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> > > void __init set_machine_trusted_keys(struct key *keyring)
> > > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> > > index 7e2583208820..8365adf842ef 100644
> > > --- a/include/keys/system_keyring.h
> > > +++ b/include/keys/system_keyring.h
> > > @@ -50,9 +50,13 @@ int
> > > restrict_link_by_digsig_builtin_and_secondary(struct key *keyring,
> > > const struct key_type *type,
> > > const union key_payload
> > > *payload,
> > > struct key *restriction_key);
> > > +void __init add_to_secondary_keyring(const char *source, const void
> > > *data, size_t len);
> > > #else
> > > #define restrict_link_by_builtin_and_secondary_trusted
> > > restrict_link_by_builtin_trusted
> > > #define restrict_link_by_digsig_builtin_and_secondary
> > > restrict_link_by_digsig_builtin
> > > +static inline void __init add_to_secondary_keyring(const char *source,
> > > const void *data, size_t len)
> > > +{
> > > +}
> > > #endif
> > >
> > > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> > > diff --git a/security/integrity/platform_certs/keyring_handler.c
> > > b/security/integrity/platform_certs/keyring_handler.c
> > > index 586027b9a3f5..13ea17207902 100644
> > > --- a/security/integrity/platform_certs/keyring_handler.c
> > > +++ b/security/integrity/platform_certs/keyring_handler.c
> > > @@ -78,6 +78,14 @@ __init efi_element_handler_t
> > > get_handler_for_ca_keys(const e
Re: [PATCH v5 0/3 RESEND] sed-opal: keyrings, discovery, revert, key store
On Mon Aug 21, 2023 at 6:26 PM EEST, Greg Joyce wrote: > On Wed, 2023-08-16 at 23:41 +0300, Jarkko Sakkinen wrote: > > On Wed Aug 16, 2023 at 10:45 PM EEST, Greg Joyce wrote: > > > It's been almost 4 weeks since the last resend and there haven't > > > been > > > any comments. Is there anything that needs to be changed for > > > acceptance? > > > > > > Thanks for your input. > > > > > > Greg > > > > > > On Fri, 2023-07-21 at 16:15 -0500, gjo...@linux.vnet.ibm.com wrote: > > > > From: Greg Joyce > > > > > > > > This patchset has gone through numerous rounds of review and > > > > all comments/suggetions have been addressed. The reviews have > > > > covered all relevant areas including reviews by block and keyring > > > > developers as well as the SED Opal maintainer. The last > > > > patchset submission has not solicited any responses in the > > > > six weeks since it was last distributed. The changes are > > > > generally useful and ready for inclusion. > > > > > > > > TCG SED Opal is a specification from The Trusted Computing Group > > > > that allows self encrypting storage devices (SED) to be locked at > > > > power on and require an authentication key to unlock the drive. > > > > > > > > The current SED Opal implementation in the block driver > > > > requires that authentication keys be provided in an ioctl > > > > so that they can be presented to the underlying SED > > > > capable drive. Currently, the key is typically entered by > > > > a user with an application like sedutil or sedcli. While > > > > this process works, it does not lend itself to automation > > > > like unlock by a udev rule. > > > > > > > > The SED block driver has been extended so it can alternatively > > > > obtain a key from a sed-opal kernel keyring. The SED ioctls > > > > will indicate the source of the key, either directly in the > > > > ioctl data or from the keyring. > > > > > > > > Two new SED ioctls have also been added. These are: > > > > 1) IOC_OPAL_REVERT_LSP to revert LSP state > > > > 2) IOC_OPAL_DISCOVERY to discover drive capabilities/state > > > > > > > > change log v5: > > > > - rebase to for-6.5/block > > > > > > > > change log v4: > > > > - rebase to 6.3-rc7 > > > > - replaced "255" magic number with U8_MAX > > > > > > > > change log: > > > > - rebase to 6.x > > > > - added latest reviews > > > > - removed platform functions for persistent key storage > > > > - replaced key update logic with key_create_or_update() > > > > - minor bracing and padding changes > > > > - add error returns > > > > - opal_key structure is application provided but kernel > > > > verified > > > > - added brief description of TCG SED Opal > > > > > > > > > > > > Greg Joyce (3): > > > > block: sed-opal: Implement IOC_OPAL_DISCOVERY > > > > block: sed-opal: Implement IOC_OPAL_REVERT_LSP > > > > block: sed-opal: keyring support for SED keys > > > > > > > > block/Kconfig | 2 + > > > > block/opal_proto.h| 4 + > > > > block/sed-opal.c | 252 > > > > +- > > > > include/linux/sed-opal.h | 5 + > > > > include/uapi/linux/sed-opal.h | 25 +++- > > > > 5 files changed, 282 insertions(+), 6 deletions(-) > > > > > > > > > > > > base-commit: 1341c7d2ccf42ed91aea80b8579d35bc1ea381e2 > > > > I can give because it looks good to me to all patches: > > > > Acked-by: Jarkko Sakkinen > > > > ... but should not probably go to my tree. > > > > BR, Jarkko > > Thanks for the ack Jarkko. Any thoughts on which tree it should go to? I get from "scripts/get_maintainer.pl block/sed-opal.c | wl-copy" Jonathan Derrick (supporter:SECURE ENCRYPTING DEVICE (SED) OPAL DRIVER) Jens Axboe (maintainer:BLOCK LAYER) linux-bl...@vger.kernel.org (open list:SECURE ENCRYPTING DEVICE (SED) OPAL DRIVER) linux-ker...@vger.kernel.org (open list) You should probably add the corresponding maintainers and linux-block to the loop. I suggest to send a new version of the patch set with my ack's added to the patches. BR, Jarkko
Re: [PATCH 1/4] block:sed-opal: SED Opal keystore
On Fri May 5, 2023 at 10:43 PM EEST, wrote:
> From: Greg Joyce
>
> Add read and write functions that allow SED Opal keys to stored
> in a permanent keystore.
Please be more verbose starting from "Self-Encrypting Drive (SED)",
instead of just "SED", and take time to explain what these keys are.
>
> Signed-off-by: Greg Joyce
> Reviewed-by: Jonathan Derrick
> ---
> block/Makefile | 2 +-
> block/sed-opal-key.c | 24
> include/linux/sed-opal-key.h | 15 +++
> 3 files changed, 40 insertions(+), 1 deletion(-)
> create mode 100644 block/sed-opal-key.c
> create mode 100644 include/linux/sed-opal-key.h
>
> diff --git a/block/Makefile b/block/Makefile
> index 4e01bb71ad6e..464a9f209552 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -35,7 +35,7 @@ obj-$(CONFIG_BLK_DEV_ZONED) += blk-zoned.o
> obj-$(CONFIG_BLK_WBT)+= blk-wbt.o
> obj-$(CONFIG_BLK_DEBUG_FS) += blk-mq-debugfs.o
> obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
> -obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o
> +obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o sed-opal-key.o
> obj-$(CONFIG_BLK_PM) += blk-pm.o
> obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += blk-crypto.o blk-crypto-profile.o \
> blk-crypto-sysfs.o
> diff --git a/block/sed-opal-key.c b/block/sed-opal-key.c
> new file mode 100644
> index ..16f380164c44
> --- /dev/null
> +++ b/block/sed-opal-key.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SED key operations.
> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for SED Opal
> + * keys. Specific keystores can provide overrides.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +
> +int __weak sed_read_key(char *keyname, char *key, u_int *keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +int __weak sed_write_key(char *keyname, char *key, u_int keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> diff --git a/include/linux/sed-opal-key.h b/include/linux/sed-opal-key.h
> new file mode 100644
> index ..c9b1447986d8
> --- /dev/null
> +++ b/include/linux/sed-opal-key.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SED key operations.
> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for SED Opal
> + * keys. Specific keystores can provide overrides.
> + *
> + */
> +
> +#include
> +
> +int sed_read_key(char *keyname, char *key, u_int *keylen);
> +int sed_write_key(char *keyname, char *key, u_int keylen);
> --
> gjo...@linux.vnet.ibm.com
BR, Jarkko
Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On Tue Jun 6, 2023 at 8:26 PM EEST, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
>
Cc: sta...@vger.kenrnel.org # v6.3
?
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Signed-off-by: Nayna Jain
> ---
> .../integrity/platform_certs/load_powerpc.c | 39 ---
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..57768cbf1fd3 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -15,6 +15,9 @@
> #include "keyring_handler.h"
> #include "../integrity.h"
>
> +#define extract_data(db, data, size, offset) \
> + do { db = data + offset; size = size - offset; } while (0)
> +
> /*
> * Get a certificate list blob from the named secure variable.
> *
> @@ -55,8 +58,10 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> */
> static int __init load_powerpc_certs(void)
> {
> + void *data = NULL;
> + u64 dsize = 0;
> + u64 offset = 0;
> void *db = NULL, *dbx = NULL;
So... what do you need db still for?
If you meant to rename 'db' to 'data', then you should not do it, since this is
a bug fix. It is zero gain, and a factor harder backport.
> - u64 dbsize = 0, dbxsize = 0;
> int rc = 0;
> ssize_t len;
> char buf[32];
> @@ -74,38 +79,46 @@ static int __init load_powerpc_certs(void)
> return -ENODEV;
> }
>
> + if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> + /* PLPKS authenticated variables ESL data is prefixed with 8
> bytes of timestamp */
> + offset = 8;
> +
> /*
>* Get db, and dbx. They might not exist, so it isn't an error if we
>* can't get them.
>*/
> - db = get_cert_list("db", 3, &dbsize);
> - if (!db) {
> + data = get_cert_list("db", 3, &dsize);
> + if (!data) {
> pr_info("Couldn't get db list from firmware\n");
> - } else if (IS_ERR(db)) {
> - rc = PTR_ERR(db);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading db from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:db", db, dbsize,
> + extract_data(db, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:db", db, dsize,
> get_handler_for_db);
> if (rc)
> pr_err("Couldn't parse db signatures: %d\n", rc);
> - kfree(db);
> + kfree(data);
> }
>
> - dbx = get_cert_list("dbx", 4, &dbxsize);
> - if (!dbx) {
> + data = get_cert_list("dbx", 4, &dsize);
> + if (!data) {
> pr_info("Couldn't get dbx list from firmware\n");
> - } else if (IS_ERR(dbx)) {
> - rc = PTR_ERR(dbx);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading dbx from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
> + extract_data(dbx, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
> get_handler_for_dbx);
> if (rc)
> pr_err("Couldn't parse dbx signatures: %d\n", rc);
> - kfree(dbx);
> + kfree(data);
> }
>
> return rc;
> --
> 2.31.1
BR, Jarkko
Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On Wed Jun 7, 2023 at 3:28 PM EEST, Nayna wrote:
>
> On 6/6/23 16:51, Jarkko Sakkinen wrote:
> > On Tue Jun 6, 2023 at 8:26 PM EEST, Nayna Jain wrote:
> >> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> >> Extract ESL by stripping off the timestamp before passing to ESL parser.
> >>
> > Cc: sta...@vger.kenrnel.org # v6.3
> >
> > ?
>
> Aah yes. Missed that.. Thanks..
>
>
> >
> >> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> >> Signed-off-by: Nayna Jain
> >> ---
> >> .../integrity/platform_certs/load_powerpc.c | 39 ---
> >> 1 file changed, 26 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/security/integrity/platform_certs/load_powerpc.c
> >> b/security/integrity/platform_certs/load_powerpc.c
> >> index b9de70b90826..57768cbf1fd3 100644
> >> --- a/security/integrity/platform_certs/load_powerpc.c
> >> +++ b/security/integrity/platform_certs/load_powerpc.c
> >> @@ -15,6 +15,9 @@
> >> #include "keyring_handler.h"
> >> #include "../integrity.h"
> >>
> >> +#define extract_data(db, data, size, offset) \
> >> + do { db = data + offset; size = size - offset; } while (0)
> >> +
> >> /*
> >>* Get a certificate list blob from the named secure variable.
> >>*
> >> @@ -55,8 +58,10 @@ static __init void *get_cert_list(u8 *key, unsigned
> >> long keylen, u64 *size)
> >>*/
> >> static int __init load_powerpc_certs(void)
> >> {
> >> + void *data = NULL;
> >> + u64 dsize = 0;
> >> + u64 offset = 0;
> >>void *db = NULL, *dbx = NULL;
> > So... what do you need db still for?
> >
> > If you meant to rename 'db' to 'data', then you should not do it, since
> > this is
> > a bug fix. It is zero gain, and a factor harder backport.
>
> In case of PowerVM guest, data points to timestamp + ESL. And then with
> offset of 8 bytes, db points to ESL.
>
> While db is used for parsing ESL, data is then later used to free
> (timestamp + ESL) memory.
>
> Hope it answers the question.
OK, cool. Only thing I have to add that it would be more consistent if
data was declared in the same line as db and dbx, given that they are
declared in the same line.
BR, Jarkko
Re: [PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries
On Thu Jun 8, 2023 at 3:04 PM EEST, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
>
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Cc: sta...@vger.kenrnel.org # v6.3
> Signed-off-by: Nayna Jain
> ---
> Changelog:
> v2: Fixed feedback from Jarkko
> * added CC to stable
> * moved *data declaration to same line as *db,*dbx
> Renamed extract_data() macro to extract_esl() for clarity
> .../integrity/platform_certs/load_powerpc.c | 40 ---
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..170789dc63d2 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -15,6 +15,9 @@
> #include "keyring_handler.h"
> #include "../integrity.h"
>
> +#define extract_esl(db, data, size, offset) \
> + do { db = data + offset; size = size - offset; } while (0)
> +
> /*
> * Get a certificate list blob from the named secure variable.
> *
> @@ -55,8 +58,9 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> */
> static int __init load_powerpc_certs(void)
> {
> - void *db = NULL, *dbx = NULL;
> - u64 dbsize = 0, dbxsize = 0;
> + void *db = NULL, *dbx = NULL, *data = NULL;
> + u64 dsize = 0;
> + u64 offset = 0;
> int rc = 0;
> ssize_t len;
> char buf[32];
> @@ -74,38 +78,46 @@ static int __init load_powerpc_certs(void)
> return -ENODEV;
> }
>
> + if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> + /* PLPKS authenticated variables ESL data is prefixed with 8
> bytes of timestamp */
> + offset = 8;
> +
> /*
>* Get db, and dbx. They might not exist, so it isn't an error if we
>* can't get them.
>*/
> - db = get_cert_list("db", 3, &dbsize);
> - if (!db) {
> + data = get_cert_list("db", 3, &dsize);
> + if (!data) {
> pr_info("Couldn't get db list from firmware\n");
> - } else if (IS_ERR(db)) {
> - rc = PTR_ERR(db);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading db from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:db", db, dbsize,
> + extract_esl(db, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:db", db, dsize,
> get_handler_for_db);
> if (rc)
> pr_err("Couldn't parse db signatures: %d\n", rc);
> - kfree(db);
> + kfree(data);
> }
>
> - dbx = get_cert_list("dbx", 4, &dbxsize);
> - if (!dbx) {
> + data = get_cert_list("dbx", 4, &dsize);
> + if (!data) {
> pr_info("Couldn't get dbx list from firmware\n");
> - } else if (IS_ERR(dbx)) {
> - rc = PTR_ERR(dbx);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading dbx from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
> + extract_esl(dbx, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
> get_handler_for_dbx);
> if (rc)
> pr_err("Couldn't parse dbx signatures: %d\n", rc);
> - kfree(dbx);
> + kfree(data);
> }
>
> return rc;
> --
> 2.31.1
Acked-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v9 2/4] tpm: of: Make of-tree specific function commonly available
On Thu May 25, 2023 at 1:56 AM EEST, Jerry Snitselaar wrote: > On Tue, Apr 18, 2023 at 09:44:07AM -0400, Stefan Berger wrote: > > Simplify tpm_read_log_of() by moving reusable parts of the code into > > an inline function that makes it commonly available so it can be > > used also for kexec support. Call the new of_tpm_get_sml_parameters() > > function from the TPM Open Firmware driver. > > > > Signed-off-by: Stefan Berger > > Cc: Jarkko Sakkinen > > Cc: Jason Gunthorpe > > Cc: Rob Herring > > Cc: Frank Rowand > > Reviewed-by: Mimi Zohar > > Tested-by: Nageswara R Sastry > > Tested-by: Coiby Xu > > Acked-by: Jarkko Sakkinen > > > > Reviewed-by: Jerry Snitselaar If I just pick tpm only patches they won't apply so maybe TPM changes should be better separated if that is by any means possible. Open for counter proposals. Just my thoughts... I.e. I'm mainly wondering why TPM patches depend on IMA patches? BR, Jarkko
Re: [PATCH v9 2/4] tpm: of: Make of-tree specific function commonly available
On Fri Jun 9, 2023 at 9:49 PM EEST, Stefan Berger wrote: > > > On 6/9/23 14:18, Jarkko Sakkinen wrote: > > On Thu May 25, 2023 at 1:56 AM EEST, Jerry Snitselaar wrote: > >> On Tue, Apr 18, 2023 at 09:44:07AM -0400, Stefan Berger wrote: > >>> Simplify tpm_read_log_of() by moving reusable parts of the code into > >>> an inline function that makes it commonly available so it can be > >>> used also for kexec support. Call the new of_tpm_get_sml_parameters() > >>> function from the TPM Open Firmware driver. > >>> > >>> Signed-off-by: Stefan Berger > >>> Cc: Jarkko Sakkinen > >>> Cc: Jason Gunthorpe > >>> Cc: Rob Herring > >>> Cc: Frank Rowand > >>> Reviewed-by: Mimi Zohar > >>> Tested-by: Nageswara R Sastry > >>> Tested-by: Coiby Xu > >>> Acked-by: Jarkko Sakkinen > >>> > >> > >> Reviewed-by: Jerry Snitselaar > > > > If I just pick tpm only patches they won't apply so maybe TPM changes > > should be better separated if that is by any means possible. > > Per the comment here I am putting this series here on hold. > https://lore.kernel.org/linux-integrity/20230418134409.177485-1-stef...@linux.ibm.com/T/#m03745c2af2c46f19f329522fcb6ccb2bf2eaedc7 OK, cool. I've mentioned this in few other emails but say this here too: I was relocating for last couple of weeks, and thus some latency. If you choose to repost the series, I'm happy to review it, thanks :-) BR, Jarkko
Re: [PATCH v8 2/4] tpm: of: Make of-tree specific function commonly available
On Thu, Sep 01, 2022 at 05:46:08PM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() by moving reusable parts of the code into
> an inline function that makes it commonly available so it can be
> used also for kexec support. Call the new of_tpm_get_sml_parameters()
> function from the TPM Open Firmware driver.
>
> Signed-off-by: Stefan Berger
> Cc: Jarkko Sakkinen
> Cc: Jason Gunthorpe
> Cc: Rob Herring
> Cc: Frank Rowand
> Reviewed-by: Mimi Zohar
> Tested-by: Nageswara R Sastry
> Reviewed-by: Jarkko Sakkinen
> Tested-by: Coiby Xu
>
> ---
> v7:
> - Added original comment back into inlined function
>
> v4:
> - converted to inline function
> ---
> drivers/char/tpm/eventlog/of.c | 31 +
> include/linux/tpm.h| 36 ++
> 2 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..f9462d19632e 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "../tpm.h"
> @@ -20,11 +21,10 @@
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> + int ret;
>
> log = &chip->log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return -ENODEV;
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + ret = of_tpm_get_sml_parameters(np, &base, &size);
> + if (ret < 0)
> + return ret;
>
> if (size == 0) {
> dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index dfeb25a0362d..6356baaa1393 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -460,4 +460,40 @@ static inline struct tpm_chip *tpm_default_chip(void)
> return NULL;
> }
> #endif
> +
> +#ifdef CONFIG_OF
> +static inline int of_tpm_get_sml_parameters(struct device_node *np,
> + u64 *base, u32 *size)
> +{
> + const u32 *sizep;
> + const u64 *basep;
> +
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return -ENODEV;
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> +
> + /*
> + * For both vtpm/tpm, firmware has log addr and log size in big
> + * endian format. But in case of vtpm, there is a method called
> + * sml-handover which is run during kernel init even before
> + * device tree is setup. This sml-handover function takes care
> + * of endianness and writes to sml-base and sml-size in little
> + * endian format. For this reason, vtpm doesn't need conversion
> + * but physical tpm needs the conversion.
> + */
> + if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> + of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + *size = be32_to_cpup((__force __be32 *)sizep);
> + *base = be64_to_cpup((__force __be64 *)basep);
> + } else {
> + *size = *sizep;
> + *base = *basep;
> + }
> + return 0;
> +}
> +#endif
> +
> #endif
> --
> 2.35.1
>
Acked-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v5 0/6] DCP as trusted keys backend
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 1/6] crypto: mxs-dcp: Add support for hardware-bound keys
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 1/6] crypto: mxs-dcp: Add support for hardware-bound keys
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 2/6] KEYS: trusted: improve scalability of trust source config
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 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
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 4/6] MAINTAINERS: add entry for DCP-based trusted keys
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 0/6] DCP as trusted keys backend
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 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Mon, Feb 27, 2023 at 06:08:31PM -0500, Stefan Berger wrote:
>
>
> On 2/23/23 22:25, Michael Ellerman wrote:
> > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> > Measurement Log) from OF to Linux, before Linux shuts down Open
> > Firmware.
> >
> > This involves creating a buffer to hold the SML, and creating two device
> > tree properties to record its base address and size. The kernel then
> > later reads those properties from the device tree to find the SML.
> >
> > When the code was initially added in commit 4a727429abec ("PPC64: Add
> > support for instantiating SML from Open Firmware") the powerpc kernel
> > was always built big endian, so the properties were created big endian
> > by default.
> >
> > However since then little endian support was added to powerpc, and now
> > the code lacks conversions to big endian when creating the properties.
> >
> > This means on little endian kernels the device tree properties are
> > little endian, which is contrary to the device tree spec, and in
> > contrast to all other device tree properties.
> >
> > To cope with that a workaround was added in tpm_read_log_of() to skip
> > the endian conversion if the properties were created via the SML
> > handover.
> >
> > A better solution is to encode the properties as big endian as they
> > should be, and remove the workaround.
> >
> > Typically changing the encoding of a property like this would present
> > problems for kexec. However the SML is not propagated across kexec, so
> > changing the encoding of the properties is a non-issue.
> >
> > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> > event log")
> > Signed-off-by: Michael Ellerman
>
> Reviewed-by: Stefan Berger
2/2 does not have a fixes tag.
BR, Jarkko
Re: [PATCH 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Tue, Feb 28, 2023 at 10:21:36PM +1100, Michael Ellerman wrote:
> Jarkko Sakkinen writes:
> > On Mon, Feb 27, 2023 at 06:08:31PM -0500, Stefan Berger wrote:
> >> On 2/23/23 22:25, Michael Ellerman wrote:
> >> > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> >> > Measurement Log) from OF to Linux, before Linux shuts down Open
> >> > Firmware.
> >> >
> >> > This involves creating a buffer to hold the SML, and creating two device
> >> > tree properties to record its base address and size. The kernel then
> >> > later reads those properties from the device tree to find the SML.
> >> >
> >> > When the code was initially added in commit 4a727429abec ("PPC64: Add
> >> > support for instantiating SML from Open Firmware") the powerpc kernel
> >> > was always built big endian, so the properties were created big endian
> >> > by default.
> >> >
> >> > However since then little endian support was added to powerpc, and now
> >> > the code lacks conversions to big endian when creating the properties.
> >> >
> >> > This means on little endian kernels the device tree properties are
> >> > little endian, which is contrary to the device tree spec, and in
> >> > contrast to all other device tree properties.
> >> >
> >> > To cope with that a workaround was added in tpm_read_log_of() to skip
> >> > the endian conversion if the properties were created via the SML
> >> > handover.
> >> >
> >> > A better solution is to encode the properties as big endian as they
> >> > should be, and remove the workaround.
> >> >
> >> > Typically changing the encoding of a property like this would present
> >> > problems for kexec. However the SML is not propagated across kexec, so
> >> > changing the encoding of the properties is a non-issue.
> >> >
> >> > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> >> > event log")
> >> > Signed-off-by: Michael Ellerman
> >>
> >> Reviewed-by: Stefan Berger
> >
> > 2/2 does not have a fixes tag.
>
> True. Arguably the bug goes back to the introduction of
> kexec_file_load() support, although the patch won't backport that far
> due to code refactoring. So that would be:
>
> Fixes: a0458284f062 ("powerpc: Add support code for kexec_file_load()")
Hmm... IMHO, it would still make sense to document this.
BR, Jarkko
Re: [PATCH v6 1/6] crypto: mxs-dcp: Add support for hardware-bound keys
E, > + .setkey = mxs_dcp_aes_setrefkey, > + .encrypt= mxs_dcp_aes_cbc_encrypt, > + .decrypt= mxs_dcp_aes_cbc_decrypt, > + .ivsize = AES_BLOCK_SIZE, > + .init = mxs_dcp_paes_init_tfm, > }, > }; > > diff --git a/include/soc/fsl/dcp.h b/include/soc/fsl/dcp.h > new file mode 100644 > index ..3ec335d8ca8b > --- /dev/null > +++ b/include/soc/fsl/dcp.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2021 sigma star gmbh > + * > + * Specifies paes key slot handles for NXP's DCP (Data Co-Processor) to be > used > + * with the crypto_skcipher_setkey(). > + */ > + > +#ifndef MXS_DCP_H > +#define MXS_DCP_H > + > +#define DCP_PAES_KEYSIZE 1 > +#define DCP_PAES_KEY_SLOT0 0x00 > +#define DCP_PAES_KEY_SLOT1 0x01 > +#define DCP_PAES_KEY_SLOT2 0x02 > +#define DCP_PAES_KEY_SLOT3 0x03 > +#define DCP_PAES_KEY_UNIQUE 0xfe > +#define DCP_PAES_KEY_OTP 0xff > + > +#endif /* MXS_DCP_H */ Looks to good enough to me: Reviewed-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v6 2/6] KEYS: trusted: improve scalability of trust source config
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 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
bool do_encrypt)
> +{
> + struct aead_request *aead_req = NULL;
> + struct scatterlist src_sg, dst_sg;
> + struct crypto_aead *aead;
> + int ret;
> +
> + aead = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(aead)) {
> + ret = PTR_ERR(aead);
> + goto out;
> + }
> +
> + ret = crypto_aead_setauthsize(aead, DCP_BLOB_AUTHLEN);
> + if (ret < 0) {
> + pr_err("Can't set crypto auth tag len: %d\n", ret);
> + goto free_aead;
> + }
> +
> + aead_req = aead_request_alloc(aead, GFP_KERNEL);
> + if (!aead_req) {
> + ret = -ENOMEM;
> + goto free_aead;
> + }
> +
> + sg_init_one(&src_sg, in, len);
> + if (do_encrypt) {
> + /*
> + * If we encrypt our buffer has extra space for the auth tag.
> + */
> + sg_init_one(&dst_sg, out, len + DCP_BLOB_AUTHLEN);
> + } else {
> + sg_init_one(&dst_sg, out, len);
> + }
> +
> + aead_request_set_crypt(aead_req, &src_sg, &dst_sg, len, nonce);
> + aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL,
> + NULL);
> + aead_request_set_ad(aead_req, 0);
> +
> + if (crypto_aead_setkey(aead, key, AES_KEYSIZE_128)) {
> + pr_err("Can't set crypto AEAD key\n");
> + ret = -EINVAL;
> + goto free_req;
> + }
> +
> + if (do_encrypt)
> + ret = crypto_aead_encrypt(aead_req);
> + else
> + ret = crypto_aead_decrypt(aead_req);
> +
> +free_req:
> + aead_request_free(aead_req);
> +free_aead:
> + crypto_free_aead(aead);
> +out:
> + return ret;
> +}
> +
> +static int decrypt_blob_key(u8 *key)
> +{
> + return do_dcp_crypto(key, key, false);
> +}
> +
> +static int encrypt_blob_key(u8 *key)
> +{
> + return do_dcp_crypto(key, key, true);
> +}
> +
> +static int trusted_dcp_seal(struct trusted_key_payload *p, char *datablob)
> +{
> + struct dcp_blob_fmt *b = (struct dcp_blob_fmt *)p->blob;
> + int blen, ret;
> +
> + blen = calc_blob_len(p->key_len);
> + if (blen > MAX_BLOB_SIZE)
> + return -E2BIG;
> +
> + b->fmt_version = DCP_BLOB_VERSION;
> + get_random_bytes(b->nonce, AES_KEYSIZE_128);
> + get_random_bytes(b->blob_key, AES_KEYSIZE_128);
> +
> + ret = do_aead_crypto(p->key, b->payload, p->key_len, b->blob_key,
> + b->nonce, true);
> + if (ret) {
> + pr_err("Unable to encrypt blob payload: %i\n", ret);
> + return ret;
> + }
> +
> + ret = encrypt_blob_key(b->blob_key);
> + if (ret) {
> + pr_err("Unable to encrypt blob key: %i\n", ret);
> + return ret;
> + }
> +
> + b->payload_len = get_unaligned_le32(&p->key_len);
> + p->blob_len = blen;
> + return 0;
> +}
> +
> +static int trusted_dcp_unseal(struct trusted_key_payload *p, char *datablob)
> +{
> + struct dcp_blob_fmt *b = (struct dcp_blob_fmt *)p->blob;
> + int blen, ret;
> +
> + if (b->fmt_version != DCP_BLOB_VERSION) {
> + pr_err("DCP blob has bad version: %i, expected %i\n",
> +b->fmt_version, DCP_BLOB_VERSION);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + p->key_len = le32_to_cpu(b->payload_len);
> + blen = calc_blob_len(p->key_len);
> + if (blen != p->blob_len) {
> + pr_err("DCP blob has bad length: %i != %i\n", blen,
> +p->blob_len);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = decrypt_blob_key(b->blob_key);
> + if (ret) {
> + pr_err("Unable to decrypt blob key: %i\n", ret);
> + goto out;
> + }
> +
> + ret = do_aead_crypto(b->payload, p->key, p->key_len + DCP_BLOB_AUTHLEN,
> + b->blob_key, b->nonce, false);
> + if (ret) {
> + pr_err("Unwrap of DCP payload failed: %i\n", ret);
> + goto out;
> + }
> +
> + ret = 0;
> +out:
> + return ret;
> +}
> +
> +static int test_for_zero_key(void)
> +{
> + static const u8 bad[] = {0x9a, 0xda, 0xe0, 0x54, 0xf6, 0x3d, 0xfa, 0xff,
> + 0x5e, 0xa1, 0x8e, 0x45, 0xed, 0xf6, 0xea,
> 0x6f};
> + void *buf = NULL;
> + int ret = 0;
> +
> + if (skip_zk_test)
> + goto out;
> +
> + buf = kmalloc(AES_BLOCK_SIZE, GFP_KERNEL);
> + if (!buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + memset(buf, 0x55, AES_BLOCK_SIZE);
> +
> + ret = do_dcp_crypto(buf, buf, true);
> + if (ret)
> + goto out;
> +
> + if (memcmp(buf, bad, AES_BLOCK_SIZE) == 0) {
> + pr_err("Device neither in secure nor trusted mode!\n");
> + ret = -EINVAL;
> + }
> +out:
> + kfree(buf);
> + return ret;
> +}
> +
> +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);
> +
> + return -EINVAL;
> + }
> +
> + return register_key_type(&key_type_trusted);
> +}
> +
> +static void trusted_dcp_exit(void)
> +{
> + unregister_key_type(&key_type_trusted);
> +}
> +
> +struct trusted_key_ops dcp_trusted_key_ops = {
> + .exit = trusted_dcp_exit,
> + .init = trusted_dcp_init,
> + .seal = trusted_dcp_seal,
> + .unseal = trusted_dcp_unseal,
> + .migratable = 0,
> +};
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
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 5/6] docs: document DCP-backed trusted keys kernel params
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 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
in short summary: s/Use/use/
On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:
> If linux,sml-log is available use it to get the TPM log rather than the
> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> on Power where after a kexec the memory pointed to by linux,sml-base may
> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
> linux,sml-size on these two platforms.
>
> Signed-off-by: Stefan Berger
So shouldn't this have a fixed tag, or not?
> ---
> drivers/char/tpm/eventlog/of.c | 36 +++---
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..e37196e64ef1 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -54,8 +54,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> const u32 *sizep;
> const u64 *basep;
> struct tpm_bios_log *log;
> + const void *logp;
> u32 size;
> - u64 base;
>
> log = &chip->log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -66,37 +66,23 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return tpm_read_log_memory_region(chip);
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + logp = of_get_property(np, "linux,sml-log", &size);
> + if (logp == NULL) {
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return tpm_read_log_memory_region(chip);
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> + logp = __va(be64_to_cpup((__force __be64 *)basep));
> size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> }
> -
> if (size == 0) {
> dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
> return -EIO;
> }
>
> - log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size,
> GFP_KERNEL);
> + log->bios_event_log = devm_kmemdup(&chip->dev, logp, size, GFP_KERNEL);
> if (!log->bios_event_log)
> return -ENOMEM;
>
Looks pretty good other than that.
BR, Jarkko
Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote: > in short summary: s/Use/use/ > > On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote: > > If linux,sml-log is available use it to get the TPM log rather than the > > pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM > > on Power where after a kexec the memory pointed to by linux,sml-base may > > have been corrupted. Also, linux,sml-log has replaced linux,sml-base and > > linux,sml-size on these two platforms. > > > > Signed-off-by: Stefan Berger > > So shouldn't this have a fixed tag, or not? In English: do we want this to be backported to stable kernel releases or not? BR, Jarkko
Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:
> linux,sml-base holds the address of a buffer with the TPM log. This
> buffer may become invalid after a kexec and therefore embed the whole TPM
> log in linux,sml-log. This helps to protect the log since it is properly
> carried across a kexec with both of the kexec syscalls.
So, I see only description of the problem but nothing how it gets
addressed.
>
> Signed-off-by: Stefan Berger
> ---
> arch/powerpc/kernel/prom_init.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index e67effdba85c..41268c30de4c 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1956,12 +1956,8 @@ static void __init prom_instantiate_sml(void)
>
> reserve_mem(base, size);
>
> - prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> - &base, sizeof(base));
> - prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> - &size, sizeof(size));
> -
> - prom_debug("sml base = 0x%llx\n", base);
> + prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
> + (void *)base, size);
> prom_debug("sml size = 0x%x\n", size);
>
> prom_debug("prom_instantiate_sml: end...\n");
BR, Jarkko
Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
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 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
On Fri Mar 8, 2024 at 2:17 PM EET, Stefan Berger wrote: > > > On 3/7/24 15:00, Jarkko Sakkinen wrote: > > On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote: > >> in short summary: s/Use/use/ > >> > >> On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote: > >>> If linux,sml-log is available use it to get the TPM log rather than the > >>> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM > >>> on Power where after a kexec the memory pointed to by linux,sml-base may > >>> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and > >>> linux,sml-size on these two platforms. > >>> > >>> Signed-off-by: Stefan Berger > >> > >> So shouldn't this have a fixed tag, or not? > > > > In English: do we want this to be backported to stable kernel releases or > > not? > > Ideally, yes. v3 will have 3 patches and all 3 of them will have to be > backported *together* and not applied otherwise if any one of them > fails. Can this be 'guaranteed'? All of them will end up to stable if the following conditions hold: - All have a fixes tag. - All have "Cc: sta...@vger.kernel.org". - We agree in the review process that they are all legit fixes. BR, Jarkko
Re: [RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote: > linux,sml-base holds the address of a buffer with the TPM log. This > buffer may become invalid after a kexec. To avoid accessing an invalid > address or corrupted buffer, embed the whole TPM log in the device tree > property linux,sml-log. This helps to protect the log since it is > properly carried across a kexec soft reboot with both of the kexec > syscalls. - Describe the environment where TPM log gets corrupted. - Describe why TPM log gets corrupted on kexec. > > Avoid having the firmware ingest the whole TPM log when calling > prom_setprop but only create the linux,sml-log property as a place holder. > Insert the actual TPM log during the tree flattening phase. This commit message should shed some light about reasons of the corruption in order to conclude that it should be fixed up like this. I.e. why the "post-state" is a legit state where can be continued despite a log being corrupted. Especially in security features this is pretty essential information. BR, Jarkko
Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size
On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> If linux,sml-log is available use it to get the TPM log rather than the
> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> on Power where after a kexec the memory pointed to by linux,sml-base may
> have become inaccessible or corrupted. Also, linux,sml-log has replaced
> linux,sml-base and linux,sml-size on these two platforms.
>
> Keep the handling of linux,sml-base/sml-size for powernv platforms that
> provide the two properties via skiboot.
>
> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
> Signed-off-by: Stefan Berger
I'm worried about not being up to date and instead using "cached" values
when verifying anything from a security chip. Does this guarantee that
TPM log is corrupted and will not get updated somehow?
BR, Jarkko
Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size
On Mon Mar 11, 2024 at 10:33 PM EET, Stefan Berger wrote:
>
>
> On 3/11/24 16:25, Jarkko Sakkinen wrote:
> > On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> >> If linux,sml-log is available use it to get the TPM log rather than the
> >> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> >> on Power where after a kexec the memory pointed to by linux,sml-base may
> >> have become inaccessible or corrupted. Also, linux,sml-log has replaced
> >> linux,sml-base and linux,sml-size on these two platforms.
> >>
> >> Keep the handling of linux,sml-base/sml-size for powernv platforms that
> >> provide the two properties via skiboot.
> >>
> >> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event
> >> log")
> >> Signed-off-by: Stefan Berger
> >
> > I'm worried about not being up to date and instead using "cached" values
> > when verifying anything from a security chip. Does this guarantee that
> > TPM log is corrupted and will not get updated somehow?
>
>
> What do you mean 'guarantee that TPM log is corrupted'?
I presume that this is for power architecture but I have no idea what
leads log being corrupted, and is the scope all of that that arch or
some subset of CPUs.
The commit message is not very detailed on kexec scenario. It more like
assumes that reader knows all the detail beforehand. So probably this
will start to make sense once the backing story is improved, that's
all.
> The TPM was handed over from the firmware to Linux and the firmware then
> freed all memory associated with the log and will then not create a new
> log or touch the TPM or do anything that would require an update to the
> handed-over log. Linux also does not append to the firmware log. So
> whatever we now find in linux,sml-log would be the latest firmware log
> and the state of the 'firmware PCRs' computed from it should correspond
> to the current state of the 'firmware PCRs'.
So on what CPU this happens and is there any bigger picture for that
design choice in the firmware?
If it is a firmware bug, this should emit FW_BUG log message. If not,
then this commit message should provide the necessary context.
BR, Jarkko
Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote: > Stefan Berger writes: > > On 3/7/24 15:00, Jarkko Sakkinen wrote: > >> On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote: > >>> in short summary: s/Use/use/ > >>> > >>> On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote: > >>>> If linux,sml-log is available use it to get the TPM log rather than the > >>>> pointer found in linux,sml-base. This resolves an issue on PowerVM and > >>>> KVM > >>>> on Power where after a kexec the memory pointed to by linux,sml-base may > >>>> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and > >>>> linux,sml-size on these two platforms. > >>>> > >>>> Signed-off-by: Stefan Berger > >>> > >>> So shouldn't this have a fixed tag, or not? > >> > >> In English: do we want this to be backported to stable kernel releases or > >> not? > > > > Ideally, yes. v3 will have 3 patches and all 3 of them will have to be > > backported *together* and not applied otherwise if any one of them > > fails. Can this be 'guaranteed'? > > You can use Depends-on: to indicate the relationship. > > cheers Thanks, I've missed depends-on tag. Stefan, please add also "Cc: sta...@vger.kernel.org" just to make sure that I don't forget to add it. Right, and since these are so small scoped commits, and bug fixes in particular, it is also possible to do PR during the release cycle. BR, Jarkko
Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml
On Tue Mar 12, 2024 at 1:11 PM EET, Lukas Wunner wrote:
> On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote:
> > Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to
> > the properties. Either this property is required or both linux,sml-base and
> > linux,sml-size are required. Add a test case for verification.
> >
> > Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device
> > tree binding documentation")
>
> The Fixes tag is confusing. The patch won't even apply cleanly to the
> v4.10 commit referenced here as the conversion to yaml happened only
> recently with v6.8.
>
> Why is the Fixes tag necessary in the first place? Same question for
> the other patches in the series. This looks like feature work rather
> than a fix. Not sure whether it satisfies the "obviously correct"
> rule per Documentation/process/stable-kernel-rules.rst.
I'm not yet sure whether these are bug fixes and or improvements because
I did not fully understand the scenario where TPM corrupts the event log
so that part reminds to be seen.
Probably once I fully understand what is going on, it is possible to
argue on that.
BR, Jarkko
Re: [PATCH v7 2/6] KEYS: trusted: improve scalability of trust source config
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 v7 5/6] docs: document DCP-backed trusted keys kernel params
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 6/6] docs: trusted-encrypted: add DCP as new trust source
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 6/6] docs: trusted-encrypted: add DCP as new trust source
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:
Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source
On Thu Mar 28, 2024 at 8:47 PM EET, Jarkko Sakkinen wrote: > 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 R
Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
p.c > b/security/keys/trusted-keys/trusted_dcp.c > index 16c44aafeab3..b5f81a05be36 100644 > --- a/security/keys/trusted-keys/trusted_dcp.c > +++ b/security/keys/trusted-keys/trusted_dcp.c > @@ -19,6 +19,25 @@ > #define DCP_BLOB_VERSION 1 > #define DCP_BLOB_AUTHLEN 16 > > +/** > + * DOC: 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 payload and authentication tag make up the blob format > together > + * with a version number, payload length and authentication tag. > + */ > + > /** > * struct dcp_blob_fmt - DCP BLOB format. > * Reviewed-by: Jarkko Sakkinen I can only test that this does not break a machine without the hardware feature. Is there anyone who could possibly peer test these patches? BR, Jarkko
Re: [EXT] Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
On Tue Apr 9, 2024 at 12:48 PM EEST, Kshitiz Varshney wrote: > Hi Jarkko, > > > > -Original Message- > > From: Jarkko Sakkinen > > Sent: Wednesday, April 3, 2024 9:18 PM > > To: David Gstir ; Mimi Zohar ; > > James Bottomley ; Herbert Xu > > ; David S. Miller > > Cc: Shawn Guo ; Jonathan Corbet > > ; Sascha Hauer ; Pengutronix > > Kernel Team ; Fabio Estevam > > ; dl-linux-imx ; Ahmad Fatoum > > ; sigma star Kernel Team > > ; David Howells ; Li > > Yang ; Paul Moore ; James > > Morris ; Serge E. Hallyn ; Paul E. > > McKenney ; Randy Dunlap ; > > Catalin Marinas ; Rafael J. Wysocki > > ; Tejun Heo ; Steven Rostedt > > (Google) ; linux-...@vger.kernel.org; linux- > > ker...@vger.kernel.org; linux-integr...@vger.kernel.org; > > keyri...@vger.kernel.org; linux-cry...@vger.kernel.org; linux-arm- > > ker...@lists.infradead.org; linuxppc-dev@lists.ozlabs.org; linux-security- > > mod...@vger.kernel.org; Richard Weinberger ; David > > Oberhollenzer > > Subject: [EXT] Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new > > trust source > > > > Caution: This is an external email. Please take care when clicking links or > > opening attachments. When in doubt, report the message using the 'Report > > this email' button > > > > > > On Wed Apr 3, 2024 at 10:21 AM EEST, 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 | 53 +++ > > > security/keys/trusted-keys/trusted_dcp.c | 19 +++ > > > 2 files changed, 72 insertions(+) > > > > > > diff --git a/Documentation/security/keys/trusted-encrypted.rst > > > b/Documentation/security/keys/trusted-encrypted.rst > > > index e989b9802f92..f4d7e162d5e4 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 > > >
Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
On Fri Apr 12, 2024 at 9:26 AM EEST, Herbert Xu wrote: > On Wed, Apr 03, 2024 at 06:47:51PM +0300, Jarkko Sakkinen wrote: > > > > Reviewed-by: Jarkko Sakkinen > > > > I can only test that this does not break a machine without the > > hardware feature. > > Please feel free to take this through your tree. > > Thanks, OK, thanks! BR, Jarkko
Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2
On Thu Apr 25, 2024 at 6:09 PM EEST, Sean Christopherson wrote: > + __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2), > + "KVM selftests from v6.8+ require > KVM_SET_USER_MEMORY_REGION2"); This would work also for casual (but not seasoned) visitor in KVM code as additionl documentation. BR, Jarkko
Re: [EXT] [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
On Tue Apr 30, 2024 at 3:03 PM EEST, David Gstir wrote: > Hi Jarkko, > > > On 30.04.2024, at 13:48, Kshitiz Varshney wrote: > > > > Hi David, > > > >> -Original Message- > >> From: David Gstir > >> Sent: Monday, April 29, 2024 5:05 PM > >> To: Kshitiz Varshney > > > >> > >> Did you get around to testing this? > >> I’d greatly appreciate a Tested-by for this. :-) > >> > >> Thanks! > >> BR, David > > > > Currently, I am bit busy with other priority activities. It will take time > > to test this patch set. > > How should we proceed here? > Do we have to miss another release cycle, because of a Tested-by? > > If any bugs pop up I’ll happily fix them, but at the moment it appears to be > more of a formality. > IMHO the patch set itself is rather small and has been thoroughly reviewed to > ensure that any huge > issues would already have been caught by now. I don't mind picking this actually since unless you consume it, it should not get in the way. I'll pick it during the weekend. Thanks for reminding. BR, Jarkko
Re: [PATCH] tpm: of: avoid __va() translation for event log address
On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > > On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > > wrote: > > > > > > > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > > > > The TPM event log is provided to the OS by the firmware, by loading > > > > > it into an area in memory and passing the physical address via a node > > > > > in the device tree. > > > > > > > > > > Currently, we use __va() to access the memory via the kernel's linear > > > > > map: however, it is not guaranteed that the linear map covers this > > > > > particular address, as we may be running under HIGHMEM on a 32-bit > > > > > architecture, or running firmware that uses a memory type for the > > > > > event log that is omitted from the linear map (such as EfiReserved). > > > > > > > > Makes perfect sense to the level that I wonder if this should have a > > > > fixes tag and/or needs to be backported to the stable kernels? > > > > > > > > > > AIUI, the code was written specifically for ppc64, which is a > > > non-highmem, non-EFI architecture. However, when we start reusing this > > > driver for ARM, this issue could pop up. > > > > > > The code itself has been refactored a couple of times, so I think it > > > will require different versions of the patch for different generations > > > of stable kernels. > > > > > > So perhaps just add Cc: , and wait and see how > > > far back it applies cleanly? > > > > Yeah, I think I'll cc it with some note before the diffstat. > > > > I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > dead easy to backport below that. > > I have this vauge recollection of pointing at this before and being > told that it had to be __va for some PPC reason? > > Do check with the PPC people first, I see none on the CC list. > > Jason Thanks, added arch/powerpc maintainers. /Jarkko
Re: [PATCH] tpm: of: avoid __va() translation for event log address
On Mon, Sep 28, 2020 at 08:20:18AM +0200, Ard Biesheuvel wrote: > On Mon, 28 Sep 2020 at 07:56, Christophe Leroy > wrote: > > > > > > > > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > >>>> wrote: > > >>>>> > > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > >>>>>> The TPM event log is provided to the OS by the firmware, by loading > > >>>>>> it into an area in memory and passing the physical address via a node > > >>>>>> in the device tree. > > >>>>>> > > >>>>>> Currently, we use __va() to access the memory via the kernel's linear > > >>>>>> map: however, it is not guaranteed that the linear map covers this > > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit > > >>>>>> architecture, or running firmware that uses a memory type for the > > >>>>>> event log that is omitted from the linear map (such as EfiReserved). > > >>>>> > > >>>>> Makes perfect sense to the level that I wonder if this should have a > > >>>>> fixes tag and/or needs to be backported to the stable kernels? > > >>>>> > > >>>> > > >>>> AIUI, the code was written specifically for ppc64, which is a > > >>>> non-highmem, non-EFI architecture. However, when we start reusing this > > >>>> driver for ARM, this issue could pop up. > > >>>> > > >>>> The code itself has been refactored a couple of times, so I think it > > >>>> will require different versions of the patch for different generations > > >>>> of stable kernels. > > >>>> > > >>>> So perhaps just add Cc: , and wait and see how > > >>>> far back it applies cleanly? > > >>> > > >>> Yeah, I think I'll cc it with some note before the diffstat. > > >>> > > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > >>> dead easy to backport below that. > > >> > > >> I have this vauge recollection of pointing at this before and being > > >> told that it had to be __va for some PPC reason? > > >> > > >> Do check with the PPC people first, I see none on the CC list. > > >> > > >> Jason > > > > > > Thanks, added arch/powerpc maintainers. > > > > > > > As far as I can see, memremap() won't work on PPC32 at least: > > > > IIUC, memremap() calls arch_memremap_wb() > > arch_memremap_wb() calls ioremap_cache() > > In case of failure, then ioremap_wt() and ioremap_wc() are tried. > > > > All ioremap calls end up in __ioremap_caller() which will return NULL in > > case you try to ioremap RAM. > > > > So the statement "So instead, use memremap(), which will reuse the linear > > mapping if > > it is valid, or create another mapping otherwise." seems to be wrong, at > > least for PPC32. > > > > Even for PPC64 which doesn't seem to have the RAM check, I can't see that > > it will "reuse the linear > > mapping". > > > > It is there, please look again. Before any of the above happens, > memremap() will call try_ram_remap() for regions that are covered by a > IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and > it is not highmem. > > So as far as I can tell, this change has no effect on PPC at all > unless its RAM is not described as IORESOURCE_SYSTEM_RAM. Any chance for someone to test this on PPC32? /Jarkko
Re: [PATCH -next] tpm: ibmvtpm: Correct the return value in tpm_ibmvtpm_probe()
On Fri, Mar 18, 2022 at 02:02:01PM +0800, Xiu Jianfeng wrote:
> Currently it returns zero when CRQ response timed out, it should return
> an error code instead.
>
> Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> proceeding")
> Signed-off-by: Xiu Jianfeng
> ---
> drivers/char/tpm/tpm_ibmvtpm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 3af4c07a9342..d3989b257f42 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -681,6 +681,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
> if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
> ibmvtpm->rtce_buf != NULL,
> HZ)) {
> + rc = -ENODEV;
> dev_err(dev, "CRQ response timed out\n");
> goto init_irq_cleanup;
> }
> --
> 2.17.1
>
Acked-by: Jarkko Sakkinen
This will require reviewed by from someone who knows this driver
better.
BR, Jarkko
Re: [PATCH -next] tpm: ibmvtpm: Correct the return value in tpm_ibmvtpm_probe()
On Fri, Mar 18, 2022 at 09:54:46AM -0400, Stefan Berger wrote:
>
>
> On 3/18/22 02:02, Xiu Jianfeng wrote:
> > Currently it returns zero when CRQ response timed out, it should return
> > an error code instead.
> >
> > Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> > proceeding")
> > Signed-off-by: Xiu Jianfeng
>
> Reviewed-by: Stefan Berger
Thank you, I applied this.
BR, Jarkko
Re: [PATCH] char: tpm: Prepare cleanup of powerpc's asm/prom.h
On Sat, Apr 02, 2022 at 12:29:19PM +0200, Christophe Leroy wrote:
> powerpc's asm/prom.h brings some headers that it doesn't
> need itself.
>
> In order to clean it up, first add missing headers in
> users of asm/prom.h
>
> Signed-off-by: Christophe Leroy
I don't understand this. It changes nothing as far as kernel is concerned.
> ---
> drivers/char/tpm/tpm_atmel.h | 2 --
> drivers/char/tpm/tpm_ibmvtpm.c | 1 -
> 2 files changed, 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> index ba37e77e8af3..959f7cce8301 100644
> --- a/drivers/char/tpm/tpm_atmel.h
> +++ b/drivers/char/tpm/tpm_atmel.h
> @@ -26,8 +26,6 @@ struct tpm_atmel_priv {
>
> #ifdef CONFIG_PPC64
>
> -#include
> -
> #define atmel_getb(priv, offset) readb(priv->iobase + offset)
> #define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> #define atmel_request_region request_mem_region
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 3af4c07a9342..1180cce7067a 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -20,7 +20,6 @@
> #include
> #include
> #include
> -#include
>
> #include "tpm.h"
> #include "tpm_ibmvtpm.h"
> --
> 2.35.1
>
BR, Jarkko
Re: [PATCH 18/20] Documentation: security/keys: eliminate duplicated word
On Tue, Jul 07, 2020 at 11:04:12AM -0700, Randy Dunlap wrote: > Drop the doubled word "in". > > Signed-off-by: Randy Dunlap > Cc: Jonathan Corbet > Cc: linux-...@vger.kernel.org > Cc: James Bottomley > Cc: Jarkko Sakkinen > Cc: Mimi Zohar > Cc: linux-integr...@vger.kernel.org > Cc: keyri...@vger.kernel.org Acked-by: Jarkko Sakkinen /Jarkko
[PATCH 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
Rename module_alloc() to text_alloc() and module_memfree() to
text_memfree(), and move them to kernel/text.c, which is unconditionally
compiled to the kernel proper. This allows kprobes, ftrace and bpf to
allocate space for executable code without requiring to compile the modules
support (CONFIG_MODULES=y) in.
Cc: Andi Kleen
Suggested-by: Peter Zijlstra
Signed-off-by: Jarkko Sakkinen
---
arch/arm/kernel/Makefile | 3 +-
arch/arm/kernel/module.c | 21 ---
arch/arm/kernel/text.c | 33 ++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/module.c | 42 --
arch/arm64/kernel/text.c | 54
arch/mips/kernel/Makefile| 2 +-
arch/mips/kernel/module.c| 9 -
arch/mips/kernel/text.c | 19 ++
arch/mips/net/bpf_jit.c | 4 +--
arch/nds32/kernel/Makefile | 2 +-
arch/nds32/kernel/module.c | 7
arch/nds32/kernel/text.c | 12 +++
arch/nios2/kernel/Makefile | 1 +
arch/nios2/kernel/module.c | 19 --
arch/nios2/kernel/text.c | 34 ++
arch/parisc/kernel/Makefile | 2 +-
arch/parisc/kernel/module.c | 11 --
arch/parisc/kernel/text.c| 22
arch/powerpc/net/bpf_jit_comp.c | 4 +--
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/module.c | 12 ---
arch/riscv/kernel/text.c | 20 +++
arch/s390/kernel/Makefile| 2 +-
arch/s390/kernel/ftrace.c| 2 +-
arch/s390/kernel/module.c| 16 -
arch/s390/kernel/text.c | 23
arch/sparc/kernel/Makefile | 1 +
arch/sparc/kernel/module.c | 30
arch/sparc/kernel/text.c | 39 +
arch/sparc/net/bpf_jit_comp_32.c | 6 ++--
arch/unicore32/kernel/Makefile | 1 +
arch/unicore32/kernel/module.c | 7
arch/unicore32/kernel/text.c | 18 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/ftrace.c | 4 +--
arch/x86/kernel/kprobes/core.c | 4 +--
arch/x86/kernel/module.c | 49 --
arch/x86/kernel/text.c | 60
include/linux/moduleloader.h | 4 +--
kernel/Makefile | 2 +-
kernel/bpf/core.c| 4 +--
kernel/kprobes.c | 4 +--
kernel/module.c | 37 ++--
kernel/text.c| 25 +
45 files changed, 400 insertions(+), 275 deletions(-)
create mode 100644 arch/arm/kernel/text.c
create mode 100644 arch/arm64/kernel/text.c
create mode 100644 arch/mips/kernel/text.c
create mode 100644 arch/nds32/kernel/text.c
create mode 100644 arch/nios2/kernel/text.c
create mode 100644 arch/parisc/kernel/text.c
create mode 100644 arch/riscv/kernel/text.c
create mode 100644 arch/s390/kernel/text.c
create mode 100644 arch/sparc/kernel/text.c
create mode 100644 arch/unicore32/kernel/text.c
create mode 100644 arch/x86/kernel/text.c
create mode 100644 kernel/text.c
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 89e5d864e923..69bfacfd60ef 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -19,7 +19,8 @@ CFLAGS_REMOVE_return_address.o = -pg
obj-y := elf.o entry-common.o irq.o opcodes.o \
process.o ptrace.o reboot.o \
setup.o signal.o sigreturn_codes.o \
- stacktrace.o sys_arm.o time.o traps.o
+ stacktrace.o sys_arm.o time.o traps.o \
+ text.o
ifneq ($(CONFIG_ARM_UNWIND),y)
obj-$(CONFIG_FRAME_POINTER)+= return_address.o
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index e15444b25ca0..13e3442a6b9f 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -33,27 +33,6 @@
#define MODULES_VADDR (((unsigned long)_exiprom + ~PMD_MASK) & PMD_MASK)
#endif
-#ifdef CONFIG_MMU
-void *module_alloc(unsigned long size)
-{
- gfp_t gfp_mask = GFP_KERNEL;
- void *p;
-
- /* Silence the initial allocation */
- if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
- gfp_mask |= __GFP_NOWARN;
-
- p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
- gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
- if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
- return p;
- return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
- GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
-}
-#endif
-
bool module_init_section(const char *name)
{
return strstarts(name, ".init") ||
diff --git a/arch/arm/kernel/text.c b/arch/arm/kernel/text.c
new file mod
[PATCH v2 0/3] kprobes: Remove MODULE dependency
Remove MODULES dependency by creating module subsystem indepdent text_alloc() and text_memfree() to allocate space for executable code. Right now one has to compile modules support only to enable kprobes. This incrases the barrier to use them in test kernels and I'd guess also in some embedded kernels (the former is my use case). v2: * Added the missing cover letter. Jarkko Sakkinen (3): module: Rename module_alloc() to text_alloc() and move to kernel proper module: Add lock_modules() and unlock_modules() kprobes: Flag out CONFIG_MODULES dependent code arch/Kconfig | 1 - arch/arm/kernel/Makefile | 3 +- arch/arm/kernel/module.c | 21 --- arch/arm/kernel/text.c | 33 +++ arch/arm64/kernel/Makefile | 2 +- arch/arm64/kernel/module.c | 42 -- arch/arm64/kernel/text.c | 54 ++ arch/mips/kernel/Makefile| 2 +- arch/mips/kernel/module.c| 9 --- arch/mips/kernel/text.c | 19 +++ arch/mips/net/bpf_jit.c | 4 +- arch/nds32/kernel/Makefile | 2 +- arch/nds32/kernel/module.c | 7 --- arch/nds32/kernel/text.c | 12 arch/nios2/kernel/Makefile | 1 + arch/nios2/kernel/module.c | 19 --- arch/nios2/kernel/text.c | 34 +++ arch/parisc/kernel/Makefile | 2 +- arch/parisc/kernel/module.c | 11 arch/parisc/kernel/text.c| 22 arch/powerpc/net/bpf_jit_comp.c | 4 +- arch/riscv/kernel/Makefile | 1 + arch/riscv/kernel/module.c | 12 arch/riscv/kernel/text.c | 20 +++ arch/s390/kernel/Makefile| 2 +- arch/s390/kernel/ftrace.c| 2 +- arch/s390/kernel/module.c| 16 -- arch/s390/kernel/text.c | 23 arch/sparc/kernel/Makefile | 1 + arch/sparc/kernel/module.c | 30 -- arch/sparc/kernel/text.c | 39 + arch/sparc/net/bpf_jit_comp_32.c | 6 +- arch/unicore32/kernel/Makefile | 1 + arch/unicore32/kernel/module.c | 7 --- arch/unicore32/kernel/text.c | 18 ++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/ftrace.c | 4 +- arch/x86/kernel/kprobes/core.c | 4 +- arch/x86/kernel/module.c | 49 arch/x86/kernel/text.c | 60 include/linux/module.h | 29 +++--- include/linux/moduleloader.h | 4 +- kernel/Makefile | 2 +- kernel/bpf/core.c| 4 +- kernel/kprobes.c | 17 -- kernel/livepatch/core.c | 8 +-- kernel/module.c | 97 +--- kernel/text.c| 25 kernel/trace/trace_kprobe.c | 20 ++- 49 files changed, 484 insertions(+), 322 deletions(-) create mode 100644 arch/arm/kernel/text.c create mode 100644 arch/arm64/kernel/text.c create mode 100644 arch/mips/kernel/text.c create mode 100644 arch/nds32/kernel/text.c create mode 100644 arch/nios2/kernel/text.c create mode 100644 arch/parisc/kernel/text.c create mode 100644 arch/riscv/kernel/text.c create mode 100644 arch/s390/kernel/text.c create mode 100644 arch/sparc/kernel/text.c create mode 100644 arch/unicore32/kernel/text.c create mode 100644 arch/x86/kernel/text.c create mode 100644 kernel/text.c -- 2.25.1
[PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
Rename module_alloc() to text_alloc() and module_memfree() to
text_memfree(), and move them to kernel/text.c, which is unconditionally
compiled to the kernel proper. This allows kprobes, ftrace and bpf to
allocate space for executable code without requiring to compile the modules
support (CONFIG_MODULES=y) in.
Cc: Andi Kleen
Suggested-by: Peter Zijlstra
Signed-off-by: Jarkko Sakkinen
---
arch/arm/kernel/Makefile | 3 +-
arch/arm/kernel/module.c | 21 ---
arch/arm/kernel/text.c | 33 ++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/module.c | 42 --
arch/arm64/kernel/text.c | 54
arch/mips/kernel/Makefile| 2 +-
arch/mips/kernel/module.c| 9 -
arch/mips/kernel/text.c | 19 ++
arch/mips/net/bpf_jit.c | 4 +--
arch/nds32/kernel/Makefile | 2 +-
arch/nds32/kernel/module.c | 7
arch/nds32/kernel/text.c | 12 +++
arch/nios2/kernel/Makefile | 1 +
arch/nios2/kernel/module.c | 19 --
arch/nios2/kernel/text.c | 34 ++
arch/parisc/kernel/Makefile | 2 +-
arch/parisc/kernel/module.c | 11 --
arch/parisc/kernel/text.c| 22
arch/powerpc/net/bpf_jit_comp.c | 4 +--
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/module.c | 12 ---
arch/riscv/kernel/text.c | 20 +++
arch/s390/kernel/Makefile| 2 +-
arch/s390/kernel/ftrace.c| 2 +-
arch/s390/kernel/module.c| 16 -
arch/s390/kernel/text.c | 23
arch/sparc/kernel/Makefile | 1 +
arch/sparc/kernel/module.c | 30
arch/sparc/kernel/text.c | 39 +
arch/sparc/net/bpf_jit_comp_32.c | 6 ++--
arch/unicore32/kernel/Makefile | 1 +
arch/unicore32/kernel/module.c | 7
arch/unicore32/kernel/text.c | 18 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/ftrace.c | 4 +--
arch/x86/kernel/kprobes/core.c | 4 +--
arch/x86/kernel/module.c | 49 --
arch/x86/kernel/text.c | 60
include/linux/moduleloader.h | 4 +--
kernel/Makefile | 2 +-
kernel/bpf/core.c| 4 +--
kernel/kprobes.c | 4 +--
kernel/module.c | 37 ++--
kernel/text.c| 25 +
45 files changed, 400 insertions(+), 275 deletions(-)
create mode 100644 arch/arm/kernel/text.c
create mode 100644 arch/arm64/kernel/text.c
create mode 100644 arch/mips/kernel/text.c
create mode 100644 arch/nds32/kernel/text.c
create mode 100644 arch/nios2/kernel/text.c
create mode 100644 arch/parisc/kernel/text.c
create mode 100644 arch/riscv/kernel/text.c
create mode 100644 arch/s390/kernel/text.c
create mode 100644 arch/sparc/kernel/text.c
create mode 100644 arch/unicore32/kernel/text.c
create mode 100644 arch/x86/kernel/text.c
create mode 100644 kernel/text.c
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 89e5d864e923..69bfacfd60ef 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -19,7 +19,8 @@ CFLAGS_REMOVE_return_address.o = -pg
obj-y := elf.o entry-common.o irq.o opcodes.o \
process.o ptrace.o reboot.o \
setup.o signal.o sigreturn_codes.o \
- stacktrace.o sys_arm.o time.o traps.o
+ stacktrace.o sys_arm.o time.o traps.o \
+ text.o
ifneq ($(CONFIG_ARM_UNWIND),y)
obj-$(CONFIG_FRAME_POINTER)+= return_address.o
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index e15444b25ca0..13e3442a6b9f 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -33,27 +33,6 @@
#define MODULES_VADDR (((unsigned long)_exiprom + ~PMD_MASK) & PMD_MASK)
#endif
-#ifdef CONFIG_MMU
-void *module_alloc(unsigned long size)
-{
- gfp_t gfp_mask = GFP_KERNEL;
- void *p;
-
- /* Silence the initial allocation */
- if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
- gfp_mask |= __GFP_NOWARN;
-
- p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
- gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
- if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
- return p;
- return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
- GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
-}
-#endif
-
bool module_init_section(const char *name)
{
return strstarts(name, ".init") ||
diff --git a/arch/arm/kernel/text.c b/arch/arm/kernel/text.c
new file mod
Re: [PATCH 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Mon, Jul 13, 2020 at 07:34:10PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Jul 13, 2020 at 09:19:37PM +0300, Jarkko Sakkinen wrote: > > Rename module_alloc() to text_alloc() and module_memfree() to > > text_memfree(), and move them to kernel/text.c, which is unconditionally > > compiled to the kernel proper. This allows kprobes, ftrace and bpf to > > allocate space for executable code without requiring to compile the modules > > support (CONFIG_MODULES=y) in. > > I'm not sure this is a good idea for 32-bit ARM. The code you are > moving for 32-bit ARM is quite specific to module use in that it also > supports allocating from the vmalloc area, where the module code > knows to add PLT entries. > > If the other proposed users of this text_alloc() do not have the logic > to add PLT entries when branches between kernel code and this > allocation are not reachable by, e.g. a 26-bit signed offset for 32-bit > ARM code, then this code is not suitable for that use. My intention is to use this in kprobes code in the place of module_alloc(). I'm not sure why moving this code out of the module subsystem could possibly break anything. Unfortunately I forgot to add covere letter to my series. Sending v2 with that to explain my use case for this. /Jarkko
Re: [PATCH 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
On Mon, Jul 13, 2020 at 10:49:48PM +0300, Ard Biesheuvel wrote: > This patch suggests that there are other reasons why conflating > allocation of module space and allocating text pages for other uses > is a bad idea, but switching all users to text_alloc() is a step in > the wrong direction. It would be better to stop using module_alloc() > in core code except in the module loader, and have a generic > text_alloc() that can be overridden by the arch if necessary. Note > that x86 and s390 are the only architectures that use module_alloc() > in ftrace code. This series essentially does this: introduces text_alloc() and text_memfree(), which have generic implementations in kernel/text.c. Those can be overriddent by arch specific implementations. What you think should be done differently than in my patch set? /Jarkko
