Re: [EXT] [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
On Tue Apr 30, 2024 at 3:03 PM EEST, David Gstir wrote: > Hi Jarkko, > > > On 30.04.2024, at 13:48, Kshitiz Varshney wrote: > > > > Hi David, > > > >> -Original Message- > >> From: David Gstir > >> Sent: Monday, April 29, 2024 5:05 PM > >> To: Kshitiz Varshney > > > >> > >> Did you get around to testing this? > >> I’d greatly appreciate a Tested-by for this. :-) > >> > >> Thanks! > >> BR, David > > > > Currently, I am bit busy with other priority activities. It will take time > > to test this patch set. > > How should we proceed here? > Do we have to miss another release cycle, because of a Tested-by? > > If any bugs pop up I’ll happily fix them, but at the moment it appears to be > more of a formality. > IMHO the patch set itself is rather small and has been thoroughly reviewed to > ensure that any huge > issues would already have been caught by now. I don't mind picking this actually since unless you consume it, it should not get in the way. I'll pick it during the weekend. Thanks for reminding. BR, Jarkko
Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2
On Thu Apr 25, 2024 at 6:09 PM EEST, Sean Christopherson wrote: > + __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2), > + "KVM selftests from v6.8+ require > KVM_SET_USER_MEMORY_REGION2"); This would work also for casual (but not seasoned) visitor in KVM code as additionl documentation. BR, Jarkko
Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
On Fri Apr 12, 2024 at 9:26 AM EEST, Herbert Xu wrote: > On Wed, Apr 03, 2024 at 06:47:51PM +0300, Jarkko Sakkinen wrote: > > > > Reviewed-by: Jarkko Sakkinen > > > > I can only test that this does not break a machine without the > > hardware feature. > > Please feel free to take this through your tree. > > Thanks, OK, thanks! BR, Jarkko
Re: [EXT] Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
On Tue Apr 9, 2024 at 12:48 PM EEST, Kshitiz Varshney wrote: > Hi Jarkko, > > > > -Original Message- > > From: Jarkko Sakkinen > > Sent: Wednesday, April 3, 2024 9:18 PM > > To: David Gstir ; Mimi Zohar ; > > James Bottomley ; Herbert Xu > > ; David S. Miller > > Cc: Shawn Guo ; Jonathan Corbet > > ; Sascha Hauer ; Pengutronix > > Kernel Team ; Fabio Estevam > > ; dl-linux-imx ; Ahmad Fatoum > > ; sigma star Kernel Team > > ; David Howells ; Li > > Yang ; Paul Moore ; James > > Morris ; Serge E. Hallyn ; Paul E. > > McKenney ; Randy Dunlap ; > > Catalin Marinas ; Rafael J. Wysocki > > ; Tejun Heo ; Steven Rostedt > > (Google) ; linux-...@vger.kernel.org; linux- > > ker...@vger.kernel.org; linux-integr...@vger.kernel.org; > > keyri...@vger.kernel.org; linux-cry...@vger.kernel.org; linux-arm- > > ker...@lists.infradead.org; linuxppc-dev@lists.ozlabs.org; linux-security- > > mod...@vger.kernel.org; Richard Weinberger ; David > > Oberhollenzer > > Subject: [EXT] Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new > > trust source > > > > Caution: This is an external email. Please take care when clicking links or > > opening attachments. When in doubt, report the message using the 'Report > > this email' button > > > > > > On Wed Apr 3, 2024 at 10:21 AM EEST, David Gstir wrote: > > > Update the documentation for trusted and encrypted KEYS with DCP as > > > new trust source: > > > > > > - Describe security properties of DCP trust source > > > - Describe key usage > > > - Document blob format > > > > > > Co-developed-by: Richard Weinberger > > > Signed-off-by: Richard Weinberger > > > Co-developed-by: David Oberhollenzer > > > > > > Signed-off-by: David Oberhollenzer > > > Signed-off-by: David Gstir > > > --- > > > .../security/keys/trusted-encrypted.rst | 53 +++ > > > security/keys/trusted-keys/trusted_dcp.c | 19 +++ > > > 2 files changed, 72 insertions(+) > > > > > > diff --git a/Documentation/security/keys/trusted-encrypted.rst > > > b/Documentation/security/keys/trusted-encrypted.rst > > > index e989b9802f92..f4d7e162d5e4 100644 > > > --- a/Documentation/security/keys/trusted-encrypted.rst > > > +++ b/Documentation/security/keys/trusted-encrypted.rst > > > @@ -42,6 +42,14 @@ safe. > > > randomly generated and fused into each SoC at manufacturing > > > time. > > > Otherwise, a common fixed test key is used instead. > > > > > > + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX > > > + SoCs) > > > + > > > + Rooted to a one-time programmable key (OTP) that is generally > > burnt > > > + in the on-chip fuses and is accessible to the DCP encryption > > > engine > > only. > > > + DCP provides two keys that can be used as root of trust: the OTP > > key > > > + and the UNIQUE key. Default is to use the UNIQUE key, but > > > selecting > > > + the OTP key can be done via a module parameter > > (dcp_use_otp_key). > > > + > > >* Execution isolation > > > > > > (1) TPM > > > @@ -57,6 +65,12 @@ safe. > > > > > > Fixed set of operations running in isolated execution > > > environment. > > > > > > + (4) DCP > > > + > > > + Fixed set of cryptographic operations running in isolated > > > execution > > > + environment. Only basic blob key encryption is executed there. > > > + The actual key sealing/unsealing is done on main > > > processor/kernel > > space. > > > + > > >* Optional binding to platform integrity state > > > > > > (1) TPM > > > @@ -79,6 +93,11 @@ safe. > > > Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs > > > for platform integrity. > > > > > > + (4) DCP > > > + > > > + Relies on Secure/Trusted boot process (called HAB by vendor) for > > > + platform integrity. > > > + > > >* Interfaces and APIs > > > > > > (1) TPM > > > @@ -94,6 +113,11 @@ safe. > > > > > > Interface is specific to silicon vendor. > > > > > > + (4) DCP > > >
Re: [PATCH v8 6/6] docs: trusted-encrypted: add DCP as new trust source
> b/security/keys/trusted-keys/trusted_dcp.c > index 16c44aafeab3..b5f81a05be36 100644 > --- a/security/keys/trusted-keys/trusted_dcp.c > +++ b/security/keys/trusted-keys/trusted_dcp.c > @@ -19,6 +19,25 @@ > #define DCP_BLOB_VERSION 1 > #define DCP_BLOB_AUTHLEN 16 > > +/** > + * DOC: dcp blob format > + * > + * The Data Co-Processor (DCP) provides hardware-bound AES keys using its > + * AES encryption engine only. It does not provide direct key > sealing/unsealing. > + * To make DCP hardware encryption keys usable as trust source, we define > + * our own custom format that uses a hardware-bound key to secure the sealing > + * key stored in the key blob. > + * > + * Whenever a new trusted key using DCP is generated, we generate a random > 128-bit > + * blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to > + * encrypt the trusted key payload using AES-128-GCM. > + * > + * The BEK itself is encrypted using the hardware-bound key using the DCP's > AES > + * encryption engine with AES-128-ECB. The encrypted BEK, generated nonce, > + * BEK-encrypted payload and authentication tag make up the blob format > together > + * with a version number, payload length and authentication tag. > + */ > + > /** > * struct dcp_blob_fmt - DCP BLOB format. > * Reviewed-by: Jarkko Sakkinen I can only test that this does not break a machine without the hardware feature. Is there anyone who could possibly peer test these patches? BR, Jarkko
Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source
On Thu Mar 28, 2024 at 8:47 PM EET, Jarkko Sakkinen wrote: > On Thu Mar 28, 2024 at 10:05 AM EET, David Gstir wrote: > > Jarkko, > > > > > On 27.03.2024, at 16:40, Jarkko Sakkinen wrote: > > > > > > On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote: > > >> Update the documentation for trusted and encrypted KEYS with DCP as new > > >> trust source: > > >> > > >> - Describe security properties of DCP trust source > > >> - Describe key usage > > >> - Document blob format > > >> > > >> Co-developed-by: Richard Weinberger > > >> Signed-off-by: Richard Weinberger > > >> Co-developed-by: David Oberhollenzer > > >> Signed-off-by: David Oberhollenzer > > >> Signed-off-by: David Gstir > > >> --- > > >> .../security/keys/trusted-encrypted.rst | 85 +++ > > >> 1 file changed, 85 insertions(+) > > >> > > >> diff --git a/Documentation/security/keys/trusted-encrypted.rst > > >> b/Documentation/security/keys/trusted-encrypted.rst > > >> index e989b9802f92..81fb3540bb20 100644 > > >> --- a/Documentation/security/keys/trusted-encrypted.rst > > >> +++ b/Documentation/security/keys/trusted-encrypted.rst > > >> @@ -42,6 +42,14 @@ safe. > > >> randomly generated and fused into each SoC at manufacturing > > >> time. > > >> Otherwise, a common fixed test key is used instead. > > >> > > >> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX > > >> SoCs) > > >> + > > >> + Rooted to a one-time programmable key (OTP) that is generally > > >> burnt > > >> + in the on-chip fuses and is accessible to the DCP encryption > > >> engine only. > > >> + DCP provides two keys that can be used as root of trust: the > > >> OTP key > > >> + and the UNIQUE key. Default is to use the UNIQUE key, but > > >> selecting > > >> + the OTP key can be done via a module parameter > > >> (dcp_use_otp_key). > > >> + > > >> * Execution isolation > > >> > > >> (1) TPM > > >> @@ -57,6 +65,12 @@ safe. > > >> > > >> Fixed set of operations running in isolated execution > > >> environment. > > >> > > >> + (4) DCP > > >> + > > >> + Fixed set of cryptographic operations running in isolated > > >> execution > > >> + environment. Only basic blob key encryption is executed there. > > >> + The actual key sealing/unsealing is done on main > > >> processor/kernel space. > > >> + > > >> * Optional binding to platform integrity state > > >> > > >> (1) TPM > > >> @@ -79,6 +93,11 @@ safe. > > >> Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs > > >> for platform integrity. > > >> > > >> + (4) DCP > > >> + > > >> + Relies on Secure/Trusted boot process (called HAB by vendor) > > >> for > > >> + platform integrity. > > >> + > > >> * Interfaces and APIs > > >> > > >> (1) TPM > > >> @@ -94,6 +113,11 @@ safe. > > >> > > >> Interface is specific to silicon vendor. > > >> > > >> + (4) DCP > > >> + > > >> + Vendor-specific API that is implemented as part of the DCP > > >> crypto driver in > > >> + ``drivers/crypto/mxs-dcp.c``. > > >> + > > >> * Threat model > > >> > > >> The strength and appropriateness of a particular trust source for a > > >> given > > >> @@ -129,6 +153,13 @@ selected trust source: > > >> CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device > > >> is probed. > > >> > > >> + * DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > > >> + > > >> + The DCP hardware device itself does not provide a dedicated RNG > > >> interface, > > >> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL > > >> do have > > >> + a dedicated hardware R
Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source
On Thu Mar 28, 2024 at 10:05 AM EET, David Gstir wrote: > Jarkko, > > > On 27.03.2024, at 16:40, Jarkko Sakkinen wrote: > > > > On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote: > >> Update the documentation for trusted and encrypted KEYS with DCP as new > >> trust source: > >> > >> - Describe security properties of DCP trust source > >> - Describe key usage > >> - Document blob format > >> > >> Co-developed-by: Richard Weinberger > >> Signed-off-by: Richard Weinberger > >> Co-developed-by: David Oberhollenzer > >> Signed-off-by: David Oberhollenzer > >> Signed-off-by: David Gstir > >> --- > >> .../security/keys/trusted-encrypted.rst | 85 +++ > >> 1 file changed, 85 insertions(+) > >> > >> diff --git a/Documentation/security/keys/trusted-encrypted.rst > >> b/Documentation/security/keys/trusted-encrypted.rst > >> index e989b9802f92..81fb3540bb20 100644 > >> --- a/Documentation/security/keys/trusted-encrypted.rst > >> +++ b/Documentation/security/keys/trusted-encrypted.rst > >> @@ -42,6 +42,14 @@ safe. > >> randomly generated and fused into each SoC at manufacturing time. > >> Otherwise, a common fixed test key is used instead. > >> > >> + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > >> + > >> + Rooted to a one-time programmable key (OTP) that is generally > >> burnt > >> + in the on-chip fuses and is accessible to the DCP encryption > >> engine only. > >> + DCP provides two keys that can be used as root of trust: the OTP > >> key > >> + and the UNIQUE key. Default is to use the UNIQUE key, but > >> selecting > >> + the OTP key can be done via a module parameter (dcp_use_otp_key). > >> + > >> * Execution isolation > >> > >> (1) TPM > >> @@ -57,6 +65,12 @@ safe. > >> > >> Fixed set of operations running in isolated execution environment. > >> > >> + (4) DCP > >> + > >> + Fixed set of cryptographic operations running in isolated > >> execution > >> + environment. Only basic blob key encryption is executed there. > >> + The actual key sealing/unsealing is done on main > >> processor/kernel space. > >> + > >> * Optional binding to platform integrity state > >> > >> (1) TPM > >> @@ -79,6 +93,11 @@ safe. > >> Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs > >> for platform integrity. > >> > >> + (4) DCP > >> + > >> + Relies on Secure/Trusted boot process (called HAB by vendor) for > >> + platform integrity. > >> + > >> * Interfaces and APIs > >> > >> (1) TPM > >> @@ -94,6 +113,11 @@ safe. > >> > >> Interface is specific to silicon vendor. > >> > >> + (4) DCP > >> + > >> + Vendor-specific API that is implemented as part of the DCP > >> crypto driver in > >> + ``drivers/crypto/mxs-dcp.c``. > >> + > >> * Threat model > >> > >> The strength and appropriateness of a particular trust source for a > >> given > >> @@ -129,6 +153,13 @@ selected trust source: > >> CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device > >> is probed. > >> > >> + * DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > >> + > >> + The DCP hardware device itself does not provide a dedicated RNG > >> interface, > >> + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL > >> do have > >> + a dedicated hardware RNG that is independent from DCP which can be > >> enabled > >> + to back the kernel RNG. > >> + > >> Users may override this by specifying ``trusted.rng=kernel`` on the kernel > >> command-line to override the used RNG with the kernel's random number pool. > >> > >> @@ -231,6 +262,19 @@ Usage:: > >> CAAM-specific format. The key length for new keys is always in bytes. > >> Trusted Keys can be 32 - 128 bytes (256 - 1024 bits). > >> > >> +Trusted Keys usage: DCP > >> +--- > >> + > >> +Usage:: >
Re: [PATCH v7 6/6] docs: trusted-encrypted: add DCP as new trust source
On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote: > Update the documentation for trusted and encrypted KEYS with DCP as new > trust source: > > - Describe security properties of DCP trust source > - Describe key usage > - Document blob format > > Co-developed-by: Richard Weinberger > Signed-off-by: Richard Weinberger > Co-developed-by: David Oberhollenzer > Signed-off-by: David Oberhollenzer > Signed-off-by: David Gstir > --- > .../security/keys/trusted-encrypted.rst | 85 +++ > 1 file changed, 85 insertions(+) > > diff --git a/Documentation/security/keys/trusted-encrypted.rst > b/Documentation/security/keys/trusted-encrypted.rst > index e989b9802f92..81fb3540bb20 100644 > --- a/Documentation/security/keys/trusted-encrypted.rst > +++ b/Documentation/security/keys/trusted-encrypted.rst > @@ -42,6 +42,14 @@ safe. > randomly generated and fused into each SoC at manufacturing time. > Otherwise, a common fixed test key is used instead. > > + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > + > + Rooted to a one-time programmable key (OTP) that is generally burnt > + in the on-chip fuses and is accessible to the DCP encryption engine > only. > + DCP provides two keys that can be used as root of trust: the OTP key > + and the UNIQUE key. Default is to use the UNIQUE key, but selecting > + the OTP key can be done via a module parameter (dcp_use_otp_key). > + >* Execution isolation > > (1) TPM > @@ -57,6 +65,12 @@ safe. > > Fixed set of operations running in isolated execution environment. > > + (4) DCP > + > + Fixed set of cryptographic operations running in isolated execution > + environment. Only basic blob key encryption is executed there. > + The actual key sealing/unsealing is done on main processor/kernel > space. > + >* Optional binding to platform integrity state > > (1) TPM > @@ -79,6 +93,11 @@ safe. > Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs > for platform integrity. > > + (4) DCP > + > + Relies on Secure/Trusted boot process (called HAB by vendor) for > + platform integrity. > + >* Interfaces and APIs > > (1) TPM > @@ -94,6 +113,11 @@ safe. > > Interface is specific to silicon vendor. > > + (4) DCP > + > + Vendor-specific API that is implemented as part of the DCP crypto > driver in > + ``drivers/crypto/mxs-dcp.c``. > + >* Threat model > > The strength and appropriateness of a particular trust source for a > given > @@ -129,6 +153,13 @@ selected trust source: > CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device > is probed. > > + * DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > + > + The DCP hardware device itself does not provide a dedicated RNG > interface, > + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL do > have > + a dedicated hardware RNG that is independent from DCP which can be > enabled > + to back the kernel RNG. > + > Users may override this by specifying ``trusted.rng=kernel`` on the kernel > command-line to override the used RNG with the kernel's random number pool. > > @@ -231,6 +262,19 @@ Usage:: > CAAM-specific format. The key length for new keys is always in bytes. > Trusted Keys can be 32 - 128 bytes (256 - 1024 bits). > > +Trusted Keys usage: DCP > +--- > + > +Usage:: > + > +keyctl add trusted name "new keylen" ring > +keyctl add trusted name "load hex_blob" ring > +keyctl print keyid > + > +"keyctl print" returns an ASCII hex copy of the sealed key, which is in > format > +specific to this DCP key-blob implementation. The key length for new keys is > +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits). > + > Encrypted Keys usage > > > @@ -426,3 +470,44 @@ string length. > privkey is the binary representation of TPM2B_PUBLIC excluding the > initial TPM2B header which can be reconstructed from the ASN.1 octed > string length. > + > +DCP Blob Format > +--- > + > +The Data Co-Processor (DCP) provides hardware-bound AES keys using its > +AES encryption engine only. It does not provide direct key sealing/unsealing. > +To make DCP hardware encryption keys usable as trust source, we define > +our own custom format that uses a hardware-bound key to secure the sealing > +key stored in the key blob. > + > +Whenever a new trusted key using DCP is generated, we generate a random > 128-bit > +blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to > +encrypt the trusted key payload using AES-128-GCM. > + > +The BEK itself is encrypted using the hardware-bound key using the DCP's AES > +encryption engine with AES-128-ECB. The encrypted BEK, generated nonce, >
Re: [PATCH v7 5/6] docs: document DCP-backed trusted keys kernel params
On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote: > Document the kernel parameters trusted.dcp_use_otp_key > and trusted.dcp_skip_zk_test for DCP-backed trusted keys. > > Co-developed-by: Richard Weinberger > Signed-off-by: Richard Weinberger > Co-developed-by: David Oberhollenzer > Signed-off-by: David Oberhollenzer > Signed-off-by: David Gstir > --- > Documentation/admin-guide/kernel-parameters.txt | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index 24c02c704049..b6944e57768a 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -6698,6 +6698,7 @@ > - "tpm" > - "tee" > - "caam" > + - "dcp" > If not specified then it defaults to iterating through > the trust source list starting with TPM and assigns the > first trust source as a backend which is initialized > @@ -6713,6 +6714,18 @@ > If not specified, "default" is used. In this case, > the RNG's choice is left to each individual trust > source. > > + trusted.dcp_use_otp_key > + This is intended to be used in combination with > + trusted.source=dcp and will select the DCP OTP key > + instead of the DCP UNIQUE key blob encryption. > + > + trusted.dcp_skip_zk_test > + This is intended to be used in combination with > + trusted.source=dcp and will disable the check if all > + the blob key is zero'ed. This is helpful for situations > where > + having this key zero'ed is acceptable. E.g. in testing > + scenarios. > + > tsc=Disable clocksource stability checks for TSC. > Format: > [x86] reliable: mark tsc clocksource as reliable, this Nicely documented, i.e. even I can understand what is said here :-) Reviewed-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v7 2/6] KEYS: trusted: improve scalability of trust source config
On Wed Mar 27, 2024 at 10:24 AM EET, David Gstir wrote: > Enabling trusted keys requires at least one trust source implementation > (currently TPM, TEE or CAAM) to be enabled. Currently, this is > done by checking each trust source's config option individually. > This does not scale when more trust sources like the one for DCP > are added, because the condition will get long and hard to read. > > Add config HAVE_TRUSTED_KEYS which is set to true by each trust source > once its enabled and adapt the check for having at least one active trust > source to use this option. Whenever a new trust source is added, it now > needs to select HAVE_TRUSTED_KEYS. > > Signed-off-by: David Gstir > --- > security/keys/trusted-keys/Kconfig | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/security/keys/trusted-keys/Kconfig > b/security/keys/trusted-keys/Kconfig > index dbfdd8536468..553dc117f385 100644 > --- a/security/keys/trusted-keys/Kconfig > +++ b/security/keys/trusted-keys/Kconfig > @@ -1,3 +1,6 @@ > +config HAVE_TRUSTED_KEYS > + bool > + > config TRUSTED_KEYS_TPM > bool "TPM-based trusted keys" > depends on TCG_TPM >= TRUSTED_KEYS > @@ -9,6 +12,7 @@ config TRUSTED_KEYS_TPM > select ASN1_ENCODER > select OID_REGISTRY > select ASN1 > + select HAVE_TRUSTED_KEYS > help > Enable use of the Trusted Platform Module (TPM) as trusted key > backend. Trusted keys are random number symmetric keys, > @@ -20,6 +24,7 @@ config TRUSTED_KEYS_TEE > bool "TEE-based trusted keys" > depends on TEE >= TRUSTED_KEYS > default y > + select HAVE_TRUSTED_KEYS > help > Enable use of the Trusted Execution Environment (TEE) as trusted > key backend. > @@ -29,10 +34,11 @@ config TRUSTED_KEYS_CAAM > depends on CRYPTO_DEV_FSL_CAAM_JR >= TRUSTED_KEYS > select CRYPTO_DEV_FSL_CAAM_BLOB_GEN > default y > + select HAVE_TRUSTED_KEYS > help > Enable use of NXP's Cryptographic Accelerator and Assurance Module > (CAAM) as trusted key backend. > > -if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM > -comment "No trust source selected!" > +if !HAVE_TRUSTED_KEYS > + comment "No trust source selected!" > endif Tested-by: Jarkko Sakkinen # for TRUSTED_KEYS_TPM Reviewed-by: Jarkko Sakkinen BR, Jarkko
Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml
On Tue Mar 12, 2024 at 1:11 PM EET, Lukas Wunner wrote:
> On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote:
> > Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to
> > the properties. Either this property is required or both linux,sml-base and
> > linux,sml-size are required. Add a test case for verification.
> >
> > Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device
> > tree binding documentation")
>
> The Fixes tag is confusing. The patch won't even apply cleanly to the
> v4.10 commit referenced here as the conversion to yaml happened only
> recently with v6.8.
>
> Why is the Fixes tag necessary in the first place? Same question for
> the other patches in the series. This looks like feature work rather
> than a fix. Not sure whether it satisfies the "obviously correct"
> rule per Documentation/process/stable-kernel-rules.rst.
I'm not yet sure whether these are bug fixes and or improvements because
I did not fully understand the scenario where TPM corrupts the event log
so that part reminds to be seen.
Probably once I fully understand what is going on, it is possible to
argue on that.
BR, Jarkko
Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote: > Stefan Berger writes: > > On 3/7/24 15:00, Jarkko Sakkinen wrote: > >> On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote: > >>> in short summary: s/Use/use/ > >>> > >>> On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote: > >>>> If linux,sml-log is available use it to get the TPM log rather than the > >>>> pointer found in linux,sml-base. This resolves an issue on PowerVM and > >>>> KVM > >>>> on Power where after a kexec the memory pointed to by linux,sml-base may > >>>> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and > >>>> linux,sml-size on these two platforms. > >>>> > >>>> Signed-off-by: Stefan Berger > >>> > >>> So shouldn't this have a fixed tag, or not? > >> > >> In English: do we want this to be backported to stable kernel releases or > >> not? > > > > Ideally, yes. v3 will have 3 patches and all 3 of them will have to be > > backported *together* and not applied otherwise if any one of them > > fails. Can this be 'guaranteed'? > > You can use Depends-on: to indicate the relationship. > > cheers Thanks, I've missed depends-on tag. Stefan, please add also "Cc: sta...@vger.kernel.org" just to make sure that I don't forget to add it. Right, and since these are so small scoped commits, and bug fixes in particular, it is also possible to do PR during the release cycle. BR, Jarkko
Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size
On Mon Mar 11, 2024 at 10:33 PM EET, Stefan Berger wrote:
>
>
> On 3/11/24 16:25, Jarkko Sakkinen wrote:
> > On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> >> If linux,sml-log is available use it to get the TPM log rather than the
> >> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> >> on Power where after a kexec the memory pointed to by linux,sml-base may
> >> have become inaccessible or corrupted. Also, linux,sml-log has replaced
> >> linux,sml-base and linux,sml-size on these two platforms.
> >>
> >> Keep the handling of linux,sml-base/sml-size for powernv platforms that
> >> provide the two properties via skiboot.
> >>
> >> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event
> >> log")
> >> Signed-off-by: Stefan Berger
> >
> > I'm worried about not being up to date and instead using "cached" values
> > when verifying anything from a security chip. Does this guarantee that
> > TPM log is corrupted and will not get updated somehow?
>
>
> What do you mean 'guarantee that TPM log is corrupted'?
I presume that this is for power architecture but I have no idea what
leads log being corrupted, and is the scope all of that that arch or
some subset of CPUs.
The commit message is not very detailed on kexec scenario. It more like
assumes that reader knows all the detail beforehand. So probably this
will start to make sense once the backing story is improved, that's
all.
> The TPM was handed over from the firmware to Linux and the firmware then
> freed all memory associated with the log and will then not create a new
> log or touch the TPM or do anything that would require an update to the
> handed-over log. Linux also does not append to the firmware log. So
> whatever we now find in linux,sml-log would be the latest firmware log
> and the state of the 'firmware PCRs' computed from it should correspond
> to the current state of the 'firmware PCRs'.
So on what CPU this happens and is there any bigger picture for that
design choice in the firmware?
If it is a firmware bug, this should emit FW_BUG log message. If not,
then this commit message should provide the necessary context.
BR, Jarkko
Re: [RFC PATCH v2 3/3] tpm: of: If available use linux,sml-log to get the log and its size
On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote:
> If linux,sml-log is available use it to get the TPM log rather than the
> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> on Power where after a kexec the memory pointed to by linux,sml-base may
> have become inaccessible or corrupted. Also, linux,sml-log has replaced
> linux,sml-base and linux,sml-size on these two platforms.
>
> Keep the handling of linux,sml-base/sml-size for powernv platforms that
> provide the two properties via skiboot.
>
> Fixes: c5df39262dd5 ("drivers/char/tpm: Add securityfs support for event log")
> Signed-off-by: Stefan Berger
I'm worried about not being up to date and instead using "cached" values
when verifying anything from a security chip. Does this guarantee that
TPM log is corrupted and will not get updated somehow?
BR, Jarkko
Re: [RFC PATCH v2 1/3] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
On Mon Mar 11, 2024 at 3:20 PM EET, Stefan Berger wrote: > linux,sml-base holds the address of a buffer with the TPM log. This > buffer may become invalid after a kexec. To avoid accessing an invalid > address or corrupted buffer, embed the whole TPM log in the device tree > property linux,sml-log. This helps to protect the log since it is > properly carried across a kexec soft reboot with both of the kexec > syscalls. - Describe the environment where TPM log gets corrupted. - Describe why TPM log gets corrupted on kexec. > > Avoid having the firmware ingest the whole TPM log when calling > prom_setprop but only create the linux,sml-log property as a place holder. > Insert the actual TPM log during the tree flattening phase. This commit message should shed some light about reasons of the corruption in order to conclude that it should be fixed up like this. I.e. why the "post-state" is a legit state where can be continued despite a log being corrupted. Especially in security features this is pretty essential information. BR, Jarkko
Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
On Fri Mar 8, 2024 at 2:17 PM EET, Stefan Berger wrote: > > > On 3/7/24 15:00, Jarkko Sakkinen wrote: > > On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote: > >> in short summary: s/Use/use/ > >> > >> On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote: > >>> If linux,sml-log is available use it to get the TPM log rather than the > >>> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM > >>> on Power where after a kexec the memory pointed to by linux,sml-base may > >>> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and > >>> linux,sml-size on these two platforms. > >>> > >>> Signed-off-by: Stefan Berger > >> > >> So shouldn't this have a fixed tag, or not? > > > > In English: do we want this to be backported to stable kernel releases or > > not? > > Ideally, yes. v3 will have 3 patches and all 3 of them will have to be > backported *together* and not applied otherwise if any one of them > fails. Can this be 'guaranteed'? All of them will end up to stable if the following conditions hold: - All have a fixes tag. - All have "Cc: sta...@vger.kernel.org". - We agree in the review process that they are all legit fixes. BR, Jarkko
Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
On Fri Mar 8, 2024 at 9:17 AM EET, David Gstir wrote:
> Hi Jarkko,
>
> > On 07.03.2024, at 20:30, Jarkko Sakkinen wrote:
>
> [...]
>
> >> +
> >> +static int trusted_dcp_init(void)
> >> +{
> >> + int ret;
> >> +
> >> + if (use_otp_key)
> >> + pr_info("Using DCP OTP key\n");
> >> +
> >> + ret = test_for_zero_key();
> >> + if (ret) {
> >> + pr_err("Test for zero'ed keys failed: %i\n", ret);
> >
> > I'm not sure whether this should err or warn.
> >
> > What sort of situations can cause the test the fail (e.g.
> > adversary/interposer, bad configuration etc.).
>
> This occurs when the hardware is not in "secure mode". I.e. it’s a bad
> configuration issue.
> Once the board is properly configured, this will never trigger again.
> Do you think a warning is better for this then?
Bad configuration is not unexpected configuration so it cannot possibly
be an error situation as far as Linux is considered. So warning is
appropriate here I'd figure.
BR, Jarkko
Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:
> linux,sml-base holds the address of a buffer with the TPM log. This
> buffer may become invalid after a kexec and therefore embed the whole TPM
> log in linux,sml-log. This helps to protect the log since it is properly
> carried across a kexec with both of the kexec syscalls.
So, I see only description of the problem but nothing how it gets
addressed.
>
> Signed-off-by: Stefan Berger
> ---
> arch/powerpc/kernel/prom_init.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index e67effdba85c..41268c30de4c 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1956,12 +1956,8 @@ static void __init prom_instantiate_sml(void)
>
> reserve_mem(base, size);
>
> - prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> - , sizeof(base));
> - prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> - , sizeof(size));
> -
> - prom_debug("sml base = 0x%llx\n", base);
> + prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
> + (void *)base, size);
> prom_debug("sml size = 0x%x\n", size);
>
> prom_debug("prom_instantiate_sml: end...\n");
BR, Jarkko
Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote: > in short summary: s/Use/use/ > > On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote: > > If linux,sml-log is available use it to get the TPM log rather than the > > pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM > > on Power where after a kexec the memory pointed to by linux,sml-base may > > have been corrupted. Also, linux,sml-log has replaced linux,sml-base and > > linux,sml-size on these two platforms. > > > > Signed-off-by: Stefan Berger > > So shouldn't this have a fixed tag, or not? In English: do we want this to be backported to stable kernel releases or not? BR, Jarkko
Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size
in short summary: s/Use/use/
On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:
> If linux,sml-log is available use it to get the TPM log rather than the
> pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
> on Power where after a kexec the memory pointed to by linux,sml-base may
> have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
> linux,sml-size on these two platforms.
>
> Signed-off-by: Stefan Berger
So shouldn't this have a fixed tag, or not?
> ---
> drivers/char/tpm/eventlog/of.c | 36 +++---
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..e37196e64ef1 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -54,8 +54,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> const u32 *sizep;
> const u64 *basep;
> struct tpm_bios_log *log;
> + const void *logp;
> u32 size;
> - u64 base;
>
> log = >log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -66,37 +66,23 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return tpm_read_log_memory_region(chip);
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + logp = of_get_property(np, "linux,sml-log", );
> + if (logp == NULL) {
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return tpm_read_log_memory_region(chip);
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> + logp = __va(be64_to_cpup((__force __be64 *)basep));
> size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> }
> -
> if (size == 0) {
> dev_warn(>dev, "%s: Event log area empty\n", __func__);
> return -EIO;
> }
>
> - log->bios_event_log = devm_kmemdup(>dev, __va(base), size,
> GFP_KERNEL);
> + log->bios_event_log = devm_kmemdup(>dev, logp, size, GFP_KERNEL);
> if (!log->bios_event_log)
> return -ENOMEM;
>
Looks pretty good other than that.
BR, Jarkko
Re: [PATCH v6 5/6] docs: document DCP-backed trusted keys kernel params
On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote: > Document the kernel parameters trusted.dcp_use_otp_key > and trusted.dcp_skip_zk_test for DCP-backed trusted keys. > > Co-developed-by: Richard Weinberger > Signed-off-by: Richard Weinberger > Co-developed-by: David Oberhollenzer > Signed-off-by: David Oberhollenzer > Signed-off-by: David Gstir > --- > Documentation/admin-guide/kernel-parameters.txt | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index 24c02c704049..b6944e57768a 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -6698,6 +6698,7 @@ > - "tpm" > - "tee" > - "caam" > + - "dcp" > If not specified then it defaults to iterating through > the trust source list starting with TPM and assigns the > first trust source as a backend which is initialized > @@ -6713,6 +6714,18 @@ > If not specified, "default" is used. In this case, > the RNG's choice is left to each individual trust > source. > > + trusted.dcp_use_otp_key > + This is intended to be used in combination with > + trusted.source=dcp and will select the DCP OTP key > + instead of the DCP UNIQUE key blob encryption. > + > + trusted.dcp_skip_zk_test > + This is intended to be used in combination with > + trusted.source=dcp and will disable the check if all > + the blob key is zero'ed. This is helpful for situations > where > + having this key zero'ed is acceptable. E.g. in testing > + scenarios. > + > tsc=Disable clocksource stability checks for TSC. > Format: > [x86] reliable: mark tsc clocksource as reliable, this I don't disagree with the API part. Mimi? BR, Jarkko
Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
> Beside of accelerated crypto operations, it also offers support for
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism similar to what CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software (i.e. the kernel).
>
> The software-based blob format used by DCP trusted keys encrypts
> the payload using AES-128-GCM with a freshly generated random key and nonce.
> The random key itself is AES-128-ECB encrypted using the DCP unique
> or OTP key.
>
> The DCP trusted key blob format is:
> /*
> * struct dcp_blob_fmt - DCP BLOB format.
> *
> * @fmt_version: Format version, currently being %1
> * @blob_key: Random AES 128 key which is used to encrypt @payload,
> *@blob_key itself is encrypted with OTP or UNIQUE device key in
> *AES-128-ECB mode by DCP.
> * @nonce: Random nonce used for @payload encryption.
> * @payload_len: Length of the plain text @payload.
> * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> *
> * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len
> +
> * AES_BLOCK_SIZE.
> */
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
>
> By default the unique key is used. It is also possible to use the
> OTP key. While the unique key should be unique it is not documented how
> this key is derived. Therefore selection the OTP key is supported as
> well via the use_otp_key module parameter.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> include/keys/trusted_dcp.h| 11 +
> security/keys/trusted-keys/Kconfig| 8 +
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 309 ++
> 5 files changed, 335 insertions(+), 1 deletion(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..9aaa42075b40
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + */
> +
> +#ifndef TRUSTED_DCP_H
> +#define TRUSTED_DCP_H
> +
> +extern struct trusted_key_ops dcp_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Kconfig
> b/security/keys/trusted-keys/Kconfig
> index 553dc117f385..1fb8aa001995 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -39,6 +39,14 @@ config TRUSTED_KEYS_CAAM
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + select HAVE_TRUSTED_KEYS
> + help
> + Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
> if !HAVE_TRUSTED_KEYS
> comment "No trust source selected!"
> endif
> diff --git a/security/keys/trusted-keys/Makefile
> b/security/keys/trusted-keys/Makefile
> index 735aa0bc08ef..f0f3b27f688b 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -14,3 +14,5 @@ trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
> trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>
> trusted-$(CONFIG_TRUSTED_KEYS_CAAM) += trusted_caam.o
> +
> +trusted-$(CONFIG_TRUSTED_KEYS_DCP) += trusted_dcp.o
> diff --git a/security/keys/trusted-keys/trusted_core.c
> b/security/keys/trusted-keys/trusted_core.c
> index fee1ab2c734d..5113aeae5628 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -30,7 +31,7 @@ MODULE_PARM_DESC(rng, "Select trusted key RNG");
>
> static char *trusted_key_source;
> module_param_named(source, trusted_key_source, charp, 0);
> -MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee or caam)");
> +MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee, caam or
> dcp)");
>
> static const struct trusted_key_source trusted_key_sources[] = {
> #if defined(CONFIG_TRUSTED_KEYS_TPM)
> @@ -42,6 +43,9 @@ static const struct trusted_key_source
> trusted_key_sources[] = {
> #if
Re: [PATCH v6 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
_sg, dst_sg;
> + struct crypto_aead *aead;
> + int ret;
> +
> + aead = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(aead)) {
> + ret = PTR_ERR(aead);
> + goto out;
> + }
> +
> + ret = crypto_aead_setauthsize(aead, DCP_BLOB_AUTHLEN);
> + if (ret < 0) {
> + pr_err("Can't set crypto auth tag len: %d\n", ret);
> + goto free_aead;
> + }
> +
> + aead_req = aead_request_alloc(aead, GFP_KERNEL);
> + if (!aead_req) {
> + ret = -ENOMEM;
> + goto free_aead;
> + }
> +
> + sg_init_one(_sg, in, len);
> + if (do_encrypt) {
> + /*
> + * If we encrypt our buffer has extra space for the auth tag.
> + */
> + sg_init_one(_sg, out, len + DCP_BLOB_AUTHLEN);
> + } else {
> + sg_init_one(_sg, out, len);
> + }
> +
> + aead_request_set_crypt(aead_req, _sg, _sg, len, nonce);
> + aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL,
> + NULL);
> + aead_request_set_ad(aead_req, 0);
> +
> + if (crypto_aead_setkey(aead, key, AES_KEYSIZE_128)) {
> + pr_err("Can't set crypto AEAD key\n");
> + ret = -EINVAL;
> + goto free_req;
> + }
> +
> + if (do_encrypt)
> + ret = crypto_aead_encrypt(aead_req);
> + else
> + ret = crypto_aead_decrypt(aead_req);
> +
> +free_req:
> + aead_request_free(aead_req);
> +free_aead:
> + crypto_free_aead(aead);
> +out:
> + return ret;
> +}
> +
> +static int decrypt_blob_key(u8 *key)
> +{
> + return do_dcp_crypto(key, key, false);
> +}
> +
> +static int encrypt_blob_key(u8 *key)
> +{
> + return do_dcp_crypto(key, key, true);
> +}
> +
> +static int trusted_dcp_seal(struct trusted_key_payload *p, char *datablob)
> +{
> + struct dcp_blob_fmt *b = (struct dcp_blob_fmt *)p->blob;
> + int blen, ret;
> +
> + blen = calc_blob_len(p->key_len);
> + if (blen > MAX_BLOB_SIZE)
> + return -E2BIG;
> +
> + b->fmt_version = DCP_BLOB_VERSION;
> + get_random_bytes(b->nonce, AES_KEYSIZE_128);
> + get_random_bytes(b->blob_key, AES_KEYSIZE_128);
> +
> + ret = do_aead_crypto(p->key, b->payload, p->key_len, b->blob_key,
> + b->nonce, true);
> + if (ret) {
> + pr_err("Unable to encrypt blob payload: %i\n", ret);
> + return ret;
> + }
> +
> + ret = encrypt_blob_key(b->blob_key);
> + if (ret) {
> + pr_err("Unable to encrypt blob key: %i\n", ret);
> + return ret;
> + }
> +
> + b->payload_len = get_unaligned_le32(>key_len);
> + p->blob_len = blen;
> + return 0;
> +}
> +
> +static int trusted_dcp_unseal(struct trusted_key_payload *p, char *datablob)
> +{
> + struct dcp_blob_fmt *b = (struct dcp_blob_fmt *)p->blob;
> + int blen, ret;
> +
> + if (b->fmt_version != DCP_BLOB_VERSION) {
> + pr_err("DCP blob has bad version: %i, expected %i\n",
> +b->fmt_version, DCP_BLOB_VERSION);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + p->key_len = le32_to_cpu(b->payload_len);
> + blen = calc_blob_len(p->key_len);
> + if (blen != p->blob_len) {
> + pr_err("DCP blob has bad length: %i != %i\n", blen,
> +p->blob_len);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = decrypt_blob_key(b->blob_key);
> + if (ret) {
> + pr_err("Unable to decrypt blob key: %i\n", ret);
> + goto out;
> + }
> +
> + ret = do_aead_crypto(b->payload, p->key, p->key_len + DCP_BLOB_AUTHLEN,
> + b->blob_key, b->nonce, false);
> + if (ret) {
> + pr_err("Unwrap of DCP payload failed: %i\n", ret);
> + goto out;
> + }
> +
> + ret = 0;
> +out:
> + return ret;
> +}
> +
> +static int test_for_zero_key(void)
> +{
> + static const u8 bad[] = {0x9a, 0xda, 0xe0, 0x54, 0xf6, 0x3d, 0xfa, 0xff,
> + 0x5e, 0xa1, 0x8e, 0x45, 0xed, 0xf6, 0xea,
> 0x6f};
> + void *buf = NULL;
> + int ret = 0;
> +
> + if (skip_zk_test)
> + goto out;
> +
> + buf = kmalloc(AES_BLOCK_SIZE, GFP_KERNEL);
> + if (!buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + memset(buf, 0x55, AES_BLOCK_SIZE);
> +
> + ret = do_dcp_crypto(buf, buf, true);
> + if (ret)
> + goto out;
> +
> + if (memcmp(buf, bad, AES_BLOCK_SIZE) == 0) {
> + pr_err("Device neither in secure nor trusted mode!\n");
> + ret = -EINVAL;
> + }
> +out:
> + kfree(buf);
> + return ret;
> +}
> +
> +static int trusted_dcp_init(void)
> +{
> + int ret;
> +
> + if (use_otp_key)
> + pr_info("Using DCP OTP key\n");
> +
> + ret = test_for_zero_key();
> + if (ret) {
> + pr_err("Test for zero'ed keys failed: %i\n", ret);
> +
> + return -EINVAL;
> + }
> +
> + return register_key_type(_type_trusted);
> +}
> +
> +static void trusted_dcp_exit(void)
> +{
> + unregister_key_type(_type_trusted);
> +}
> +
> +struct trusted_key_ops dcp_trusted_key_ops = {
> + .exit = trusted_dcp_exit,
> + .init = trusted_dcp_init,
> + .seal = trusted_dcp_seal,
> + .unseal = trusted_dcp_unseal,
> + .migratable = 0,
> +};
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v6 2/6] KEYS: trusted: improve scalability of trust source config
On Thu Mar 7, 2024 at 5:38 PM EET, David Gstir wrote: > Enabling trusted keys requires at least one trust source implementation > (currently TPM, TEE or CAAM) to be enabled. Currently, this is > done by checking each trust source's config option individually. > This does not scale when more trust sources like the one for DCP > are added. nit: just to complete sentence and tie up the story "added because ..." > > Add config HAVE_TRUSTED_KEYS which is set to true by each trust source > once its enabled and adapt the check for having at least one active trust > source to use this option. Whenever a new trust source is added, it now > needs to select HAVE_TRUSTED_KEYS. > > Signed-off-by: David Gstir > --- > security/keys/trusted-keys/Kconfig | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/security/keys/trusted-keys/Kconfig > b/security/keys/trusted-keys/Kconfig > index dbfdd8536468..553dc117f385 100644 > --- a/security/keys/trusted-keys/Kconfig > +++ b/security/keys/trusted-keys/Kconfig > @@ -1,3 +1,6 @@ > +config HAVE_TRUSTED_KEYS > + bool > + > config TRUSTED_KEYS_TPM > bool "TPM-based trusted keys" > depends on TCG_TPM >= TRUSTED_KEYS > @@ -9,6 +12,7 @@ config TRUSTED_KEYS_TPM > select ASN1_ENCODER > select OID_REGISTRY > select ASN1 > + select HAVE_TRUSTED_KEYS > help > Enable use of the Trusted Platform Module (TPM) as trusted key > backend. Trusted keys are random number symmetric keys, > @@ -20,6 +24,7 @@ config TRUSTED_KEYS_TEE > bool "TEE-based trusted keys" > depends on TEE >= TRUSTED_KEYS > default y > + select HAVE_TRUSTED_KEYS > help > Enable use of the Trusted Execution Environment (TEE) as trusted > key backend. > @@ -29,10 +34,11 @@ config TRUSTED_KEYS_CAAM > depends on CRYPTO_DEV_FSL_CAAM_JR >= TRUSTED_KEYS > select CRYPTO_DEV_FSL_CAAM_BLOB_GEN > default y > + select HAVE_TRUSTED_KEYS > help > Enable use of NXP's Cryptographic Accelerator and Assurance Module > (CAAM) as trusted key backend. > > -if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM > -comment "No trust source selected!" > +if !HAVE_TRUSTED_KEYS > + comment "No trust source selected!" > endif otherwise, it is in reasonable shape and form. BR, Jarkko
Re: [PATCH v6 1/6] crypto: mxs-dcp: Add support for hardware-bound keys
.setkey = mxs_dcp_aes_setrefkey, > + .encrypt= mxs_dcp_aes_cbc_encrypt, > + .decrypt= mxs_dcp_aes_cbc_decrypt, > + .ivsize = AES_BLOCK_SIZE, > + .init = mxs_dcp_paes_init_tfm, > }, > }; > > diff --git a/include/soc/fsl/dcp.h b/include/soc/fsl/dcp.h > new file mode 100644 > index ..3ec335d8ca8b > --- /dev/null > +++ b/include/soc/fsl/dcp.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2021 sigma star gmbh > + * > + * Specifies paes key slot handles for NXP's DCP (Data Co-Processor) to be > used > + * with the crypto_skcipher_setkey(). > + */ > + > +#ifndef MXS_DCP_H > +#define MXS_DCP_H > + > +#define DCP_PAES_KEYSIZE 1 > +#define DCP_PAES_KEY_SLOT0 0x00 > +#define DCP_PAES_KEY_SLOT1 0x01 > +#define DCP_PAES_KEY_SLOT2 0x02 > +#define DCP_PAES_KEY_SLOT3 0x03 > +#define DCP_PAES_KEY_UNIQUE 0xfe > +#define DCP_PAES_KEY_OTP 0xff > + > +#endif /* MXS_DCP_H */ Looks to good enough to me: Reviewed-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v5 0/6] DCP as trusted keys backend
On Tue Dec 19, 2023 at 2:45 AM EET, Paul Moore wrote:
> On Fri, Dec 15, 2023 at 6:07 AM David Gstir wrote:
> >
> > This is a revival of the previous patch set submitted by Richard Weinberger:
> > https://lore.kernel.org/linux-integrity/20210614201620.30451-1-rich...@nod.at/
> >
> > v4 is here:
> > https://lore.kernel.org/keyrings/20231024162024.51260-1-da...@sigma-star.at/
> >
> > v4 -> v5:
> > - Make Kconfig for trust source check scalable as suggested by Jarkko
> > Sakkinen
> > - Add Acked-By from Herbert Xu to patch #1 - thanks!
> > v3 -> v4:
> > - Split changes on MAINTAINERS and documentation into dedicated patches
> > - Use more concise wording in commit messages as suggested by Jarkko
> > Sakkinen
> > v2 -> v3:
> > - Addressed review comments from Jarkko Sakkinen
> > v1 -> v2:
> > - Revive and rebase to latest version
> > - Include review comments from Ahmad Fatoum
> >
> > The Data CoProcessor (DCP) is an IP core built into many NXP SoCs such
> > as i.mx6ull.
> >
> > Similar to the CAAM engine used in more powerful SoCs, DCP can AES-
> > encrypt/decrypt user data using a unique, never-disclosed,
> > device-specific key. Unlike CAAM though, it cannot directly wrap and
> > unwrap blobs in hardware. As DCP offers only the bare minimum feature
> > set and a blob mechanism needs aid from software. A blob in this case
> > is a piece of sensitive data (e.g. a key) that is encrypted and
> > authenticated using the device-specific key so that unwrapping can only
> > be done on the hardware where the blob was wrapped.
> >
> > This patch series adds a DCP based, trusted-key backend and is similar
> > in spirit to the one by Ahmad Fatoum [0] that does the same for CAAM.
> > It is of interest for similar use cases as the CAAM patch set, but for
> > lower end devices, where CAAM is not available.
> >
> > Because constructing and parsing the blob has to happen in software,
> > we needed to decide on a blob format and chose the following:
> >
> > struct dcp_blob_fmt {
> > __u8 fmt_version;
> > __u8 blob_key[AES_KEYSIZE_128];
> > __u8 nonce[AES_KEYSIZE_128];
> > __le32 payload_len;
> > __u8 payload[];
> > } __packed;
> >
> > The `fmt_version` is currently 1.
> >
> > The encrypted key is stored in the payload area. It is AES-128-GCM
> > encrypted using `blob_key` and `nonce`, GCM auth tag is attached at
> > the end of the payload (`payload_len` does not include the size of
> > the auth tag).
> >
> > The `blob_key` itself is encrypted in AES-128-ECB mode by DCP using
> > the OTP or UNIQUE device key. A new `blob_key` and `nonce` are generated
> > randomly, when sealing/exporting the DCP blob.
> >
> > This patchset was tested with dm-crypt on an i.MX6ULL board.
> >
> > [0]
> > https://lore.kernel.org/keyrings/20220513145705.2080323-1-a.fat...@pengutronix.de/
> >
> > David Gstir (6):
> > crypto: mxs-dcp: Add support for hardware-bound keys
> > KEYS: trusted: improve scalability of trust source config
> > KEYS: trusted: Introduce NXP DCP-backed trusted keys
> > MAINTAINERS: add entry for DCP-based trusted keys
> > docs: document DCP-backed trusted keys kernel params
> > docs: trusted-encrypted: add DCP as new trust source
> >
> > .../admin-guide/kernel-parameters.txt | 13 +
> > .../security/keys/trusted-encrypted.rst | 85 +
> > MAINTAINERS | 9 +
> > drivers/crypto/mxs-dcp.c | 104 +-
> > include/keys/trusted_dcp.h| 11 +
> > include/soc/fsl/dcp.h | 17 +
> > security/keys/trusted-keys/Kconfig| 18 +-
> > security/keys/trusted-keys/Makefile | 2 +
> > security/keys/trusted-keys/trusted_core.c | 6 +-
> > security/keys/trusted-keys/trusted_dcp.c | 311 ++
> > 10 files changed, 562 insertions(+), 14 deletions(-)
> > create mode 100644 include/keys/trusted_dcp.h
> > create mode 100644 include/soc/fsl/dcp.h
> > create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> Jarkko, Mimi, David - if this patchset isn't already in your review
> queue, can you take a look at it from a security/keys perspective?
>
> Thanks.
I gave my 5 cents. I had no intention not to review it, somehow just
slipped. I try to do my best but sometimes this still does happen :-) So
please ping me if there is radio silence.
BR, Jarkko
Re: [PATCH v5 4/6] MAINTAINERS: add entry for DCP-based trusted keys
On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote: > This covers trusted keys backed by NXP's DCP (Data Co-Processor) chip > found in smaller i.MX SoCs. > > Signed-off-by: David Gstir > --- > MAINTAINERS | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 90f13281d297..988d01226131 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11647,6 +11647,15 @@ S: Maintained > F: include/keys/trusted_caam.h > F: security/keys/trusted-keys/trusted_caam.c > > +KEYS-TRUSTED-DCP > +M: David Gstir > +R: sigma star Kernel Team > +L: linux-integr...@vger.kernel.org > +L: keyri...@vger.kernel.org > +S: Supported > +F: include/keys/trusted_dcp.h > +F: security/keys/trusted-keys/trusted_dcp.c > + > KEYS-TRUSTED-TEE > M: Sumit Garg > L: linux-integr...@vger.kernel.org Acked-by: Jarkko Sakkinen I can for sure put this. The code quality is *not* bad :-) However, your backing story really needs rework. It is otherwise impossible to understand the code changes later on because amount of information is vast, and you tend to forget details of stuff that you are not actively working on. That is why we care so deeply about them. BR, Jarkko
Re: [PATCH v5 3/6] KEYS: trusted: Introduce NXP DCP-backed trusted keys
On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
> Beside of accelerated crypto operations, it also offers support for
Why acronym is not opened already in the first patch? Also, that does
not mean it could not be opened also here. Sometimes redundancy is for
better...
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism similar to what CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software (i.e. the kernel).
>
> The software-based blob format used by DCP trusted keys encrypts
> the payload using AES-128-GCM with a freshly generated random key and nonce.
> The random key itself is AES-128-ECB encrypted using the DCP unique
> or OTP key.
>
> The DCP trusted key blob format is:
> /*
> * struct dcp_blob_fmt - DCP BLOB format.
> *
> * @fmt_version: Format version, currently being %1
> * @blob_key: Random AES 128 key which is used to encrypt @payload,
> *@blob_key itself is encrypted with OTP or UNIQUE device key in
> *AES-128-ECB mode by DCP.
> * @nonce: Random nonce used for @payload encryption.
> * @payload_len: Length of the plain text @payload.
> * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> *
> * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len
> +
> * AES_BLOCK_SIZE.
> */
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
>
> By default the unique key is used. It is also possible to use the
> OTP key. While the unique key should be unique it is not documented how
> this key is derived. Therefore selection the OTP key is supported as
> well via the use_otp_key module parameter.
This is pretty good but I'll look it in more detail in the next
iteration of the patch set.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> include/keys/trusted_dcp.h| 11 +
> security/keys/trusted-keys/Kconfig| 8 +
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 311 ++
> 5 files changed, 337 insertions(+), 1 deletion(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..9aaa42075b40
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + */
> +
> +#ifndef TRUSTED_DCP_H
> +#define TRUSTED_DCP_H
> +
> +extern struct trusted_key_ops dcp_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Kconfig
> b/security/keys/trusted-keys/Kconfig
> index 553dc117f385..1fb8aa001995 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -39,6 +39,14 @@ config TRUSTED_KEYS_CAAM
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + select HAVE_TRUSTED_KEYS
> + help
> + Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
> if !HAVE_TRUSTED_KEYS
> comment "No trust source selected!"
> endif
> diff --git a/security/keys/trusted-keys/Makefile
> b/security/keys/trusted-keys/Makefile
> index 735aa0bc08ef..f0f3b27f688b 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -14,3 +14,5 @@ trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
> trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
>
> trusted-$(CONFIG_TRUSTED_KEYS_CAAM) += trusted_caam.o
> +
> +trusted-$(CONFIG_TRUSTED_KEYS_DCP) += trusted_dcp.o
> diff --git a/security/keys/trusted-keys/trusted_core.c
> b/security/keys/trusted-keys/trusted_core.c
> index c6fc50d67214..8af0988be850 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -30,7 +31,7 @@ MODULE_PARM_DESC(rng, "Select trusted key RNG");
>
> static char *trusted_key_source;
> module_param_named(source, trusted_key_source, charp, 0);
> -MODULE_PARM_DESC(source, "Select trusted keys source (tpm, tee or caam)");
> +MODULE_PARM_DESC(source, "Select trusted keys source
Re: [PATCH v5 2/6] KEYS: trusted: improve scalability of trust source config
On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote: > Checking if at least one valid trust source is selected does not scale > and becomes hard to read. This improves this in preparation for the DCP > trust source. This commit needs a complete rewrite and I do not have time and energy to propose one but here's what it should contain: 1. Add HAVE_TRUSTED_KEYS to th trusted-keys/Kconfig. 2. The use and purpose of HAVE_TRUSTED_KEYS. If you read your commit message, do you see anything at all concerning the code change? It only tells a story about something that is not properly being defined to be "hard to read", which is no rationale to change anything at all in the kernel. If you put factors more focus on being as straight and easy to get in the commit messages, it will also improve the round-trip time between sending the patch set and getting reviewed, because people with limited time at their hands tend to pick the low-hanging fruit first. BR, Jarkko
Re: [PATCH v5 1/6] crypto: mxs-dcp: Add support for hardware-bound keys
Further remarks.
On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> DCP is capable of performing AES with two hardware-bound keys:
>
> - The one-time programmable (OTP) key which is burnt via on-chip fuses
> - The unique key (UK) which is derived from the OTP key
>
> In addition to the two hardware-bound keys, DCP also supports
> storing keys in 4 dedicated key slots within its secure memory area
> (internal SRAM).
>
> These keys are not stored in main memory and are therefore
> not directly accessible by the operating system. To use them
> for AES operations, a one-byte key reference has to supplied
> with the DCP operation descriptor in the control register.
>
> This adds support for using any of these 6 keys through the crypto API
> via their key reference after they have been set up. The main purpose
Please write actions always in imperative form. E.g. instead of "This
adds" you could just as well simply write "Add", right?
Also, "adding support" is somewhat abstract expression tbh. You should
rather point out the driver exactly you are modifying (completely
missing BTW) and what sort of new functionalities this mysetery word
"support" maps into.
More cocrete and dumbed you can ever be, the better is the commit
message and more likely we also get the code changes you are doing.
> is to add support for DCP-backed trusted keys. Other use cases are
> possible too (see similar existing paes implementations), but these
> should carefully be evaluated as e.g. enabling AF_ALG will give
> userspace full access to use keys. In scenarios with untrustworthy
> userspace, this will enable en-/decryption oracles.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> Acked-by: Herbert Xu
> ---
> drivers/crypto/mxs-dcp.c | 104 ++-
> include/soc/fsl/dcp.h| 17 +++
> 2 files changed, 110 insertions(+), 11 deletions(-)
> create mode 100644 include/soc/fsl/dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index f6b7bce0e656..2dc664fb2faf 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolkey_referenced;
> };
>
> struct dcp_aes_req_ctx {
> @@ -155,6 +157,7 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL0_HASH_TERM (1 << 13)
> #define MXS_DCP_CONTROL0_HASH_INIT (1 << 12)
> #define MXS_DCP_CONTROL0_PAYLOAD_KEY (1 << 11)
> +#define MXS_DCP_CONTROL0_OTP_KEY (1 << 10)
> #define MXS_DCP_CONTROL0_CIPHER_ENCRYPT (1 << 8)
> #define MXS_DCP_CONTROL0_CIPHER_INIT (1 << 9)
> #define MXS_DCP_CONTROL0_ENABLE_HASH (1 << 6)
> @@ -168,6 +171,8 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL1_CIPHER_MODE_ECB (0 << 4)
> #define MXS_DCP_CONTROL1_CIPHER_SELECT_AES128(0 << 0)
>
> +#define MXS_DCP_CONTROL1_KEY_SELECT_SHIFT8
> +
> static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
> {
> int dma_err;
> @@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> struct dcp *sdcp = global_sdcp;
> struct dcp_dma_desc *desc = >coh->desc[actx->chan];
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + bool key_referenced = actx->key_referenced;
> int ret;
>
> - key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> - 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> - ret = dma_mapping_error(sdcp->dev, key_phys);
> - if (ret)
> - return ret;
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + ret = dma_mapping_error(sdcp->dev, key_phys);
> + if (ret)
> + return ret;
> + }
>
> src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> DCP_BUF_SZ, DMA_TO_DEVICE);
> @@ -255,8 +263,12 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> MXS_DCP_CONTROL0_INTERRUPT |
> MXS_DCP_CONTROL0_ENABLE_CIPHER;
>
> - /* Payload contains the key. */
> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + if (key_referenced)
> + /* Set OTP key bit to select the key via KEY_SELECT. */
> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
> + else
> + /* Payload contains the key. */
> + desc->control0 |=
Re: [PATCH v5 1/6] crypto: mxs-dcp: Add support for hardware-bound keys
On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote: > DCP is capable of performing AES with two hardware-bound keys: > > - The one-time programmable (OTP) key which is burnt via on-chip fuses > - The unique key (UK) which is derived from the OTP key This is somewhat cryptic explanation for the commmon and reoccuring theme of having a fused random seed and a key derivation function. I'd just write it is all about. "DCP is able to derive private keys for a fused random seed, which can be referenced by handle but not accessed by the CPU. These keys can be used to perform AES encryption." My explanation neither includes acronyms OTP and UK and still delivers the message so much better. That actually further makes it better because less crappy standard consortium terminology is always better :-) BR, Jarkko
Re: [PATCH v5 0/6] DCP as trusted keys backend
On Mon Feb 26, 2024 at 12:20 AM EET, Richard Weinberger wrote: > Mimi, James, Jarkko, David, > > you remained silent for a whole release cycle. > Is there anything we can do to get this forward? > > Thanks, > //richard Thanks for reminding. >From my side, I've had pretty busy month as I've adapted to a new work project: https://sochub.fi/ I exported the thread [1] and will look into it within this or next week in detail (thus the large'ish time window). [1] https://lore.kernel.org/linux-integrity/1733761.uacIGzncQW@somecomputer/t.mbox.gz BR, Jarkko
Re: [PATCH v4 2/5] KEYS: trusted: Introduce NXP DCP-backed trusted keys
On Tue Oct 24, 2023 at 7:20 PM EEST, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
> Beside of accelerated crypto operations, it also offers support for
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism similar to what CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software (i.e. the kernel).
>
> The software-based blob format used by DCP trusted keys encrypts
> the payload using AES-128-GCM with a freshly generated random key and nonce.
> The random key itself is AES-128-ECB encrypted using the DCP unique
> or OTP key.
>
> The DCP trusted key blob format is:
> /*
> * struct dcp_blob_fmt - DCP BLOB format.
> *
> * @fmt_version: Format version, currently being %1
> * @blob_key: Random AES 128 key which is used to encrypt @payload,
> *@blob_key itself is encrypted with OTP or UNIQUE device key in
> *AES-128-ECB mode by DCP.
> * @nonce: Random nonce used for @payload encryption.
> * @payload_len: Length of the plain text @payload.
> * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> *
> * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len
> +
> * AES_BLOCK_SIZE.
> */
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
>
> By default the unique key is used. It is also possible to use the
> OTP key. While the unique key should be unique it is not documented how
> this key is derived. Therefore selection the OTP key is supported as
> well via the use_otp_key module parameter.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> include/keys/trusted_dcp.h| 11 +
> security/keys/trusted-keys/Kconfig| 9 +-
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 311 ++
> 5 files changed, 337 insertions(+), 2 deletions(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..9aaa42075b40
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + */
> +
> +#ifndef TRUSTED_DCP_H
> +#define TRUSTED_DCP_H
> +
> +extern struct trusted_key_ops dcp_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Kconfig
> b/security/keys/trusted-keys/Kconfig
> index dbfdd8536468..c6b80b7e5c78 100644
> --- a/security/keys/trusted-keys/Kconfig
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -33,6 +33,13 @@ config TRUSTED_KEYS_CAAM
> Enable use of NXP's Cryptographic Accelerator and Assurance Module
> (CAAM) as trusted key backend.
>
> -if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM
> +config TRUSTED_KEYS_DCP
> + bool "DCP-based trusted keys"
> + depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
> + default y
> + help
> + Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
> +
> +if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE && !TRUSTED_KEYS_CAAM &&
> !TRUSTED_KEYS_DCP
This does not scale tbh.
I'd suggest to add additional patch before adding the new key type,
which clears this up a little bit.
First:
config HAVE_TRUSTED_KEYS
bool
And then following this pattern to all trusted key types:
config TRUSTED_KEYS_DCP
bool "DCP-based trusted keys"
depends on CRYPTO_DEV_MXS_DCP >= TRUSTED_KEYS
default y
select HAVE_TRUSTED_KEYS
help
Enable use of NXP's DCP (Data Co-Processor) as trusted key backend.
And finally:
if !HAVE_TRUSTED_KEYS
comment "No trust source selected!"
endif
BR, Jarkko
Re: [PATCH v4 1/5] crypto: mxs-dcp: Add support for hardware-bound keys
On Tue Oct 24, 2023 at 7:20 PM EEST, David Gstir wrote:
> DCP is capable of performing AES with two hardware-bound keys:
>
> - The one-time programmable (OTP) key which is burnt via on-chip fuses
> - The unique key (UK) which is derived from the OTP key
>
> In addition to the two hardware-bound keys, DCP also supports
> storing keys in 4 dedicated key slots within its secure memory area
> (internal SRAM).
>
> These keys are not stored in main memory and are therefore
> not directly accessible by the operating system. To use them
> for AES operations, a one-byte key reference has to supplied
> with the DCP operation descriptor in the control register.
>
> This adds support for using any of these 6 keys through the crypto API
> via their key reference after they have been set up. The main purpose
> is to add support for DCP-backed trusted keys. Other use cases are
> possible too (see similar existing paes implementations), but these
> should carefully be evaluated as e.g. enabling AF_ALG will give
> userspace full access to use keys. In scenarios with untrustworthy
> userspace, this will enable en-/decryption oracles.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> drivers/crypto/mxs-dcp.c | 104 ++-
> include/soc/fsl/dcp.h| 17 +++
> 2 files changed, 110 insertions(+), 11 deletions(-)
> create mode 100644 include/soc/fsl/dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index f6b7bce0e656..2dc664fb2faf 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolkey_referenced;
> };
>
> struct dcp_aes_req_ctx {
> @@ -155,6 +157,7 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL0_HASH_TERM (1 << 13)
> #define MXS_DCP_CONTROL0_HASH_INIT (1 << 12)
> #define MXS_DCP_CONTROL0_PAYLOAD_KEY (1 << 11)
> +#define MXS_DCP_CONTROL0_OTP_KEY (1 << 10)
> #define MXS_DCP_CONTROL0_CIPHER_ENCRYPT (1 << 8)
> #define MXS_DCP_CONTROL0_CIPHER_INIT (1 << 9)
> #define MXS_DCP_CONTROL0_ENABLE_HASH (1 << 6)
> @@ -168,6 +171,8 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL1_CIPHER_MODE_ECB (0 << 4)
> #define MXS_DCP_CONTROL1_CIPHER_SELECT_AES128(0 << 0)
>
> +#define MXS_DCP_CONTROL1_KEY_SELECT_SHIFT8
> +
> static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
> {
> int dma_err;
> @@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> struct dcp *sdcp = global_sdcp;
> struct dcp_dma_desc *desc = >coh->desc[actx->chan];
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + bool key_referenced = actx->key_referenced;
> int ret;
>
> - key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> - 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> - ret = dma_mapping_error(sdcp->dev, key_phys);
> - if (ret)
> - return ret;
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + ret = dma_mapping_error(sdcp->dev, key_phys);
> + if (ret)
> + return ret;
> + }
>
> src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> DCP_BUF_SZ, DMA_TO_DEVICE);
> @@ -255,8 +263,12 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> MXS_DCP_CONTROL0_INTERRUPT |
> MXS_DCP_CONTROL0_ENABLE_CIPHER;
>
> - /* Payload contains the key. */
> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + if (key_referenced)
> + /* Set OTP key bit to select the key via KEY_SELECT. */
> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
> + else
> + /* Payload contains the key. */
> + desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
>
> if (rctx->enc)
> desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -270,6 +282,9 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> else
> desc->control1 |= MXS_DCP_CONTROL1_CIPHER_MODE_CBC;
>
> + if (key_referenced)
> + desc->control1 |= sdcp->coh->aes_key[0] <<
> MXS_DCP_CONTROL1_KEY_SELECT_SHIFT;
> +
> desc->next_cmd_addr = 0;
> desc->source = src_phys;
> desc->destination = dst_phys;
> @@ -284,9 +299,9 @@ static int
Re: [PATCH v3 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Wed Sep 27, 2023 at 9:25 AM EEST, David Gstir wrote: > Jarkko, > > > On 25.09.2023, at 17:22, Jarkko Sakkinen wrote: > > > > On Mon Sep 18, 2023 at 5:18 PM EEST, David Gstir wrote: > >> DCP is capable to performing AES with hardware-bound keys. > >> These keys are not stored in main memory and are therefore not directly > >> accessible by the operating system. > >> > >> So instead of feeding the key into DCP, we need to place a > >> reference to such a key before initiating the crypto operation. > >> Keys are referenced by a one byte identifiers. > > > > Not sure what the action of feeding key into DCP even means if such > > action does not exists. > > > > What you probably would want to describe here is how keys get created > > and how they are referenced by the kernel. > > > > For the "use" part please try to avoid academic paper style long > > expression starting with "we" pronomine. > > > > So the above paragraph would normalize into "The keys inside DCP > > are referenced by one byte identifier". Here of course would be > > for the context nice to know what is this set of DCP keys. E.g. > > are total 256 keys or some subset? > > > > When using too much prose there can be surprsingly little digestable > > information, thus this nitpicking. > > Thanks for reviewing that in detail! I’ll rephrase the commit > messages on all patches to get rid of the academic paper style. > > > > > >> DCP supports 6 different keys: 4 slots in the secure memory area, > >> a one time programmable key which can be burnt via on-chip fuses > >> and an unique device key. > >> > >> Using these keys is restricted to in-kernel users that use them as building > >> block for other crypto tools such as trusted keys. Allowing userspace > >> (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security > >> risk, because there is no access control mechanism. > > > > Unless this patch has anything else than trusted keys this should not > > be an open-ended sentence. You want to say roughly that DCP hardware > > keys are implemented for the sake to implement trusted keys support, > > and exactly and only that. > > > > This description also lacks actions taken by the code changes below, > > which is really the beef of any commit description. > > You’re right. I’ll add that. Yup, I'm just doing my part of the job, as I'm expected to do it :-) Thanks for understanding. > Thanks, > - David BR, Jarkko
Re: [PATCH v3 3/3] doc: trusted-encrypted: add DCP as new trust source
On Mon Sep 18, 2023 at 5:18 PM EEST, David Gstir wrote: > Update the documentation for trusted and encrypted KEYS with DCP as new > trust source: > > - Describe security properties of DCP trust source > - Describe key usage > - Document blob format > > Co-developed-by: Richard Weinberger > Signed-off-by: Richard Weinberger > Co-developed-by: David Oberhollenzer > Signed-off-by: David Oberhollenzer > Signed-off-by: David Gstir > --- > .../security/keys/trusted-encrypted.rst | 85 +++ > 1 file changed, 85 insertions(+) > > diff --git a/Documentation/security/keys/trusted-encrypted.rst > b/Documentation/security/keys/trusted-encrypted.rst > index 9bc9db8ec651..4452070afbe9 100644 > --- a/Documentation/security/keys/trusted-encrypted.rst > +++ b/Documentation/security/keys/trusted-encrypted.rst > @@ -42,6 +42,14 @@ safe. > randomly generated and fused into each SoC at manufacturing time. > Otherwise, a common fixed test key is used instead. > > + (4) DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > + > + Rooted to a one-time programmable key (OTP) that is generally burnt > + in the on-chip fuses and is accessible to the DCP encryption engine > only. > + DCP provides two keys that can be used as root of trust: the OTP key > + and the UNIQUE key. Default is to use the UNIQUE key, but selecting > + the OTP key can be done via a module parameter (dcp_use_otp_key). > + >* Execution isolation > > (1) TPM > @@ -57,6 +65,12 @@ safe. > > Fixed set of operations running in isolated execution environment. > > + (4) DCP > + > + Fixed set of cryptographic operations running in isolated execution > + environment. Only basic blob key encryption is executed there. > + The actual key sealing/unsealing is done on main processor/kernel > space. > + >* Optional binding to platform integrity state > > (1) TPM > @@ -79,6 +93,11 @@ safe. > Relies on the High Assurance Boot (HAB) mechanism of NXP SoCs > for platform integrity. > > + (4) DCP > + > + Relies on Secure/Trusted boot process (called HAB by vendor) for > + platform integrity. > + >* Interfaces and APIs > > (1) TPM > @@ -94,6 +113,11 @@ safe. > > Interface is specific to silicon vendor. > > + (4) DCP > + > + Vendor-specific API that is implemented as part of the DCP crypto > driver in > + ``drivers/crypto/mxs-dcp.c``. > + >* Threat model > > The strength and appropriateness of a particular trust source for a > given > @@ -129,6 +153,13 @@ selected trust source: > CAAM HWRNG, enable CRYPTO_DEV_FSL_CAAM_RNG_API and ensure the device > is probed. > > + * DCP (Data Co-Processor: crypto accelerator of various i.MX SoCs) > + > + The DCP hardware device itself does not provide a dedicated RNG > interface, > + so the kernel default RNG is used. SoCs with DCP like the i.MX6ULL do > have > + a dedicated hardware RNG that is independent from DCP which can be > enabled > + to back the kernel RNG. > + > Users may override this by specifying ``trusted.rng=kernel`` on the kernel > command-line to override the used RNG with the kernel's random number pool. > > @@ -231,6 +262,19 @@ Usage:: > CAAM-specific format. The key length for new keys is always in bytes. > Trusted Keys can be 32 - 128 bytes (256 - 1024 bits). > > +Trusted Keys usage: DCP > +--- > + > +Usage:: > + > +keyctl add trusted name "new keylen" ring > +keyctl add trusted name "load hex_blob" ring > +keyctl print keyid > + > +"keyctl print" returns an ASCII hex copy of the sealed key, which is in > format > +specific to this DCP key-blob implementation. The key length for new keys is > +always in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits). > + > Encrypted Keys usage > > > @@ -426,3 +470,44 @@ string length. > privkey is the binary representation of TPM2B_PUBLIC excluding the > initial TPM2B header which can be reconstructed from the ASN.1 octed > string length. > + > +DCP Blob Format > +--- > + > +The Data Co-Processor (DCP) provides hardware-bound AES keys using its > +AES encryption engine only. It does not provide direct key sealing/unsealing. > +To make DCP hardware encryption keys usable as trust source, we define > +our own custom format that uses a hardware-bound key to secure the sealing > +key stored in the key blob. > + > +Whenever a new trusted key using DCP is generated, we generate a random > 128-bit > +blob encryption key (BEK) and 128-bit nonce. The BEK and nonce are used to > +encrypt the trusted key payload using AES-128-GCM. "When a new trusted key using DCP is created, a random 128-bit blob encryption key (BEK) and 128-bit nonce are generated." ... or along the lines. BR, Jarkko
Re: [PATCH v3 2/3] KEYS: trusted: Introduce support for NXP DCP-based trusted keys
On Mon Sep 18, 2023 at 5:18 PM EEST, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
>
> Beside of accelerated crypto operations, it also offers support for
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism just like CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software.
>
> We chose the following format for the blob:
Who is we?
And there is no choosing anything if the below structure if hardware
defined (not software defined):
> /*
> * struct dcp_blob_fmt - DCP BLOB format.
> *
> * @fmt_version: Format version, currently being %1
> * @blob_key: Random AES 128 key which is used to encrypt @payload,
> *@blob_key itself is encrypted with OTP or UNIQUE device key in
> *AES-128-ECB mode by DCP.
> * @nonce: Random nonce used for @payload encryption.
> * @payload_len: Length of the plain text @payload.
> * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> *
> * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len
> +
> * AES_BLOCK_SIZE.
> */
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
>
> @payload is the key provided by trusted_key_ops->seal().
>
> By default the UNIQUE device key is used, it is also possible to use
> the OTP key. While the UNIQUE device key should be unique it is not
> entirely clear whether this is the case due to unclear documentation.
> If someone wants to be sure they can burn their own unique key
> into the OTP fuse and set the use_otp_key module parameter.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> .../admin-guide/kernel-parameters.txt | 13 +
Separate commit for this.
> MAINTAINERS | 9 +
Ditto (i.e. total two additional patches).
> include/keys/trusted_dcp.h| 11 +
> security/keys/trusted-keys/Kconfig| 9 +-
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 311 ++
> 7 files changed, 359 insertions(+), 2 deletions(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..c11eda8b38e0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6566,6 +6566,7 @@
> - "tpm"
> - "tee"
> - "caam"
> + - "dcp"
> If not specified then it defaults to iterating through
> the trust source list starting with TPM and assigns the
> first trust source as a backend which is initialized
> @@ -6581,6 +6582,18 @@
> If not specified, "default" is used. In this case,
> the RNG's choice is left to each individual trust
> source.
>
> + trusted.dcp_use_otp_key
> + This is intended to be used in combination with
> + trusted.source=dcp and will select the DCP OTP key
> + instead of the DCP UNIQUE key blob encryption.
> +
> + trusted.dcp_skip_zk_test
> + This is intended to be used in combination with
> + trusted.source=dcp and will disable the check if all
> + the blob key is zero'ed. This is helpful for situations
> where
> + having this key zero'ed is acceptable. E.g. in testing
> + scenarios.
> +
> tsc=Disable clocksource stability checks for TSC.
> Format:
> [x86] reliable: mark tsc clocksource as reliable, this
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..988d01226131 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11647,6 +11647,15 @@ S: Maintained
> F: include/keys/trusted_caam.h
> F: security/keys/trusted-keys/trusted_caam.c
>
> +KEYS-TRUSTED-DCP
> +M: David Gstir
> +R: sigma star Kernel Team
> +L: linux-integr...@vger.kernel.org
> +L: keyri...@vger.kernel.org
> +S: Supported
> +F: include/keys/trusted_dcp.h
> +F: security/keys/trusted-keys/trusted_dcp.c
> +
> KEYS-TRUSTED-TEE
> M: Sumit Garg
> L: linux-integr...@vger.kernel.org
> diff --git a/include/keys/trusted_dcp.h
Re: [PATCH v3 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Mon Sep 18, 2023 at 5:18 PM EEST, David Gstir wrote: > DCP is capable to performing AES with hardware-bound keys. > These keys are not stored in main memory and are therefore not directly > accessible by the operating system. > > So instead of feeding the key into DCP, we need to place a > reference to such a key before initiating the crypto operation. > Keys are referenced by a one byte identifiers. Not sure what the action of feeding key into DCP even means if such action does not exists. What you probably would want to describe here is how keys get created and how they are referenced by the kernel. For the "use" part please try to avoid academic paper style long expression starting with "we" pronomine. So the above paragraph would normalize into "The keys inside DCP are referenced by one byte identifier". Here of course would be for the context nice to know what is this set of DCP keys. E.g. are total 256 keys or some subset? When using too much prose there can be surprsingly little digestable information, thus this nitpicking. > DCP supports 6 different keys: 4 slots in the secure memory area, > a one time programmable key which can be burnt via on-chip fuses > and an unique device key. > > Using these keys is restricted to in-kernel users that use them as building > block for other crypto tools such as trusted keys. Allowing userspace > (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security > risk, because there is no access control mechanism. Unless this patch has anything else than trusted keys this should not be an open-ended sentence. You want to say roughly that DCP hardware keys are implemented for the sake to implement trusted keys support, and exactly and only that. This description also lacks actions taken by the code changes below, which is really the beef of any commit description. BR, Jarkko
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue Sep 12, 2023 at 10:22 PM EEST, Mimi Zohar wrote: > On Tue, 2023-09-12 at 12:49 +0300, Jarkko Sakkinen wrote: > > On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > > > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > > > Adding more CC's from the original patch, looks like get_maintainers > > > > > is > > > > > not that great for this file. > > > > > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > > > > > This is policy that should be decided by the administrator, not > > > > > > Kconfig > > > > > > dependencies. > > > > > > > > We certainly agree that flexibility is important. However, in this case, > > > > this also implies that we are expecting system admins to be security > > > > experts. As per our understanding, CA based infrastructure(PKI) is the > > > > standard to be followed and not the policy decision. And we can only > > > > speak > > > > for Power. > > > > > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed leaf > > > > certs. > > > > > > And that's the problem. > > > > > > From a distribution point of view there are two types of leaf certs: > > > > > > - leaf certs signed by the distribution CA which need not be imported > > >because the distribution CA cert is enrolled one way or another > > > - user generated ad-hoc certificates that are not signed in any way, > > >and enrolled by the user > > > > > > The latter are vouched for by the user by enrolling the certificate, and > > > confirming that they really want to trust this certificate. Enrolling > > > user certificates is vital for usability or secure boot. Adding extra > > > step of creating a CA certificate stored on the same system only > > > complicates things with no added benefit. > > > > This all comes down to the generic fact that kernel should not > > proactively define what it *expects* sysadmins. > > > > CA based infrastructure like anything is a policy decision not > > a decision to be enforced by kernel. > > Secure boot requires a signature chain of trust. IMA extends the > secure and trusted boot concepts to the kernel. Missing from that > signature chain of trust is the ability of allowing the end > machine/system owner to load other certificates without recompiling the > kernel. The introduction of the machine keyring was to address this. > > I'm not questioning the end user's intent on loading local or third > party keys via the normal mechanisms. If the existing mechanism(s) for > loading local or third party keys were full-proof, then loading a > single certificate, self-signed or not, would be fine. However, that > isn't the reality. The security of the two-stage approach is simply > not equivalent to loading a single certificate. Documentation could > help the end user/system owner to safely create (and manage) separate > certificate signing and code signing certs. > > Unlike UEFI based systems, PowerVM defines two variables trustedcadb > and moduledb, for storing certificate signing and code signing > certificates respectively. First the certs on the trustedcadb are > loaded and then the ones on moduledb are loaded. There's pragmatic reasons to make things more open than they should be in production. As a hardware example I still possess Raspberry Pi 3B for test workloads because it has a broken TZ implementation. The world is really bigger than production workloads. It would be better to document what you said rather than enforce the right choice IMHO (e.g. extend Kconfig documentation). BR, Jarkko
Re: [PATCH v2 2/3] KEYS: trusted: Introduce support for NXP DCP-based trusted keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
> DCP (Data Co-Processor) is the little brother of NXP's CAAM IP.
>
> Beside of accelerated crypto operations, it also offers support for
> hardware-bound keys. Using this feature it is possible to implement a blob
> mechanism just like CAAM offers. Unlike on CAAM, constructing and
> parsing the blob has to happen in software.
>
> We chose the following format for the blob:
> /*
> * struct dcp_blob_fmt - DCP BLOB format.
> *
> * @fmt_version: Format version, currently being %1
> * @blob_key: Random AES 128 key which is used to encrypt @payload,
> *@blob_key itself is encrypted with OTP or UNIQUE device key in
> *AES-128-ECB mode by DCP.
> * @nonce: Random nonce used for @payload encryption.
> * @payload_len: Length of the plain text @payload.
> * @payload: The payload itself, encrypted using AES-128-GCM and @blob_key,
> * GCM auth tag of size AES_BLOCK_SIZE is attached at the end of it.
> *
> * The total size of a DCP BLOB is sizeof(struct dcp_blob_fmt) + @payload_len
> +
> * AES_BLOCK_SIZE.
> */
> struct dcp_blob_fmt {
> __u8 fmt_version;
> __u8 blob_key[AES_KEYSIZE_128];
> __u8 nonce[AES_KEYSIZE_128];
> __le32 payload_len;
> __u8 payload[];
> } __packed;
>
> @payload is the key provided by trusted_key_ops->seal().
>
> By default the UNIQUE device key is used, it is also possible to use
> the OTP key. While the UNIQUE device key should be unique it is not
> entirely clear whether this is the case due to unclear documentation.
> If someone wants to be sure they can burn their own unique key
> into the OTP fuse and set the use_otp_key module parameter.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> .../admin-guide/kernel-parameters.txt | 13 +
> MAINTAINERS | 9 +
> include/keys/trusted_dcp.h| 13 +
> security/keys/trusted-keys/Kconfig| 9 +-
> security/keys/trusted-keys/Makefile | 2 +
> security/keys/trusted-keys/trusted_core.c | 6 +-
> security/keys/trusted-keys/trusted_dcp.c | 313 ++
> 7 files changed, 363 insertions(+), 2 deletions(-)
> create mode 100644 include/keys/trusted_dcp.h
> create mode 100644 security/keys/trusted-keys/trusted_dcp.c
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..c11eda8b38e0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6566,6 +6566,7 @@
> - "tpm"
> - "tee"
> - "caam"
> + - "dcp"
> If not specified then it defaults to iterating through
> the trust source list starting with TPM and assigns the
> first trust source as a backend which is initialized
> @@ -6581,6 +6582,18 @@
> If not specified, "default" is used. In this case,
> the RNG's choice is left to each individual trust
> source.
>
> + trusted.dcp_use_otp_key
> + This is intended to be used in combination with
> + trusted.source=dcp and will select the DCP OTP key
> + instead of the DCP UNIQUE key blob encryption.
> +
> + trusted.dcp_skip_zk_test
> + This is intended to be used in combination with
> + trusted.source=dcp and will disable the check if all
> + the blob key is zero'ed. This is helpful for situations
> where
> + having this key zero'ed is acceptable. E.g. in testing
> + scenarios.
> +
> tsc=Disable clocksource stability checks for TSC.
> Format:
> [x86] reliable: mark tsc clocksource as reliable, this
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..988d01226131 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11647,6 +11647,15 @@ S: Maintained
> F: include/keys/trusted_caam.h
> F: security/keys/trusted-keys/trusted_caam.c
>
> +KEYS-TRUSTED-DCP
> +M: David Gstir
> +R: sigma star Kernel Team
> +L: linux-integr...@vger.kernel.org
> +L: keyri...@vger.kernel.org
> +S: Supported
> +F: include/keys/trusted_dcp.h
> +F: security/keys/trusted-keys/trusted_dcp.c
> +
> KEYS-TRUSTED-TEE
> M: Sumit Garg
> L: linux-integr...@vger.kernel.org
> diff --git a/include/keys/trusted_dcp.h b/include/keys/trusted_dcp.h
> new file mode 100644
> index ..7b2a1275c527
> --- /dev/null
> +++ b/include/keys/trusted_dcp.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolrefkey;
> };
s/refkey/key_referenced/
No reason to obfuscate it, especially since there is no supporting
documentation.
BR, Jarkko
Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
> DCP is capable to performing AES with hardware-bound keys.
> These keys are not stored in main memory and are therefore not directly
> accessible by the operating system.
>
> So instead of feeding the key into DCP, we need to place a
> reference to such a key before initiating the crypto operation.
> Keys are referenced by a one byte identifiers.
>
> DCP supports 6 different keys: 4 slots in the secure memory area,
> a one time programmable key which can be burnt via on-chip fuses
> and an unique device key.
>
> Using these keys is restricted to in-kernel users that use them as building
> block for other crypto tools such as trusted keys. Allowing userspace
> (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security
> risk, because there is no access control mechanism.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> drivers/crypto/mxs-dcp.c | 107 +++
> include/soc/fsl/dcp.h| 19 +++
> 2 files changed, 115 insertions(+), 11 deletions(-)
> create mode 100644 include/soc/fsl/dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index f6b7bce0e656..d525cb41f2ca 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolrefkey;
> };
>
> struct dcp_aes_req_ctx {
> @@ -155,6 +157,7 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL0_HASH_TERM (1 << 13)
> #define MXS_DCP_CONTROL0_HASH_INIT (1 << 12)
> #define MXS_DCP_CONTROL0_PAYLOAD_KEY (1 << 11)
> +#define MXS_DCP_CONTROL0_OTP_KEY (1 << 10)
> #define MXS_DCP_CONTROL0_CIPHER_ENCRYPT (1 << 8)
> #define MXS_DCP_CONTROL0_CIPHER_INIT (1 << 9)
> #define MXS_DCP_CONTROL0_ENABLE_HASH (1 << 6)
> @@ -168,6 +171,8 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL1_CIPHER_MODE_ECB (0 << 4)
> #define MXS_DCP_CONTROL1_CIPHER_SELECT_AES128(0 << 0)
>
> +#define MXS_DCP_CONTROL1_KEY_SELECT_SHIFT8
> +
> static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
> {
> int dma_err;
> @@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> struct dcp *sdcp = global_sdcp;
> struct dcp_dma_desc *desc = >coh->desc[actx->chan];
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + bool key_referenced = actx->refkey;
> int ret;
>
> - key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> - 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> - ret = dma_mapping_error(sdcp->dev, key_phys);
> - if (ret)
> - return ret;
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + ret = dma_mapping_error(sdcp->dev, key_phys);
> + if (ret)
> + return ret;
> + }
>
> src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> DCP_BUF_SZ, DMA_TO_DEVICE);
> @@ -255,8 +263,13 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> MXS_DCP_CONTROL0_INTERRUPT |
> MXS_DCP_CONTROL0_ENABLE_CIPHER;
>
> - /* Payload contains the key. */
> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + if (key_referenced) {
> + /* Set OTP key bit to select the key via KEY_SELECT. */
> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
> + } else {
> + /* Payload contains the key. */
> + desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + }
Remove curly braces (coding style).
>
> if (rctx->enc)
> desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -270,6 +283,9 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> else
> desc->control1 |= MXS_DCP_CONTROL1_CIPHER_MODE_CBC;
>
> + if (key_referenced)
> + desc->control1 |= sdcp->coh->aes_key[0] <<
> MXS_DCP_CONTROL1_KEY_SELECT_SHIFT;
> +
> desc->next_cmd_addr = 0;
> desc->source = src_phys;
> desc->destination = dst_phys;
> @@ -284,9 +300,10 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> err_dst:
> dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE);
> err_src:
> - dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> -
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue Sep 12, 2023 at 6:39 AM EEST, Nayna wrote: > > On 9/7/23 13:32, Michal Suchánek wrote: > > Adding more CC's from the original patch, looks like get_maintainers is > > not that great for this file. > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > >> No other platform needs CA_MACHINE_KEYRING, either. > >> > >> This is policy that should be decided by the administrator, not Kconfig > >> dependencies. > > We certainly agree that flexibility is important. However, in this case, > this also implies that we are expecting system admins to be security > experts. As per our understanding, CA based infrastructure(PKI) is the > standard to be followed and not the policy decision. And we can only > speak for Power. In the end this is dictating policy for no compelling reason, and that is the bottom line here, not playing a mind game what type of expertise a sysadmin might or might not have. BR, Jarkko
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > Adding more CC's from the original patch, looks like get_maintainers is > > > not that great for this file. > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > This is policy that should be decided by the administrator, not Kconfig > > > > dependencies. > > > > We certainly agree that flexibility is important. However, in this case, > > this also implies that we are expecting system admins to be security > > experts. As per our understanding, CA based infrastructure(PKI) is the > > standard to be followed and not the policy decision. And we can only speak > > for Power. > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed leaf > > certs. > > And that's the problem. > > From a distribution point of view there are two types of leaf certs: > > - leaf certs signed by the distribution CA which need not be imported >because the distribution CA cert is enrolled one way or another > - user generated ad-hoc certificates that are not signed in any way, >and enrolled by the user > > The latter are vouched for by the user by enrolling the certificate, and > confirming that they really want to trust this certificate. Enrolling > user certificates is vital for usability or secure boot. Adding extra > step of creating a CA certificate stored on the same system only > complicates things with no added benefit. This all comes down to the generic fact that kernel should not proactively define what it *expects* sysadmins. CA based infrastructure like anything is a policy decision not a decision to be enforced by kernel. BR, Jarkko
Re: [PATCH v5 0/3 RESEND] sed-opal: keyrings, discovery, revert, key store
On Mon Aug 21, 2023 at 6:26 PM EEST, Greg Joyce wrote: > On Wed, 2023-08-16 at 23:41 +0300, Jarkko Sakkinen wrote: > > On Wed Aug 16, 2023 at 10:45 PM EEST, Greg Joyce wrote: > > > It's been almost 4 weeks since the last resend and there haven't > > > been > > > any comments. Is there anything that needs to be changed for > > > acceptance? > > > > > > Thanks for your input. > > > > > > Greg > > > > > > On Fri, 2023-07-21 at 16:15 -0500, gjo...@linux.vnet.ibm.com wrote: > > > > From: Greg Joyce > > > > > > > > This patchset has gone through numerous rounds of review and > > > > all comments/suggetions have been addressed. The reviews have > > > > covered all relevant areas including reviews by block and keyring > > > > developers as well as the SED Opal maintainer. The last > > > > patchset submission has not solicited any responses in the > > > > six weeks since it was last distributed. The changes are > > > > generally useful and ready for inclusion. > > > > > > > > TCG SED Opal is a specification from The Trusted Computing Group > > > > that allows self encrypting storage devices (SED) to be locked at > > > > power on and require an authentication key to unlock the drive. > > > > > > > > The current SED Opal implementation in the block driver > > > > requires that authentication keys be provided in an ioctl > > > > so that they can be presented to the underlying SED > > > > capable drive. Currently, the key is typically entered by > > > > a user with an application like sedutil or sedcli. While > > > > this process works, it does not lend itself to automation > > > > like unlock by a udev rule. > > > > > > > > The SED block driver has been extended so it can alternatively > > > > obtain a key from a sed-opal kernel keyring. The SED ioctls > > > > will indicate the source of the key, either directly in the > > > > ioctl data or from the keyring. > > > > > > > > Two new SED ioctls have also been added. These are: > > > > 1) IOC_OPAL_REVERT_LSP to revert LSP state > > > > 2) IOC_OPAL_DISCOVERY to discover drive capabilities/state > > > > > > > > change log v5: > > > > - rebase to for-6.5/block > > > > > > > > change log v4: > > > > - rebase to 6.3-rc7 > > > > - replaced "255" magic number with U8_MAX > > > > > > > > change log: > > > > - rebase to 6.x > > > > - added latest reviews > > > > - removed platform functions for persistent key storage > > > > - replaced key update logic with key_create_or_update() > > > > - minor bracing and padding changes > > > > - add error returns > > > > - opal_key structure is application provided but kernel > > > > verified > > > > - added brief description of TCG SED Opal > > > > > > > > > > > > Greg Joyce (3): > > > > block: sed-opal: Implement IOC_OPAL_DISCOVERY > > > > block: sed-opal: Implement IOC_OPAL_REVERT_LSP > > > > block: sed-opal: keyring support for SED keys > > > > > > > > block/Kconfig | 2 + > > > > block/opal_proto.h| 4 + > > > > block/sed-opal.c | 252 > > > > +- > > > > include/linux/sed-opal.h | 5 + > > > > include/uapi/linux/sed-opal.h | 25 +++- > > > > 5 files changed, 282 insertions(+), 6 deletions(-) > > > > > > > > > > > > base-commit: 1341c7d2ccf42ed91aea80b8579d35bc1ea381e2 > > > > I can give because it looks good to me to all patches: > > > > Acked-by: Jarkko Sakkinen > > > > ... but should not probably go to my tree. > > > > BR, Jarkko > > Thanks for the ack Jarkko. Any thoughts on which tree it should go to? I get from "scripts/get_maintainer.pl block/sed-opal.c | wl-copy" Jonathan Derrick (supporter:SECURE ENCRYPTING DEVICE (SED) OPAL DRIVER) Jens Axboe (maintainer:BLOCK LAYER) linux-bl...@vger.kernel.org (open list:SECURE ENCRYPTING DEVICE (SED) OPAL DRIVER) linux-ker...@vger.kernel.org (open list) You should probably add the corresponding maintainers and linux-block to the loop. I suggest to send a new version of the patch set with my ack's added to the patches. BR, Jarkko
Re: [PATCH v4 6/6] integrity: PowerVM support for loading third party code signing keys
On Thu Aug 17, 2023 at 12:06 AM EEST, Mimi Zohar wrote:
> On Wed, 2023-08-16 at 23:36 +0300, Jarkko Sakkinen wrote:
> > On Tue Aug 15, 2023 at 2:27 PM EEST, Nayna Jain wrote:
> > > On secure boot enabled PowerVM LPAR, third party code signing keys are
> > > needed during early boot to verify signed third party modules. These
> > > third party keys are stored in moduledb object in the Platform
> > > KeyStore (PKS).
> > >
> > > Load third party code signing keys onto .secondary_trusted_keys keyring.
> > >
> > > Signed-off-by: Nayna Jain
> > > ---
> > > certs/system_keyring.c| 30 +++
> > > include/keys/system_keyring.h | 4 +++
> > > .../platform_certs/keyring_handler.c | 8 +
> > > .../platform_certs/keyring_handler.h | 5
> > > .../integrity/platform_certs/load_powerpc.c | 17 +++
> > > 5 files changed, 64 insertions(+)
> > >
> > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > index b348e0898d34..33841c91f12c 100644
> > > --- a/certs/system_keyring.c
> > > +++ b/certs/system_keyring.c
> > > @@ -152,6 +152,36 @@ static __init struct key_restriction
> > > *get_builtin_and_secondary_restriction(void
> > >
> > > return restriction;
> > > }
> > > +
> > > +/**
> > > + * add_to_secondary_keyring - Add to secondary keyring.
> > > + * @source: Source of key
> > > + * @data: The blob holding the key
> > > + * @len: The length of the data blob
> > > + *
> > > + * Add a key to the secondary keyring. The key must be vouched for by a
> > > key in the builtin,
> > > + * machine or secondary keyring itself.
> > > + */
> > > +void __init add_to_secondary_keyring(const char *source, const void
> > > *data, size_t len)
> > > +{
> > > + key_ref_t key;
> > > + key_perm_t perm;
> > > +
> > > + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> > > +
> > > + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> > > +"asymmetric",
> > > +NULL, data, len, perm,
> > > +KEY_ALLOC_NOT_IN_QUOTA);
> > > + if (IS_ERR(key)) {
> > > + pr_err("Problem loading X.509 certificate from %s to secondary
> > > keyring %ld\n",
> > > +source, PTR_ERR(key));
> > > + return;
> > > + }
> > > +
> > > + pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
> > > + key_ref_put(key);
> > > +}
> > > #endif
> > > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> > > void __init set_machine_trusted_keys(struct key *keyring)
> > > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> > > index 7e2583208820..8365adf842ef 100644
> > > --- a/include/keys/system_keyring.h
> > > +++ b/include/keys/system_keyring.h
> > > @@ -50,9 +50,13 @@ int
> > > restrict_link_by_digsig_builtin_and_secondary(struct key *keyring,
> > > const struct key_type *type,
> > > const union key_payload
> > > *payload,
> > > struct key *restriction_key);
> > > +void __init add_to_secondary_keyring(const char *source, const void
> > > *data, size_t len);
> > > #else
> > > #define restrict_link_by_builtin_and_secondary_trusted
> > > restrict_link_by_builtin_trusted
> > > #define restrict_link_by_digsig_builtin_and_secondary
> > > restrict_link_by_digsig_builtin
> > > +static inline void __init add_to_secondary_keyring(const char *source,
> > > const void *data, size_t len)
> > > +{
> > > +}
> > > #endif
> > >
> > > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> > > diff --git a/security/integrity/platform_certs/keyring_handler.c
> > > b/security/integrity/platform_certs/keyring_handler.c
> > > index 586027b9a3f5..13ea17207902 100644
> > > --- a/security/integrity/platform_certs/keyring_handler.c
> > > +++ b/security/integrity/platform_certs/keyring_handler.c
> > > @@ -78,6 +78,14 @@ __init efi_element_handler_t
> > > get_handler_for_ca_keys(const efi_guid_t
Re: [PATCH v3 2/6] integrity: ignore keys failing CA restrictions on non-UEFI platform
On Wed Aug 16, 2023 at 3:58 PM EEST, Mimi Zohar wrote:
> On Mon, 2023-08-14 at 20:38 +0300, Jarkko Sakkinen wrote:
> > On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> > > On non-UEFI platforms, handle restrict_link_by_ca failures differently.
> > >
> > > Certificates which do not satisfy CA restrictions on non-UEFI platforms
> > > are ignored.
> > >
> > > Signed-off-by: Nayna Jain
> > > Reviewed-and-tested-by: Mimi Zohar
> > > ---
> > > security/integrity/platform_certs/machine_keyring.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/security/integrity/platform_certs/machine_keyring.c
> > > b/security/integrity/platform_certs/machine_keyring.c
> > > index 7aaed7950b6e..389a6e7c9245 100644
> > > --- a/security/integrity/platform_certs/machine_keyring.c
> > > +++ b/security/integrity/platform_certs/machine_keyring.c
> > > @@ -36,7 +36,7 @@ void __init add_to_machine_keyring(const char *source,
> > > const void *data, size_t
> > >* If the restriction check does not pass and the platform keyring
> > >* is configured, try to add it into that keyring instead.
> > >*/
> > > - if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > > + if (rc && efi_enabled(EFI_BOOT) &&
> > > IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
> > > rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
> > >data, len, perm);
> > >
> > > --
> > > 2.31.1
> >
> > Acked-by: Jarkko Sakkinen
>
> Hi Jarkko,
>
> Without the following two commits in your master branch, the last patch
> in this series "[PATCH v4 6/6] integrity: PowerVM support for loading
> third party code signing keys" doesn't apply cleanly.
>
> - commit 409b465f8a83 ("integrity: Enforce digitalSignature usage in
> the ima and evm keyrings")
> - commit e34a6c7dd192 ("KEYS: DigitalSignature link restriction")
>
> If you're not planning on upstreaming this patch set, I'd appreciate
> your creating a topic branch with these two commits.
They reside now in my -next. I'll send PR for the next release Fri.
> --
> thanks,
>
> Mimi
BR, Jarkko
Re: [PATCH v5 0/3 RESEND] sed-opal: keyrings, discovery, revert, key store
On Wed Aug 16, 2023 at 10:45 PM EEST, Greg Joyce wrote: > It's been almost 4 weeks since the last resend and there haven't been > any comments. Is there anything that needs to be changed for > acceptance? > > Thanks for your input. > > Greg > > On Fri, 2023-07-21 at 16:15 -0500, gjo...@linux.vnet.ibm.com wrote: > > From: Greg Joyce > > > > This patchset has gone through numerous rounds of review and > > all comments/suggetions have been addressed. The reviews have > > covered all relevant areas including reviews by block and keyring > > developers as well as the SED Opal maintainer. The last > > patchset submission has not solicited any responses in the > > six weeks since it was last distributed. The changes are > > generally useful and ready for inclusion. > > > > TCG SED Opal is a specification from The Trusted Computing Group > > that allows self encrypting storage devices (SED) to be locked at > > power on and require an authentication key to unlock the drive. > > > > The current SED Opal implementation in the block driver > > requires that authentication keys be provided in an ioctl > > so that they can be presented to the underlying SED > > capable drive. Currently, the key is typically entered by > > a user with an application like sedutil or sedcli. While > > this process works, it does not lend itself to automation > > like unlock by a udev rule. > > > > The SED block driver has been extended so it can alternatively > > obtain a key from a sed-opal kernel keyring. The SED ioctls > > will indicate the source of the key, either directly in the > > ioctl data or from the keyring. > > > > Two new SED ioctls have also been added. These are: > > 1) IOC_OPAL_REVERT_LSP to revert LSP state > > 2) IOC_OPAL_DISCOVERY to discover drive capabilities/state > > > > change log v5: > > - rebase to for-6.5/block > > > > change log v4: > > - rebase to 6.3-rc7 > > - replaced "255" magic number with U8_MAX > > > > change log: > > - rebase to 6.x > > - added latest reviews > > - removed platform functions for persistent key storage > > - replaced key update logic with key_create_or_update() > > - minor bracing and padding changes > > - add error returns > > - opal_key structure is application provided but kernel > > verified > > - added brief description of TCG SED Opal > > > > > > Greg Joyce (3): > > block: sed-opal: Implement IOC_OPAL_DISCOVERY > > block: sed-opal: Implement IOC_OPAL_REVERT_LSP > > block: sed-opal: keyring support for SED keys > > > > block/Kconfig | 2 + > > block/opal_proto.h | 4 + > > block/sed-opal.c | 252 > > +- > > include/linux/sed-opal.h | 5 + > > include/uapi/linux/sed-opal.h | 25 +++- > > 5 files changed, 282 insertions(+), 6 deletions(-) > > > > > > base-commit: 1341c7d2ccf42ed91aea80b8579d35bc1ea381e2 I can give because it looks good to me to all patches: Acked-by: Jarkko Sakkinen ... but should not probably go to my tree. BR, Jarkko
Re: [PATCH v4 6/6] integrity: PowerVM support for loading third party code signing keys
lar signature list types for code signing
> keys.
> + */
> +efi_element_handler_t get_handler_for_code_signing_keys(const efi_guid_t
> *sig_type);
> +
> /*
> * Return the handler for particular signature list types found in the dbx.
> */
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index 339053d9726d..c85febca3343 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -60,6 +60,7 @@ static int __init load_powerpc_certs(void)
> {
> void *db = NULL, *dbx = NULL, *data = NULL;
> void *trustedca;
> + void *moduledb;
> u64 dsize = 0;
> u64 offset = 0;
> int rc = 0;
> @@ -137,6 +138,22 @@ static int __init load_powerpc_certs(void)
> kfree(data);
> }
>
> + data = get_cert_list("moduledb", 9, );
> + if (!data) {
> + pr_info("Couldn't get moduledb list from firmware\n");
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> + pr_err("Error reading moduledb from firmware: %d\n", rc);
> + } else {
> + extract_esl(moduledb, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:moduledb", moduledb,
> dsize,
> +
> get_handler_for_code_signing_keys);
> + if (rc)
> + pr_err("Couldn't parse moduledb signatures: %d\n", rc);
> + kfree(data);
> + }
> +
> return rc;
> }
> late_initcall(load_powerpc_certs);
> --
> 2.31.1
Reviewed-by: Jarkko Sakkinen
I can pick this. My last PR did not went too great partly because of
mess with tpm_tis but now things are calmer.
BR, Jarkko
Re: [PATCH v4 1/6] integrity: PowerVM support for loading CA keys on machine keyring
On Tue Aug 15, 2023 at 2:27 PM EEST, Nayna Jain wrote:
> Keys that derive their trust from an entity such as a security officer,
> administrator, system owner, or machine owner are said to have "imputed
> trust". CA keys with imputed trust can be loaded onto the machine keyring.
> The mechanism for loading these keys onto the machine keyring is platform
> dependent.
>
> Load keys stored in the variable trustedcadb onto the .machine keyring
> on PowerVM platform.
>
> Signed-off-by: Nayna Jain
> Reviewed-and-tested-by: Mimi Zohar
> ---
> .../integrity/platform_certs/keyring_handler.c | 8
> .../integrity/platform_certs/keyring_handler.h | 5 +
> .../integrity/platform_certs/load_powerpc.c | 17 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/security/integrity/platform_certs/keyring_handler.c
> b/security/integrity/platform_certs/keyring_handler.c
> index 8a1124e4d769..1649d047e3b8 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -69,6 +69,14 @@ __init efi_element_handler_t get_handler_for_mok(const
> efi_guid_t *sig_type)
> return NULL;
> }
>
> +__init efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t
> *sig_type)
> +{
> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> + return add_to_machine_keyring;
> +
> + return NULL;
> +}
> +
> /*
> * Return the appropriate handler for particular signature list types found
> in
> * the UEFI dbx and MokListXRT tables.
> diff --git a/security/integrity/platform_certs/keyring_handler.h
> b/security/integrity/platform_certs/keyring_handler.h
> index 212d894a8c0c..6f15bb4cc8dc 100644
> --- a/security/integrity/platform_certs/keyring_handler.h
> +++ b/security/integrity/platform_certs/keyring_handler.h
> @@ -29,6 +29,11 @@ efi_element_handler_t get_handler_for_db(const efi_guid_t
> *sig_type);
> */
> efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
>
> +/*
> + * Return the handler for particular signature list types for CA keys.
> + */
> +efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
> +
> /*
> * Return the handler for particular signature list types found in the dbx.
> */
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index 170789dc63d2..339053d9726d 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -59,6 +59,7 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> static int __init load_powerpc_certs(void)
> {
> void *db = NULL, *dbx = NULL, *data = NULL;
> + void *trustedca;
> u64 dsize = 0;
> u64 offset = 0;
> int rc = 0;
> @@ -120,6 +121,22 @@ static int __init load_powerpc_certs(void)
> kfree(data);
> }
>
> + data = get_cert_list("trustedcadb", 12, );
> + if (!data) {
> + pr_info("Couldn't get trustedcadb list from firmware\n");
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> + pr_err("Error reading trustedcadb from firmware: %d\n", rc);
> + } else {
> + extract_esl(trustedca, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:trustedca", trustedca,
> dsize,
> + get_handler_for_ca_keys);
> + if (rc)
> + pr_err("Couldn't parse trustedcadb signatures: %d\n",
> rc);
> + kfree(data);
> + }
> +
> return rc;
> }
> late_initcall(load_powerpc_certs);
> --
> 2.31.1
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v3 6/6] integrity: PowerVM support for loading third party code signing keys
On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> On secure boot enabled PowerVM LPAR, third party code signing keys are
> needed during early boot to verify signed third party modules. These
> third party keys are stored in moduledb object in the Platform
> KeyStore(PKS).
>
> Load third party code signing keys onto .secondary_trusted_keys keyring.
>
> Signed-off-by: Nayna Jain
> ---
> certs/system_keyring.c| 30 +++
> include/keys/system_keyring.h | 7 +
> security/integrity/integrity.h| 1 +
> .../platform_certs/keyring_handler.c | 8 +
> .../platform_certs/keyring_handler.h | 5
> .../integrity/platform_certs/load_powerpc.c | 18 ++-
> 6 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index b348e0898d34..e458d414918d 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -396,3 +396,33 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
> platform_trusted_keys = keyring;
> }
> #endif
> +
> +/**
> + * add_to_secondary_keyring - Add to secondary keyring.
> + * @source: Source of key
> + * @data: The blob holding the key
> + * @len: The length of the data blob
> + *
> + * Add a key to the secondary keyring. The key must be vouched for by a key
> in the builtin,
> + * machine or secondary keyring itself.
> + */
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> size_t len)
> +{
> + key_ref_t key;
> + key_perm_t perm;
> +
> + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> +
> + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +"asymmetric",
> +NULL, data, len, perm,
> +KEY_ALLOC_NOT_IN_QUOTA);
> + if (IS_ERR(key)) {
> + pr_err("Problem loading X.509 certificate from %s to secondary
> keyring %ld\n",
> +source, PTR_ERR(key));
> + return;
> + }
> +
> + pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
> + key_ref_put(key);
> +}
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 7e2583208820..4188f75d1bac 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -50,9 +50,16 @@ int restrict_link_by_digsig_builtin_and_secondary(struct
> key *keyring,
> const struct key_type *type,
> const union key_payload
> *payload,
> struct key *restriction_key);
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> + size_t len);
> +
> #else
> #define restrict_link_by_builtin_and_secondary_trusted
> restrict_link_by_builtin_trusted
> #define restrict_link_by_digsig_builtin_and_secondary
> restrict_link_by_digsig_builtin
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> + size_t len)
> +{
> +}
> #endif
>
> #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d7553c93f5c0..efaa2eb789ad 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -228,6 +228,7 @@ static inline int __init integrity_load_cert(const
> unsigned int id,
> {
> return 0;
> }
> +
> #endif /* CONFIG_INTEGRITY_SIGNATURE */
>
> #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> diff --git a/security/integrity/platform_certs/keyring_handler.c
> b/security/integrity/platform_certs/keyring_handler.c
> index 586027b9a3f5..13ea17207902 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -78,6 +78,14 @@ __init efi_element_handler_t get_handler_for_ca_keys(const
> efi_guid_t *sig_type)
> return NULL;
> }
>
> +__init efi_element_handler_t get_handler_for_code_signing_keys(const
> efi_guid_t *sig_type)
> +{
> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> + return add_to_secondary_keyring;
> +
> + return NULL;
> +}
> +
> /*
> * Return the appropriate handler for particular signature list types found
> in
> * the UEFI dbx and MokListXRT tables.
> diff --git a/security/integrity/platform_certs/keyring_handler.h
> b/security/integrity/platform_certs/keyring_handler.h
> index 6f15bb4cc8dc..f92895cc50f6 100644
> --- a/security/integrity/platform_certs/keyring_handler.h
> +++ b/security/integrity/platform_certs/keyring_handler.h
> @@ -34,6 +34,11 @@ efi_element_handler_t get_handler_for_mok(const efi_guid_t
> *sig_type);
> */
> efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
>
> +/*
> + *
Re: [PATCH v3 2/6] integrity: ignore keys failing CA restrictions on non-UEFI platform
On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote: > On non-UEFI platforms, handle restrict_link_by_ca failures differently. > > Certificates which do not satisfy CA restrictions on non-UEFI platforms > are ignored. > > Signed-off-by: Nayna Jain > Reviewed-and-tested-by: Mimi Zohar > --- > security/integrity/platform_certs/machine_keyring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/platform_certs/machine_keyring.c > b/security/integrity/platform_certs/machine_keyring.c > index 7aaed7950b6e..389a6e7c9245 100644 > --- a/security/integrity/platform_certs/machine_keyring.c > +++ b/security/integrity/platform_certs/machine_keyring.c > @@ -36,7 +36,7 @@ void __init add_to_machine_keyring(const char *source, > const void *data, size_t >* If the restriction check does not pass and the platform keyring >* is configured, try to add it into that keyring instead. >*/ > - if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) > + if (rc && efi_enabled(EFI_BOOT) && > IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) > rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source, > data, len, perm); > > -- > 2.31.1 Acked-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v3 1/6] integrity: PowerVM support for loading CA keys on machine keyring
On Sun Aug 13, 2023 at 5:15 AM EEST, Nayna Jain wrote:
> Keys that derive their trust from an entity such as a security officer,
> administrator, system owner, or machine owner are said to have "imputed
> trust". CA keys with imputed trust can be loaded onto the machine keyring.
> The mechanism for loading these keys onto the machine keyring is platform
> dependent.
>
> Load keys stored in the variable trustedcadb onto the .machine keyring
> on PowerVM platform.
>
> Signed-off-by: Nayna Jain
> Reviewed-and-tested-by: Mimi Zohar
> ---
> .../integrity/platform_certs/keyring_handler.c | 8
> .../integrity/platform_certs/keyring_handler.h | 5 +
> .../integrity/platform_certs/load_powerpc.c | 17 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/security/integrity/platform_certs/keyring_handler.c
> b/security/integrity/platform_certs/keyring_handler.c
> index 8a1124e4d769..1649d047e3b8 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -69,6 +69,14 @@ __init efi_element_handler_t get_handler_for_mok(const
> efi_guid_t *sig_type)
> return NULL;
> }
>
> +__init efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t
> *sig_type)
> +{
> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> + return add_to_machine_keyring;
> +
> + return NULL;
> +}
> +
> /*
> * Return the appropriate handler for particular signature list types found
> in
> * the UEFI dbx and MokListXRT tables.
> diff --git a/security/integrity/platform_certs/keyring_handler.h
> b/security/integrity/platform_certs/keyring_handler.h
> index 212d894a8c0c..6f15bb4cc8dc 100644
> --- a/security/integrity/platform_certs/keyring_handler.h
> +++ b/security/integrity/platform_certs/keyring_handler.h
> @@ -29,6 +29,11 @@ efi_element_handler_t get_handler_for_db(const efi_guid_t
> *sig_type);
> */
> efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
>
> +/*
> + * Return the handler for particular signature list types for CA keys.
> + */
> +efi_element_handler_t get_handler_for_ca_keys(const efi_guid_t *sig_type);
> +
> /*
> * Return the handler for particular signature list types found in the dbx.
> */
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index 170789dc63d2..6263ce3b3f1e 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -59,6 +59,7 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> static int __init load_powerpc_certs(void)
> {
> void *db = NULL, *dbx = NULL, *data = NULL;
> + void *trustedca = NULL;
Could this be just "void *trustedca;" ?
> u64 dsize = 0;
> u64 offset = 0;
> int rc = 0;
> @@ -120,6 +121,22 @@ static int __init load_powerpc_certs(void)
> kfree(data);
> }
>
> + data = get_cert_list("trustedcadb", 12, );
> + if (!data) {
> + pr_info("Couldn't get trustedcadb list from firmware\n");
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> + pr_err("Error reading trustedcadb from firmware: %d\n", rc);
> + } else {
> + extract_esl(trustedca, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:trustedca", trustedca,
> dsize,
> + get_handler_for_ca_keys);
> + if (rc)
> + pr_err("Couldn't parse trustedcadb signatures: %d\n",
> rc);
> + kfree(data);
> + }
> +
> return rc;
> }
> late_initcall(load_powerpc_certs);
> --
> 2.31.1
BR, Jarkko
Re: [PATCH v2 6/6] integrity: PowerVM support for loading third party code signing keys
On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote:
> On secure boot enabled PowerVM LPAR, third party code signing keys are
> needed during early boot to verify signed third party modules. These
> third party keys are stored in moduledb object in the Platform
> KeyStore(PKS).
^ space
>
> Load third party code signing keys onto .secondary_trusted_keys keyring.
>
> Signed-off-by: Nayna Jain
> ---
> certs/system_keyring.c| 23 +++
> include/keys/system_keyring.h | 7 ++
> security/integrity/integrity.h| 1 +
> .../platform_certs/keyring_handler.c | 8 +++
> .../platform_certs/keyring_handler.h | 5
> .../integrity/platform_certs/load_powerpc.c | 18 ++-
> 6 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index b348e0898d34..3435d4936fb2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -396,3 +396,26 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
> platform_trusted_keys = keyring;
> }
> #endif
> +
spurious newline character
> +void __init add_to_secondary_keyring(const char *source, const void *data,
> + size_t len)
Documentation is lacking, and should be in a single line, as it totals
less than 100 characters.
> +{
> + key_ref_t key;
> + key_perm_t perm; the following structure
> + int rc = 0;
int rc;
> +
> + perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> +
> + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +"asymmetric",
> +NULL, data, len, perm,
> +KEY_ALLOC_NOT_IN_QUOTA);
> + if (IS_ERR(key)) {
> + rc = PTR_ERR(key);
This helper variable is not very useful.
> + pr_err("Problem loading X.509 certificate %d\n", rc);
Why pr_err()? What kind of object is "a problem"?
Also X.509 certificates are everywhere. If these are printed to the
klog, how can e.g. an admin deduce the problem over here?
Even without having these log messages at all I could trace the called
function and be informed that some X.509 cert has an issues. Actually
then I could even deduce the location, thanks to call backtrace.
These have a potential to lead into wrong conclusions.
> + } else {
> + pr_notice("Loaded X.509 cert '%s'\n",
> + key_ref_to_ptr(key)->description);
single line
> + key_ref_put(key);
> + }
I'd suggest instead the following structure:
if (IS_ERR(key)) {
pr_err("Problem loading X.509 certificate %d\n", PTR_ERR(key));
return;
}
pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
key_ref_put(key);
}
BR, Jarkko
Re: [PATCH v2 5/6] integrity: PowerVM machine keyring enablement
On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote: > Update Kconfig to enable machine keyring and limit to CA certificates > on PowerVM. Only key signing CA keys are allowed. > > Signed-off-by: Nayna Jain > Reviewed-and-tested-by: Mimi Zohar > > --- > security/integrity/Kconfig | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig > index ec6e0d789da1..232191ee09e3 100644 > --- a/security/integrity/Kconfig > +++ b/security/integrity/Kconfig > @@ -67,7 +67,9 @@ config INTEGRITY_MACHINE_KEYRING > depends on SECONDARY_TRUSTED_KEYRING > depends on INTEGRITY_ASYMMETRIC_KEYS > depends on SYSTEM_BLACKLIST_KEYRING > - depends on LOAD_UEFI_KEYS > + depends on LOAD_UEFI_KEYS || LOAD_PPC_KEYS > + select INTEGRITY_CA_MACHINE_KEYRING if LOAD_PPC_KEYS > + select INTEGRITY_CA_MACHINE_KEYRING_MAX if LOAD_PPC_KEYS > help >If set, provide a keyring to which Machine Owner Keys (MOK) may >be added. This keyring shall contain just MOK keys. Unlike keys > -- > 2.31.1 Reviewed-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v2 3/6] integrity: remove global variable from machine_keyring.c
On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote:
> trust_mok variable is accessed within a single function locally.
>
> Change trust_mok from global to local static variable.
>
> Signed-off-by: Nayna Jain
> Reviewed-and-tested-by: Mimi Zohar
> ---
> security/integrity/platform_certs/machine_keyring.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/platform_certs/machine_keyring.c
> b/security/integrity/platform_certs/machine_keyring.c
> index 389a6e7c9245..9482e16cb2ca 100644
> --- a/security/integrity/platform_certs/machine_keyring.c
> +++ b/security/integrity/platform_certs/machine_keyring.c
> @@ -8,8 +8,6 @@
> #include
> #include "../integrity.h"
>
> -static bool trust_mok;
> -
> static __init int machine_keyring_init(void)
> {
> int rc;
> @@ -65,9 +63,11 @@ static __init bool uefi_check_trust_mok_keys(void)
> bool __init trust_moklist(void)
> {
> static bool initialized;
> + static bool trust_mok;
>
> if (!initialized) {
> initialized = true;
> + trust_mok = false;
>
> if (uefi_check_trust_mok_keys())
> trust_mok = true;
Nice catch.
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [RFC PATCH v11 01/29] KVM: Wrap kvm_gfn_range.pte in a per-action union
On Wed Jul 19, 2023 at 2:44 AM EEST, Sean Christopherson wrote:
> /* Huge pages aren't expected to be modified without first being
> zapped. */
> - WARN_ON(pte_huge(range->pte) || range->start + 1 != range->end);
> + WARN_ON(pte_huge(range->arg.pte) || range->start + 1 != range->end);
Not familiar with this code. Just checking whether whether instead
pr_{warn,err}() combined with return false would be a more graceful
option?
BR, Jarkko
Re: [PATCH] char: Explicitly include correct DT includes
nclude > #include > diff --git a/drivers/char/hw_random/omap3-rom-rng.c > b/drivers/char/hw_random/omap3-rom-rng.c > index f06e4f95114f..18dc46b1b58e 100644 > --- a/drivers/char/hw_random/omap3-rom-rng.c > +++ b/drivers/char/hw_random/omap3-rom-rng.c > @@ -20,7 +20,6 @@ > #include > #include > #include > -#include > #include > #include > > diff --git a/drivers/char/hw_random/pasemi-rng.c > b/drivers/char/hw_random/pasemi-rng.c > index 2498d4ef9fe2..6959d6edd44c 100644 > --- a/drivers/char/hw_random/pasemi-rng.c > +++ b/drivers/char/hw_random/pasemi-rng.c > @@ -9,11 +9,10 @@ > > #include > #include > +#include > #include > #include > #include > -#include > -#include > #include > > #define SDCRNG_CTL_REG 0x00 > diff --git a/drivers/char/hw_random/pic32-rng.c > b/drivers/char/hw_random/pic32-rng.c > index 99c8bd0859a1..728b68b1a496 100644 > --- a/drivers/char/hw_random/pic32-rng.c > +++ b/drivers/char/hw_random/pic32-rng.c > @@ -12,9 +12,8 @@ > #include > #include > #include > +#include > #include > -#include > -#include > #include > #include > > diff --git a/drivers/char/hw_random/stm32-rng.c > b/drivers/char/hw_random/stm32-rng.c > index a6731cf0627a..efb6a9f9a11b 100644 > --- a/drivers/char/hw_random/stm32-rng.c > +++ b/drivers/char/hw_random/stm32-rng.c > @@ -10,8 +10,9 @@ > #include > #include > #include > +#include > #include > -#include > +#include > #include > #include > #include > diff --git a/drivers/char/hw_random/xgene-rng.c > b/drivers/char/hw_random/xgene-rng.c > index 7c8f3cb7c6af..c25bb169563d 100644 > --- a/drivers/char/hw_random/xgene-rng.c > +++ b/drivers/char/hw_random/xgene-rng.c > @@ -15,9 +15,8 @@ > #include > #include > #include > -#include > -#include > -#include > +#include > +#include > #include > > #define RNG_MAX_DATUM4 > diff --git a/drivers/char/hw_random/xiphera-trng.c > b/drivers/char/hw_random/xiphera-trng.c > index 2a9fea72b2e0..2c586d1fe8a9 100644 > --- a/drivers/char/hw_random/xiphera-trng.c > +++ b/drivers/char/hw_random/xiphera-trng.c > @@ -7,7 +7,6 @@ > #include > #include > #include > -#include > #include > #include > > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c > b/drivers/char/ipmi/kcs_bmc_aspeed.c > index 2dea8cd5a09a..72640da55380 100644 > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c > @@ -14,7 +14,6 @@ > #include > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c > index 528f35b14fb6..76adb108076c 100644 > --- a/drivers/char/tpm/tpm_ftpm_tee.c > +++ b/drivers/char/tpm/tpm_ftpm_tee.c > @@ -11,7 +11,6 @@ > > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 7db3593941ea..e4030404c64e 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -25,7 +25,6 @@ > #include > #include > #include > -#include > #include > #include > #include "tpm.h" > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c > b/drivers/char/tpm/tpm_tis_spi_main.c > index 1f5207974a17..c6101914629d 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -28,7 +28,7 @@ > #include > #include > > -#include > +#include > #include > #include > > diff --git a/drivers/char/tpm/tpm_tis_synquacer.c > b/drivers/char/tpm/tpm_tis_synquacer.c > index 49278746b0e2..7f9b4bfceb6e 100644 > --- a/drivers/char/tpm/tpm_tis_synquacer.c > +++ b/drivers/char/tpm/tpm_tis_synquacer.c > @@ -9,7 +9,6 @@ > #include > #include > #include > -#include > #include > #include "tpm.h" > #include "tpm_tis_core.h" > -- > 2.40.1 drivers/char/tpm/** Acked-by: Jarkko Sakkinen BR, Jarkko
Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Wed Jul 12, 2023 at 12:39 PM UTC, Michael Ellerman wrote:
> Jarkko Sakkinen writes:
> > On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
> >> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> >> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> >> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> >> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> >> > > Firmware.
> >> > >
> >> > > This involves creating a buffer to hold the SML, and creating two
> >> > > device
> >> > > tree properties to record its base address and size. The kernel then
> >> > > later reads those properties from the device tree to find the SML.
> >> > >
> >> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> >> > > support for instantiating SML from Open Firmware") the powerpc kernel
> >> > > was always built big endian, so the properties were created big endian
> >> > > by default.
> >> > >
> >> > > However since then little endian support was added to powerpc, and now
> >> > > the code lacks conversions to big endian when creating the properties.
> >> > >
> >> > > This means on little endian kernels the device tree properties are
> >> > > little endian, which is contrary to the device tree spec, and in
> >> > > contrast to all other device tree properties.
> >> > >
> >> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> >> > > the endian conversion if the properties were created via the SML
> >> > > handover.
> >> > >
> >> > > A better solution is to encode the properties as big endian as they
> >> > > should be, and remove the workaround.
> >> > >
> >> > > Typically changing the encoding of a property like this would present
> >> > > problems for kexec. However the SML is not propagated across kexec, so
> >> > > changing the encoding of the properties is a non-issue.
> >> > >
> >> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical
> >> > > TPM event log")
> >> > > Signed-off-by: Michael Ellerman
> >> > > Reviewed-by: Stefan Berger
> >> > > ---
> >> > > arch/powerpc/kernel/prom_init.c | 8 ++--
> >> > > drivers/char/tpm/eventlog/of.c | 23 ---
> >> > > 2 files changed, 10 insertions(+), 21 deletions(-)
> >> >
> >> > Split into two patches (producer and consumer).
> >>
> >> I think this wouldn't be right since it would break the system when only
> >> one patch is applied since it would be reading the fields in the wrong
> >> endianess.
> >
> > I think it would help if the commit message would better explain
> > what is going on. It is somewhat difficult to decipher, if you
> > don't have deep knowledge of the powerpc architecture.
>
> I mean, it's already 8 paragraphs ¯\_(ツ)_/¯
>
> But I'm happy to expand it. I just don't really know what extra detail
> is needed to make it clearer.
Adding more text is not the right way to clarify things. I'd start
by explaining shortly SML and then move to the handover. It can't
be that hard, right?
Just adding new paragraphs would probably just make it even more
confusing.
BR, Jarkko
Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
>
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> > > Firmware.
> > >
> > > This involves creating a buffer to hold the SML, and creating two device
> > > tree properties to record its base address and size. The kernel then
> > > later reads those properties from the device tree to find the SML.
> > >
> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> > > support for instantiating SML from Open Firmware") the powerpc kernel
> > > was always built big endian, so the properties were created big endian
> > > by default.
> > >
> > > However since then little endian support was added to powerpc, and now
> > > the code lacks conversions to big endian when creating the properties.
> > >
> > > This means on little endian kernels the device tree properties are
> > > little endian, which is contrary to the device tree spec, and in
> > > contrast to all other device tree properties.
> > >
> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> > > the endian conversion if the properties were created via the SML
> > > handover.
> > >
> > > A better solution is to encode the properties as big endian as they
> > > should be, and remove the workaround.
> > >
> > > Typically changing the encoding of a property like this would present
> > > problems for kexec. However the SML is not propagated across kexec, so
> > > changing the encoding of the properties is a non-issue.
> > >
> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> > > event log")
> > > Signed-off-by: Michael Ellerman
> > > Reviewed-by: Stefan Berger
> > > ---
> > > arch/powerpc/kernel/prom_init.c | 8 ++--
> > > drivers/char/tpm/eventlog/of.c | 23 ---
> > > 2 files changed, 10 insertions(+), 21 deletions(-)
> >
> > Split into two patches (producer and consumer).
>
> I think this wouldn't be right since it would break the system when only one
> patch is applied since it would be reading the fields in the wrong endianess.
I think it would help if the commit message would better explain
what is going on. It is somewhat difficult to decipher, if you
don't have deep knowledge of the powerpc architecture.
BR, Jarkko
Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> Measurement Log) from OF to Linux, before Linux shuts down Open
> Firmware.
>
> This involves creating a buffer to hold the SML, and creating two device
> tree properties to record its base address and size. The kernel then
> later reads those properties from the device tree to find the SML.
>
> When the code was initially added in commit 4a727429abec ("PPC64: Add
> support for instantiating SML from Open Firmware") the powerpc kernel
> was always built big endian, so the properties were created big endian
> by default.
>
> However since then little endian support was added to powerpc, and now
> the code lacks conversions to big endian when creating the properties.
>
> This means on little endian kernels the device tree properties are
> little endian, which is contrary to the device tree spec, and in
> contrast to all other device tree properties.
>
> To cope with that a workaround was added in tpm_read_log_of() to skip
> the endian conversion if the properties were created via the SML
> handover.
>
> A better solution is to encode the properties as big endian as they
> should be, and remove the workaround.
>
> Typically changing the encoding of a property like this would present
> problems for kexec. However the SML is not propagated across kexec, so
> changing the encoding of the properties is a non-issue.
>
> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> event log")
> Signed-off-by: Michael Ellerman
> Reviewed-by: Stefan Berger
> ---
> arch/powerpc/kernel/prom_init.c | 8 ++--
> drivers/char/tpm/eventlog/of.c | 23 ---
> 2 files changed, 10 insertions(+), 21 deletions(-)
Split into two patches (producer and consumer).
BR, Jarkko
>
> v2: Add Stefan's reviewed-by.
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index d464ba412084..72fe306b6820 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
> u32 entry = 0, size = 0, succ = 0;
> u64 base;
> __be32 val;
> + __be64 val64;
>
> prom_debug("prom_instantiate_sml: start...\n");
>
> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
>
> reserve_mem(base, size);
>
> + val64 = cpu_to_be64(base);
> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> - , sizeof(base));
> + , sizeof(val64));
> +
> + val = cpu_to_be32(size);
> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> - , sizeof(size));
> + , sizeof(val));
>
> prom_debug("sml base = 0x%llx\n", base);
> prom_debug("sml size = 0x%x\n", size);
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..0bc0cb6333c6 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> + const __be32 *sizep;
> + const __be64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (sizep == NULL || basep == NULL)
> return -EIO;
>
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + size = be32_to_cpup(sizep);
> + base = be64_to_cpup(basep);
>
> if (size == 0) {
> dev_warn(>dev, "%s: Event log area empty\n", __func__);
Re: [PATCH v9 2/4] tpm: of: Make of-tree specific function commonly available
On Fri, 2023-06-09 at 14:49 -0400, Stefan Berger wrote: > > On 6/9/23 14:18, Jarkko Sakkinen wrote: > > On Thu May 25, 2023 at 1:56 AM EEST, Jerry Snitselaar wrote: > > > On Tue, Apr 18, 2023 at 09:44:07AM -0400, Stefan Berger wrote: > > > > Simplify tpm_read_log_of() by moving reusable parts of the code into > > > > an inline function that makes it commonly available so it can be > > > > used also for kexec support. Call the new of_tpm_get_sml_parameters() > > > > function from the TPM Open Firmware driver. > > > > > > > > Signed-off-by: Stefan Berger > > > > Cc: Jarkko Sakkinen > > > > Cc: Jason Gunthorpe > > > > Cc: Rob Herring > > > > Cc: Frank Rowand > > > > Reviewed-by: Mimi Zohar > > > > Tested-by: Nageswara R Sastry > > > > Tested-by: Coiby Xu > > > > Acked-by: Jarkko Sakkinen > > > > > > > > > > Reviewed-by: Jerry Snitselaar > > > > If I just pick tpm only patches they won't apply so maybe TPM changes > > should be better separated if that is by any means possible. > > Per the comment here I am putting this series here on hold. > https://lore.kernel.org/linux-integrity/20230418134409.177485-1-stef...@linux.ibm.com/T/#m03745c2af2c46f19f329522fcb6ccb2bf2eaedc7 Hi, sorry for late response. The Midsummer weekend really messed my schedules (it is a big thing Finland). This year the timing with the kernel cycle has been conflicting. OK cool. BR, Jarkko
Re: [PATCH v9 2/4] tpm: of: Make of-tree specific function commonly available
On Fri Jun 9, 2023 at 9:49 PM EEST, Stefan Berger wrote: > > > On 6/9/23 14:18, Jarkko Sakkinen wrote: > > On Thu May 25, 2023 at 1:56 AM EEST, Jerry Snitselaar wrote: > >> On Tue, Apr 18, 2023 at 09:44:07AM -0400, Stefan Berger wrote: > >>> Simplify tpm_read_log_of() by moving reusable parts of the code into > >>> an inline function that makes it commonly available so it can be > >>> used also for kexec support. Call the new of_tpm_get_sml_parameters() > >>> function from the TPM Open Firmware driver. > >>> > >>> Signed-off-by: Stefan Berger > >>> Cc: Jarkko Sakkinen > >>> Cc: Jason Gunthorpe > >>> Cc: Rob Herring > >>> Cc: Frank Rowand > >>> Reviewed-by: Mimi Zohar > >>> Tested-by: Nageswara R Sastry > >>> Tested-by: Coiby Xu > >>> Acked-by: Jarkko Sakkinen > >>> > >> > >> Reviewed-by: Jerry Snitselaar > > > > If I just pick tpm only patches they won't apply so maybe TPM changes > > should be better separated if that is by any means possible. > > Per the comment here I am putting this series here on hold. > https://lore.kernel.org/linux-integrity/20230418134409.177485-1-stef...@linux.ibm.com/T/#m03745c2af2c46f19f329522fcb6ccb2bf2eaedc7 OK, cool. I've mentioned this in few other emails but say this here too: I was relocating for last couple of weeks, and thus some latency. If you choose to repost the series, I'm happy to review it, thanks :-) BR, Jarkko
Re: [PATCH v9 2/4] tpm: of: Make of-tree specific function commonly available
On Thu May 25, 2023 at 1:56 AM EEST, Jerry Snitselaar wrote: > On Tue, Apr 18, 2023 at 09:44:07AM -0400, Stefan Berger wrote: > > Simplify tpm_read_log_of() by moving reusable parts of the code into > > an inline function that makes it commonly available so it can be > > used also for kexec support. Call the new of_tpm_get_sml_parameters() > > function from the TPM Open Firmware driver. > > > > Signed-off-by: Stefan Berger > > Cc: Jarkko Sakkinen > > Cc: Jason Gunthorpe > > Cc: Rob Herring > > Cc: Frank Rowand > > Reviewed-by: Mimi Zohar > > Tested-by: Nageswara R Sastry > > Tested-by: Coiby Xu > > Acked-by: Jarkko Sakkinen > > > > Reviewed-by: Jerry Snitselaar If I just pick tpm only patches they won't apply so maybe TPM changes should be better separated if that is by any means possible. Open for counter proposals. Just my thoughts... I.e. I'm mainly wondering why TPM patches depend on IMA patches? BR, Jarkko
Re: [PATCH v2] security/integrity: fix pointer to ESL data and its size on pseries
On Thu Jun 8, 2023 at 3:04 PM EEST, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
>
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Cc: sta...@vger.kenrnel.org # v6.3
> Signed-off-by: Nayna Jain
> ---
> Changelog:
> v2: Fixed feedback from Jarkko
> * added CC to stable
> * moved *data declaration to same line as *db,*dbx
> Renamed extract_data() macro to extract_esl() for clarity
> .../integrity/platform_certs/load_powerpc.c | 40 ---
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..170789dc63d2 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -15,6 +15,9 @@
> #include "keyring_handler.h"
> #include "../integrity.h"
>
> +#define extract_esl(db, data, size, offset) \
> + do { db = data + offset; size = size - offset; } while (0)
> +
> /*
> * Get a certificate list blob from the named secure variable.
> *
> @@ -55,8 +58,9 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> */
> static int __init load_powerpc_certs(void)
> {
> - void *db = NULL, *dbx = NULL;
> - u64 dbsize = 0, dbxsize = 0;
> + void *db = NULL, *dbx = NULL, *data = NULL;
> + u64 dsize = 0;
> + u64 offset = 0;
> int rc = 0;
> ssize_t len;
> char buf[32];
> @@ -74,38 +78,46 @@ static int __init load_powerpc_certs(void)
> return -ENODEV;
> }
>
> + if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> + /* PLPKS authenticated variables ESL data is prefixed with 8
> bytes of timestamp */
> + offset = 8;
> +
> /*
>* Get db, and dbx. They might not exist, so it isn't an error if we
>* can't get them.
>*/
> - db = get_cert_list("db", 3, );
> - if (!db) {
> + data = get_cert_list("db", 3, );
> + if (!data) {
> pr_info("Couldn't get db list from firmware\n");
> - } else if (IS_ERR(db)) {
> - rc = PTR_ERR(db);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading db from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:db", db, dbsize,
> + extract_esl(db, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:db", db, dsize,
> get_handler_for_db);
> if (rc)
> pr_err("Couldn't parse db signatures: %d\n", rc);
> - kfree(db);
> + kfree(data);
> }
>
> - dbx = get_cert_list("dbx", 4, );
> - if (!dbx) {
> + data = get_cert_list("dbx", 4, );
> + if (!data) {
> pr_info("Couldn't get dbx list from firmware\n");
> - } else if (IS_ERR(dbx)) {
> - rc = PTR_ERR(dbx);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading dbx from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
> + extract_esl(dbx, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
> get_handler_for_dbx);
> if (rc)
> pr_err("Couldn't parse dbx signatures: %d\n", rc);
> - kfree(dbx);
> + kfree(data);
> }
>
> return rc;
> --
> 2.31.1
Acked-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On Wed Jun 7, 2023 at 3:28 PM EEST, Nayna wrote:
>
> On 6/6/23 16:51, Jarkko Sakkinen wrote:
> > On Tue Jun 6, 2023 at 8:26 PM EEST, Nayna Jain wrote:
> >> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> >> Extract ESL by stripping off the timestamp before passing to ESL parser.
> >>
> > Cc: sta...@vger.kenrnel.org # v6.3
> >
> > ?
>
> Aah yes. Missed that.. Thanks..
>
>
> >
> >> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> >> Signed-off-by: Nayna Jain
> >> ---
> >> .../integrity/platform_certs/load_powerpc.c | 39 ---
> >> 1 file changed, 26 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/security/integrity/platform_certs/load_powerpc.c
> >> b/security/integrity/platform_certs/load_powerpc.c
> >> index b9de70b90826..57768cbf1fd3 100644
> >> --- a/security/integrity/platform_certs/load_powerpc.c
> >> +++ b/security/integrity/platform_certs/load_powerpc.c
> >> @@ -15,6 +15,9 @@
> >> #include "keyring_handler.h"
> >> #include "../integrity.h"
> >>
> >> +#define extract_data(db, data, size, offset) \
> >> + do { db = data + offset; size = size - offset; } while (0)
> >> +
> >> /*
> >>* Get a certificate list blob from the named secure variable.
> >>*
> >> @@ -55,8 +58,10 @@ static __init void *get_cert_list(u8 *key, unsigned
> >> long keylen, u64 *size)
> >>*/
> >> static int __init load_powerpc_certs(void)
> >> {
> >> + void *data = NULL;
> >> + u64 dsize = 0;
> >> + u64 offset = 0;
> >>void *db = NULL, *dbx = NULL;
> > So... what do you need db still for?
> >
> > If you meant to rename 'db' to 'data', then you should not do it, since
> > this is
> > a bug fix. It is zero gain, and a factor harder backport.
>
> In case of PowerVM guest, data points to timestamp + ESL. And then with
> offset of 8 bytes, db points to ESL.
>
> While db is used for parsing ESL, data is then later used to free
> (timestamp + ESL) memory.
>
> Hope it answers the question.
OK, cool. Only thing I have to add that it would be more consistent if
data was declared in the same line as db and dbx, given that they are
declared in the same line.
BR, Jarkko
Re: [PATCH] security/integrity: fix pointer to ESL data and its size on pseries
On Tue Jun 6, 2023 at 8:26 PM EEST, Nayna Jain wrote:
> On PowerVM guest, variable data is prefixed with 8 bytes of timestamp.
> Extract ESL by stripping off the timestamp before passing to ESL parser.
>
Cc: sta...@vger.kenrnel.org # v6.3
?
> Fixes: 4b3e71e9a34c ("integrity/powerpc: Support loading keys from PLPKS")
> Signed-off-by: Nayna Jain
> ---
> .../integrity/platform_certs/load_powerpc.c | 39 ---
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> index b9de70b90826..57768cbf1fd3 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -15,6 +15,9 @@
> #include "keyring_handler.h"
> #include "../integrity.h"
>
> +#define extract_data(db, data, size, offset) \
> + do { db = data + offset; size = size - offset; } while (0)
> +
> /*
> * Get a certificate list blob from the named secure variable.
> *
> @@ -55,8 +58,10 @@ static __init void *get_cert_list(u8 *key, unsigned long
> keylen, u64 *size)
> */
> static int __init load_powerpc_certs(void)
> {
> + void *data = NULL;
> + u64 dsize = 0;
> + u64 offset = 0;
> void *db = NULL, *dbx = NULL;
So... what do you need db still for?
If you meant to rename 'db' to 'data', then you should not do it, since this is
a bug fix. It is zero gain, and a factor harder backport.
> - u64 dbsize = 0, dbxsize = 0;
> int rc = 0;
> ssize_t len;
> char buf[32];
> @@ -74,38 +79,46 @@ static int __init load_powerpc_certs(void)
> return -ENODEV;
> }
>
> + if (strcmp("ibm,plpks-sb-v1", buf) == 0)
> + /* PLPKS authenticated variables ESL data is prefixed with 8
> bytes of timestamp */
> + offset = 8;
> +
> /*
>* Get db, and dbx. They might not exist, so it isn't an error if we
>* can't get them.
>*/
> - db = get_cert_list("db", 3, );
> - if (!db) {
> + data = get_cert_list("db", 3, );
> + if (!data) {
> pr_info("Couldn't get db list from firmware\n");
> - } else if (IS_ERR(db)) {
> - rc = PTR_ERR(db);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading db from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:db", db, dbsize,
> + extract_data(db, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:db", db, dsize,
> get_handler_for_db);
> if (rc)
> pr_err("Couldn't parse db signatures: %d\n", rc);
> - kfree(db);
> + kfree(data);
> }
>
> - dbx = get_cert_list("dbx", 4, );
> - if (!dbx) {
> + data = get_cert_list("dbx", 4, );
> + if (!data) {
> pr_info("Couldn't get dbx list from firmware\n");
> - } else if (IS_ERR(dbx)) {
> - rc = PTR_ERR(dbx);
> + } else if (IS_ERR(data)) {
> + rc = PTR_ERR(data);
> pr_err("Error reading dbx from firmware: %d\n", rc);
> return rc;
> } else {
> - rc = parse_efi_signature_list("powerpc:dbx", dbx, dbxsize,
> + extract_data(dbx, data, dsize, offset);
> +
> + rc = parse_efi_signature_list("powerpc:dbx", dbx, dsize,
> get_handler_for_dbx);
> if (rc)
> pr_err("Couldn't parse dbx signatures: %d\n", rc);
> - kfree(dbx);
> + kfree(data);
> }
>
> return rc;
> --
> 2.31.1
BR, Jarkko
Re: [PATCH 1/4] block:sed-opal: SED Opal keystore
On Fri May 5, 2023 at 10:43 PM EEST, wrote:
> From: Greg Joyce
>
> Add read and write functions that allow SED Opal keys to stored
> in a permanent keystore.
Please be more verbose starting from "Self-Encrypting Drive (SED)",
instead of just "SED", and take time to explain what these keys are.
>
> Signed-off-by: Greg Joyce
> Reviewed-by: Jonathan Derrick
> ---
> block/Makefile | 2 +-
> block/sed-opal-key.c | 24
> include/linux/sed-opal-key.h | 15 +++
> 3 files changed, 40 insertions(+), 1 deletion(-)
> create mode 100644 block/sed-opal-key.c
> create mode 100644 include/linux/sed-opal-key.h
>
> diff --git a/block/Makefile b/block/Makefile
> index 4e01bb71ad6e..464a9f209552 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -35,7 +35,7 @@ obj-$(CONFIG_BLK_DEV_ZONED) += blk-zoned.o
> obj-$(CONFIG_BLK_WBT)+= blk-wbt.o
> obj-$(CONFIG_BLK_DEBUG_FS) += blk-mq-debugfs.o
> obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
> -obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o
> +obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o sed-opal-key.o
> obj-$(CONFIG_BLK_PM) += blk-pm.o
> obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += blk-crypto.o blk-crypto-profile.o \
> blk-crypto-sysfs.o
> diff --git a/block/sed-opal-key.c b/block/sed-opal-key.c
> new file mode 100644
> index ..16f380164c44
> --- /dev/null
> +++ b/block/sed-opal-key.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SED key operations.
> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for SED Opal
> + * keys. Specific keystores can provide overrides.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +
> +int __weak sed_read_key(char *keyname, char *key, u_int *keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +int __weak sed_write_key(char *keyname, char *key, u_int keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> diff --git a/include/linux/sed-opal-key.h b/include/linux/sed-opal-key.h
> new file mode 100644
> index ..c9b1447986d8
> --- /dev/null
> +++ b/include/linux/sed-opal-key.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SED key operations.
> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for SED Opal
> + * keys. Specific keystores can provide overrides.
> + *
> + */
> +
> +#include
> +
> +int sed_read_key(char *keyname, char *key, u_int *keylen);
> +int sed_write_key(char *keyname, char *key, u_int keylen);
> --
> gjo...@linux.vnet.ibm.com
BR, Jarkko
Re: [PATCH 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Tue, Feb 28, 2023 at 10:21:36PM +1100, Michael Ellerman wrote:
> Jarkko Sakkinen writes:
> > On Mon, Feb 27, 2023 at 06:08:31PM -0500, Stefan Berger wrote:
> >> On 2/23/23 22:25, Michael Ellerman wrote:
> >> > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> >> > Measurement Log) from OF to Linux, before Linux shuts down Open
> >> > Firmware.
> >> >
> >> > This involves creating a buffer to hold the SML, and creating two device
> >> > tree properties to record its base address and size. The kernel then
> >> > later reads those properties from the device tree to find the SML.
> >> >
> >> > When the code was initially added in commit 4a727429abec ("PPC64: Add
> >> > support for instantiating SML from Open Firmware") the powerpc kernel
> >> > was always built big endian, so the properties were created big endian
> >> > by default.
> >> >
> >> > However since then little endian support was added to powerpc, and now
> >> > the code lacks conversions to big endian when creating the properties.
> >> >
> >> > This means on little endian kernels the device tree properties are
> >> > little endian, which is contrary to the device tree spec, and in
> >> > contrast to all other device tree properties.
> >> >
> >> > To cope with that a workaround was added in tpm_read_log_of() to skip
> >> > the endian conversion if the properties were created via the SML
> >> > handover.
> >> >
> >> > A better solution is to encode the properties as big endian as they
> >> > should be, and remove the workaround.
> >> >
> >> > Typically changing the encoding of a property like this would present
> >> > problems for kexec. However the SML is not propagated across kexec, so
> >> > changing the encoding of the properties is a non-issue.
> >> >
> >> > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> >> > event log")
> >> > Signed-off-by: Michael Ellerman
> >>
> >> Reviewed-by: Stefan Berger
> >
> > 2/2 does not have a fixes tag.
>
> True. Arguably the bug goes back to the introduction of
> kexec_file_load() support, although the patch won't backport that far
> due to code refactoring. So that would be:
>
> Fixes: a0458284f062 ("powerpc: Add support code for kexec_file_load()")
Hmm... IMHO, it would still make sense to document this.
BR, Jarkko
Re: [PATCH 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
On Mon, Feb 27, 2023 at 06:08:31PM -0500, Stefan Berger wrote:
>
>
> On 2/23/23 22:25, Michael Ellerman wrote:
> > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> > Measurement Log) from OF to Linux, before Linux shuts down Open
> > Firmware.
> >
> > This involves creating a buffer to hold the SML, and creating two device
> > tree properties to record its base address and size. The kernel then
> > later reads those properties from the device tree to find the SML.
> >
> > When the code was initially added in commit 4a727429abec ("PPC64: Add
> > support for instantiating SML from Open Firmware") the powerpc kernel
> > was always built big endian, so the properties were created big endian
> > by default.
> >
> > However since then little endian support was added to powerpc, and now
> > the code lacks conversions to big endian when creating the properties.
> >
> > This means on little endian kernels the device tree properties are
> > little endian, which is contrary to the device tree spec, and in
> > contrast to all other device tree properties.
> >
> > To cope with that a workaround was added in tpm_read_log_of() to skip
> > the endian conversion if the properties were created via the SML
> > handover.
> >
> > A better solution is to encode the properties as big endian as they
> > should be, and remove the workaround.
> >
> > Typically changing the encoding of a property like this would present
> > problems for kexec. However the SML is not propagated across kexec, so
> > changing the encoding of the properties is a non-issue.
> >
> > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM
> > event log")
> > Signed-off-by: Michael Ellerman
>
> Reviewed-by: Stefan Berger
2/2 does not have a fixes tag.
BR, Jarkko
Re: [PATCH v8 2/4] tpm: of: Make of-tree specific function commonly available
On Thu, Sep 01, 2022 at 05:46:08PM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() by moving reusable parts of the code into
> an inline function that makes it commonly available so it can be
> used also for kexec support. Call the new of_tpm_get_sml_parameters()
> function from the TPM Open Firmware driver.
>
> Signed-off-by: Stefan Berger
> Cc: Jarkko Sakkinen
> Cc: Jason Gunthorpe
> Cc: Rob Herring
> Cc: Frank Rowand
> Reviewed-by: Mimi Zohar
> Tested-by: Nageswara R Sastry
> Reviewed-by: Jarkko Sakkinen
> Tested-by: Coiby Xu
>
> ---
> v7:
> - Added original comment back into inlined function
>
> v4:
> - converted to inline function
> ---
> drivers/char/tpm/eventlog/of.c | 31 +
> include/linux/tpm.h| 36 ++
> 2 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..f9462d19632e 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "../tpm.h"
> @@ -20,11 +21,10 @@
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> + int ret;
>
> log = >log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return -ENODEV;
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + ret = of_tpm_get_sml_parameters(np, , );
> + if (ret < 0)
> + return ret;
>
> if (size == 0) {
> dev_warn(>dev, "%s: Event log area empty\n", __func__);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index dfeb25a0362d..6356baaa1393 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -460,4 +460,40 @@ static inline struct tpm_chip *tpm_default_chip(void)
> return NULL;
> }
> #endif
> +
> +#ifdef CONFIG_OF
> +static inline int of_tpm_get_sml_parameters(struct device_node *np,
> + u64 *base, u32 *size)
> +{
> + const u32 *sizep;
> + const u64 *basep;
> +
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return -ENODEV;
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> +
> + /*
> + * For both vtpm/tpm, firmware has log addr and log size in big
> + * endian format. But in case of vtpm, there is a method called
> + * sml-handover which is run during kernel init even before
> + * device tree is setup. This sml-handover function takes care
> + * of endianness and writes to sml-base and sml-size in little
> + * endian format. For this reason, vtpm doesn't need conversion
> + * but physical tpm needs the conversion.
> + */
> + if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> + of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + *size = be32_to_cpup((__force __be32 *)sizep);
> + *base = be64_to_cpup((__force __be64 *)basep);
> + } else {
> + *size = *sizep;
> + *base = *basep;
> + }
> + return 0;
> +}
> +#endif
> +
> #endif
> --
> 2.35.1
>
Acked-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v7 4/6] tpm: of: Make of-tree specific function commonly available
On Sun, Aug 14, 2022 at 10:16:09PM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 12, 2022 at 12:43:03PM -0400, Stefan Berger wrote:
> > Simplify tpm_read_log_of() by moving reusable parts of the code into
> > an inline function that makes it commonly available so it can be
> > used also for kexec support. Call the new of_tpm_get_sml_parameters()
> > function from the TPM Open Firmware driver.
> >
> > Signed-off-by: Stefan Berger
> > Cc: Jarkko Sakkinen
> > Cc: Jason Gunthorpe
> > Cc: Rob Herring
> > Cc: Frank Rowand
> > Reviewed-by: Mimi Zohar
> > Tested-by: Nageswara R Sastry
> >
> > ---
> > v7:
> > - Added original comment back into inlined function
> >
> > v4:
> > - converted to inline function
> > ---
> > drivers/char/tpm/eventlog/of.c | 31 +
> > include/linux/tpm.h| 36 ++
> > 2 files changed, 41 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> > index a9ce66d09a75..f9462d19632e 100644
> > --- a/drivers/char/tpm/eventlog/of.c
> > +++ b/drivers/char/tpm/eventlog/of.c
> > @@ -12,6 +12,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "../tpm.h"
> > @@ -20,11 +21,10 @@
> > int tpm_read_log_of(struct tpm_chip *chip)
> > {
> > struct device_node *np;
> > - const u32 *sizep;
> > - const u64 *basep;
> > struct tpm_bios_log *log;
> > u32 size;
> > u64 base;
> > + int ret;
> >
> > log = >log;
> > if (chip->dev.parent && chip->dev.parent->of_node)
> > @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > if (of_property_read_bool(np, "powered-while-suspended"))
> > chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
> >
> > - sizep = of_get_property(np, "linux,sml-size", NULL);
> > - basep = of_get_property(np, "linux,sml-base", NULL);
> > - if (sizep == NULL && basep == NULL)
> > - return -ENODEV;
> > - if (sizep == NULL || basep == NULL)
> > - return -EIO;
> > -
> > - /*
> > -* For both vtpm/tpm, firmware has log addr and log size in big
> > -* endian format. But in case of vtpm, there is a method called
> > -* sml-handover which is run during kernel init even before
> > -* device tree is setup. This sml-handover function takes care
> > -* of endianness and writes to sml-base and sml-size in little
> > -* endian format. For this reason, vtpm doesn't need conversion
> > -* but physical tpm needs the conversion.
> > -*/
> > - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> > - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> > - size = be32_to_cpup((__force __be32 *)sizep);
> > - base = be64_to_cpup((__force __be64 *)basep);
> > - } else {
> > - size = *sizep;
> > - base = *basep;
> > - }
> > + ret = of_tpm_get_sml_parameters(np, , );
> > + if (ret < 0)
> > + return ret;
> >
> > if (size == 0) {
> > dev_warn(>dev, "%s: Event log area empty\n", __func__);
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index dfeb25a0362d..6356baaa1393 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -460,4 +460,40 @@ static inline struct tpm_chip *tpm_default_chip(void)
> > return NULL;
> > }
> > #endif
> > +
> > +#ifdef CONFIG_OF
> > +static inline int of_tpm_get_sml_parameters(struct device_node *np,
> > + u64 *base, u32 *size)
> > +{
> > + const u32 *sizep;
> > + const u64 *basep;
> > +
> > + sizep = of_get_property(np, "linux,sml-size", NULL);
> > + basep = of_get_property(np, "linux,sml-base", NULL);
> > + if (sizep == NULL && basep == NULL)
> > + return -ENODEV;
> > + if (sizep == NULL || basep == NULL)
> > + return -EIO;
> > +
> > + /*
> > +* For both vtpm/tpm, firmware has log addr and log size in big
> > +* endian format. But in case of vtpm, there is a method called
> > +* sml-handover which is run during kernel init even before
> > +
Re: [PATCH v7 4/6] tpm: of: Make of-tree specific function commonly available
On Fri, Aug 12, 2022 at 12:43:03PM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() by moving reusable parts of the code into
> an inline function that makes it commonly available so it can be
> used also for kexec support. Call the new of_tpm_get_sml_parameters()
> function from the TPM Open Firmware driver.
>
> Signed-off-by: Stefan Berger
> Cc: Jarkko Sakkinen
> Cc: Jason Gunthorpe
> Cc: Rob Herring
> Cc: Frank Rowand
> Reviewed-by: Mimi Zohar
> Tested-by: Nageswara R Sastry
>
> ---
> v7:
> - Added original comment back into inlined function
>
> v4:
> - converted to inline function
> ---
> drivers/char/tpm/eventlog/of.c | 31 +
> include/linux/tpm.h| 36 ++
> 2 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..f9462d19632e 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "../tpm.h"
> @@ -20,11 +21,10 @@
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> + int ret;
>
> log = >log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return -ENODEV;
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + ret = of_tpm_get_sml_parameters(np, , );
> + if (ret < 0)
> + return ret;
>
> if (size == 0) {
> dev_warn(>dev, "%s: Event log area empty\n", __func__);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index dfeb25a0362d..6356baaa1393 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -460,4 +460,40 @@ static inline struct tpm_chip *tpm_default_chip(void)
> return NULL;
> }
> #endif
> +
> +#ifdef CONFIG_OF
> +static inline int of_tpm_get_sml_parameters(struct device_node *np,
> + u64 *base, u32 *size)
> +{
> + const u32 *sizep;
> + const u64 *basep;
> +
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return -ENODEV;
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> +
> + /*
> + * For both vtpm/tpm, firmware has log addr and log size in big
> + * endian format. But in case of vtpm, there is a method called
> + * sml-handover which is run during kernel init even before
> + * device tree is setup. This sml-handover function takes care
> + * of endianness and writes to sml-base and sml-size in little
> + * endian format. For this reason, vtpm doesn't need conversion
> + * but physical tpm needs the conversion.
> + */
> + if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> + of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + *size = be32_to_cpup((__force __be32 *)sizep);
> + *base = be64_to_cpup((__force __be64 *)basep);
> + } else {
> + *size = *sizep;
> + *base = *basep;
> + }
> + return 0;
> +}
> +#endif
> +
> #endif
> --
> 2.35.1
>
Reviewed-by: Jarkko Sakkinen
BR, Jarkkko
Re: [PATCH v5 4/6] tpm: of: Make of-tree specific function commonly available
On Fri, Jul 15, 2022 at 12:01:36PM -0400, Stefan Berger wrote:
>
>
> On 7/10/22 23:04, Jarkko Sakkinen wrote:
> > On Wed, Jul 06, 2022 at 11:23:27AM -0400, Stefan Berger wrote:
> > > Simplify tpm_read_log_of() by moving reusable parts of the code into
> > > an inline function that makes it commonly available so it can be
> > > used also for kexec support. Call the new of_tpm_get_sml_parameters()
> > > function from the TPM Open Firmware driver.
> > >
> > > Signed-off-by: Stefan Berger
> > > Cc: Jarkko Sakkinen
> > > Cc: Jason Gunthorpe
> > > Cc: Rob Herring
> > > Cc: Frank Rowand
> > >
> > > ---
> > > v4:
> > > - converted to inline function
> > > ---
> > > drivers/char/tpm/eventlog/of.c | 31 +--
> > > include/linux/tpm.h| 27 +++
> > > 2 files changed, 32 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/eventlog/of.c
> > > b/drivers/char/tpm/eventlog/of.c
> > > index a9ce66d09a75..f9462d19632e 100644
> > > --- a/drivers/char/tpm/eventlog/of.c
> > > +++ b/drivers/char/tpm/eventlog/of.c
> > > @@ -12,6 +12,7 @@
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include "../tpm.h"
> > > @@ -20,11 +21,10 @@
> > > int tpm_read_log_of(struct tpm_chip *chip)
> > > {
> > > struct device_node *np;
> > > - const u32 *sizep;
> > > - const u64 *basep;
> > > struct tpm_bios_log *log;
> > > u32 size;
> > > u64 base;
> > > + int ret;
> > > log = >log;
> > > if (chip->dev.parent && chip->dev.parent->of_node)
> > > @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > > if (of_property_read_bool(np, "powered-while-suspended"))
> > > chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
> > > - sizep = of_get_property(np, "linux,sml-size", NULL);
> > > - basep = of_get_property(np, "linux,sml-base", NULL);
> > > - if (sizep == NULL && basep == NULL)
> > > - return -ENODEV;
> > > - if (sizep == NULL || basep == NULL)
> > > - return -EIO;
> > > -
> > > - /*
> > > - * For both vtpm/tpm, firmware has log addr and log size in big
> > > - * endian format. But in case of vtpm, there is a method called
> > > - * sml-handover which is run during kernel init even before
> > > - * device tree is setup. This sml-handover function takes care
> > > - * of endianness and writes to sml-base and sml-size in little
> > > - * endian format. For this reason, vtpm doesn't need conversion
> > > - * but physical tpm needs the conversion.
> > > - */
>
>
> Jarkko,
>
> can I apply your R-b tag after putting this lost comment back into the
> inline function?
>Stefan
If there's a change I want to see the updated patch first.
BR, Jarkko
Re: [PATCH v5 4/6] tpm: of: Make of-tree specific function commonly available
On Wed, Jul 06, 2022 at 11:23:27AM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() by moving reusable parts of the code into
> an inline function that makes it commonly available so it can be
> used also for kexec support. Call the new of_tpm_get_sml_parameters()
> function from the TPM Open Firmware driver.
>
> Signed-off-by: Stefan Berger
> Cc: Jarkko Sakkinen
> Cc: Jason Gunthorpe
> Cc: Rob Herring
> Cc: Frank Rowand
>
> ---
> v4:
> - converted to inline function
> ---
> drivers/char/tpm/eventlog/of.c | 31 +--
> include/linux/tpm.h| 27 +++
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..f9462d19632e 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "../tpm.h"
> @@ -20,11 +21,10 @@
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> + int ret;
>
> log = >log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return -ENODEV;
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + ret = of_tpm_get_sml_parameters(np, , );
> + if (ret < 0)
> + return ret;
>
> if (size == 0) {
> dev_warn(>dev, "%s: Event log area empty\n", __func__);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index dfeb25a0362d..b3dff255bc58 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -460,4 +460,31 @@ static inline struct tpm_chip *tpm_default_chip(void)
> return NULL;
> }
> #endif
> +
> +#ifdef CONFIG_OF
> +static inline int of_tpm_get_sml_parameters(struct device_node *np,
> + u64 *base, u32 *size)
> +{
> + const u32 *sizep;
> + const u64 *basep;
> +
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return -ENODEV;
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> +
> + if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> + of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + *size = be32_to_cpup((__force __be32 *)sizep);
> + *base = be64_to_cpup((__force __be64 *)basep);
> + } else {
> + *size = *sizep;
> + *base = *basep;
> + }
> + return 0;
> +}
> +#endif
> +
> #endif
> --
> 2.35.1
>
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH v4 3/5] tpm: of: Make of-tree specific function commonly available
On Thu, Jun 30, 2022 at 10:26:01PM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() by moving reusable parts of the code into
> an inline function that makes it commonly available so it can be
> used also for kexec support. Call the new of_tpm_get_sml_parameters()
> function from the TPM Open Firmware driver.
>
> Signed-off-by: Stefan Berger
> Cc: Jarkko Sakkinen
> Cc: Jason Gunthorpe
> Cc: Rob Herring
> Cc: Frank Rowand
>
> ---
> v4:
> - converted to inline function
> ---
> drivers/char/tpm/eventlog/of.c | 31 +--
> include/linux/tpm.h| 27 +++
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..f9462d19632e 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "../tpm.h"
> @@ -20,11 +21,10 @@
> int tpm_read_log_of(struct tpm_chip *chip)
> {
> struct device_node *np;
> - const u32 *sizep;
> - const u64 *basep;
> struct tpm_bios_log *log;
> u32 size;
> u64 base;
> + int ret;
>
> log = >log;
> if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
> if (of_property_read_bool(np, "powered-while-suspended"))
> chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>
> - sizep = of_get_property(np, "linux,sml-size", NULL);
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (sizep == NULL && basep == NULL)
> - return -ENODEV;
> - if (sizep == NULL || basep == NULL)
> - return -EIO;
> -
> - /*
> - * For both vtpm/tpm, firmware has log addr and log size in big
> - * endian format. But in case of vtpm, there is a method called
> - * sml-handover which is run during kernel init even before
> - * device tree is setup. This sml-handover function takes care
> - * of endianness and writes to sml-base and sml-size in little
> - * endian format. For this reason, vtpm doesn't need conversion
> - * but physical tpm needs the conversion.
> - */
> - if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> - of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> - size = be32_to_cpup((__force __be32 *)sizep);
> - base = be64_to_cpup((__force __be64 *)basep);
> - } else {
> - size = *sizep;
> - base = *basep;
> - }
> + ret = of_tpm_get_sml_parameters(np, , );
> + if (ret < 0)
> + return ret;
>
> if (size == 0) {
> dev_warn(>dev, "%s: Event log area empty\n", __func__);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index dfeb25a0362d..b3dff255bc58 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -460,4 +460,31 @@ static inline struct tpm_chip *tpm_default_chip(void)
> return NULL;
> }
> #endif
> +
> +#ifdef CONFIG_OF
> +static inline int of_tpm_get_sml_parameters(struct device_node *np,
> + u64 *base, u32 *size)
> +{
> + const u32 *sizep;
> + const u64 *basep;
> +
> + sizep = of_get_property(np, "linux,sml-size", NULL);
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return -ENODEV;
> + if (sizep == NULL || basep == NULL)
> + return -EIO;
> +
> + if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> + of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> + *size = be32_to_cpup((__force __be32 *)sizep);
> + *base = be64_to_cpup((__force __be64 *)basep);
> + } else {
> + *size = *sizep;
> + *base = *basep;
> + }
> + return 0;
> +}
> +#endif
> +
> #endif
> --
> 2.35.1
>
Reviewed-by: Jarkko Sakkinen
BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
r...@esmil.dk>, Jordan Niethe , Atish Patra , Alexei Starovoitov , Will Deacon , Masahiro Yamada , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , "linux-par...@vger.kernel.org" , Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , "sparcli...@vger.kernel.org" , Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , "x...@kernel.org" , "Russell King \(Oracle\)" , "linux-ri...@lists.infradead.org" , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , "linux-m...@vger.kernel.org" , Changbin Du , Palmer Dabbelt , "linuxppc-dev@lists.ozlabs.org" , "linux-modu...@vger.kernel.org" Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Tue, Jun 14, 2022 at 12:36:25PM +, Christophe Leroy wrote: > > > Le 14/06/2022 à 14:26, Jarkko Sakkinen a écrit : > > On Thu, Jun 09, 2022 at 06:44:45AM -0700, Luis Chamberlain wrote: > >> On Thu, Jun 09, 2022 at 08:47:38AM +0100, Russell King (Oracle) wrote: > >>> On Wed, Jun 08, 2022 at 02:59:27AM +0300, Jarkko Sakkinen wrote: > >>>> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile > >>>> index 553866751e1a..d2bb954cd54f 100644 > >>>> --- a/arch/arm/kernel/Makefile > >>>> +++ b/arch/arm/kernel/Makefile > >>>> @@ -44,6 +44,11 @@ obj-$(CONFIG_CPU_IDLE)+= cpuidle.o > >>>> obj-$(CONFIG_ISA_DMA_API) += dma.o > >>>> obj-$(CONFIG_FIQ) += fiq.o fiqasm.o > >>>> obj-$(CONFIG_MODULES) += armksyms.o module.o > >>>> +ifeq ($(CONFIG_MODULES),y) > >>>> +obj-y += module_alloc.o > >>>> +else > >>>> +obj-$(CONFIG_KPROBES) += module_alloc.o > >>>> +endif > >>> > >>> Doesn't: > >>> > >>> obj-$(CONFIG_MODULES) += module_alloc.o > >>> obj-$(CONFIG_KPROBES) += module_alloc.o > >> > >> That just begs for a new kconfig symbol for the object, and for > >> the object then to be built with it. > >> > >> The archs which override the default can use ARCH_HAS_VM_ALLOC_EXEC. > >> Please note that the respective free is important as well and its > >> not clear if we need an another define for the free. Someone has > >> to do that work. We want to ensure to noexec the code on free and > >> this can vary on each arch. > > > > Let me check if I understand this (not 100% sure). > > > > So if arch define ARCH_HAS_VMALLOC_EXEC, then this would set > > config flag CONFIG_VMALLOC_EXEC, which would be used to include > > the compilation unit? > > > > I guess you have two possible approaches. > > Either architectures select CONFIG_ARCH_HAS_VMALLOC_EXEC at all time and > then you add a CONFIG_VMALLOC_EXEC which depends on > CONFIG_ARCH_HAS_VMALLOC_EXEC and CONFIG_MODULES or CONFIG_KPROBES, > > Or architectures select CONFIG_ARCH_HAS_VMALLOC_EXEC only when either > CONFIG_MODULES or CONFIG_KPROBES is selected, in that case there is no > need for a CONFIG_VMALLOC_EXEC. Right, got it now. Thanks for the elaboration. > Christophe BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
kkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , linux-par...@vger.kernel.org, Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparcli...@vger.kernel.org, Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , X86 ML , Russell King , linux-ri...@lists.infradead.org, Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , linux-m...@vger.kernel.org, Changbin Du , Palmer Dabbelt , linuxppc-dev@lists.ozlabs.org, linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Sun, Jun 12, 2022 at 09:30:41PM +0900, Masami Hiramatsu wrote: > On Wed, 8 Jun 2022 11:19:19 -0700 > Song Liu wrote: > > > On Wed, Jun 8, 2022 at 9:28 AM Ard Biesheuvel wrote: > > > > > > Hello Jarkko, > > > > > > On Wed, 8 Jun 2022 at 02:02, Jarkko Sakkinen wrote: > > > > > > > > Tracing with kprobes while running a monolithic kernel is currently > > > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. This > > > > dependency is a result of kprobes code using the module allocator for > > > > the > > > > trampoline code. > > > > > > > > Detaching kprobes from modules helps to squeeze down the user space, > > > > e.g. when developing new core kernel features, while still having all > > > > the nice tracing capabilities. > > > > > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() to > > > > module_alloc.c, and compile as part of vmlinux when either > > > > CONFIG_MODULES > > > > or CONFIG_KPROBES is enabled. In addition, flag kernel module specific > > > > code with CONFIG_MODULES. > > > > > > > > As the result, kprobes can be used with a monolithic kernel. > > > > > > I think I may have mentioned this the previous time as well, but I > > > don't think this is the right approach. > > > > > > Kprobes uses alloc_insn_page() to allocate executable memory, but the > > > requirements for this memory are radically different compared to > > > loadable modules, which need to be within an arch-specific distance of > > > the core kernel, need KASAN backing etc etc. > > > > I think the distance of core kernel requirement is the same for kprobe > > alloc_insn_page and modules, no? > > This strongly depends on how kprobes (software breakpoint and > single-step) is implemented on the arch. For example, x86 implements > the so-called "kprobe-booster" which jumps back from the single > stepping trampoline buffer. Then the buffer address must be within > the range where it can jump to the original address. > However, if the arch implements single-step as an instruction > emulation, it has no such limitation. As far as I know, arm64 > will do emulation for the instructions which change PC register > and will do direct execution with another software breakpoint > for other instructions. > > Why I'm using module_alloc() for a generic function, is that > can cover the limitation most widely. > Thus, if we have CONFIG_ARCH_HAVE_ALLOC_INSN_PAGE flag and > kprobes can check it instead of using __weak function, the > kprobes may not need to depend on module_alloc() in general. OK, I guess this is what Luis meant. I'll try to carve up something based on this. BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
@kernel.org>, Masahiro Yamada , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , linux-par...@vger.kernel.org, Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparcli...@vger.kernel.org, Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , x...@kernel.org, "Russell King \(Oracle\)" , linux-ri...@lists.infradead.org, Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , linux-m...@vger.kernel.org, Changbin Du , Palmer Dabbelt , linuxppc-dev@lists.ozlabs.org, linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jun 09, 2022 at 06:44:45AM -0700, Luis Chamberlain wrote: > On Thu, Jun 09, 2022 at 08:47:38AM +0100, Russell King (Oracle) wrote: > > On Wed, Jun 08, 2022 at 02:59:27AM +0300, Jarkko Sakkinen wrote: > > > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile > > > index 553866751e1a..d2bb954cd54f 100644 > > > --- a/arch/arm/kernel/Makefile > > > +++ b/arch/arm/kernel/Makefile > > > @@ -44,6 +44,11 @@ obj-$(CONFIG_CPU_IDLE) += cpuidle.o > > > obj-$(CONFIG_ISA_DMA_API)+= dma.o > > > obj-$(CONFIG_FIQ)+= fiq.o fiqasm.o > > > obj-$(CONFIG_MODULES)+= armksyms.o module.o > > > +ifeq ($(CONFIG_MODULES),y) > > > +obj-y+= module_alloc.o > > > +else > > > +obj-$(CONFIG_KPROBES)+= module_alloc.o > > > +endif > > > > Doesn't: > > > > obj-$(CONFIG_MODULES) += module_alloc.o > > obj-$(CONFIG_KPROBES) += module_alloc.o > > That just begs for a new kconfig symbol for the object, and for > the object then to be built with it. > > The archs which override the default can use ARCH_HAS_VM_ALLOC_EXEC. > Please note that the respective free is important as well and its > not clear if we need an another define for the free. Someone has > to do that work. We want to ensure to noexec the code on free and > this can vary on each arch. Let me check if I understand this (not 100% sure). So if arch define ARCH_HAS_VMALLOC_EXEC, then this would set config flag CONFIG_VMALLOC_EXEC, which would be used to include the compilation unit? BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
l...@kernel.org>, Masahiro Yamada , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , Parisc List , Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparclinux , Tiezhu Yang , Miroslav Benes , Chen Zhongjin , linux-riscv , the arch/x86 maintainers , Russell King , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , "open list:BROADCOM NVRAM DRIVER" , Changbin Du , Palmer Dabbelt , linuxppc-dev , linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jun 09, 2022 at 03:23:16PM +0200, Ard Biesheuvel wrote: > On Thu, 9 Jun 2022 at 15:14, Jarkko Sakkinen wrote: > > > > On Wed, Jun 08, 2022 at 09:12:34AM -0700, Song Liu wrote: > > > On Wed, Jun 8, 2022 at 7:21 AM Masami Hiramatsu > > > wrote: > > > > > > > > Hi Jarkko, > > > > > > > > On Wed, 8 Jun 2022 08:25:38 +0300 > > > > Jarkko Sakkinen wrote: > > > > > > > > > On Wed, Jun 08, 2022 at 10:35:42AM +0800, Guo Ren wrote: > > > > > > . > > > > > > > > > > > > On Wed, Jun 8, 2022 at 8:02 AM Jarkko Sakkinen > > > > > > wrote: > > > > > > > > > > > > > > Tracing with kprobes while running a monolithic kernel is > > > > > > > currently > > > > > > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. > > > > > > > This > > > > > > > dependency is a result of kprobes code using the module allocator > > > > > > > for the > > > > > > > trampoline code. > > > > > > > > > > > > > > Detaching kprobes from modules helps to squeeze down the user > > > > > > > space, > > > > > > > e.g. when developing new core kernel features, while still having > > > > > > > all > > > > > > > the nice tracing capabilities. > > > > > > > > > > > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() > > > > > > > to > > > > > > > module_alloc.c, and compile as part of vmlinux when either > > > > > > > CONFIG_MODULES > > > > > > > or CONFIG_KPROBES is enabled. In addition, flag kernel module > > > > > > > specific > > > > > > > code with CONFIG_MODULES. > > > > > > > > > > > > > > As the result, kprobes can be used with a monolithic kernel. > > > > > > It's strange when MODULES is n, but vmlinux still obtains > > > > > > module_alloc. > > > > > > > > > > > > Maybe we need a kprobe_alloc, right? > > > > > > > > > > Perhaps not the best name but at least it documents the fact that > > > > > they use the same allocator. > > > > > > > > > > Few years ago I carved up something "half-way there" for kprobes, > > > > > and I used the name text_alloc() [*]. > > > > > > > > > > [*] > > > > > https://lore.kernel.org/all/20200724050553.1724168-1-jarkko.sakki...@linux.intel.com/ > > > > > > > > Yeah, I remember that. Thank you for updating your patch! > > > > I think the idea (split module_alloc() from CONFIG_MODULE) is good to > > > > me. > > > > If module support maintainers think this name is not good, you may be > > > > able to rename it as text_alloc() and make the module_alloc() as a > > > > wrapper of it. > > > > > > IIUC, most users of module_alloc() use it to allocate memory for text, > > > except > > > that module code uses it for both text and data. Therefore, I guess > > > calling it > > > text_alloc() is not 100% accurate until we change the module code (to use > > > a different API to allocate memory for data). > > > > After reading the feedback, I'd stay on using module_alloc() because > > it has arch-specific quirks baked in. Easier to deal with them in one > > place. > > > > In that case, please ensure that you enable this only on architectures > where it is needed. arm64 implements alloc_insn_page() without relying > on module_alloc() so I would not expect to see any changes there. Right, got it, thanks for remark. BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
ada , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , Parisc List , Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparclinux , Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , the arch/x86 maintainers , Russell King , linux-riscv , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , "open list:BROADCOM NVRAM DRIVER" , Changbin Du , Palmer Dabbelt , linuxppc-dev , linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Jun 08, 2022 at 09:12:34AM -0700, Song Liu wrote: > On Wed, Jun 8, 2022 at 7:21 AM Masami Hiramatsu wrote: > > > > Hi Jarkko, > > > > On Wed, 8 Jun 2022 08:25:38 +0300 > > Jarkko Sakkinen wrote: > > > > > On Wed, Jun 08, 2022 at 10:35:42AM +0800, Guo Ren wrote: > > > > . > > > > > > > > On Wed, Jun 8, 2022 at 8:02 AM Jarkko Sakkinen > > > > wrote: > > > > > > > > > > Tracing with kprobes while running a monolithic kernel is currently > > > > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. > > > > > This > > > > > dependency is a result of kprobes code using the module allocator for > > > > > the > > > > > trampoline code. > > > > > > > > > > Detaching kprobes from modules helps to squeeze down the user space, > > > > > e.g. when developing new core kernel features, while still having all > > > > > the nice tracing capabilities. > > > > > > > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() to > > > > > module_alloc.c, and compile as part of vmlinux when either > > > > > CONFIG_MODULES > > > > > or CONFIG_KPROBES is enabled. In addition, flag kernel module > > > > > specific > > > > > code with CONFIG_MODULES. > > > > > > > > > > As the result, kprobes can be used with a monolithic kernel. > > > > It's strange when MODULES is n, but vmlinux still obtains module_alloc. > > > > > > > > Maybe we need a kprobe_alloc, right? > > > > > > Perhaps not the best name but at least it documents the fact that > > > they use the same allocator. > > > > > > Few years ago I carved up something "half-way there" for kprobes, > > > and I used the name text_alloc() [*]. > > > > > > [*] > > > https://lore.kernel.org/all/20200724050553.1724168-1-jarkko.sakki...@linux.intel.com/ > > > > Yeah, I remember that. Thank you for updating your patch! > > I think the idea (split module_alloc() from CONFIG_MODULE) is good to me. > > If module support maintainers think this name is not good, you may be > > able to rename it as text_alloc() and make the module_alloc() as a > > wrapper of it. > > IIUC, most users of module_alloc() use it to allocate memory for text, except > that module code uses it for both text and data. Therefore, I guess calling it > text_alloc() is not 100% accurate until we change the module code (to use > a different API to allocate memory for data). After reading the feedback, I'd stay on using module_alloc() because it has arch-specific quirks baked in. Easier to deal with them in one place. > Thanks, > Song BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
o...@kernel.org>, Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , Parisc List , Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparclinux , Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , the arch/x86 maintainers , Russell King , linux-riscv , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , "open list:BROADCOM NVRAM DRIVER" , Changbin Du , Palmer Dabbelt , linuxppc-dev , linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Jun 08, 2022 at 11:21:15PM +0900, Masami Hiramatsu wrote: > Hi Jarkko, > > On Wed, 8 Jun 2022 08:25:38 +0300 > Jarkko Sakkinen wrote: > > > On Wed, Jun 08, 2022 at 10:35:42AM +0800, Guo Ren wrote: > > > . > > > > > > On Wed, Jun 8, 2022 at 8:02 AM Jarkko Sakkinen wrote: > > > > > > > > Tracing with kprobes while running a monolithic kernel is currently > > > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. This > > > > dependency is a result of kprobes code using the module allocator for > > > > the > > > > trampoline code. > > > > > > > > Detaching kprobes from modules helps to squeeze down the user space, > > > > e.g. when developing new core kernel features, while still having all > > > > the nice tracing capabilities. > > > > > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() to > > > > module_alloc.c, and compile as part of vmlinux when either > > > > CONFIG_MODULES > > > > or CONFIG_KPROBES is enabled. In addition, flag kernel module specific > > > > code with CONFIG_MODULES. > > > > > > > > As the result, kprobes can be used with a monolithic kernel. > > > It's strange when MODULES is n, but vmlinux still obtains module_alloc. > > > > > > Maybe we need a kprobe_alloc, right? > > > > Perhaps not the best name but at least it documents the fact that > > they use the same allocator. > > > > Few years ago I carved up something "half-way there" for kprobes, > > and I used the name text_alloc() [*]. > > > > [*] > > https://lore.kernel.org/all/20200724050553.1724168-1-jarkko.sakki...@linux.intel.com/ > > > > Yeah, I remember that. Thank you for updating your patch! > I think the idea (split module_alloc() from CONFIG_MODULE) is good to me. > If module support maintainers think this name is not good, you may be > able to rename it as text_alloc() and make the module_alloc() as a > wrapper of it. > > Acked-by: Masami Hiramatsu (Google) > for kprobe side. Thanks a lot! If I split that code into its own patch with no code changes, can I attach this to the patch? I.e. most likely I'll split arch's into their own patches. > Thank you, > > -- > Masami Hiramatsu (Google) BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
Alexei Starovoitov , Will Deacon , Masahiro Yamada , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , "linux-par...@vger.kernel.org" , Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , "sparcli...@vger.kernel.org" , Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , "x...@kernel.org" , Russell King , "linux-ri...@lists.infradead.org" , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , "linux-m...@vger.kernel.org" , Changbin Du , Palmer Dabbelt , "linuxppc-dev@lists.ozlabs.org" , "linux-modu...@vger.kernel.org" Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jun 09, 2022 at 08:30:12AM +, Christophe Leroy wrote: > > > Le 08/06/2022 à 01:59, Jarkko Sakkinen a écrit : > > [You don't often get email from jar...@profian.com. Learn why this is > > important at https://aka.ms/LearnAboutSenderIdentification ] > > > > Tracing with kprobes while running a monolithic kernel is currently > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. This > > dependency is a result of kprobes code using the module allocator for the > > trampoline code. > > > > Detaching kprobes from modules helps to squeeze down the user space, > > e.g. when developing new core kernel features, while still having all > > the nice tracing capabilities. > > Nice idea, could also be nice to have BPF without MODULES. Yeah, for sure. You have to start from somewhere :-) I'd guess this a step forward also for BPF. > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() to > > module_alloc.c, and compile as part of vmlinux when either CONFIG_MODULES > > or CONFIG_KPROBES is enabled. In addition, flag kernel module specific > > code with CONFIG_MODULES. > > Nice, but that's not enough. You have to audit every peace of code that > depends on CONFIG_MODULES and see if it needs to be activated for your > case as well. For instance some powerpc configurations don't honor exec > page faults on kernel pages when CONFIG_MODULES is not selected. Thanks for pointing this out. With "every peace of code" you probably are referring to the 13 arch-folders, which support kprobes in the first place (just checking)? > > As the result, kprobes can be used with a monolithic kernel. > > > > Signed-off-by: Jarkko Sakkinen > > I think this patch should be split in a several patches, one (or even > one per architectures ?) to make modules_alloc() independant of > CONFIG_MODULES, then a patch to make CONFIG_KPROBES independant on > CONFIG_MOUDLES. Agreed. And also because of your previous remark, i.e. each arch needs it own conclusions of the changes. I purposely did this first as a one patch in order to get a better picture of the situation. > > --- > > Tested with the help of BuildRoot and QEMU: > > - arm (function tracer) > > - arm64 (function tracer) > > - mips (function tracer) > > - powerpc (function tracer) > > - riscv (function tracer) > > - s390 (function tracer) > > - sparc (function tracer) > > - x86 (function tracer) > > - sh (function tracer, for the "pure" kernel/modules_alloc.c path) > > --- > > arch/Kconfig | 1 - > > arch/arm/kernel/Makefile | 5 +++ > > arch/arm/kernel/module.c | 32 > > arch/arm/kernel/module_alloc.c | 42 > > arch/arm64/kernel/Makefile | 5 +++ > > arch/arm64/kernel/module.c | 47 --- > > arch/arm64/kernel/module_alloc.c | 57 > > arch/mips/kernel/Makefile | 5 +++ > > arch/mips/kernel/module.c | 9 - > > arch/mips/kernel/module_alloc.c| 18 + > > arch/parisc/kernel/Makefile| 5 +++ > > arch/parisc/kernel/module.c| 11 -- > > arch/parisc/kernel/module_alloc.c | 23 +++ > > arch/powerpc/kernel/Makefile | 5 +++ > > arch/powerpc/kernel/module.c | 37 -- > > arch/powerpc/kernel/module_alloc.c | 47 +++ > > You are missing necessary changes for powerpc. > > On powerpc 8xx or powerpc
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
>, Jarkko Sakkinen , Sami Tolvanen >, "Naveen N. Rao" , Marco >Elver , Kees Cook , Steven Rostedt >, Nathan Chancellor , Mark Brown >, Borislav Petkov , Alexander Egorenkov >, Thomas Bogendoerfer , >linux-par...@vger.kernel.org, Nathaniel McCallum , >Dmitry Torokhov , "David S. Miller" >, "Kirill A. Shutemov" , >Tobias Huschle , "Peter Zijlstra \(Intel\)" >, "H. Peter Anvin" , >sparcli...@vger.kernel.org, Tiezhu Yang , Miroslav >Benes , Chen Zhongjin , Ard >Biesheuvel , x...@kernel.org, >linux-ri...@lists.infradead.org, In go Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , linux-m...@vger.kernel.org, Changbin Du , Palmer Dabbelt , linuxppc-dev@lists.ozlabs.org, linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jun 09, 2022 at 08:47:38AM +0100, Russell King (Oracle) wrote: > On Wed, Jun 08, 2022 at 02:59:27AM +0300, Jarkko Sakkinen wrote: > > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile > > index 553866751e1a..d2bb954cd54f 100644 > > --- a/arch/arm/kernel/Makefile > > +++ b/arch/arm/kernel/Makefile > > @@ -44,6 +44,11 @@ obj-$(CONFIG_CPU_IDLE) += cpuidle.o > > obj-$(CONFIG_ISA_DMA_API) += dma.o > > obj-$(CONFIG_FIQ) += fiq.o fiqasm.o > > obj-$(CONFIG_MODULES) += armksyms.o module.o > > +ifeq ($(CONFIG_MODULES),y) > > +obj-y += module_alloc.o > > +else > > +obj-$(CONFIG_KPROBES) += module_alloc.o > > +endif > > Doesn't: > > obj-$(CONFIG_MODULES) += module_alloc.o > obj-$(CONFIG_KPROBES) += module_alloc.o > > work just as well? The kbuild modules.rst documentation says: > > The order of files in $(obj-y) is significant. Duplicates in > the lists are allowed: the first instance will be linked into > built-in.a and succeeding instances will be ignored. > > so you should be fine... or the documentation is wrong! OK, I did not know this. Thanks for the tip! BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
Yamada , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , linux-par...@vger.kernel.org, Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparcli...@vger.kernel.org, Tiezhu Yang , Miroslav Benes , Chen Zhongjin , l inux-ri...@lists.infradead.org, X86 ML , Russell King , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , linux-m...@vger.kernel.org, Changbin Du , Palmer Dabbelt , linuxppc-dev@lists.ozlabs.org, linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Jun 08, 2022 at 06:27:51PM +0200, Ard Biesheuvel wrote: > Hello Jarkko, > > On Wed, 8 Jun 2022 at 02:02, Jarkko Sakkinen wrote: > > > > Tracing with kprobes while running a monolithic kernel is currently > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. This > > dependency is a result of kprobes code using the module allocator for the > > trampoline code. > > > > Detaching kprobes from modules helps to squeeze down the user space, > > e.g. when developing new core kernel features, while still having all > > the nice tracing capabilities. > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() to > > module_alloc.c, and compile as part of vmlinux when either CONFIG_MODULES > > or CONFIG_KPROBES is enabled. In addition, flag kernel module specific > > code with CONFIG_MODULES. > > > > As the result, kprobes can be used with a monolithic kernel. > > I think I may have mentioned this the previous time as well, but I > don't think this is the right approach. OK, I apologize for my ignorance. It's been a while. > Kprobes uses alloc_insn_page() to allocate executable memory, but the > requirements for this memory are radically different compared to > loadable modules, which need to be within an arch-specific distance of > the core kernel, need KASAN backing etc etc. > > This is why arm64, for instance, does not implement alloc_insn_page() > in terms of module_alloc() [and likely does not belong in this patch > for that reason] > > Is there any reason kprobes cannot simply use vmalloc()? All arch's, except nios2 use vmalloc() in the end for module_alloc(). nios2 uses kmalloc() for the reasons that I'm not aware of, but it does not support kprobes in the first place. Based on this, I think that could work out just fine. I could cope with that. BR, Jarkko
Re: [PATCH] kprobes: Enable tracing for mololithic kernel images
da , Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , Parisc List , Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparclinux , Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , the arch/x86 maintainers , Russell King , linux-riscv , Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , "open list:BROADCOM NVRAM DRIVER" , Changbin Du , Palmer Dabbelt , linuxppc-dev , linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Jun 08, 2022 at 10:35:42AM +0800, Guo Ren wrote: > . > > On Wed, Jun 8, 2022 at 8:02 AM Jarkko Sakkinen wrote: > > > > Tracing with kprobes while running a monolithic kernel is currently > > impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. This > > dependency is a result of kprobes code using the module allocator for the > > trampoline code. > > > > Detaching kprobes from modules helps to squeeze down the user space, > > e.g. when developing new core kernel features, while still having all > > the nice tracing capabilities. > > > > For kernel/ and arch/*, move module_alloc() and module_memfree() to > > module_alloc.c, and compile as part of vmlinux when either CONFIG_MODULES > > or CONFIG_KPROBES is enabled. In addition, flag kernel module specific > > code with CONFIG_MODULES. > > > > As the result, kprobes can be used with a monolithic kernel. > It's strange when MODULES is n, but vmlinux still obtains module_alloc. > > Maybe we need a kprobe_alloc, right? Perhaps not the best name but at least it documents the fact that they use the same allocator. Few years ago I carved up something "half-way there" for kprobes, and I used the name text_alloc() [*]. [*] https://lore.kernel.org/all/20200724050553.1724168-1-jarkko.sakki...@linux.intel.com/ BR, Jarkko
[PATCH] kprobes: Enable tracing for mololithic kernel images
nel.org>, Jarkko Sakkinen , Sami Tolvanen , "Naveen N. Rao" , Marco Elver , Kees Cook , Steven Rostedt , Nathan Chancellor , "Russell King \(Oracle\)" , Mark Brown , Borislav Petkov , Alexander Egorenkov , Thomas Bogendoerfer , linux-par...@vger.kernel.org, Nathaniel McCallum , Dmitry Torokhov , "David S. Miller" , "Kirill A. Shutemov" , Tobias Huschle , "Peter Zijlstra \(Intel\)" , "H. Peter Anvin" , sparcli...@vger.kernel.org, Tiezhu Yang , Miroslav Benes , Chen Zhongjin , Ard Biesheuvel , x...@kernel.org, Russell King , linux-ri...@lists.infradead.org, Ingo Molnar , Aaron Tomlin , Albert Ou , Heiko Carstens , Liao Chang , Paul Walmsley , Josh Poimboeuf , Thomas Richter , linux-m...@vger.kernel.org, Changbin Du , Palmer Dabbelt , linuxppc-dev@lists.ozlabs.org, linux-modu...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" Tracing with kprobes while running a monolithic kernel is currently impossible because CONFIG_KPROBES is dependent of CONFIG_MODULES. This dependency is a result of kprobes code using the module allocator for the trampoline code. Detaching kprobes from modules helps to squeeze down the user space, e.g. when developing new core kernel features, while still having all the nice tracing capabilities. For kernel/ and arch/*, move module_alloc() and module_memfree() to module_alloc.c, and compile as part of vmlinux when either CONFIG_MODULES or CONFIG_KPROBES is enabled. In addition, flag kernel module specific code with CONFIG_MODULES. As the result, kprobes can be used with a monolithic kernel. Signed-off-by: Jarkko Sakkinen --- Tested with the help of BuildRoot and QEMU: - arm (function tracer) - arm64 (function tracer) - mips (function tracer) - powerpc (function tracer) - riscv (function tracer) - s390 (function tracer) - sparc (function tracer) - x86 (function tracer) - sh (function tracer, for the "pure" kernel/modules_alloc.c path) --- arch/Kconfig | 1 - arch/arm/kernel/Makefile | 5 +++ arch/arm/kernel/module.c | 32 arch/arm/kernel/module_alloc.c | 42 arch/arm64/kernel/Makefile | 5 +++ arch/arm64/kernel/module.c | 47 --- arch/arm64/kernel/module_alloc.c | 57 arch/mips/kernel/Makefile | 5 +++ arch/mips/kernel/module.c | 9 - arch/mips/kernel/module_alloc.c| 18 + arch/parisc/kernel/Makefile| 5 +++ arch/parisc/kernel/module.c| 11 -- arch/parisc/kernel/module_alloc.c | 23 +++ arch/powerpc/kernel/Makefile | 5 +++ arch/powerpc/kernel/module.c | 37 -- arch/powerpc/kernel/module_alloc.c | 47 +++ arch/riscv/kernel/Makefile | 5 +++ arch/riscv/kernel/module.c | 10 - arch/riscv/kernel/module_alloc.c | 19 ++ arch/s390/kernel/Makefile | 5 +++ arch/s390/kernel/module.c | 17 - arch/s390/kernel/module_alloc.c| 33 arch/sparc/kernel/Makefile | 5 +++ arch/sparc/kernel/module.c | 30 --- arch/sparc/kernel/module_alloc.c | 39 +++ arch/x86/kernel/Makefile | 5 +++ arch/x86/kernel/module.c | 50 arch/x86/kernel/module_alloc.c | 61 ++ kernel/Makefile| 5 +++ kernel/kprobes.c | 10 + kernel/module/main.c | 17 - kernel/module_alloc.c | 26 + kernel/trace/trace_kprobe.c| 10 - 33 files changed, 434 insertions(+), 262 deletions(-) create mode 100644 arch/arm/kernel/module_alloc.c create mode 100644 arch/arm64/kernel/module_alloc.c create mode 100644 arch/mips/kernel/module_alloc.c create mode 100644 arch/parisc/kernel/module_alloc.c create mode 100644 arch/powerpc/kernel/module_alloc.c create mode 100644 arch/riscv/kernel/module_alloc.c create mode 100644 arch/s390/kernel/module_alloc.c create mode 100644 arch/sparc/kernel/module_alloc.c create mode 100644 arch/x86/kernel/module_alloc.c create mode 100644 kernel/module_alloc.c diff --git a/arch/Kconfig b/arch/Kconfig index fcf9a41a4ef5..e8e3e7998a2e 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -39,7 +39,6 @@ config GENERIC_ENTRY config KPROBES bool "Kprobes" - depends on MODULES depends on HAVE_KPROBES select KALLSYMS select TASKS_RCU if PREEMPTION diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index 553866751e1a..d2bb954cd54f 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -44,6 +44,11 @@ obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_ISA_DMA_API) += dma.o obj-$(CONF
Re: [PATCH] char: tpm: Prepare cleanup of powerpc's asm/prom.h
On Sat, Apr 02, 2022 at 12:29:19PM +0200, Christophe Leroy wrote:
> powerpc's asm/prom.h brings some headers that it doesn't
> need itself.
>
> In order to clean it up, first add missing headers in
> users of asm/prom.h
>
> Signed-off-by: Christophe Leroy
I don't understand this. It changes nothing as far as kernel is concerned.
> ---
> drivers/char/tpm/tpm_atmel.h | 2 --
> drivers/char/tpm/tpm_ibmvtpm.c | 1 -
> 2 files changed, 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> index ba37e77e8af3..959f7cce8301 100644
> --- a/drivers/char/tpm/tpm_atmel.h
> +++ b/drivers/char/tpm/tpm_atmel.h
> @@ -26,8 +26,6 @@ struct tpm_atmel_priv {
>
> #ifdef CONFIG_PPC64
>
> -#include
> -
> #define atmel_getb(priv, offset) readb(priv->iobase + offset)
> #define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> #define atmel_request_region request_mem_region
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 3af4c07a9342..1180cce7067a 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -20,7 +20,6 @@
> #include
> #include
> #include
> -#include
>
> #include "tpm.h"
> #include "tpm_ibmvtpm.h"
> --
> 2.35.1
>
BR, Jarkko
Re: [PATCH -next] tpm: ibmvtpm: Correct the return value in tpm_ibmvtpm_probe()
On Fri, Mar 18, 2022 at 09:54:46AM -0400, Stefan Berger wrote:
>
>
> On 3/18/22 02:02, Xiu Jianfeng wrote:
> > Currently it returns zero when CRQ response timed out, it should return
> > an error code instead.
> >
> > Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> > proceeding")
> > Signed-off-by: Xiu Jianfeng
>
> Reviewed-by: Stefan Berger
Thank you, I applied this.
BR, Jarkko
Re: [PATCH -next] tpm: ibmvtpm: Correct the return value in tpm_ibmvtpm_probe()
On Fri, Mar 18, 2022 at 02:02:01PM +0800, Xiu Jianfeng wrote:
> Currently it returns zero when CRQ response timed out, it should return
> an error code instead.
>
> Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> proceeding")
> Signed-off-by: Xiu Jianfeng
> ---
> drivers/char/tpm/tpm_ibmvtpm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 3af4c07a9342..d3989b257f42 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -681,6 +681,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
> if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
> ibmvtpm->rtce_buf != NULL,
> HZ)) {
> + rc = -ENODEV;
> dev_err(dev, "CRQ response timed out\n");
> goto init_irq_cleanup;
> }
> --
> 2.17.1
>
Acked-by: Jarkko Sakkinen
This will require reviewed by from someone who knows this driver
better.
BR, Jarkko
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
On Tue, Dec 21, 2021 at 09:01:06AM -0500, Stefan Berger wrote:
>
> On 12/21/21 03:47, Jarkko Sakkinen wrote:
> > On Sat, Dec 11, 2021 at 08:28:04PM -0500, Stefan Berger wrote:
> > > Fix the following crash on kexec by checking chip->ops for a NULL pointer
> > > in tpm_chip_start() and returning an error code if this is the case.
> > >
> > > BUG: Kernel NULL pointer dereference on read at 0x0060
> > > Faulting instruction address: 0xc099a06c
> > > Oops: Kernel access of bad area, sig: 11 [#1]
> > > ...
> > > NIP [c099a06c] tpm_chip_start+0x2c/0x140
> > > LR [c099a808] tpm_chip_unregister+0x108/0x170
> > > Call Trace:
> > > [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8
> > > (unreliable)
> > > [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
> > > [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
> > > [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
> > > [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
> > > [c000188bfbc0] [c0196e14] kernel_restart_prepare+0x54/0x70
> > >
> > > The referenced patch below introduced a function to shut down the VIO bus.
> > > The bus shutdown now calls tpm_del_char_device (via tpm_chip_unregister)
> > > after a call to tpm_class_shutdown, which already set chip->ops to NULL.
> > > The crash occurrs when tpm_del_char_device calls tpm_chip_start with the
> > > chip->ops NULL pointer.
> > >
> > > Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver and
> > > vio_bus")
> > > Signed-off-by: Stefan Berger
> > > ---
> > > drivers/char/tpm/tpm-chip.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index ddaeceb7e109..cca1bde296ee 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -101,6 +101,9 @@ int tpm_chip_start(struct tpm_chip *chip)
> > > {
> > > int ret;
> > > + if (!chip->ops)
> > > + return -EINVAL;
> > This triggers to all drivers, not just tpm_ibmvtpm, i.e. the fix has
> > side-effects.
>
> What are those side-effects?
It does change behaviour for all drivers, which is not acceptable for a
bug fix.
/Jarkko
Re: [PATCH] tpm: Fix kexec crash due to access to ops NULL pointer (powerpc)
On Sat, Dec 11, 2021 at 08:28:04PM -0500, Stefan Berger wrote:
> Fix the following crash on kexec by checking chip->ops for a NULL pointer
> in tpm_chip_start() and returning an error code if this is the case.
>
> BUG: Kernel NULL pointer dereference on read at 0x0060
> Faulting instruction address: 0xc099a06c
> Oops: Kernel access of bad area, sig: 11 [#1]
> ...
> NIP [c099a06c] tpm_chip_start+0x2c/0x140
> LR [c099a808] tpm_chip_unregister+0x108/0x170
> Call Trace:
> [c000188bfa00] [c2b03930] fw_devlink_strict+0x0/0x8 (unreliable)
> [c000188bfa30] [c099a808] tpm_chip_unregister+0x108/0x170
> [c000188bfa70] [c09a3874] tpm_ibmvtpm_remove+0x34/0x130
> [c000188bfae0] [c0110dbc] vio_bus_remove+0x5c/0xb0
> [c000188bfb20] [c09bc154] device_shutdown+0x1d4/0x3a8
> [c000188bfbc0] [c0196e14] kernel_restart_prepare+0x54/0x70
>
> The referenced patch below introduced a function to shut down the VIO bus.
> The bus shutdown now calls tpm_del_char_device (via tpm_chip_unregister)
> after a call to tpm_class_shutdown, which already set chip->ops to NULL.
> The crash occurrs when tpm_del_char_device calls tpm_chip_start with the
> chip->ops NULL pointer.
>
> Fixes: 39d0099f9439 ("powerpc/pseries: Add shutdown() to vio_driver and
> vio_bus")
> Signed-off-by: Stefan Berger
> ---
> drivers/char/tpm/tpm-chip.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb7e109..cca1bde296ee 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -101,6 +101,9 @@ int tpm_chip_start(struct tpm_chip *chip)
> {
> int ret;
>
> + if (!chip->ops)
> + return -EINVAL;
This triggers to all drivers, not just tpm_ibmvtpm, i.e. the fix has
side-effects.
/Jarkko
Re: [PATCH] tpm: ibmvtpm: Make use of dma_alloc_coherent()
On Tue, 2021-10-12 at 12:43 -0300, Jason Gunthorpe wrote: > On Tue, Oct 12, 2021 at 06:29:58PM +0300, Jarkko Sakkinen wrote: > > On Mon, 2021-10-11 at 00:01 +0800, Cai Huoqing wrote: > > > Replacing kmalloc/kfree/get_zeroed_page/free_page/dma_map_single/ > > ~ > > Replace > > > > > dma_unmap_single() with dma_alloc_coherent/dma_free_coherent() > > > helps to reduce code size, and simplify the code, and coherent > > > DMA will not clear the cache every time. > > > > > > Signed-off-by: Cai Huoqing > > > > If this does not do functionally anything useful, there's no > > reason to apply this. > > At least in this case it looks like the ibmvtpm is not using the DMA > API properly. There is no sync after each data transfer. Replacing > this wrong usage with the coherent API is reasonable. Thank you. As long as this is documented to the commit message, I'm cool with the change itself. E.g. something like this would be perfectly fine replacement for the current commit message: "The current usage pattern for the DMA API is inappropriate, as data transfers are not synced. Replace the existing DMA code with the coherent DMA API." /Jarkko
Re: [PATCH] tpm: ibmvtpm: Make use of dma_alloc_coherent()
On Mon, 2021-10-11 at 00:01 +0800, Cai Huoqing wrote: > Replacing kmalloc/kfree/get_zeroed_page/free_page/dma_map_single/ ~ Replace > dma_unmap_single() with dma_alloc_coherent/dma_free_coherent() > helps to reduce code size, and simplify the code, and coherent > DMA will not clear the cache every time. > > Signed-off-by: Cai Huoqing If this does not do functionally anything useful, there's no reason to apply this. It is also missing information why the substitution is possible. Field tested code is better than clean code, i.e. we don not risk at having possible new regressions just for a bit nicer layout... /Jarkko
Re: [PATCH v2] tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()
On Fri, 2021-01-29 at 13:57 -0500, Stefan Berger wrote:
> On 1/29/21 12:35 PM, Jarkko Sakkinen wrote:
> > On Mon, Jan 25, 2021 at 08:47:53PM -0500, Stefan Berger wrote:
> > > From: Stefan Berger
> > >
> > > Return error code -ETIMEDOUT rather than '0' when waiting for the
> > > rtce_buf to be set has timed out.
> > >
> > > Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> > > proceeding")
> > > Reported-by: Hulk Robot
> > > Signed-off-by: Wang Hai
> > > Signed-off-by: Stefan Berger
> > > ---
> > Reviewed-by: Jarkko Sakkinen
> >
> > Thanks! Should I add
> >
> > Cc: sta...@vger.kernel.org to this?
>
> Yes, that would be good! Thank you!
OK, it's applied.
> Stefan
/Jarkko
Re: [PATCH v2] tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()
On Mon, Jan 25, 2021 at 08:47:53PM -0500, Stefan Berger wrote:
> From: Stefan Berger
>
> Return error code -ETIMEDOUT rather than '0' when waiting for the
> rtce_buf to be set has timed out.
>
> Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> proceeding")
> Reported-by: Hulk Robot
> Signed-off-by: Wang Hai
> Signed-off-by: Stefan Berger
> ---
Reviewed-by: Jarkko Sakkinen
Thanks! Should I add
Cc: sta...@vger.kernel.org to this?
/Jarkko
> drivers/char/tpm/tpm_ibmvtpm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 994385bf37c0..813eb2cac0ce 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -687,6 +687,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
> ibmvtpm->rtce_buf != NULL,
> HZ)) {
> dev_err(dev, "CRQ response timed out\n");
> + rc = -ETIMEDOUT;
> goto init_irq_cleanup;
> }
>
> --
> 2.25.4
>
>
Re: [PATCH] drivers: char: tpm: remove unneeded MODULE_VERSION() usage
On Wed, Dec 02, 2020 at 01:15:53PM +0100, Enrico Weigelt, metux IT consult
wrote:
> Remove MODULE_VERSION(), as it isn't needed at all: the only version
> making sense is the kernel version.
Kernel version neither does make sense here. Why are mentioning it
in the commit message? Please just derive the commit message from
the one that Greg wrote.
> Link: https://lkml.org/lkml/2017/11/22/480
>
Remove the spurious empty line.
> Signed-off-by: Enrico Weigelt
> ---
> drivers/char/tpm/st33zp24/i2c.c | 1 -
> drivers/char/tpm/st33zp24/spi.c | 1 -
> drivers/char/tpm/st33zp24/st33zp24.c | 1 -
> drivers/char/tpm/tpm-interface.c | 1 -
> drivers/char/tpm/tpm_atmel.c | 1 -
> drivers/char/tpm/tpm_crb.c | 1 -
> drivers/char/tpm/tpm_i2c_infineon.c | 1 -
> drivers/char/tpm/tpm_ibmvtpm.c | 1 -
> drivers/char/tpm/tpm_infineon.c | 1 -
> drivers/char/tpm/tpm_nsc.c | 1 -
> drivers/char/tpm/tpm_tis.c | 1 -
> drivers/char/tpm/tpm_tis_core.c | 1 -
> drivers/char/tpm/tpm_vtpm_proxy.c| 1 -
> 13 files changed, 13 deletions(-)
>
> diff --git a/drivers/char/tpm/st33zp24/i2c.c b/drivers/char/tpm/st33zp24/i2c.c
> index 7c617edff4ca..7ed9829cacc4 100644
> --- a/drivers/char/tpm/st33zp24/i2c.c
> +++ b/drivers/char/tpm/st33zp24/i2c.c
> @@ -313,5 +313,4 @@ module_i2c_driver(st33zp24_i2c_driver);
>
> MODULE_AUTHOR("TPM support (tpmsupp...@list.st.com)");
> MODULE_DESCRIPTION("STM TPM 1.2 I2C ST33 Driver");
> -MODULE_VERSION("1.3.0");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c
> index a75dafd39445..147efea4eb05 100644
> --- a/drivers/char/tpm/st33zp24/spi.c
> +++ b/drivers/char/tpm/st33zp24/spi.c
> @@ -430,5 +430,4 @@ module_spi_driver(st33zp24_spi_driver);
>
> MODULE_AUTHOR("TPM support (tpmsupp...@list.st.com)");
> MODULE_DESCRIPTION("STM TPM 1.2 SPI ST33 Driver");
> -MODULE_VERSION("1.3.0");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/st33zp24/st33zp24.c
> b/drivers/char/tpm/st33zp24/st33zp24.c
> index 4ec10ab5e576..e0f1a5828993 100644
> --- a/drivers/char/tpm/st33zp24/st33zp24.c
> +++ b/drivers/char/tpm/st33zp24/st33zp24.c
> @@ -646,5 +646,4 @@ EXPORT_SYMBOL(st33zp24_pm_resume);
>
> MODULE_AUTHOR("TPM support (tpmsupp...@list.st.com)");
> MODULE_DESCRIPTION("ST33ZP24 TPM 1.2 driver");
> -MODULE_VERSION("1.3.0");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 1621ce818705..dfdc68b8bf88 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -514,5 +514,4 @@ module_exit(tpm_exit);
>
> MODULE_AUTHOR("Leendert van Doorn (leend...@watson.ibm.com)");
> MODULE_DESCRIPTION("TPM Driver");
> -MODULE_VERSION("2.0");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 54a6750a6757..35bf249cc95a 100644
> --- a/drivers/char/tpm/tpm_atmel.c
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -231,5 +231,4 @@ module_exit(cleanup_atmel);
>
> MODULE_AUTHOR("Leendert van Doorn (leend...@watson.ibm.com)");
> MODULE_DESCRIPTION("TPM Driver");
> -MODULE_VERSION("2.0");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index a9dcf31eadd2..3e72b7b99cce 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -748,5 +748,4 @@ static struct acpi_driver crb_acpi_driver = {
> module_acpi_driver(crb_acpi_driver);
> MODULE_AUTHOR("Jarkko Sakkinen ");
> MODULE_DESCRIPTION("TPM2 Driver");
> -MODULE_VERSION("0.1");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
> b/drivers/char/tpm/tpm_i2c_infineon.c
> index a19d32cb4e94..8920b7c19fcb 100644
> --- a/drivers/char/tpm/tpm_i2c_infineon.c
> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> @@ -731,5 +731,4 @@ static struct i2c_driver tpm_tis_i2c_driver = {
> module_i2c_driver(tpm_tis_i2c_driver);
> MODULE_AUTHOR("Peter Huewe ");
> MODULE_DESCRIPTION("TPM TIS I2C Infineon Driver");
> -MODULE_VERSION("2.2.0");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 994385bf37c0..5b04d113f634 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -750,5 +750,4 @@ module_exit(ibmvtpm_module_exit);
>
> MODULE_AUTHOR(&
Re: [PATCH] tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()
On Tue, 2020-11-24 at 21:52 +0800, Wang Hai wrote:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: d8d74ea3c002 ("tpm: ibmvtpm: Wait for buffer to be set before
> proceeding")
> Reported-by: Hulk Robot
> Signed-off-by: Wang Hai
Provide a reasoning for -ETIMEOUT in the commit message.
/Jarkko
> ---
> drivers/char/tpm/tpm_ibmvtpm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c
> index 994385bf37c0..813eb2cac0ce 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -687,6 +687,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev,
> ibmvtpm->rtce_buf != NULL,
> HZ)) {
> dev_err(dev, "CRQ response timed out\n");
> + rc = -ETIMEDOUT;
> goto init_irq_cleanup;
> }
>
Re: [PATCH] tpm: of: avoid __va() translation for event log address
On Mon, Sep 28, 2020 at 08:20:18AM +0200, Ard Biesheuvel wrote: > On Mon, 28 Sep 2020 at 07:56, Christophe Leroy > wrote: > > > > > > > > Le 28/09/2020 à 01:44, Jarkko Sakkinen a écrit : > > > On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > > >> On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > >>> On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > >>>> On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > >>>> wrote: > > >>>>> > > >>>>> On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > >>>>>> The TPM event log is provided to the OS by the firmware, by loading > > >>>>>> it into an area in memory and passing the physical address via a node > > >>>>>> in the device tree. > > >>>>>> > > >>>>>> Currently, we use __va() to access the memory via the kernel's linear > > >>>>>> map: however, it is not guaranteed that the linear map covers this > > >>>>>> particular address, as we may be running under HIGHMEM on a 32-bit > > >>>>>> architecture, or running firmware that uses a memory type for the > > >>>>>> event log that is omitted from the linear map (such as EfiReserved). > > >>>>> > > >>>>> Makes perfect sense to the level that I wonder if this should have a > > >>>>> fixes tag and/or needs to be backported to the stable kernels? > > >>>>> > > >>>> > > >>>> AIUI, the code was written specifically for ppc64, which is a > > >>>> non-highmem, non-EFI architecture. However, when we start reusing this > > >>>> driver for ARM, this issue could pop up. > > >>>> > > >>>> The code itself has been refactored a couple of times, so I think it > > >>>> will require different versions of the patch for different generations > > >>>> of stable kernels. > > >>>> > > >>>> So perhaps just add Cc: , and wait and see how > > >>>> far back it applies cleanly? > > >>> > > >>> Yeah, I think I'll cc it with some note before the diffstat. > > >>> > > >>> I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > >>> dead easy to backport below that. > > >> > > >> I have this vauge recollection of pointing at this before and being > > >> told that it had to be __va for some PPC reason? > > >> > > >> Do check with the PPC people first, I see none on the CC list. > > >> > > >> Jason > > > > > > Thanks, added arch/powerpc maintainers. > > > > > > > As far as I can see, memremap() won't work on PPC32 at least: > > > > IIUC, memremap() calls arch_memremap_wb() > > arch_memremap_wb() calls ioremap_cache() > > In case of failure, then ioremap_wt() and ioremap_wc() are tried. > > > > All ioremap calls end up in __ioremap_caller() which will return NULL in > > case you try to ioremap RAM. > > > > So the statement "So instead, use memremap(), which will reuse the linear > > mapping if > > it is valid, or create another mapping otherwise." seems to be wrong, at > > least for PPC32. > > > > Even for PPC64 which doesn't seem to have the RAM check, I can't see that > > it will "reuse the linear > > mapping". > > > > It is there, please look again. Before any of the above happens, > memremap() will call try_ram_remap() for regions that are covered by a > IORESOURCE_SYSTEM_RAM, and map it using __va() if its PFN is valid and > it is not highmem. > > So as far as I can tell, this change has no effect on PPC at all > unless its RAM is not described as IORESOURCE_SYSTEM_RAM. Any chance for someone to test this on PPC32? /Jarkko
Re: [PATCH] tpm: of: avoid __va() translation for event log address
On Fri, Sep 25, 2020 at 09:00:18AM -0300, Jason Gunthorpe wrote: > On Fri, Sep 25, 2020 at 01:29:20PM +0300, Jarkko Sakkinen wrote: > > On Fri, Sep 25, 2020 at 09:00:56AM +0200, Ard Biesheuvel wrote: > > > On Fri, 25 Sep 2020 at 07:56, Jarkko Sakkinen > > > wrote: > > > > > > > > On Tue, Sep 22, 2020 at 11:41:28AM +0200, Ard Biesheuvel wrote: > > > > > The TPM event log is provided to the OS by the firmware, by loading > > > > > it into an area in memory and passing the physical address via a node > > > > > in the device tree. > > > > > > > > > > Currently, we use __va() to access the memory via the kernel's linear > > > > > map: however, it is not guaranteed that the linear map covers this > > > > > particular address, as we may be running under HIGHMEM on a 32-bit > > > > > architecture, or running firmware that uses a memory type for the > > > > > event log that is omitted from the linear map (such as EfiReserved). > > > > > > > > Makes perfect sense to the level that I wonder if this should have a > > > > fixes tag and/or needs to be backported to the stable kernels? > > > > > > > > > > AIUI, the code was written specifically for ppc64, which is a > > > non-highmem, non-EFI architecture. However, when we start reusing this > > > driver for ARM, this issue could pop up. > > > > > > The code itself has been refactored a couple of times, so I think it > > > will require different versions of the patch for different generations > > > of stable kernels. > > > > > > So perhaps just add Cc: , and wait and see how > > > far back it applies cleanly? > > > > Yeah, I think I'll cc it with some note before the diffstat. > > > > I'm thinking to cap it to only 5.x kernels (at least first) unless it is > > dead easy to backport below that. > > I have this vauge recollection of pointing at this before and being > told that it had to be __va for some PPC reason? > > Do check with the PPC people first, I see none on the CC list. > > Jason Thanks, added arch/powerpc maintainers. /Jarkko
