* Re: issues about selinux namespace
From: Paul Moore @ 2021-07-20 2:56 UTC (permalink / raw)
To: xiujianfeng
Cc: Stephen Smalley, jamorris, Likun(OSLab), linux-security-module,
selinux
In-Reply-To: <ec36e53f-5a6d-b86e-790c-d58b7b503aae@huawei.com>
On Mon, Jul 19, 2021 at 9:55 AM xiujianfeng <xiujianfeng@huawei.com> wrote:
>
> thanks stepthen, I've found James's patch in
> https://lwn.net/Articles/737949/,
>
> but it seems can't resolve my questions, so any futher discussion would
> be helpfull and welcome.
>
> 在 2021/7/14 20:11, Stephen Smalley 写道:
> > Please take your email to the selinux@vger.kernel.org. You are the
> > second person to ask about selinux namespaces within the past week or
> > so. I did upstream the refactoring and encapsulation of the data
> > structures and code via the selinux_state patches, so those are in the
> > mainline kernel these days, and Paul Moore and I have periodically
> > re-based the remaining patches on top of upstream over in the
> > https://github.com/SELinuxProject/selinux-kernel/tree/working-selinuxns
> > branch. However, I had to drop the inode and superblock per-ns patches
> > temporarily because of changes to LSM (inode blob management moved to
> > the LSM framework out of the security modules), so that would need to
> > be revisited. There was a separate patch from James Morris to support
> > per-namespace security.selinux extended attributes; you can dig that
> > out from the history or mailing lists if you want to revive that. I
> > won't be able to look at it again until October at the earliest.
> >
> > On Wed, Jul 14, 2021 at 6:54 AM xiujianfeng <xiujianfeng@huawei.com> wrote:
> >> Hi Stephen,
> >>
> >> I am writing to discuss about selinux namespace because I found your
> >> previous work on github and I think selinux namespace is helpful to
> >> harden container security. So I try to do further work but there are
> >> some issues mentioned in the commit message and I have no idea how to
> >> fix them, it would be great if I can get help from you.
> >> First is about selinux hook functions, we need to update each hook to
> >> perform its processing on current namespace and all of its ancestors,
> >> for object, we can have different sid/tag in different namespace based
> >> on inode namespace support, but for task, do we need to maintain each
> >> security context generated in the corresponding namespace?
> >> Second is the lifecycle management of on-disk inode labels. it's not
> >> easy to handle this, should we clean all corresponding labels on disk
> >> when namespace exit? if we do this, it may cost long time to iterate
> >> inode on disk and must relabel files when container restart, if not, the
> >> inode xattr space maybe full and cannot write label again when new
> >> namespace starts.
> >> BTW, do you have plan to finish the work?
> >>
> >> I look forward to receiving your reply.
> >>
> >> Best wishes.
I understand that many mail clients do not encourage inline/bottom
replies, but when posting to the various Linux Kernel mailing lists
please make the effort to reply inline, or at the bottom, as
appropriate.
Namespacing the SELinux kernel code is a rather tricky thing, both
with respect to the design and the mechanics of the implementation. I
don't think we have a concrete idea yet on how we want to proceed in
all of the areas mentioned; designs - and implementations - have been
offered, but I think we are missing someone to drive the topic forward
with demonstrations, sample implementations, etc. It is never a bad
idea to ask how you can help a project, but in this case I think the
answer is to step back for a moment, describe your use-case/problem,
explain how you envision a namespaced SELinux helping you resolve
this, and finally how you would want the namespaced SELinux
implementation to work (how would you interact with it both via policy
and runtime management).
On a personal note, the regular rebasing of the SELinux namespace work
has suffered lately due to other time commitments at work. I have
recently (today) started a new position which should allow me to
dedicate much more of my working hours to upstream development; it may
take me a couple of weeks to get settled in, but you can expect the
regular rebasing of selinux/working-selinuxns to resume in the future.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] ima: Support euid keyword for buffer measurement
From: Mimi Zohar @ 2021-07-19 21:30 UTC (permalink / raw)
To: Roberto Sassu, Lakshmi Ramasubramanian
Cc: tusharsu@linux.microsoft.com, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <35a1ef50bf534933a10fd350aee9baa2@huawei.com>
Hi Roberto,
On Wed, 2021-07-07 at 07:15 +0000, Roberto Sassu wrote:
> > From: Lakshmi Ramasubramanian [mailto:nramas@linux.microsoft.com]
> > Sent: Tuesday, July 6, 2021 9:30 PM
> > On 7/5/2021 4:56 AM, Roberto Sassu wrote:
> >
> > Hi Roberto,
> >
> > > This patch makes the 'euid' keyword available for buffer measurement rules,
> > > in the same way as for other rules. Currently, there is only support for
> > > the 'uid' keyword.
> > >
> > > With this change, buffer measurement (or non-measurement) can depend
> > also
> > > on the process effective UID.
> >
> > Who (kernel component) will be using this?
>
> Hi Lakshmi
>
> I'm using it in a (not yet submitted) test for digest lists.
>
> It is in a dont_measure rule to try to unload a digest list
> without measurement and to check that this is not allowed
> if the digest list was measured at addition time (to ensure
> completeness of information).
>
> > Maybe you could make this change as part of the patch set in which the
> > above "euid" support will be used.
>
> I wanted to send the digest lists patch set without anything
> else. I could resend the patch as part of that patch set if it is
> preferred.
Unless there is another usecase, please keep it with the digest list
tests patch set.
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v3 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
From: Mimi Zohar @ 2021-07-19 20:59 UTC (permalink / raw)
To: Roberto Sassu, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux
In-Reply-To: <20210705090922.3321178-4-roberto.sassu@huawei.com>
Hi Roberto,
On Mon, 2021-07-05 at 11:09 +0200, Roberto Sassu wrote:
> This patch adds the 'digest' and 'digest_len' parameters to
> ima_measure_critical_data() and process_buffer_measurement(), so that
> callers can get the digest of the passed buffer.
>
> These functions calculate the digest even if there is no suitable rule in
> the IMA policy and, in this case, they simply return 1 before generating a
> new measurement entry.
I agree ima_measure_critical_data() is special. Both this patch
description and 1/3 describe "what", not "why". Please provide the
motivation for these changes at least in the cover letter, if not in
the patch description.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v3 2/3] ima: Return int in the functions to measure a buffer
From: Mimi Zohar @ 2021-07-19 20:28 UTC (permalink / raw)
To: Roberto Sassu, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux
In-Reply-To: <20210705090922.3321178-3-roberto.sassu@huawei.com>
Hi Roberto,
On Mon, 2021-07-05 at 11:09 +0200, Roberto Sassu wrote:
> ima_measure_critical_data() and process_buffer_measurement() currently
> don't return a result. A caller wouldn't be able to know whether those
> functions were executed successfully.
Missing is an explanation as to why these functions aren't currently
returning a result. The LSM/IMA hooks only return a negative result
for failure to appraise a file's integrity, not measure a file. Only
failure to appraise a file's integrity results in preventing the file
from being read/executed/mmaped. Other failures are only audited.
>
> This patch modifies the return type from void to int, and returns 0 if the
> buffer has been successfully measured, a negative value otherwise.
Needed here is an explanation as to why ima_measure_critical_data() is
special.
>
> Also, this patch does not modify the behavior of existing callers by
> processing the returned value. For those, the return value is ignored.
I agree that the existing behavior shouldn't change, but will this
result in the bots complaining?
thanks,
Mimi
^ permalink raw reply
* Re: issues about selinux namespace
From: xiujianfeng @ 2021-07-19 13:54 UTC (permalink / raw)
To: Stephen Smalley, jamorris; +Cc: Likun(OSLab), linux-security-module, selinux
In-Reply-To: <CAEjxPJ5-w83HMRGuDHHqMthkju3bxT0gZ-EiiTE=t5UhQqQ_ug@mail.gmail.com>
thanks stepthen, I've found James's patch in
https://lwn.net/Articles/737949/,
but it seems can't resolve my questions, so any futher discussion would
be helpfull and welcome.
在 2021/7/14 20:11, Stephen Smalley 写道:
> Please take your email to the selinux@vger.kernel.org. You are the
> second person to ask about selinux namespaces within the past week or
> so. I did upstream the refactoring and encapsulation of the data
> structures and code via the selinux_state patches, so those are in the
> mainline kernel these days, and Paul Moore and I have periodically
> re-based the remaining patches on top of upstream over in the
> https://github.com/SELinuxProject/selinux-kernel/tree/working-selinuxns
> branch. However, I had to drop the inode and superblock per-ns patches
> temporarily because of changes to LSM (inode blob management moved to
> the LSM framework out of the security modules), so that would need to
> be revisited. There was a separate patch from James Morris to support
> per-namespace security.selinux extended attributes; you can dig that
> out from the history or mailing lists if you want to revive that. I
> won't be able to look at it again until October at the earliest.
>
> On Wed, Jul 14, 2021 at 6:54 AM xiujianfeng <xiujianfeng@huawei.com> wrote:
>> Hi Stephen,
>>
>> I am writing to discuss about selinux namespace because I found your
>> previous work on github and I think selinux namespace is helpful to
>> harden container security. So I try to do further work but there are
>> some issues mentioned in the commit message and I have no idea how to
>> fix them, it would be great if I can get help from you.
>> First is about selinux hook functions, we need to update each hook to
>> perform its processing on current namespace and all of its ancestors,
>> for object, we can have different sid/tag in different namespace based
>> on inode namespace support, but for task, do we need to maintain each
>> security context generated in the corresponding namespace?
>> Second is the lifecycle management of on-disk inode labels. it's not
>> easy to handle this, should we clean all corresponding labels on disk
>> when namespace exit? if we do this, it may cost long time to iterate
>> inode on disk and must relabel files when container restart, if not, the
>> inode xattr space maybe full and cannot write label again when new
>> namespace starts.
>> BTW, do you have plan to finish the work?
>>
>> I look forward to receiving your reply.
>>
>> Best wishes.
> .
^ permalink raw reply
* Re: [PATCH v2 1/6] KEYS: trusted: allow use of TEE as backend without TCG_TPM support
From: Ahmad Fatoum @ 2021-07-19 9:09 UTC (permalink / raw)
To: Sumit Garg
Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
kernel, James Morris, Eric Biggers, Serge E. Hallyn,
Horia Geantă, Aymen Sghaier, Udit Agarwal, Jan Luebbe,
David Gstir, Richard Weinberger, Franck LENORMAND,
open list:ASYMMETRIC KEYS,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-integrity,
Linux Kernel Mailing List, open list:SECURITY SUBSYSTEM,
Andreas Rammhold
In-Reply-To: <CAFA6WYMJp5u_+3bNc0ykFzveakOS4nqJfPvSoaFGQApFctL47A@mail.gmail.com>
Hello Sumit,
On 19.07.21 10:04, Sumit Garg wrote:
> Hi Ahmad,
>
> On Tue, 22 Jun 2021 at 18:08, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
>> With recent rework, trusted keys are no longer limited to TPM as trust
>> source. The Kconfig symbol is unchanged however leading to a few issues:
>>
>> - TCG_TPM is required, even if only TEE is to be used
>> - Enabling TCG_TPM, but excluding it from available trusted sources
>> is not possible
>> - TEE=m && TRUSTED_KEYS=y will lead to TEE support being silently
>> dropped, which is not the best user experience
>>
>> Remedy these issues by introducing two new Kconfig symbols:
>> TRUSTED_KEYS_TPM and TRUSTED_KEYS_TEE with the appropriate
>> dependencies.
>>
>
> This should include a fixes tag to the rework commit.
Yes. I wasn't aware of the regression that Andreas (CC'd) recently
reported. Knowing, it now indeed warrants a backport. Will add in v2.
>> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
>> index 64b81abd087e..6fdb953b319f 100644
>> --- a/security/keys/Kconfig
>> +++ b/security/keys/Kconfig
>> @@ -70,23 +70,23 @@ config BIG_KEYS
>>
>> config TRUSTED_KEYS
>> tristate "TRUSTED KEYS"
>> - depends on KEYS && TCG_TPM
>> + depends on KEYS
>
>> select CRYPTO
>> select CRYPTO_HMAC
>> select CRYPTO_SHA1
>> select CRYPTO_HASH_INFO
>
> Should move these as well to TRUSTED_KEYS_TPM as the core code doesn't
> mandate their need.
Ok, will test and change appropriately.
>
>> - select ASN1_ENCODER
>> - select OID_REGISTRY
>> - select ASN1
>> $(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
>> -trusted-y += trusted_tpm2.o
>> -trusted-y += tpm2key.asn1.o
>> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
>> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>> +
>> +trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>>
>> trusted-$(CONFIG_TEE) += trusted_tee.o
>
> This should be dropped.
Right..
Thanks for the review. I'll isolate this patch for v2.
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: Fix trusted key backends when building as module
From: andreas @ 2021-07-19 9:13 UTC (permalink / raw)
To: Sumit Garg, Ahmad Fatoum
Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
James Morris, Serge E . Hallyn, linux-integrity,
open list:ASYMMETRIC KEYS, open list:SECURITY SUBSYSTEM,
Linux Kernel Mailing List, Pengutronix Kernel Team
In-Reply-To: <CAFA6WYNC2xasX4uKmgcK+ZmA4HUh5PhCci+e12VFWPfF0b0eWQ@mail.gmail.com>
On 13:36 19.07.21, Sumit Garg wrote:
> On Mon, 19 Jul 2021 at 12:40, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > Hello Andreas,
> >
> > On 16.07.21 10:17, Andreas Rammhold wrote:
> > > Before this commit the kernel could end up with no trusted key sources
> > > even thought both of the currently supported backends (tpm & tee) were
> > > compoiled as modules. This manifested in the trusted key type not being
> > > registered at all.
> >
> > I assume (TPM) trusted key module use worked before the TEE rework? If so,
> >
> > an appropriate Fixes: Tag would then be in order.
> >
> > > When checking if a CONFIG_… preprocessor variable is defined we only
> > > test for the builtin (=y) case and not the module (=m) case. By using
> > > the IS_ENABLE(…) macro we to test for both cases.
> >
> > It looks to me like you could now provoke a link error if TEE is a module
> > and built-in trusted key core tries to link against trusted_key_tee_ops.
> >
>
> That's true.
>
> > One solution for that IS_REACHABLE(). Another is to address the root cause,
> > which is the inflexible trusted keys Kconfig description:
> >
> > - Trusted keys despite TEE support can still only be built when TCG_TPM is enabled
> > - There is no support to have TEE or TPM enabled without using those for
> > enabled trusted keys as well
> > - As you noticed, module build of the backend has issues
> >
> > I addressed these three issues in a patch[1], a month ago, but have yet to
> > receive feedback.
>
> That's an oversight on my part since this patch was part of the new
> CAAM trust source patch-set. Although I do admit that it was on my
> TODO list. So I have provided some feedback on that patch. Can you
> post the next version as an independent fix patch?
Thank you both for the feedback. In light of thes feedback and the
patchset that Ahmad posted I'll not address the issue and not send a v2
of this.
I'll try to squeeze in some time to test the other patch and provide
feedback.
Andi
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: Fix trusted key backends when building as module
From: Sumit Garg @ 2021-07-19 8:06 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: Andreas Rammhold, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, James Morris, Serge E . Hallyn, linux-integrity,
open list:ASYMMETRIC KEYS, open list:SECURITY SUBSYSTEM,
Linux Kernel Mailing List, Pengutronix Kernel Team
In-Reply-To: <0a684d56-66d0-184e-4853-9faafa2d243d@pengutronix.de>
On Mon, 19 Jul 2021 at 12:40, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Andreas,
>
> On 16.07.21 10:17, Andreas Rammhold wrote:
> > Before this commit the kernel could end up with no trusted key sources
> > even thought both of the currently supported backends (tpm & tee) were
> > compoiled as modules. This manifested in the trusted key type not being
> > registered at all.
>
> I assume (TPM) trusted key module use worked before the TEE rework? If so,
>
> an appropriate Fixes: Tag would then be in order.
>
> > When checking if a CONFIG_… preprocessor variable is defined we only
> > test for the builtin (=y) case and not the module (=m) case. By using
> > the IS_ENABLE(…) macro we to test for both cases.
>
> It looks to me like you could now provoke a link error if TEE is a module
> and built-in trusted key core tries to link against trusted_key_tee_ops.
>
That's true.
> One solution for that IS_REACHABLE(). Another is to address the root cause,
> which is the inflexible trusted keys Kconfig description:
>
> - Trusted keys despite TEE support can still only be built when TCG_TPM is enabled
> - There is no support to have TEE or TPM enabled without using those for
> enabled trusted keys as well
> - As you noticed, module build of the backend has issues
>
> I addressed these three issues in a patch[1], a month ago, but have yet to
> receive feedback.
That's an oversight on my part since this patch was part of the new
CAAM trust source patch-set. Although I do admit that it was on my
TODO list. So I have provided some feedback on that patch. Can you
post the next version as an independent fix patch?
-Sumit
>
> [1]: https://lore.kernel.org/linux-integrity/f8285eb0135ba30c9d846cf9dd395d1f5f8b1efc.1624364386.git-series.a.fatoum@pengutronix.de/
>
> Cheers,
> Ahmad
>
> > Signed-off-by: Andreas Rammhold <andreas@rammhold.de>
> > ---
> > security/keys/trusted-keys/trusted_core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> > index d5c891d8d353..fd640614b168 100644
> > --- a/security/keys/trusted-keys/trusted_core.c
> > +++ b/security/keys/trusted-keys/trusted_core.c
> > @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
> > MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
> >
> > static const struct trusted_key_source trusted_key_sources[] = {
> > -#if defined(CONFIG_TCG_TPM)
> > +#if IS_ENABLED(CONFIG_TCG_TPM)
> > { "tpm", &trusted_key_tpm_ops },
> > #endif
> > -#if defined(CONFIG_TEE)
> > +#if IS_ENABLED(CONFIG_TEE)
> > { "tee", &trusted_key_tee_ops },
> > #endif
> > };
> >
>
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v2 1/6] KEYS: trusted: allow use of TEE as backend without TCG_TPM support
From: Sumit Garg @ 2021-07-19 8:04 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
kernel, James Morris, Eric Biggers, Serge E. Hallyn,
Horia Geantă, Aymen Sghaier, Udit Agarwal, Jan Luebbe,
David Gstir, Richard Weinberger, Franck LENORMAND,
open list:ASYMMETRIC KEYS,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-integrity,
Linux Kernel Mailing List, open list:SECURITY SUBSYSTEM
In-Reply-To: <f8285eb0135ba30c9d846cf9dd395d1f5f8b1efc.1624364386.git-series.a.fatoum@pengutronix.de>
Hi Ahmad,
On Tue, 22 Jun 2021 at 18:08, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> With recent rework, trusted keys are no longer limited to TPM as trust
> source. The Kconfig symbol is unchanged however leading to a few issues:
>
> - TCG_TPM is required, even if only TEE is to be used
> - Enabling TCG_TPM, but excluding it from available trusted sources
> is not possible
> - TEE=m && TRUSTED_KEYS=y will lead to TEE support being silently
> dropped, which is not the best user experience
>
> Remedy these issues by introducing two new Kconfig symbols:
> TRUSTED_KEYS_TPM and TRUSTED_KEYS_TEE with the appropriate
> dependencies.
>
This should include a fixes tag to the rework commit.
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> To: James Bottomley <jejb@linux.ibm.com>
> To: Jarkko Sakkinen <jarkko@kernel.org>
> To: Mimi Zohar <zohar@linux.ibm.com>
> To: David Howells <dhowells@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: "Horia Geantă" <horia.geanta@nxp.com>
> Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
> Cc: Udit Agarwal <udit.agarwal@nxp.com>
> Cc: Jan Luebbe <j.luebbe@pengutronix.de>
> Cc: David Gstir <david@sigma-star.at>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Franck LENORMAND <franck.lenormand@nxp.com>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: keyrings@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> ---
> security/keys/Kconfig | 14 ++++++-------
> security/keys/trusted-keys/Kconfig | 25 ++++++++++++++++++++++++-
> security/keys/trusted-keys/Makefile | 8 +++++---
> security/keys/trusted-keys/trusted_core.c | 4 ++--
> 4 files changed, 39 insertions(+), 12 deletions(-)
> create mode 100644 security/keys/trusted-keys/Kconfig
>
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 64b81abd087e..6fdb953b319f 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -70,23 +70,23 @@ config BIG_KEYS
>
> config TRUSTED_KEYS
> tristate "TRUSTED KEYS"
> - depends on KEYS && TCG_TPM
> + depends on KEYS
> select CRYPTO
> select CRYPTO_HMAC
> select CRYPTO_SHA1
> select CRYPTO_HASH_INFO
Should move these as well to TRUSTED_KEYS_TPM as the core code doesn't
mandate their need.
> - select ASN1_ENCODER
> - select OID_REGISTRY
> - select ASN1
> help
> This option provides support for creating, sealing, and unsealing
> keys in the kernel. Trusted keys are random number symmetric keys,
> - generated and RSA-sealed by the TPM. The TPM only unseals the keys,
> - if the boot PCRs and other criteria match. Userspace will only ever
> - see encrypted blobs.
> + generated and sealed by a trust source selected at kernel boot-time.
> + Userspace will only ever see encrypted blobs.
>
> If you are unsure as to whether this is required, answer N.
>
> +if TRUSTED_KEYS
> +source "security/keys/trusted-keys/Kconfig"
> +endif
> +
> config ENCRYPTED_KEYS
> tristate "ENCRYPTED KEYS"
> depends on KEYS
> diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
> new file mode 100644
> index 000000000000..24af4aaceebf
> --- /dev/null
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -0,0 +1,25 @@
> +config TRUSTED_KEYS_TPM
> + bool "TPM-based trusted keys"
> + depends on TCG_TPM >= TRUSTED_KEYS
> + default y
> + select ASN1_ENCODER
> + select OID_REGISTRY
> + select ASN1
> + help
> + Enable use of the Trusted Platform Module (TPM) as trusted key
> + backend. Trusted keys are are random number symmetric keys,
> + which will be generated and RSA-sealed by the TPM.
> + The TPM only unseals the keys, if the boot PCRs and other
> + criteria match.
> +
> +config TRUSTED_KEYS_TEE
> + bool "TEE-based trusted keys"
> + depends on TEE >= TRUSTED_KEYS
> + default y
> + help
> + Enable use of the Trusted Execution Environment (TEE) as trusted
> + key backend.
> +
> +if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE
> +comment "No trust source selected!"
> +endif
> diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> index feb8b6c3cc79..96fc6c377398 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -5,10 +5,12 @@
>
> obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
> trusted-y += trusted_core.o
> -trusted-y += trusted_tpm1.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
>
> $(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
> -trusted-y += trusted_tpm2.o
> -trusted-y += tpm2key.asn1.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
> +
> +trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>
> trusted-$(CONFIG_TEE) += trusted_tee.o
This should be dropped.
-Sumit
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..8cab69e5d0da 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
> MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
>
> static const struct trusted_key_source trusted_key_sources[] = {
> -#if defined(CONFIG_TCG_TPM)
> +#if defined(CONFIG_TRUSTED_KEYS_TPM)
> { "tpm", &trusted_key_tpm_ops },
> #endif
> -#if defined(CONFIG_TEE)
> +#if defined(CONFIG_TRUSTED_KEYS_TEE)
> { "tee", &trusted_key_tee_ops },
> #endif
> };
> --
> git-series 0.9.1
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: Fix trusted key backends when building as module
From: Ahmad Fatoum @ 2021-07-19 7:10 UTC (permalink / raw)
To: Andreas Rammhold, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, James Morris, Serge E . Hallyn, linux-integrity,
keyrings, linux-security-module, linux-kernel,
Pengutronix Kernel Team
In-Reply-To: <20210716081722.4130161-1-andreas@rammhold.de>
Hello Andreas,
On 16.07.21 10:17, Andreas Rammhold wrote:
> Before this commit the kernel could end up with no trusted key sources
> even thought both of the currently supported backends (tpm & tee) were
> compoiled as modules. This manifested in the trusted key type not being
> registered at all.
I assume (TPM) trusted key module use worked before the TEE rework? If so,
an appropriate Fixes: Tag would then be in order.
> When checking if a CONFIG_… preprocessor variable is defined we only
> test for the builtin (=y) case and not the module (=m) case. By using
> the IS_ENABLE(…) macro we to test for both cases.
It looks to me like you could now provoke a link error if TEE is a module
and built-in trusted key core tries to link against trusted_key_tee_ops.
One solution for that IS_REACHABLE(). Another is to address the root cause,
which is the inflexible trusted keys Kconfig description:
- Trusted keys despite TEE support can still only be built when TCG_TPM is enabled
- There is no support to have TEE or TPM enabled without using those for
enabled trusted keys as well
- As you noticed, module build of the backend has issues
I addressed these three issues in a patch[1], a month ago, but have yet to
receive feedback.
[1]: https://lore.kernel.org/linux-integrity/f8285eb0135ba30c9d846cf9dd395d1f5f8b1efc.1624364386.git-series.a.fatoum@pengutronix.de/
Cheers,
Ahmad
> Signed-off-by: Andreas Rammhold <andreas@rammhold.de>
> ---
> security/keys/trusted-keys/trusted_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..fd640614b168 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
> MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
>
> static const struct trusted_key_source trusted_key_sources[] = {
> -#if defined(CONFIG_TCG_TPM)
> +#if IS_ENABLED(CONFIG_TCG_TPM)
> { "tpm", &trusted_key_tpm_ops },
> #endif
> -#if defined(CONFIG_TEE)
> +#if IS_ENABLED(CONFIG_TEE)
> { "tee", &trusted_key_tee_ops },
> #endif
> };
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: Fix trusted key backends when building as module
From: Sumit Garg @ 2021-07-19 6:26 UTC (permalink / raw)
To: Andreas Rammhold
Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
James Morris, Serge E . Hallyn, linux-integrity,
open list:ASYMMETRIC KEYS, open list:SECURITY SUBSYSTEM,
Linux Kernel Mailing List
In-Reply-To: <20210716081722.4130161-1-andreas@rammhold.de>
On Fri, 16 Jul 2021 at 13:54, Andreas Rammhold <andreas@rammhold.de> wrote:
>
> Before this commit the kernel could end up with no trusted key sources
> even thought both of the currently supported backends (tpm & tee) were
s/thought/though/
> compoiled as modules. This manifested in the trusted key type not being
s/compoiled/compiled/
> registered at all.
>
> When checking if a CONFIG_… preprocessor variable is defined we only
> test for the builtin (=y) case and not the module (=m) case. By using
> the IS_ENABLE(…) macro we to test for both cases.
>
s/to/do/
> Signed-off-by: Andreas Rammhold <andreas@rammhold.de>
> ---
> security/keys/trusted-keys/trusted_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Apart from minor nits above, add a corresponding fixes tag. With that:
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
-Sumit
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..fd640614b168 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
> MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
>
> static const struct trusted_key_source trusted_key_sources[] = {
> -#if defined(CONFIG_TCG_TPM)
> +#if IS_ENABLED(CONFIG_TCG_TPM)
> { "tpm", &trusted_key_tpm_ops },
> #endif
> -#if defined(CONFIG_TEE)
> +#if IS_ENABLED(CONFIG_TEE)
> { "tee", &trusted_key_tee_ops },
> #endif
> };
> --
> 2.32.0
>
^ permalink raw reply
* [PATCH] KEYS: trusted: Fix trusted key backends when building as module
From: Andreas Rammhold @ 2021-07-16 8:17 UTC (permalink / raw)
To: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
James Morris, Serge E . Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Cc: Andreas Rammhold
Before this commit the kernel could end up with no trusted key sources
even thought both of the currently supported backends (tpm & tee) were
compoiled as modules. This manifested in the trusted key type not being
registered at all.
When checking if a CONFIG_… preprocessor variable is defined we only
test for the builtin (=y) case and not the module (=m) case. By using
the IS_ENABLE(…) macro we to test for both cases.
Signed-off-by: Andreas Rammhold <andreas@rammhold.de>
---
security/keys/trusted-keys/trusted_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
index d5c891d8d353..fd640614b168 100644
--- a/security/keys/trusted-keys/trusted_core.c
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
static const struct trusted_key_source trusted_key_sources[] = {
-#if defined(CONFIG_TCG_TPM)
+#if IS_ENABLED(CONFIG_TCG_TPM)
{ "tpm", &trusted_key_tpm_ops },
#endif
-#if defined(CONFIG_TEE)
+#if IS_ENABLED(CONFIG_TEE)
{ "tee", &trusted_key_tee_ops },
#endif
};
--
2.32.0
^ permalink raw reply related
* Re: [PATCH] Smack: Fix wrong semantics in smk_access_entry()
From: Casey Schaufler @ 2021-07-15 15:15 UTC (permalink / raw)
To: Tianjia Zhang, James Morris, Serge E. Hallyn,
linux-security-module, linux-kernel
Cc: Casey Schaufler
In-Reply-To: <20210715091724.45768-1-tianjia.zhang@linux.alibaba.com>
On 7/15/2021 2:17 AM, Tianjia Zhang wrote:
> In the smk_access_entry() function, if no matching rule is found
> in the rust_list, a negative error code will be used to perform bit
> operations with the MAY_ enumeration value. This is semantically
> wrong. This patch fixes this issue.
Indeed, the code as written is functioning correctly by
sheer luck. I will take this patch. Thank you.
>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> ---
> security/smack/smack_access.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index 1f391f6a3d47..d2186e2757be 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -81,23 +81,22 @@ int log_policy = SMACK_AUDIT_DENIED;
> int smk_access_entry(char *subject_label, char *object_label,
> struct list_head *rule_list)
> {
> - int may = -ENOENT;
> struct smack_rule *srp;
>
> list_for_each_entry_rcu(srp, rule_list, list) {
> if (srp->smk_object->smk_known == object_label &&
> srp->smk_subject->smk_known == subject_label) {
> - may = srp->smk_access;
> - break;
> + int may = srp->smk_access;
> + /*
> + * MAY_WRITE implies MAY_LOCK.
> + */
> + if ((may & MAY_WRITE) == MAY_WRITE)
> + may |= MAY_LOCK;
> + return may;
> }
> }
>
> - /*
> - * MAY_WRITE implies MAY_LOCK.
> - */
> - if ((may & MAY_WRITE) == MAY_WRITE)
> - may |= MAY_LOCK;
> - return may;
> + return -ENOENT;
> }
>
> /**
^ permalink raw reply
* [PATCH] Smack: Fix wrong semantics in smk_access_entry()
From: Tianjia Zhang @ 2021-07-15 9:17 UTC (permalink / raw)
To: Casey Schaufler, James Morris, Serge E. Hallyn,
linux-security-module, linux-kernel
Cc: Tianjia Zhang
In the smk_access_entry() function, if no matching rule is found
in the rust_list, a negative error code will be used to perform bit
operations with the MAY_ enumeration value. This is semantically
wrong. This patch fixes this issue.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
security/smack/smack_access.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 1f391f6a3d47..d2186e2757be 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -81,23 +81,22 @@ int log_policy = SMACK_AUDIT_DENIED;
int smk_access_entry(char *subject_label, char *object_label,
struct list_head *rule_list)
{
- int may = -ENOENT;
struct smack_rule *srp;
list_for_each_entry_rcu(srp, rule_list, list) {
if (srp->smk_object->smk_known == object_label &&
srp->smk_subject->smk_known == subject_label) {
- may = srp->smk_access;
- break;
+ int may = srp->smk_access;
+ /*
+ * MAY_WRITE implies MAY_LOCK.
+ */
+ if ((may & MAY_WRITE) == MAY_WRITE)
+ may |= MAY_LOCK;
+ return may;
}
}
- /*
- * MAY_WRITE implies MAY_LOCK.
- */
- if ((may & MAY_WRITE) == MAY_WRITE)
- may |= MAY_LOCK;
- return may;
+ return -ENOENT;
}
/**
--
2.19.1.3.ge56e4f7
^ permalink raw reply related
* [PATCH v2] hardening: Introduce CONFIG_ZERO_CALL_USED_REGS
From: Kees Cook @ 2021-07-14 22:01 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Mark Rutland, Qing Zhao, linux-security-module,
linux-hardening
When CONFIG_ZERO_CALL_USED_REGS is enabled, build the kernel with
"-fzero-call-used-regs=used-gpr" (in GCC 11). This option will zero any
caller-used register contents just before returning from a function,
ensuring that temporary values are not leaked beyond the function
boundary. This means that register contents are less likely to be
available for side channel attacks and information exposures.
Additionally this helps reduce the number of useful ROP gadgets in the
kernel image by about 20%:
$ ROPgadget.py --nosys --nojop --binary vmlinux.stock | tail -n1
Unique gadgets found: 337245
$ ROPgadget.py --nosys --nojop --binary vmlinux.zero-call-regs | tail -n1
Unique gadgets found: 267175
and more notably removes simple "write-what-where" gadgets:
$ ROPgadget.py --ropchain --binary vmlinux.stock | sed -n '/Step 1/,/Step 2/p'
- Step 1 -- Write-what-where gadgets
[+] Gadget found: 0xffffffff8102d76c mov qword ptr [rsi], rdx ; ret
[+] Gadget found: 0xffffffff81000cf5 pop rsi ; ret
[+] Gadget found: 0xffffffff8104d7c8 pop rdx ; ret
[-] Can't find the 'xor rdx, rdx' gadget. Try with another 'mov [reg], reg'
[+] Gadget found: 0xffffffff814c2b4c mov qword ptr [rsi], rdi ; ret
[+] Gadget found: 0xffffffff81000cf5 pop rsi ; ret
[+] Gadget found: 0xffffffff81001e51 pop rdi ; ret
[-] Can't find the 'xor rdi, rdi' gadget. Try with another 'mov [reg], reg'
[+] Gadget found: 0xffffffff81540d61 mov qword ptr [rsi], rdi ; pop rbx ; pop rbp ; ret
[+] Gadget found: 0xffffffff81000cf5 pop rsi ; ret
[+] Gadget found: 0xffffffff81001e51 pop rdi ; ret
[-] Can't find the 'xor rdi, rdi' gadget. Try with another 'mov [reg], reg'
[+] Gadget found: 0xffffffff8105341e mov qword ptr [rsi], rax ; ret
[+] Gadget found: 0xffffffff81000cf5 pop rsi ; ret
[+] Gadget found: 0xffffffff81029a11 pop rax ; ret
[+] Gadget found: 0xffffffff811f1c3b xor rax, rax ; ret
- Step 2 -- Init syscall number gadgets
$ ROPgadget.py --ropchain --binary vmlinux.zero* | sed -n '/Step 1/,/Step 2/p'
- Step 1 -- Write-what-where gadgets
[-] Can't find the 'mov qword ptr [r64], r64' gadget
For an x86_64 parallel build tests, this has a less than 1% performance
impact, and grows the image size less than 1%:
$ size vmlinux.stock vmlinux.zero-call-regs
text data bss dec hex filename
22437676 8559152 14127340 45124168 2b08a48 vmlinux.stock
22453184 8563248 14110956 45127388 2b096dc vmlinux.zero-call-regs
Impact for other architectures may vary. For example, arm64 sees a 5.5%
image size growth, mainly due to needing to always clear x16 and x17:
https://lore.kernel.org/lkml/20210510134503.GA88495@C02TD0UTHF1T.local/
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2: adjust image size language to reflect arm64 differences (mark)
v1: https://lore.kernel.org/lkml/20210505191804.4015873-1-keescook@chromium.org/
---
Makefile | 5 +++++
security/Kconfig.hardening | 19 +++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/Makefile b/Makefile
index c3f9bd191b89..bed9c68d93d3 100644
--- a/Makefile
+++ b/Makefile
@@ -841,6 +841,11 @@ endif
# for the randomize_kstack_offset feature. Disable it for all compilers.
KBUILD_CFLAGS += $(call cc-option, -fno-stack-clash-protection)
+# Clear used registers at func exit (to reduce data lifetime and ROP gadgets).
+ifdef CONFIG_ZERO_CALL_USED_REGS
+KBUILD_CFLAGS += -fzero-call-used-regs=used-gpr
+endif
+
DEBUG_CFLAGS :=
# Workaround for GCC versions < 5.0
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index a56c36470cb1..023aea5e117c 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -217,6 +217,25 @@ config INIT_ON_FREE_DEFAULT_ON
touching "cold" memory areas. Most cases see 3-5% impact. Some
synthetic workloads have measured as high as 8%.
+config CC_HAS_ZERO_CALL_USED_REGS
+ def_bool $(cc-option,-fzero-call-used-regs=used-gpr)
+
+config ZERO_CALL_USED_REGS
+ bool "Enable register zeroing on function exit"
+ depends on CC_HAS_ZERO_CALL_USED_REGS
+ help
+ At the end of functions, always zero any caller-used register
+ contents. This helps ensure that temporary values are not
+ leaked beyond the function boundary. This means that register
+ contents are less likely to be available for side channels
+ and information exposures. Additionally, this helps reduce the
+ number of useful ROP gadgets by about 20% (and removes compiler
+ generated "write-what-where" gadgets) in the resulting kernel
+ image. This has a less than 1% performance impact on most
+ workloads. Image size growth depends on architecture, and should
+ be evaluated for suitability. For example, x86_64 grows by less
+ than 1%, and arm64 grows by about 5%.
+
endmenu
endmenu
--
2.30.2
^ permalink raw reply related
* [PATCH v2] LSM: add NULL check for kcalloc()
From: Austin Kim @ 2021-07-14 21:51 UTC (permalink / raw)
To: jmorris, serge
Cc: linux-security-module, linux-kernel, austin.kim, kernel-team,
austindh.kim
From: Austin Kim <austin.kim@lge.com>
kcalloc() may return NULL when memory allocation fails.
So it is necessary to add NULL check after the call to kcalloc() is made.
Signed-off-by: Austin Kim <austin.kim@lge.com>
---
security/security.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/security.c b/security/security.c
index 09533cbb7221..f885c9e9bc35 100644
--- a/security/security.c
+++ b/security/security.c
@@ -321,6 +321,8 @@ static void __init ordered_lsm_init(void)
ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
GFP_KERNEL);
+ if (!ordered_lsms)
+ return;
if (chosen_lsm_order) {
if (chosen_major_lsm) {
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] LSM: add NULL check for kcalloc()
From: Austin Kim @ 2021-07-14 21:44 UTC (permalink / raw)
To: James Morris
Cc: Serge E. Hallyn, Kees Cook, linux-security-module,
Linux Kernel Mailing List, 김동현, kernel-team
In-Reply-To: <e0a2655b-3bc6-fd45-bd84-8df2dc6bc5f@namei.org>
2021년 7월 15일 (목) 오전 4:12, James Morris <jmorris@namei.org>님이 작성:
>
> On Tue, 13 Jul 2021, Austin Kim wrote:
>
> > From: Austin Kim <austin.kim@lge.com>
> >
> > kcalloc() may return NULL when memory allocation fails.
> > So it is necessary to add NULL check after the call to kcalloc() is made.
> >
> > Signed-off-by: Austin Kim <austin.kim@lge.com>
> > ---
> > security/security.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/security/security.c b/security/security.c
> > index 09533cbb7221..f885c9e9bc35 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -321,6 +321,8 @@ static void __init ordered_lsm_init(void)
> >
> > ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
> > GFP_KERNEL);
> > + if (ordered_lsms)
> > + return;
>
> Your logic is reversed here.
I feel very sorry for my terrible mistake.
'if (ordered_lsms)' should have been 'if (!ordered_lsms)'.
Let me resend patch(v2) soon.
Thanks,
Austin Kim
>
> Should this also be a kernel panic?
>
> >
> > if (chosen_lsm_order) {
> > if (chosen_major_lsm) {
> > --
> > 2.20.1
> >
>
> --
> James Morris
> <jmorris@namei.org>
>
^ permalink raw reply
* Re: [PATCH] LSM: add NULL check for kcalloc()
From: James Morris @ 2021-07-14 19:05 UTC (permalink / raw)
To: Austin Kim
Cc: serge, keescook, linux-security-module, linux-kernel, austin.kim,
kernel-team
In-Reply-To: <20210712234434.GA29205@raspberrypi>
On Tue, 13 Jul 2021, Austin Kim wrote:
> From: Austin Kim <austin.kim@lge.com>
>
> kcalloc() may return NULL when memory allocation fails.
> So it is necessary to add NULL check after the call to kcalloc() is made.
>
> Signed-off-by: Austin Kim <austin.kim@lge.com>
> ---
> security/security.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index 09533cbb7221..f885c9e9bc35 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -321,6 +321,8 @@ static void __init ordered_lsm_init(void)
>
> ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
> GFP_KERNEL);
> + if (ordered_lsms)
> + return;
Your logic is reversed here.
Should this also be a kernel panic?
>
> if (chosen_lsm_order) {
> if (chosen_major_lsm) {
> --
> 2.20.1
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 1/3] crypto: mxs-dcp: Add support for hardware provided keys
From: Ahmad Fatoum @ 2021-07-14 11:01 UTC (permalink / raw)
To: Richard Weinberger
Cc: open list, ASYMMETRIC KEYS, david, David Howells, davem, festevam,
Herbert Xu, James Bottomley, James Morris, Jarkko Sakkinen,
Jonathan Corbet, linux-arm-kernel, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM,
Mimi Zohar, linux-imx, kernel, Sascha Hauer, Serge E. Hallyn,
shawnguo
In-Reply-To: <1409091619.25467.1626259183269.JavaMail.zimbra@nod.at>
Hi,
On 14.07.21 12:39, Richard Weinberger wrote:
> Ahmad,
>
> ----- Ursprüngliche Mail -----
>> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
>> Let's trade reviews to get the ball rolling?
>
> Sounds like a fair deal. :-)
:)
> [...]
>
>>> --- a/drivers/crypto/mxs-dcp.c
>>> +++ b/drivers/crypto/mxs-dcp.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/stmp_device.h>
>>> #include <linux/clk.h>
>>> +#include <linux/mxs-dcp.h>
>>
>> The CAAM specific headers are in <soc/fsl/*.h>.
>> Should this be done likewise here as well?
>
> I have no preferences. If soc/fsl/ is the way to go, fine by me.
I think it's the more appropriate place, but if the maintainers
are fine with <linux/mxs-dcp.h>, I don't mind.
>
> [...]
>
>>> @@ -219,15 +224,18 @@ 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);
>>> + dma_addr_t src_phys, dst_phys, key_phys = {0};
>>
>> Why = {0}; ? dma_addr_t is a scalar type and the value is always
>> written here before access.
>
> Initializing a scalar with {} is allowed in C, the braces are optional.
> I like the braces because it works even when the underlaying type changes.
> But that's just a matter of taste.
>
> key_phys is initialized because it triggered a false positive gcc warning
> on one of my targets. Let me re-run again to be sure, the code saw a lot of
> refactoring since that.
>
> [...]
>
>>> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
>>> + unsigned int len)
>>> +{
>>> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
>>> + int ret = -EINVAL;
>>> +
>>> + if (len != DCP_PAES_KEYSIZE)
>>> + goto out;
>>
>> Nitpick: there is no cleanup, so why not return -EINVAL here
>> and unconditionally return 0 below?
>
> What is the benefit?
Similar to why you wouldn't write:
if (len == DCP_PAES_KEYSIZE) {
/* longer code block */
}
return ret;
Code is easier to scan through with early-exits.
> Usually I try to use goto to have a single exit point of a function
> but I don't have a strong preference...
It's just a nitpick. I am fine with it either way.
>>> +
>>> + actx->key_len = len;
>>> + actx->refkey = true;
>>> +
>>> + switch (key[0]) {
>>> + case DCP_PAES_KEY_SLOT0:
>>> + case DCP_PAES_KEY_SLOT1:
>>> + case DCP_PAES_KEY_SLOT2:
>>> + case DCP_PAES_KEY_SLOT3:
>>> + case DCP_PAES_KEY_UNIQUE:
>>> + case DCP_PAES_KEY_OTP:
>>> + memcpy(actx->key, key, len);
>>> + ret = 0;
>>> + }
>>
>> In the error case you return -EINVAL below, but you still write
>> into actx. Is that intentional?
>
> You mean acts->key_len and actk->refkey?
> Is this a problem?
It's easier to reason about code when it doesn't leave objects
it operates on in invalid states on failure. Changing key_len,
but leaving actx->key uninitialized is surprising IMO.
I can't judge whether this is a problem in practice, but less
surprises are a worthwhile goal.
Cheers,
Ahmad
>
> Thanks,
> //richard
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH 2/3] KEYS: trusted: Introduce support for NXP DCP-based trusted keys
From: Richard Weinberger @ 2021-07-14 10:44 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: open list, ASYMMETRIC KEYS, david, David Howells, davem, festevam,
Herbert Xu, James Bottomley, James Morris, Jarkko Sakkinen,
Jonathan Corbet, linux-arm-kernel, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM,
Mimi Zohar, linux-imx, kernel, Sascha Hauer, Serge E. Hallyn,
shawnguo
In-Reply-To: <714571a1-e8dd-3417-b5ab-2a6d611fb3ee@pengutronix.de>
Ahmad,
----- Ursprüngliche Mail -----
> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
[...]
>> /*
>> * 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[0];
>
> There's been ongoing effort to replace the [0] GNU extension with C99
> flexible array members. Please use [] here as well.
Makes sense!
[...]
>> +KEYS-TRUSTED-DCP
>> +M: David Gstir <david@sigma-star.at>
>> +M: Richard Weinberger <richard@nod.at>
>> +L: linux-integrity@vger.kernel.org
>> +L: keyrings@vger.kernel.org
>> +S: Supported
>> +F: include/keys/trusted_dcp.h
>> +F: security/keys/trusted-keys/trusted_dcp.c
>
> Hmm, I didn't add a MAINTAINERS entry for CAAM trusted keys. Do you think I
> should?
Sure, why not? It shows that you will also in future take care of it.
[...]
>> +} __packed;
>> +
>> +static bool use_otp_key;
>> +module_param_named(dcp_use_otp_key, use_otp_key, bool, 0);
>> +MODULE_PARM_DESC(dcp_use_otp_key, "Use OTP instead of UNIQUE key for sealing");
>
> Shouldn't these be documented in admin-guide/kernel-parameters.txt as well?
Yes. Will do.
>> +static bool skip_zk_test;
>> +module_param_named(dcp_skip_zk_test, skip_zk_test, bool, 0);
>> +MODULE_PARM_DESC(dcp_skip_zk_test, "Don't test whether device keys are
>> zero'ed");
>
> Does this need to be configurible? I'd assume this can only happen when using an
> unfused OTP. In such a case, it's ok to always warn, so you don't need to make
> this configurible.
We found such a setting super useful while working with targets where the keys are
zero'ed for various reasons.
There are cases where you want to use/test trusted keys even when the master key
is void. Our detection logic does not only print a warning, it refuses to load
blobs. So IMHO the config knob makes sense.
>> +
>> +static unsigned int calc_blob_len(unsigned int payload_len)
>> +{
>> + return sizeof(struct dcp_blob_fmt) + payload_len + DCP_BLOB_AUTHLEN;
>> +}
>> +
>> +static int do_dcp_crypto(u8 *in, u8 *out, bool is_encrypt)
>
> I assume in can't be const because the use with sg APIs?
I'm pretty sure this was the main reason, but I can check again.
>> +{
>> + int res = 0;
>> + struct skcipher_request *req = NULL;
>> + DECLARE_CRYPTO_WAIT(wait);
>> + struct scatterlist src_sg, dst_sg;
>> + struct crypto_skcipher *tfm;
>> + u8 paes_key[DCP_PAES_KEYSIZE];
>> +
>> + if (!use_otp_key)
>
> I'd invert this. Makes code easier to read.
Ok. :-)
>> + paes_key[0] = DCP_PAES_KEY_UNIQUE;
>> + else
>> + paes_key[0] = DCP_PAES_KEY_OTP;
>> +
>> + tfm = crypto_alloc_skcipher("ecb-paes-dcp", CRYPTO_ALG_INTERNAL,
>> + CRYPTO_ALG_INTERNAL);
>> + if (IS_ERR(tfm)) {
>> + res = PTR_ERR(tfm);
>> + pr_err("Unable to request DCP pAES-ECB cipher: %i\n", res);
>
> Can you define pr_fmt above? There's also %pe now that can directly print out an
> error pointer.
pr_fmt is not defined on purpose. include/keys/trusted-type.h defines already one
and I assumed "trusted_key:" is the desired prefix for all kinds of trusted keys.
[...]
> - payload_len is at offset 33, but MIN_KEY_SIZE == 32 and there are no minimum
> size checks. Couldn't you read beyond the buffer this way?
The key has a minimum size of MIN_KEY_SIZE, but p->blob (being struct trusted_key_payload->blob[MAX_BLOB_SIZE])
is much larger.
So the assumption is that a DCP blob will always be smaller than MAX_BLOB_SIZE.
> - offset 33 is unaligned for payload_len. Please use get_unaligned_le32 here.
Oh yes. Makes sense!
[...]
>
> jfyi, in the prelude of my CAAM series, I made this the default
> when .get_random == NULL.
Right. :-)
[...]
>> + 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");
>
> What's the difference between secure and trusted? Can't this test be skipped
> if use_otp_key == false?
DCP has many modes of operation. Secure is one level above trusted.
For the gory details see "Security Reference Manual for the i.MX 6ULL Applications Processor".
I'm not sure whether all information my manual describes is publicly available so I
don't dare to copy&paste from it.
As David and I understood the logic, both OTP and UNIQUE keys can be zero'ed.
It is also possible that DCP has no support at all for these keys,
then you'll also get a zero key. That's why we have this check here.
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH 1/3] crypto: mxs-dcp: Add support for hardware provided keys
From: Richard Weinberger @ 2021-07-14 10:39 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: open list, ASYMMETRIC KEYS, david, David Howells, davem, festevam,
Herbert Xu, James Bottomley, James Morris, Jarkko Sakkinen,
Jonathan Corbet, linux-arm-kernel, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM,
Mimi Zohar, linux-imx, kernel, Sascha Hauer, Serge E. Hallyn,
shawnguo
In-Reply-To: <76db3736-5a5f-bf7b-3b52-62d01a84ee2d@pengutronix.de>
Ahmad,
----- Ursprüngliche Mail -----
> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
> Let's trade reviews to get the ball rolling?
Sounds like a fair deal. :-)
[...]
>> --- a/drivers/crypto/mxs-dcp.c
>> +++ b/drivers/crypto/mxs-dcp.c
>> @@ -15,6 +15,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/stmp_device.h>
>> #include <linux/clk.h>
>> +#include <linux/mxs-dcp.h>
>
> The CAAM specific headers are in <soc/fsl/*.h>.
> Should this be done likewise here as well?
I have no preferences. If soc/fsl/ is the way to go, fine by me.
[...]
>> @@ -219,15 +224,18 @@ 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);
>> + dma_addr_t src_phys, dst_phys, key_phys = {0};
>
> Why = {0}; ? dma_addr_t is a scalar type and the value is always
> written here before access.
Initializing a scalar with {} is allowed in C, the braces are optional.
I like the braces because it works even when the underlaying type changes.
But that's just a matter of taste.
key_phys is initialized because it triggered a false positive gcc warning
on one of my targets. Let me re-run again to be sure, the code saw a lot of
refactoring since that.
[...]
>> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
>> + unsigned int len)
>> +{
>> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
>> + int ret = -EINVAL;
>> +
>> + if (len != DCP_PAES_KEYSIZE)
>> + goto out;
>
> Nitpick: there is no cleanup, so why not return -EINVAL here
> and unconditionally return 0 below?
What is the benefit?
Usually I try to use goto to have a single exit point of a function
but I don't have a strong preference...
>> +
>> + actx->key_len = len;
>> + actx->refkey = true;
>> +
>> + switch (key[0]) {
>> + case DCP_PAES_KEY_SLOT0:
>> + case DCP_PAES_KEY_SLOT1:
>> + case DCP_PAES_KEY_SLOT2:
>> + case DCP_PAES_KEY_SLOT3:
>> + case DCP_PAES_KEY_UNIQUE:
>> + case DCP_PAES_KEY_OTP:
>> + memcpy(actx->key, key, len);
>> + ret = 0;
>> + }
>
> In the error case you return -EINVAL below, but you still write
> into actx. Is that intentional?
You mean acts->key_len and actk->refkey?
Is this a problem?
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH 3/3] doc: trusted-encrypted: add DCP as new trust source
From: Ahmad Fatoum @ 2021-07-14 9:32 UTC (permalink / raw)
To: Richard Weinberger, keyrings
Cc: David Gstir, David Howells, David S. Miller, Fabio Estevam,
Herbert Xu, James Bottomley, James Morris, Jarkko Sakkinen,
Jonathan Corbet, linux-arm-kernel, linux-crypto, linux-doc,
linux-integrity, linux-kernel, linux-security-module, Mimi Zohar,
NXP Linux Team, Pengutronix Kernel Team, Sascha Hauer,
Serge E. Hallyn, Shawn Guo
In-Reply-To: <20210614201620.30451-4-richard@nod.at>
Hello Richard,
Hello David,
On 14.06.21 22:16, Richard Weinberger wrote:
> From: David Gstir <david@sigma-star.at>
>
> 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
>
> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: David Gstir <david@sigma-star.at>
> Cc: David Howells <dhowells@redhat.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: keyrings@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Co-developed-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: David Gstir <david@sigma-star.at>
> ---
> .../security/keys/trusted-encrypted.rst | 84 ++++++++++++++++++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
> index 80d5a5af62a1..e8413122e4bc 100644
> --- a/Documentation/security/keys/trusted-encrypted.rst
> +++ b/Documentation/security/keys/trusted-encrypted.rst
> @@ -35,6 +35,11 @@ safe.
> Rooted to Hardware Unique Key (HUK) which is generally burnt in on-chip
> fuses and is accessible to TEE only.
>
> + (3) 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 accessbile to the DCP encryption engine only.
s/accessbile/accessible/ . In the code you differentiate between UNIQUE and OTP.
Here you use OTP to mean both. Perhaps explicitly mention this?
> +
> * Execution isolation
>
> (1) TPM
> @@ -46,6 +51,12 @@ safe.
> Customizable set of operations running in isolated execution
> environment verified via Secure/Trusted boot process.
>
> + (3) 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
> @@ -63,6 +74,11 @@ safe.
> Relies on Secure/Trusted boot process for platform integrity. It can
> be extended with TEE based measured boot process.
>
> + (3) DCP
> +
> + Relies on Secure/Trusted boot process (called HAB by vendor) for
> + platform integrity.
> +
> * Interfaces and APIs
>
> (1) TPM
> @@ -74,10 +90,14 @@ safe.
> TEEs have well-documented, standardized client interface and APIs. For
> more details refer to ``Documentation/staging/tee.rst``.
>
> + (3) 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 TPM or TEE for a given
> + The strength and appropriateness of a particular TPM, TEE or DCP for a given
> purpose must be assessed when using them to protect security-relevant data.
>
>
> @@ -103,6 +123,14 @@ access control policy within the trust source.
> from platform specific hardware RNG or a software based Fortuna CSPRNG
> which can be seeded via multiple entropy sources.
>
> + * 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.
> +
> +
> Encrypted Keys
> --------------
>
> @@ -188,6 +216,19 @@ Usage::
> specific to TEE device implementation. 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
> --------------------
>
> @@ -370,3 +411,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 tusted key using DCP is generated, we generate a random 128-bit
s/tusted/trusted/
> +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.
> + *
> + * @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[0];
[] ?
> + } __packed;
>
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH 2/3] KEYS: trusted: Introduce support for NXP DCP-based trusted keys
From: Ahmad Fatoum @ 2021-07-14 9:29 UTC (permalink / raw)
To: Richard Weinberger, keyrings
Cc: David Gstir, David Howells, David S. Miller, Fabio Estevam,
Herbert Xu, James Bottomley, James Morris, Jarkko Sakkinen,
Jonathan Corbet, linux-arm-kernel, linux-crypto, linux-doc,
linux-integrity, linux-kernel, linux-security-module, Mimi Zohar,
NXP Linux Team, Pengutronix Kernel Team, Sascha Hauer,
Serge E. Hallyn, Shawn Guo
In-Reply-To: <20210614201620.30451-3-richard@nod.at>
Hello Richard,
On 14.06.21 22:16, Richard Weinberger 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[0];
There's been ongoing effort to replace the [0] GNU extension with C99
flexible array members. Please use [] here as well.
> } __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.
>
> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: David Gstir <david@sigma-star.at>
> Cc: David Howells <dhowells@redhat.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: keyrings@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Co-developed-by: David Gstir <david@sigma-star.at>
> Signed-off-by: David Gstir <david@sigma-star.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> .../admin-guide/kernel-parameters.txt | 1 +
> MAINTAINERS | 9 +
> include/keys/trusted_dcp.h | 13 +
> security/keys/trusted-keys/Makefile | 1 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 325 ++++++++++++++++++
> 6 files changed, 354 insertions(+), 1 deletion(-)
> 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 cb89dbdedc46..d8b2aa94a566 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5639,6 +5639,7 @@
> sources:
> - "tpm"
> - "tee"
> + - "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
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b706dd20ff2b..779cac00827c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10099,6 +10099,15 @@ F: include/keys/trusted-type.h
> F: include/keys/trusted_tpm.h
> F: security/keys/trusted-keys/
>
> +KEYS-TRUSTED-DCP
> +M: David Gstir <david@sigma-star.at>
> +M: Richard Weinberger <richard@nod.at>
> +L: linux-integrity@vger.kernel.org
> +L: keyrings@vger.kernel.org
> +S: Supported
> +F: include/keys/trusted_dcp.h
> +F: security/keys/trusted-keys/trusted_dcp.c
Hmm, I didn't add a MAINTAINERS entry for CAAM trusted keys. Do you think I should?
> +
> KEYS-TRUSTED-TEE
> M: Sumit Garg <sumit.garg@linaro.org>
> L: linux-integrity@vger.kernel.org
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index 000000000000..7b2a1275c527
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + * Authors: David Gstir <david@sigma-star.at>
> + * Richard Weinberger <richard@sigma-star.at>
> + */
> +
> +#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/Makefile b/security/keys/trusted-keys/Makefile
> index feb8b6c3cc79..992b591692dc 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -12,3 +12,4 @@ trusted-y += trusted_tpm2.o
> trusted-y += tpm2key.asn1.o
>
> trusted-$(CONFIG_TEE) += trusted_tee.o
> +trusted-$(CONFIG_CRYPTO_DEV_MXS_DCP) += trusted_dcp.o
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..66b631ffe876 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -8,6 +8,7 @@
>
> #include <keys/user-type.h>
> #include <keys/trusted-type.h>
> +#include <keys/trusted_dcp.h>
> #include <keys/trusted_tee.h>
> #include <keys/trusted_tpm.h>
> #include <linux/capability.h>
> @@ -24,7 +25,7 @@
>
> static char *trusted_key_source;
> module_param_named(source, trusted_key_source, charp, 0);
> -MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
> +MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee or dcp)");
>
> static const struct trusted_key_source trusted_key_sources[] = {
> #if defined(CONFIG_TCG_TPM)
> @@ -33,6 +34,9 @@ static const struct trusted_key_source trusted_key_sources[] = {
> #if defined(CONFIG_TEE)
> { "tee", &trusted_key_tee_ops },
> #endif
> +#if defined(CONFIG_CRYPTO_DEV_MXS_DCP)
> + { "dcp", &dcp_trusted_key_ops },
> +#endif
> };
>
> DEFINE_STATIC_CALL_NULL(trusted_key_init, *trusted_key_sources[0].ops->init);
> diff --git a/security/keys/trusted-keys/trusted_dcp.c b/security/keys/trusted-keys/trusted_dcp.c
> new file mode 100644
> index 000000000000..02db5dc261c7
> --- /dev/null
> +++ b/security/keys/trusted-keys/trusted_dcp.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + * Authors: David Gstir <david@sigma-star.at>
> + * Richard Weinberger <richard@sigma-star.at>
> + */
> +
> +#include <crypto/aead.h>
> +#include <crypto/aes.h>
> +#include <crypto/algapi.h>
> +#include <crypto/gcm.h>
> +#include <crypto/skcipher.h>
> +#include <keys/trusted-type.h>
> +#include <linux/key-type.h>
> +#include <linux/module.h>
> +#include <linux/mxs-dcp.h>
> +#include <linux/printk.h>
> +#include <linux/random.h>
> +#include <linux/scatterlist.h>
> +
> +#define DCP_BLOB_VERSION 1
> +#define DCP_BLOB_AUTHLEN 16
> +
> +/**
> + * 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 DCP_BLOB_AUTHLEN is attached at the end of it.
> + *
> + * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len +
> + * DCP_BLOB_AUTHLEN.
> + */
> +struct dcp_blob_fmt {
> + __u8 fmt_version;
> + __u8 blob_key[AES_KEYSIZE_128];
> + __u8 nonce[AES_KEYSIZE_128];
> + __le32 payload_len;
> + __u8 payload[0];
[], see above.
> +} __packed;
> +
> +static bool use_otp_key;
> +module_param_named(dcp_use_otp_key, use_otp_key, bool, 0);
> +MODULE_PARM_DESC(dcp_use_otp_key, "Use OTP instead of UNIQUE key for sealing");
Shouldn't these be documented in admin-guide/kernel-parameters.txt as well?
> +static bool skip_zk_test;
> +module_param_named(dcp_skip_zk_test, skip_zk_test, bool, 0);
> +MODULE_PARM_DESC(dcp_skip_zk_test, "Don't test whether device keys are zero'ed");
Does this need to be configurible? I'd assume this can only happen when using an
unfused OTP. In such a case, it's ok to always warn, so you don't need to make
this configurible.
> +
> +static unsigned int calc_blob_len(unsigned int payload_len)
> +{
> + return sizeof(struct dcp_blob_fmt) + payload_len + DCP_BLOB_AUTHLEN;
> +}
> +
> +static int do_dcp_crypto(u8 *in, u8 *out, bool is_encrypt)
I assume in can't be const because the use with sg APIs?
> +{
> + int res = 0;
> + struct skcipher_request *req = NULL;
> + DECLARE_CRYPTO_WAIT(wait);
> + struct scatterlist src_sg, dst_sg;
> + struct crypto_skcipher *tfm;
> + u8 paes_key[DCP_PAES_KEYSIZE];
> +
> + if (!use_otp_key)
I'd invert this. Makes code easier to read.
> + paes_key[0] = DCP_PAES_KEY_UNIQUE;
> + else
> + paes_key[0] = DCP_PAES_KEY_OTP;
> +
> + tfm = crypto_alloc_skcipher("ecb-paes-dcp", CRYPTO_ALG_INTERNAL,
> + CRYPTO_ALG_INTERNAL);
> + if (IS_ERR(tfm)) {
> + res = PTR_ERR(tfm);
> + pr_err("Unable to request DCP pAES-ECB cipher: %i\n", res);
Can you define pr_fmt above? There's also %pe now that can directly print out an
error pointer.
> + tfm = NULL;
> + goto out;
> + }
> +
> + req = skcipher_request_alloc(tfm, GFP_NOFS);
> + if (!req) {
> + res = -ENOMEM;
> + goto out;
> + }
> +
> + skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> + CRYPTO_TFM_REQ_MAY_SLEEP,
> + crypto_req_done, &wait);
> + res = crypto_skcipher_setkey(tfm, paes_key, sizeof(paes_key));
> + if (res < 0)
> + goto out;
> +
> + sg_init_one(&src_sg, in, AES_KEYSIZE_128);
> + sg_init_one(&dst_sg, out, AES_KEYSIZE_128);
> + skcipher_request_set_crypt(req, &src_sg, &dst_sg, AES_KEYSIZE_128,
> + NULL);
> +
> + if (is_encrypt)
> + res = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
> + else
> + res = crypto_wait_req(crypto_skcipher_decrypt(req), &wait);
> +
> +out:
> + skcipher_request_free(req);
> + crypto_free_skcipher(tfm);
> +
> + return res;
> +}
> +
> +static int do_aead_crypto(u8 *in, u8 *out, size_t len, u8 *key, u8 *nonce,
> + bool is_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);
> + pr_err("Unable to request AES-GCM cipher: %i\n", ret);
> + 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 (is_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 (is_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) {
> + ret = -E2BIG;
> + goto out;
Nitpick, just return and drop the goto?
> + }
> +
> + 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);
> + goto out;
> + }
> +
> + ret = encrypt_blob_key(b->blob_key);
> + if (ret) {
> + pr_err("Unable to encrypt blob key: %i\n", ret);
> + goto out;
> + }
> +
> + b->payload_len = cpu_to_le32(p->key_len);
- payload_len is at offset 33, but MIN_KEY_SIZE == 32 and there are no minimum
size checks. Couldn't you read beyond the buffer this way?
- offset 33 is unaligned for payload_len. Please use get_unaligned_le32 here.
> + p->blob_len = blen;
> + ret = 0;
> +out:
> + return ret;
> +}
> +
> +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 trusted_dcp_get_random(unsigned char *key, size_t key_len)
> +{
> + get_random_bytes(key, key_len);
> +
> + return key_len;
> +}
jfyi, in the prelude of my CAAM series, I made this the default
when .get_random == NULL.
> +
> +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");
What's the difference between secure and trusted? Can't this test be skipped
if use_otp_key == false?
> + 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,
> + .get_random = trusted_dcp_get_random,
> + .migratable = 0,
> +};
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH 1/3] crypto: mxs-dcp: Add support for hardware provided keys
From: Ahmad Fatoum @ 2021-07-14 9:24 UTC (permalink / raw)
To: Richard Weinberger, keyrings
Cc: David Gstir, David Howells, David S. Miller, Fabio Estevam,
Herbert Xu, James Bottomley, James Morris, Jarkko Sakkinen,
Jonathan Corbet, linux-arm-kernel, linux-crypto, linux-doc,
linux-integrity, linux-kernel, linux-security-module, Mimi Zohar,
NXP Linux Team, Pengutronix Kernel Team, Sascha Hauer,
Serge E. Hallyn, Shawn Guo
In-Reply-To: <20210614201620.30451-2-richard@nod.at>
Hi Richard,
Let's trade reviews to get the ball rolling?
On 14.06.21 22:16, Richard Weinberger 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.
>
> Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: David Gstir <david@sigma-star.at>
> Cc: David Howells <dhowells@redhat.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: keyrings@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Co-developed-by: David Gstir <david@sigma-star.at>
> Signed-off-by: David Gstir <david@sigma-star.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> drivers/crypto/mxs-dcp.c | 110 ++++++++++++++++++++++++++++++++++-----
> include/linux/mxs-dcp.h | 19 +++++++
> 2 files changed, 117 insertions(+), 12 deletions(-)
> create mode 100644 include/linux/mxs-dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index d6a7784d2988..c3e0c0ccbc20 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
> #include <linux/platform_device.h>
> #include <linux/stmp_device.h>
> #include <linux/clk.h>
> +#include <linux/mxs-dcp.h>
The CAAM specific headers are in <soc/fsl/*.h>.
Should this be done likewise here as well?
>
> #include <crypto/aes.h>
> #include <crypto/sha1.h>
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned int key_len;
> uint8_t key[AES_KEYSIZE_128];
> + bool refkey;
> };
>
> 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_SHIFT 8
> +
> static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
> {
> struct dcp *sdcp = global_sdcp;
> @@ -219,15 +224,18 @@ 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);
> + dma_addr_t src_phys, dst_phys, key_phys = {0};
Why = {0}; ? dma_addr_t is a scalar type and the value is always
written here before access.
> + bool key_referenced = actx->refkey;
> int ret;
>
> - dma_addr_t key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> - 2 * AES_KEYSIZE_128,
> - DMA_TO_DEVICE);
> - dma_addr_t src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> - DCP_BUF_SZ, DMA_TO_DEVICE);
> - dma_addr_t dst_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_out_buf,
> - DCP_BUF_SZ, DMA_FROM_DEVICE);
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + }
> + src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf, DCP_BUF_SZ,
> + DMA_TO_DEVICE);
> + dst_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_out_buf, DCP_BUF_SZ,
> + DMA_FROM_DEVICE);
>
> if (actx->fill % AES_BLOCK_SIZE) {
> dev_err(sdcp->dev, "Invalid block size!\n");
> @@ -240,8 +248,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;
> + }
>
> if (rctx->enc)
> desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -255,6 +268,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;
> @@ -265,8 +281,10 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> ret = mxs_dcp_start_dma(actx);
>
> aes_done_run:
> - dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> - DMA_TO_DEVICE);
> + if (!key_referenced) {
> + dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> + DMA_TO_DEVICE);
> + }
> dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE);
> dma_unmap_single(sdcp->dev, dst_phys, DCP_BUF_SZ, DMA_FROM_DEVICE);
>
> @@ -454,7 +472,7 @@ static int mxs_dcp_aes_enqueue(struct skcipher_request *req, int enc, int ecb)
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> int ret;
>
> - if (unlikely(actx->key_len != AES_KEYSIZE_128))
> + if (unlikely(actx->key_len != AES_KEYSIZE_128 && !actx->refkey))
> return mxs_dcp_block_fallback(req, enc);
>
> rctx->enc = enc;
> @@ -501,6 +519,7 @@ static int mxs_dcp_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
> * there can still be an operation in progress.
> */
> actx->key_len = len;
> + actx->refkey = false;
> if (len == AES_KEYSIZE_128) {
> memcpy(actx->key, key, len);
> return 0;
> @@ -517,6 +536,33 @@ static int mxs_dcp_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
> return crypto_skcipher_setkey(actx->fallback, key, len);
> }
>
> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
> + unsigned int len)
> +{
> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
> + int ret = -EINVAL;
> +
> + if (len != DCP_PAES_KEYSIZE)
> + goto out;
Nitpick: there is no cleanup, so why not return -EINVAL here
and unconditionally return 0 below?
> +
> + actx->key_len = len;
> + actx->refkey = true;
> +
> + switch (key[0]) {
> + case DCP_PAES_KEY_SLOT0:
> + case DCP_PAES_KEY_SLOT1:
> + case DCP_PAES_KEY_SLOT2:
> + case DCP_PAES_KEY_SLOT3:
> + case DCP_PAES_KEY_UNIQUE:
> + case DCP_PAES_KEY_OTP:
> + memcpy(actx->key, key, len);
> + ret = 0;
> + }
In the error case you return -EINVAL below, but you still write
into actx. Is that intentional?
> +
> +out:
> + return ret;
> +}
> +
> static int mxs_dcp_aes_fallback_init_tfm(struct crypto_skcipher *tfm)
> {
> const char *name = crypto_tfm_alg_name(crypto_skcipher_tfm(tfm));
> @@ -540,6 +586,13 @@ static void mxs_dcp_aes_fallback_exit_tfm(struct crypto_skcipher *tfm)
> crypto_free_skcipher(actx->fallback);
> }
>
> +static int mxs_dcp_paes_init_tfm(struct crypto_skcipher *tfm)
> +{
> + crypto_skcipher_set_reqsize(tfm, sizeof(struct dcp_aes_req_ctx));
> +
> + return 0;
> +}
> +
> /*
> * Hashing (SHA1/SHA256)
> */
> @@ -882,6 +935,39 @@ static struct skcipher_alg dcp_aes_algs[] = {
> .ivsize = AES_BLOCK_SIZE,
> .init = mxs_dcp_aes_fallback_init_tfm,
> .exit = mxs_dcp_aes_fallback_exit_tfm,
> + }, {
> + .base.cra_name = "ecb(paes)",
> + .base.cra_driver_name = "ecb-paes-dcp",
> + .base.cra_priority = 401,
> + .base.cra_alignmask = 15,
> + .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_INTERNAL,
> + .base.cra_blocksize = AES_BLOCK_SIZE,
> + .base.cra_ctxsize = sizeof(struct dcp_async_ctx),
> + .base.cra_module = THIS_MODULE,
> +
> + .min_keysize = DCP_PAES_KEYSIZE,
> + .max_keysize = DCP_PAES_KEYSIZE,
> + .setkey = mxs_dcp_aes_setrefkey,
> + .encrypt = mxs_dcp_aes_ecb_encrypt,
> + .decrypt = mxs_dcp_aes_ecb_decrypt,
> + .init = mxs_dcp_paes_init_tfm,
> + }, {
> + .base.cra_name = "cbc(paes)",
> + .base.cra_driver_name = "cbc-paes-dcp",
> + .base.cra_priority = 401,
> + .base.cra_alignmask = 15,
> + .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_INTERNAL,
> + .base.cra_blocksize = AES_BLOCK_SIZE,
> + .base.cra_ctxsize = sizeof(struct dcp_async_ctx),
> + .base.cra_module = THIS_MODULE,
> +
> + .min_keysize = DCP_PAES_KEYSIZE,
> + .max_keysize = DCP_PAES_KEYSIZE,
> + .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/linux/mxs-dcp.h b/include/linux/mxs-dcp.h
> new file mode 100644
> index 000000000000..df6678ee10a1
> --- /dev/null
> +++ b/include/linux/mxs-dcp.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + * Authors: David Gstir <david@sigma-star.at>
> + * Richard Weinberger <richard@sigma-star.at>
> + */
> +
> +#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 */
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v2 0/6] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
From: Ahmad Fatoum @ 2021-07-14 6:36 UTC (permalink / raw)
To: Jarkko Sakkinen, Horia Geantă, Mimi Zohar, Aymen Sghaier,
Herbert Xu, David S. Miller, James Bottomley
Cc: Jan Luebbe, Udit Agarwal, Sumit Garg, David Gstir, Eric Biggers,
Franck LENORMAND, Richard Weinberger, James Morris, linux-kernel,
David Howells, linux-security-module, keyrings, linux-crypto,
kernel, linux-integrity, Steffen Trumtrar, Serge E. Hallyn
In-Reply-To: <cover.1dfbb73645d917b3c76d01290804a3410bd9932e.1624364386.git-series.a.fatoum@pengutronix.de>
Dear Trusted Keys and CAAM maintainers/reviewers,
On 22.06.21 14:37, Ahmad Fatoum wrote:
> The first three patches are new and are applicable regardless of this
> series, but the rest won't apply cleanly without them. I chose to
> include them this time, but I can split them up for v3 if that's
> preferred.
>
> v1 -> v2:
> - Added new commit to make trusted key Kconfig option independent
> of TPM and added new Kconfig file and symbols for trusted keys
> - Add new commit for importing existing key material (Jan)
> - Allow users to force use of kernel RNG (Jarkko)
> - Enforce maximum keymod size (Horia)
> - Use append_seq_(in|out)_ptr_intlen instead of append_seq_(in|out)_ptr
> (Horia)
> - Make blobifier handle private to CAAM glue code file (Horia)
> - Extend trusted keys documentation for CAAM
> - Rebased on v5.12-rc7 and updated cover letter:
>
> The Cryptographic Acceleration and Assurance Module (CAAM) is an IP core
> built into many newer i.MX and QorIQ SoCs by NXP.
>
> Its blob mechanism can AES encrypt/decrypt user data using a unique
> never-disclosed device-specific key.
>
> There has been multiple discussions on how to represent this within the kernel:
>
> The Cryptographic Acceleration and Assurance Module (CAAM) is an IP core
> built into many newer i.MX and QorIQ SoCs by NXP.
>
> Its blob mechanism can AES encrypt/decrypt user data using a unique
> never-disclosed device-specific key. There has been multiple
> discussions on how to represent this within the kernel:
>
> - [RFC] crypto: caam - add red blobifier
> Steffen implemented[1] a PoC sysfs driver to start a discussion on how to
> best integrate the blob mechanism.
> Mimi suggested that it could be used to implement trusted keys.
> Trusted keys back then were a TPM-only feature.
>
> - security/keys/secure_key: Adds the secure key support based on CAAM.
> Udit added[2] a new "secure" key type with the CAAM as backend. The key
> material stays within the kernel only.
> Mimi and James agreed that this needs a generic interface, not specific
> to CAAM. Mimi suggested trusted keys. Jan noted that this could serve as
> basis for TEE-backed keys.
>
> - [RFC] drivers: crypto: caam: key: Add caam_tk key type
> Franck added[3] a new "caam_tk" key type based on Udit's work. This time
> it uses CAAM "black blobs" instead of "red blobs", so key material stays
> within the CAAM and isn't exposed to kernel in plaintext.
> James voiced the opinion that there should be just one user-facing generic
> wrap/unwrap key type with multiple possible handlers.
> David suggested trusted keys.
>
> - Introduce TEE based Trusted Keys support
> Sumit reworked[4] trusted keys to support multiple possible backends with
> one chosen at boot time and added a new TEE backend along with TPM.
> This now sits in Jarkko's master branch to be sent out for v5.13
>
> This patch series builds on top of Sumit's rework to have the CAAM as yet another
> trusted key backend.
>
> The CAAM bits are based on Steffen's initial patch from 2015. His work had been
> used in the field for some years now, so I preferred not to deviate too much from it.
>
> This series has been tested with dmcrypt[5] on an i.MX6DL.
>
> Looking forward to your feedback.
Gentle Ping.
> Cheers,
> Ahmad
>
> [1]: https://lore.kernel.org/linux-crypto/1447082306-19946-2-git-send-email-s.trumtrar@pengutronix.de/
> [2]: https://lore.kernel.org/linux-integrity/20180723111432.26830-1-udit.agarwal@nxp.com/
> [3]: https://lore.kernel.org/lkml/1551456599-10603-2-git-send-email-franck.lenormand@nxp.com/
> [4]: https://lore.kernel.org/lkml/1604419306-26105-1-git-send-email-sumit.garg@linaro.org/
> [5]: https://lore.kernel.org/linux-integrity/20210122084321.24012-2-a.fatoum@pengutronix.de/
>
> ---
> To: Jarkko Sakkinen <jarkko@kernel.org>
> To: "Horia Geantă" <horia.geanta@nxp.com>
> To: Mimi Zohar <zohar@linux.ibm.com>
> To: Aymen Sghaier <aymen.sghaier@nxp.com>
> To: Herbert Xu <herbert@gondor.apana.org.au>
> To: "David S. Miller" <davem@davemloft.net>
> To: James Bottomley <jejb@linux.ibm.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Cc: Udit Agarwal <udit.agarwal@nxp.com>
> Cc: Jan Luebbe <j.luebbe@pengutronix.de>
> Cc: David Gstir <david@sigma-star.at>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Franck LENORMAND <franck.lenormand@nxp.com>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: linux-integrity@vger.kernel.org
> Cc: keyrings@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
>
> Ahmad Fatoum (6):
> KEYS: trusted: allow use of TEE as backend without TCG_TPM support
> KEYS: trusted: Allow import from existing key material for development
> KEYS: trusted: allow users to use kernel RNG for key material
> KEYS: trusted: allow trust sources to use kernel RNG for key material
> crypto: caam - add in-kernel interface for blob generator
> KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
>
> Documentation/admin-guide/kernel-parameters.txt | 8 +-
> Documentation/security/keys/trusted-encrypted.rst | 74 ++++-
> drivers/crypto/caam/Kconfig | 3 +-
> drivers/crypto/caam/Makefile | 1 +-
> drivers/crypto/caam/blob_gen.c | 230 +++++++++++++++-
> include/keys/trusted-type.h | 2 +-
> include/keys/trusted_caam.h | 11 +-
> include/soc/fsl/caam-blob.h | 56 ++++-
> security/keys/Kconfig | 14 +-
> security/keys/trusted-keys/Kconfig | 49 +++-
> security/keys/trusted-keys/Makefile | 10 +-
> security/keys/trusted-keys/trusted_caam.c | 74 +++++-
> security/keys/trusted-keys/trusted_core.c | 48 ++-
> 13 files changed, 554 insertions(+), 26 deletions(-)
> create mode 100644 drivers/crypto/caam/blob_gen.c
> create mode 100644 include/keys/trusted_caam.h
> create mode 100644 include/soc/fsl/caam-blob.h
> create mode 100644 security/keys/trusted-keys/Kconfig
> create mode 100644 security/keys/trusted-keys/trusted_caam.c
>
> base-commit: 13311e74253fe64329390df80bed3f07314ddd61
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox