Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
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. INTEGRITY_CA_MACHINE_KEYRING_MAX ensures that CA is only allowed to do key signing and not code signing. Having CA signed certs also permits easy revocation of all leaf certs. Loading certificates is completely new for Power Systems. We would like to make it as clean as possible from the start. We want to enforce CA signed leaf certificates(INTEGRITY_CA_MACHINE_KEYRING). As per keyUsage(INTEGRITY_CA_MACHINE_KEYRING_MAX), if we want more flexibility, probably a boot time override can be considered. Thanks & Regards, - Nayna cc: joeyli Signed-off-by: Michal Suchanek --- security/integrity/Kconfig | 2 -- 1 file changed, 2 deletions(-) diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index 232191ee09e3..b6e074ac0227 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -68,8 +68,6 @@ config INTEGRITY_MACHINE_KEYRING depends on INTEGRITY_ASYMMETRIC_KEYS depends on SYSTEM_BLACKLIST_KEYRING 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.41.0
Re: linux-next: Tree for Apr 16 (IMA appraise causing build error)
On 4/16/21 2:53 PM, Randy Dunlap wrote: On 4/16/21 4:36 AM, Stephen Rothwell wrote: Hi all, Changes since 20210415: I noticed this build error message (on an i386 build): ../certs/Makefile:52: *** Could not determine digest type to use from kernel config. Stop. and when I was checking on why it happened, I noticed that # CONFIG_MODULES is not set and hence ifndef CONFIG_MODULE_SIG_HASH $(error Could not determine digest type to use from kernel config) endif CONFIG_MODULE_SIG_HASH is not set/enabled/defined. However, the .config file does have CONFIG_IMA_APPRAISE=y # CONFIG_IMA_ARCH_POLICY is not set # CONFIG_IMA_APPRAISE_BUILD_POLICY is not set CONFIG_IMA_APPRAISE_BOOTPARAM=y CONFIG_IMA_APPRAISE_MODSIG=y as well as CONFIG_MODULE_SIG_FORMAT=y due to a "select" by IMA_APPRAISE_MODSIG. (although I see that MODULE_SIG_FORMAT does not depend on MODULES) Is there anything that you can do (or recommend) to prevent the build error? BTW, it looks like this: config IMA_APPRAISE_REQUIRE_MODULE_SIGS bool "Appraise kernel modules signatures" depends on IMA_APPRAISE_BUILD_POLICY could also depend on MODULES. Full i386 randconfig file is attached. With the new patchset "ima: kernel build support for loading the kernel module signing key", there shouldn't be a difference when generating the config file between MODULE_SIG and IMA_APPRAISE_MODSIG. Both prompt for the hash algorithm. Can you please explain how you generate randconfig? Do you use make xconfig? Thanks & Regards, - Nayna
[PATCH v4 3/3] ima: enable loading of build time generated key on .ima keyring
The kernel currently only loads the kernel module signing key onto the
builtin trusted keyring. Load the module signing key onto the IMA keyring
as well.
Signed-off-by: Nayna Jain
Acked-by: Stefan Berger
---
certs/system_certificates.S | 13 -
certs/system_keyring.c| 50 ---
include/keys/system_keyring.h | 7 +
security/integrity/digsig.c | 2 ++
4 files changed, 61 insertions(+), 11 deletions(-)
diff --git a/certs/system_certificates.S b/certs/system_certificates.S
index 8f29058adf93..dcad27ea8527 100644
--- a/certs/system_certificates.S
+++ b/certs/system_certificates.S
@@ -8,9 +8,11 @@
.globl system_certificate_list
system_certificate_list:
__cert_list_start:
-#ifdef CONFIG_MODULE_SIG
+__module_cert_start:
+#if defined(CONFIG_MODULE_SIG) || defined(CONFIG_IMA_APPRAISE_MODSIG)
.incbin "certs/signing_key.x509"
#endif
+__module_cert_end:
.incbin "certs/x509_certificate_list"
__cert_list_end:
@@ -35,3 +37,12 @@ system_certificate_list_size:
#else
.long __cert_list_end - __cert_list_start
#endif
+
+ .align 8
+ .globl module_cert_size
+module_cert_size:
+#ifdef CONFIG_64BIT
+ .quad __module_cert_end - __module_cert_start
+#else
+ .long __module_cert_end - __module_cert_start
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 4b693da488f1..2b3ad375ecc1 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -27,6 +27,7 @@ static struct key *platform_trusted_keys;
extern __initconst const u8 system_certificate_list[];
extern __initconst const unsigned long system_certificate_list_size;
+extern __initconst const unsigned long module_cert_size;
/**
* restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
@@ -132,19 +133,11 @@ static __init int system_trusted_keyring_init(void)
*/
device_initcall(system_trusted_keyring_init);
-/*
- * Load the compiled-in list of X.509 certificates.
- */
-static __init int load_system_certificate_list(void)
+static __init int load_cert(const u8 *p, const u8 *end, struct key *keyring)
{
key_ref_t key;
- const u8 *p, *end;
size_t plen;
- pr_notice("Loading compiled-in X.509 certificates\n");
-
- p = system_certificate_list;
- end = p + system_certificate_list_size;
while (p < end) {
/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
* than 256 bytes in size.
@@ -159,7 +152,7 @@ static __init int load_system_certificate_list(void)
if (plen > end - p)
goto dodgy_cert;
- key = key_create_or_update(make_key_ref(builtin_trusted_keys,
1),
+ key = key_create_or_update(make_key_ref(keyring, 1),
"asymmetric",
NULL,
p,
@@ -186,6 +179,43 @@ static __init int load_system_certificate_list(void)
pr_err("Problem parsing in-kernel X.509 certificate list\n");
return 0;
}
+
+__init int load_module_cert(struct key *keyring)
+{
+ const u8 *p, *end;
+
+ if (!IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG))
+ return 0;
+
+ pr_notice("Loading compiled-in module X.509 certificates\n");
+
+ p = system_certificate_list;
+ end = p + module_cert_size;
+
+ return load_cert(p, end, keyring);
+}
+
+/*
+ * Load the compiled-in list of X.509 certificates.
+ */
+static __init int load_system_certificate_list(void)
+{
+ const u8 *p, *end;
+ unsigned long size;
+
+ pr_notice("Loading compiled-in X.509 certificates\n");
+
+#ifdef CONFIG_MODULE_SIG
+ p = system_certificate_list;
+ size = system_certificate_list_size;
+#else
+ p = system_certificate_list + module_cert_size;
+ size = system_certificate_list_size - module_cert_size;
+#endif
+
+ end = p + size;
+ return load_cert(p, end, builtin_trusted_keys);
+}
late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..f954276c616a 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -16,9 +16,16 @@ extern int restrict_link_by_builtin_trusted(struct key
*keyring,
const struct key_type *type,
const union key_payload *payload,
struct key *restriction_key);
+extern __init int load_module_cert(struct key *keyring);
#else
#define restrict_link_by_builtin_trusted restrict_link_reject
+
+static inline __init int load_module_cert(struct key *keyring)
+{
+ return 0;
+}
+
#endif
#ifdef CONFIG_SECONDARY_TRUSTED_KE
[PATCH v4 1/3] keys: cleanup build time module signing keys
The "mrproper" target is still looking for build time generated keys in
the kernel root directory instead of certs directory. Fix the path and
remove the names of the files which are no longer generated.
Fixes: cfc411e7fff3 ("Move certificate handling to its own directory")
Signed-off-by: Nayna Jain
Reviewed-by: Stefan Berger
Reviewed-by: Mimi Zohar
Reviewed-by: Jarkko Sakkinen
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index cc77fd45ca64..d64c94f41edb 100644
--- a/Makefile
+++ b/Makefile
@@ -1523,9 +1523,9 @@ MRPROPER_FILES += include/config include/generated
\
debian snap tar-install \
.config .config.old .version \
Module.symvers \
- signing_key.pem signing_key.priv signing_key.x509 \
- x509.genkey extra_certificates signing_key.x509.keyid \
- signing_key.x509.signer vmlinux-gdb.py \
+ certs/signing_key.pem certs/signing_key.x509 \
+ certs/x509.genkey \
+ vmlinux-gdb.py \
*.spec
# Directories & files removed with 'make distclean'
--
2.29.2
[PATCH v4 2/3] ima: enable signing of modules with build time generated key
The kernel build process currently only signs kernel modules when MODULE_SIG is enabled. Also, sign the kernel modules at build time when IMA_APPRAISE_MODSIG is enabled. Signed-off-by: Nayna Jain Acked-by: Stefan Berger --- certs/Kconfig | 2 +- certs/Makefile | 8 init/Kconfig | 6 +++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/certs/Kconfig b/certs/Kconfig index c94e93d8bccf..48675ad319db 100644 --- a/certs/Kconfig +++ b/certs/Kconfig @@ -4,7 +4,7 @@ menu "Certificates for signature checking" config MODULE_SIG_KEY string "File name or PKCS#11 URI of module signing key" default "certs/signing_key.pem" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Provide the file name of a private key/certificate in PEM format, or a PKCS#11 URI according to RFC7512. The file should contain, or diff --git a/certs/Makefile b/certs/Makefile index f4c25b67aad9..e3185c57fbd8 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -32,6 +32,14 @@ endif # CONFIG_SYSTEM_TRUSTED_KEYRING clean-files := x509_certificate_list .x509.list ifeq ($(CONFIG_MODULE_SIG),y) + SIGN_KEY = y +endif + +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) + SIGN_KEY = y +endif + +ifdef SIGN_KEY ### # # If module signing is requested, say by allyesconfig, but a key has not been diff --git a/init/Kconfig b/init/Kconfig index 5f5c776ef192..85e48a578f90 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2164,7 +2164,7 @@ config MODULE_SIG_FORCE config MODULE_SIG_ALL bool "Automatically sign all modules" default y - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Sign all modules during make modules_install. Without this option, modules must be signed manually, using the scripts/sign-file tool. @@ -2174,7 +2174,7 @@ comment "Do not forget to sign required modules with scripts/sign-file" choice prompt "Which hash algorithm should modules be signed with?" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help This determines which sort of hashing algorithm will be used during signature generation. This algorithm _must_ be built into the kernel @@ -2206,7 +2206,7 @@ endchoice config MODULE_SIG_HASH string - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG default "sha1" if MODULE_SIG_SHA1 default "sha224" if MODULE_SIG_SHA224 default "sha256" if MODULE_SIG_SHA256 -- 2.29.2
[PATCH v4 0/3] ima: kernel build support for loading the kernel module signing key
Kernel modules are currently only signed when CONFIG_MODULE_SIG is enabled.
The kernel module signing key is a self-signed CA only loaded onto the
.builtin_trusted_key keyring. On secure boot enabled systems with an arch
specific IMA policy enabled, but without MODULE_SIG enabled, kernel modules
are not signed, nor is the kernel module signing public key loaded onto the
IMA keyring.
In order to load the the kernel module signing key onto the IMA trusted
keyring ('.ima'), the certificate needs to be signed by a CA key either on
the builtin or secondary keyrings. The original version of this patch set
created and loaded a kernel-CA key onto the builtin keyring. The kernel-CA
key signed the kernel module signing key, allowing it to be loaded onto the
IMA trusted keyring.
However, missing from this version was support for the kernel-CA to sign the
hardware token certificate. Adding that support would add additional
complexity.
Since the kernel module signing key is embedded into the Linux kernel at
build time, instead of creating and loading a kernel-CA onto the builtin
trusted keyring, this version makes an exception and allows the
self-signed kernel module signing key to be loaded directly onto the
trusted IMA keyring.
v4:
* Updated Jarkko's Reviewed-by and Stefan's Ack-by.
* Fixed a bug where size was miscalculated for the case when only
IMA_APPRAISE_MODSIG is enabled. Thanks Mimi for noticing it.
v3:
* Fix the "Fixes" tag as suggested by Stefan for Patch 1/3.
* Revert back the CA signed module signing key to only self-signed.
* Allow self signed key as exception only for build time generated
module signing key onto .ima keyring.
v2:
* Include feedback from Stefan - corrected the Fixes commit id in Patch 1
and cleaned Patch 5/5.
* Fix the issue reported by kernel test bot.
* Include Jarkko's feedback on patch description.
Nayna Jain (3):
keys: cleanup build time module signing keys
ima: enable signing of modules with build time generated key
ima: enable loading of build time generated key on .ima keyring
Makefile | 6 ++---
certs/Kconfig | 2 +-
certs/Makefile| 8 ++
certs/system_certificates.S | 13 -
certs/system_keyring.c| 50 ---
include/keys/system_keyring.h | 7 +
init/Kconfig | 6 ++---
security/integrity/digsig.c | 2 ++
8 files changed, 76 insertions(+), 18 deletions(-)
--
2.29.2
[PATCH v3 0/3] ima: kernel build support for loading the kernel module signing key
Kernel modules are currently only signed when CONFIG_MODULE_SIG is enabled.
The kernel module signing key is a self-signed CA only loaded onto the
.builtin_trusted_key keyring. On secure boot enabled systems with an arch
specific IMA policy enabled, but without MODULE_SIG enabled, kernel modules
are not signed, nor is the kernel module signing public key loaded onto the
IMA keyring.
In order to load the the kernel module signing key onto the IMA trusted
keyring ('.ima'), the certificate needs to be signed by a CA key either on
the builtin or secondary keyrings. The original version of this patch set
created and loaded a kernel-CA key onto the builtin keyring. The kernel-CA
key signed the kernel module signing key, allowing it to be loaded onto the
IMA trusted keyring.
However, missing from this version was support for the kernel-CA to sign the
hardware token certificate. Adding that support would add additional
complexity.
Since the kernel module signing key is embedded into the Linux kernel at
build time, instead of creating and loading a kernel-CA onto the builtin
trusted keyring, this version makes an exception and allows the
self-signed kernel module signing key to be loaded directly onto the
trusted IMA keyring
v3:
* Fix the "Fixes" tag as suggested by Stefan for Patch 1/3.
* Revert back the CA signed module signing key to only self-signed.
* Allow self signed key as exception only for build time generated
module signing key onto .ima keyring.
v2:
* Include feedback from Stefan - corrected the Fixes commit id in Patch 1
and cleaned Patch 5/5.
* Fix the issue reported by kernel test bot.
* Include Jarkko's feedback on patch description.
Nayna Jain (3):
keys: cleanup build time module signing keys
ima: enable signing of modules with build time generated key
ima: enable loading of build time generated key on .ima keyring
Makefile | 6 ++---
certs/Kconfig | 2 +-
certs/Makefile| 8 ++
certs/system_certificates.S | 13 +-
certs/system_keyring.c| 47 +++
include/keys/system_keyring.h | 7 ++
init/Kconfig | 6 ++---
security/integrity/digsig.c | 2 ++
8 files changed, 73 insertions(+), 18 deletions(-)
--
2.29.2
[PATCH v3 3/3] ima: enable loading of build time generated key on .ima keyring
The kernel currently only loads the kernel module signing key onto the
builtin trusted keyring. Load the module signing key onto the IMA keyring
as well.
Signed-off-by: Nayna Jain
---
certs/system_certificates.S | 13 +-
certs/system_keyring.c| 47 +++
include/keys/system_keyring.h | 7 ++
security/integrity/digsig.c | 2 ++
4 files changed, 58 insertions(+), 11 deletions(-)
diff --git a/certs/system_certificates.S b/certs/system_certificates.S
index 8f29058adf93..dcad27ea8527 100644
--- a/certs/system_certificates.S
+++ b/certs/system_certificates.S
@@ -8,9 +8,11 @@
.globl system_certificate_list
system_certificate_list:
__cert_list_start:
-#ifdef CONFIG_MODULE_SIG
+__module_cert_start:
+#if defined(CONFIG_MODULE_SIG) || defined(CONFIG_IMA_APPRAISE_MODSIG)
.incbin "certs/signing_key.x509"
#endif
+__module_cert_end:
.incbin "certs/x509_certificate_list"
__cert_list_end:
@@ -35,3 +37,12 @@ system_certificate_list_size:
#else
.long __cert_list_end - __cert_list_start
#endif
+
+ .align 8
+ .globl module_cert_size
+module_cert_size:
+#ifdef CONFIG_64BIT
+ .quad __module_cert_end - __module_cert_start
+#else
+ .long __module_cert_end - __module_cert_start
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 4b693da488f1..bb122bf4cc17 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -27,6 +27,7 @@ static struct key *platform_trusted_keys;
extern __initconst const u8 system_certificate_list[];
extern __initconst const unsigned long system_certificate_list_size;
+extern __initconst const unsigned long module_cert_size;
/**
* restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
@@ -132,19 +133,11 @@ static __init int system_trusted_keyring_init(void)
*/
device_initcall(system_trusted_keyring_init);
-/*
- * Load the compiled-in list of X.509 certificates.
- */
-static __init int load_system_certificate_list(void)
+static __init int load_cert(const u8 *p, const u8 *end, struct key *keyring)
{
key_ref_t key;
- const u8 *p, *end;
size_t plen;
- pr_notice("Loading compiled-in X.509 certificates\n");
-
- p = system_certificate_list;
- end = p + system_certificate_list_size;
while (p < end) {
/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
* than 256 bytes in size.
@@ -159,7 +152,7 @@ static __init int load_system_certificate_list(void)
if (plen > end - p)
goto dodgy_cert;
- key = key_create_or_update(make_key_ref(builtin_trusted_keys,
1),
+ key = key_create_or_update(make_key_ref(keyring, 1),
"asymmetric",
NULL,
p,
@@ -186,6 +179,40 @@ static __init int load_system_certificate_list(void)
pr_err("Problem parsing in-kernel X.509 certificate list\n");
return 0;
}
+
+__init int load_module_cert(struct key *keyring)
+{
+ const u8 *p, *end;
+
+ if (!IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG))
+ return 0;
+
+ pr_notice("Loading compiled-in module X.509 certificates\n");
+
+ p = system_certificate_list;
+ end = p + module_cert_size;
+
+ return load_cert(p, end, keyring);
+}
+
+/*
+ * Load the compiled-in list of X.509 certificates.
+ */
+static __init int load_system_certificate_list(void)
+{
+ const u8 *p, *end;
+
+ pr_notice("Loading compiled-in X.509 certificates\n");
+
+#ifdef CONFIG_MODULE_SIG
+ p = system_certificate_list;
+#else
+ p = system_certificate_list + module_cert_size;
+#endif
+
+ end = p + system_certificate_list_size;
+ return load_cert(p, end, builtin_trusted_keys);
+}
late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..f954276c616a 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -16,9 +16,16 @@ extern int restrict_link_by_builtin_trusted(struct key
*keyring,
const struct key_type *type,
const union key_payload *payload,
struct key *restriction_key);
+extern __init int load_module_cert(struct key *keyring);
#else
#define restrict_link_by_builtin_trusted restrict_link_reject
+
+static inline __init int load_module_cert(struct key *keyring)
+{
+ return 0;
+}
+
#endif
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 250fb0836156..3b06a01bd0fd 100644
--- a/security/integri
[PATCH v3 2/3] ima: enable signing of modules with build time generated key
The kernel build process currently only signs kernel modules when MODULE_SIG is enabled. Also, sign the kernel modules at build time when IMA_APPRAISE_MODSIG is enabled. Signed-off-by: Nayna Jain --- certs/Kconfig | 2 +- certs/Makefile | 8 init/Kconfig | 6 +++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/certs/Kconfig b/certs/Kconfig index c94e93d8bccf..48675ad319db 100644 --- a/certs/Kconfig +++ b/certs/Kconfig @@ -4,7 +4,7 @@ menu "Certificates for signature checking" config MODULE_SIG_KEY string "File name or PKCS#11 URI of module signing key" default "certs/signing_key.pem" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Provide the file name of a private key/certificate in PEM format, or a PKCS#11 URI according to RFC7512. The file should contain, or diff --git a/certs/Makefile b/certs/Makefile index f4c25b67aad9..e3185c57fbd8 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -32,6 +32,14 @@ endif # CONFIG_SYSTEM_TRUSTED_KEYRING clean-files := x509_certificate_list .x509.list ifeq ($(CONFIG_MODULE_SIG),y) + SIGN_KEY = y +endif + +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) + SIGN_KEY = y +endif + +ifdef SIGN_KEY ### # # If module signing is requested, say by allyesconfig, but a key has not been diff --git a/init/Kconfig b/init/Kconfig index 5f5c776ef192..85e48a578f90 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2164,7 +2164,7 @@ config MODULE_SIG_FORCE config MODULE_SIG_ALL bool "Automatically sign all modules" default y - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Sign all modules during make modules_install. Without this option, modules must be signed manually, using the scripts/sign-file tool. @@ -2174,7 +2174,7 @@ comment "Do not forget to sign required modules with scripts/sign-file" choice prompt "Which hash algorithm should modules be signed with?" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help This determines which sort of hashing algorithm will be used during signature generation. This algorithm _must_ be built into the kernel @@ -2206,7 +2206,7 @@ endchoice config MODULE_SIG_HASH string - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG default "sha1" if MODULE_SIG_SHA1 default "sha224" if MODULE_SIG_SHA224 default "sha256" if MODULE_SIG_SHA256 -- 2.29.2
[PATCH v3 1/3] keys: cleanup build time module signing keys
The "mrproper" target is still looking for build time generated keys in
the kernel root directory instead of certs directory. Fix the path and
remove the names of the files which are no longer generated.
Fixes: cfc411e7fff3 ("Move certificate handling to its own directory")
Signed-off-by: Nayna Jain
Reviewed-by: Stefan Berger
Reviewed-by: Mimi Zohar
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index d4784d181123..b7c2ed2a8684 100644
--- a/Makefile
+++ b/Makefile
@@ -1523,9 +1523,9 @@ MRPROPER_FILES += include/config include/generated
\
debian snap tar-install \
.config .config.old .version \
Module.symvers \
- signing_key.pem signing_key.priv signing_key.x509 \
- x509.genkey extra_certificates signing_key.x509.keyid \
- signing_key.x509.signer vmlinux-gdb.py \
+ certs/signing_key.pem certs/signing_key.x509 \
+ certs/x509.genkey \
+ vmlinux-gdb.py \
*.spec
# Directories & files removed with 'make distclean'
--
2.29.2
Re: [PATCH 2/5] keys: generate self-signed module signing key using CSR
On 2/11/21 5:01 PM, Stefan Berger wrote: On 2/11/21 2:54 PM, Nayna Jain wrote: Loading a key on the IMA trusted keyring requires the key be signed by an existing key on the builtin or secondary trusted keyring. Creating a Certificate Signing Request (CSR) allows the certificate to be self-signed or signed by a CA. This patch generates a self-signed module signing key using CSR. Signed-off-by: Nayna Jain --- Makefile | 3 ++- certs/Makefile | 15 +++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index af18aab6bbee..9c87fdd600d8 100644 --- a/Makefile +++ b/Makefile @@ -1473,7 +1473,8 @@ MRPROPER_FILES += include/config include/generated \ .config .config.old .version \ Module.symvers \ certs/signing_key.pem certs/signing_key.x509 \ - certs/x509.genkey \ + certs/x509.genkey certs/signing_key.key \ + certs/signing_key.crt certs/signing_key.csr \ vmlinux-gdb.py \ *.spec diff --git a/certs/Makefile b/certs/Makefile index f4c25b67aad9..b2be7eb413d3 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -60,11 +60,18 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "### needs to be run as root, and uses a hardware random" @$(kecho) "### number generator if one is available." @$(kecho) "###" - $(Q)openssl req -new -nodes -utf8 -$(CONFIG_MODULE_SIG_HASH) -days 36500 \ - -batch -x509 -config $(obj)/x509.genkey \ - -outform PEM -out $(obj)/signing_key.pem \ - -keyout $(obj)/signing_key.pem \ + $(Q)openssl req -new -nodes -utf8 \ + -batch -config $(obj)/x509.genkey \ + -outform PEM -out $(obj)/signing_key.csr \ + -keyout $(obj)/signing_key.key -extensions myexts \ $($(quiet)redirect_openssl) + $(Q)openssl x509 -req -days 36500 -in $(obj)/signing_key.csr \ + -outform PEM -out $(obj)/signing_key.crt \ + -signkey $(obj)/signing_key.key \ + -$(CONFIG_MODULE_SIG_HASH) -extensions myexts \ + -extfile $(obj)/x509.genkey \ + $($(quiet)redirect_openssl) + @cat $(obj)/signing_key.key $(obj)/signing_key.crt >> $(obj)/signing_key.pem Could you not just rename signing_key.key to signing_key.pem (as it was before) and that would be it? Why do you need the .crt in that pem bundle? I had also thought so, but the PEM file contains both the private key and the certificate. I found the reasoning in the commit "fb1179499134 modsign: Use single PEM file for autogenerated key". I addressed your other feedback in v2, posted just now. Thanks & Regards, - Nayna
[PATCH v2 5/5] ima: enable loading of build time generated key on .ima keyring
The kernel currently only loads the kernel module signing key onto
the builtin trusted keyring. To support IMA, load the module signing
key selectively either onto the builtin or IMA keyring based on MODULE_SIG
or MODULE_APPRAISE_MODSIG config respectively; and loads the CA kernel
key onto the builtin trusted keyring.
Signed-off-by: Nayna Jain
---
certs/system_keyring.c| 55 ++-
include/keys/system_keyring.h | 9 +-
security/integrity/digsig.c | 4 +++
3 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 798291177186..ea3826627729 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -26,6 +26,7 @@ static struct key *platform_trusted_keys;
extern __initconst const u8 system_certificate_list[];
extern __initconst const unsigned long system_certificate_list_size;
+extern __initconst const unsigned long module_cert_size;
/**
* restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
@@ -131,19 +132,12 @@ static __init int system_trusted_keyring_init(void)
*/
device_initcall(system_trusted_keyring_init);
-/*
- * Load the compiled-in list of X.509 certificates.
- */
-static __init int load_system_certificate_list(void)
+static __init int load_cert(const u8 *p, const u8 *end, struct key *keyring,
+ unsigned long flags)
{
key_ref_t key;
- const u8 *p, *end;
size_t plen;
- pr_notice("Loading compiled-in X.509 certificates\n");
-
- p = system_certificate_list;
- end = p + system_certificate_list_size;
while (p < end) {
/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
* than 256 bytes in size.
@@ -158,16 +152,15 @@ static __init int load_system_certificate_list(void)
if (plen > end - p)
goto dodgy_cert;
- key = key_create_or_update(make_key_ref(builtin_trusted_keys,
1),
+ key = key_create_or_update(make_key_ref(keyring, 1),
"asymmetric",
NULL,
p,
plen,
((KEY_POS_ALL & ~KEY_POS_SETATTR) |
KEY_USR_VIEW | KEY_USR_READ),
- KEY_ALLOC_NOT_IN_QUOTA |
- KEY_ALLOC_BUILT_IN |
- KEY_ALLOC_BYPASS_RESTRICTION);
+ flags);
+
if (IS_ERR(key)) {
pr_err("Problem loading in-kernel X.509 certificate
(%ld)\n",
PTR_ERR(key));
@@ -185,6 +178,42 @@ static __init int load_system_certificate_list(void)
pr_err("Problem parsing in-kernel X.509 certificate list\n");
return 0;
}
+
+__init int load_module_cert(struct key *keyring, unsigned long flags)
+{
+ const u8 *p, *end;
+
+ if (!IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG))
+ return 0;
+
+ pr_notice("Loading compiled-in module X.509 certificates\n");
+
+ p = system_certificate_list;
+ end = p + module_cert_size;
+
+ return load_cert(p, end, keyring, flags);
+}
+
+/*
+ * Load the compiled-in list of X.509 certificates.
+ */
+static __init int load_system_certificate_list(void)
+{
+ const u8 *p, *end;
+
+ pr_notice("Loading compiled-in X.509 certificates\n");
+
+#ifdef CONFIG_MODULE_SIG
+ p = system_certificate_list;
+#else
+ p = system_certificate_list + module_cert_size;
+#endif
+ end = p + system_certificate_list_size;
+
+ return load_cert(p, end, builtin_trusted_keys, KEY_ALLOC_NOT_IN_QUOTA |
+ KEY_ALLOC_BUILT_IN |
+ KEY_ALLOC_BYPASS_RESTRICTION);
+}
late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..e91c03376599 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -16,9 +16,16 @@ extern int restrict_link_by_builtin_trusted(struct key
*keyring,
const struct key_type *type,
const union key_payload *payload,
struct key *restriction_key);
-
+extern __init int load_module_cert(struct key *keyring, unsigned long flags);
#else
#define restrict_link_by_builtin_trusted restrict_link_reject
+
+static inline __init int load_module_cert(struct key *keyring,
+
[PATCH v2 4/5] keys: define build time generated ephemeral kernel CA key
Certificates being loaded onto the IMA trusted keyring must be signed by a key on either the builtin or secondary trusted keyring. Create and include in the kernel image an ephemeral CA key at build time when IMA_APPRAISE_MODSIG is enabled. Reported-by: kernel test robot (redirect openssl stderr) Signed-off-by: Nayna Jain --- Makefile| 2 ++ certs/Makefile | 68 ++--- certs/system_certificates.S | 16 - 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index a971d4ae40bd..15e8344836b1 100644 --- a/Makefile +++ b/Makefile @@ -1475,6 +1475,8 @@ MRPROPER_FILES += include/config include/generated \ certs/signing_key.pem certs/signing_key.x509 \ certs/x509.genkey certs/signing_key.key \ certs/signing_key.crt certs/signing_key.csr \ + certs/ca_signing_key.pem certs/ca_signing_key.x509 \ + certs/ca_signing_key.srl \ vmlinux-gdb.py \ *.spec diff --git a/certs/Makefile b/certs/Makefile index b2be7eb413d3..3fe6b73786fa 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -32,6 +32,14 @@ endif # CONFIG_SYSTEM_TRUSTED_KEYRING clean-files := x509_certificate_list .x509.list ifeq ($(CONFIG_MODULE_SIG),y) +SIGN_KEY = y +endif + +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) +SIGN_KEY = y +endif + +ifdef SIGN_KEY ### # # If module signing is requested, say by allyesconfig, but a key has not been @@ -51,6 +59,16 @@ silent_redirect_openssl = 2>/dev/null # external private key, because 'make randconfig' might enable such a # boolean option and we unfortunately can't make it depend on !RANDCONFIG. ifeq ($(CONFIG_MODULE_SIG_KEY),"certs/signing_key.pem") + +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) +# openssl arguments for CA Signed certificate. +CA_KEY = certs/ca_signing_key.pem +SIGNER = -CA $(CA_KEY) -CAkey $(CA_KEY) -CAcreateserial +else +# openssl arguments for Self Signed certificate. +SIGNER = -signkey $(obj)/signing_key.key +endif # CONFIG_IMA_APPRAISE_MODSIG + $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "###" @$(kecho) "### Now generating an X.509 key pair to be used for signing modules." @@ -60,14 +78,23 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "### needs to be run as root, and uses a hardware random" @$(kecho) "### number generator if one is available." @$(kecho) "###" +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) + # Generate kernel build time CA Certificate. + @$(Q)openssl req -new -nodes -utf8 \ + -$(CONFIG_MODULE_SIG_HASH) -days 36500 \ + -subj "/CN=Build time autogenerated kernel CA key" \ + -batch -x509 -config $(obj)/x509.genkey \ + -outform PEM -out $(CA_KEY) \ + -keyout $(CA_KEY) -extensions ca_ext \ + $($(quiet)redirect_openssl) +endif # CONFIG_IMA_APPRAISE_MODSIG $(Q)openssl req -new -nodes -utf8 \ -batch -config $(obj)/x509.genkey \ -outform PEM -out $(obj)/signing_key.csr \ -keyout $(obj)/signing_key.key -extensions myexts \ $($(quiet)redirect_openssl) $(Q)openssl x509 -req -days 36500 -in $(obj)/signing_key.csr \ - -outform PEM -out $(obj)/signing_key.crt \ - -signkey $(obj)/signing_key.key \ + -outform PEM -out $(obj)/signing_key.crt $(SIGNER) \ -$(CONFIG_MODULE_SIG_HASH) -extensions myexts \ -extfile $(obj)/x509.genkey \ $($(quiet)redirect_openssl) @@ -95,19 +122,50 @@ $(obj)/x509.genkey: @echo >>$@ "keyUsage=digitalSignature" @echo >>$@ "subjectKeyIdentifier=hash" @echo >>$@ "authorityKeyIdentifier=keyid" + @echo >>$@ + @echo >>$@ "[ ca_ext ]" + @echo >>$@ "keyUsage=critical,keyCertSign" + @echo >>$@ "basicConstraints=critical,CA:TRUE,pathlen:0" + @echo >>$@ "subjectKeyIdentifier=hash" + @echo >>$@ "authorityKeyIdentifier=keyid" endif # CONFIG_MODULE_SIG_KEY $(eval $(call config_filename,MODULE_SIG_KEY)) +SUBJECT=CN = Build time autogenerated kernel key +ISSUER=$(shell openssl x509 -in certs/signing_key.crt -noout -issuer $($(quiet)redirect_openssl)) # If CONFIG_MODULE_SIG_KEY isn't a PKCS#11 URI, depend on it + +# GCC PR#66871 again. +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) + +# Remove existing keys if it is self-signed. +$(if $(findstring $(SUBJECT),$(ISSUER)),$(shell rm -f certs/signing_key.* certs/x509.genkey)) +CA_KEY = certs/ca_signing_key.pem + +$(obj)/system_certificates.o: $(obj)/
[PATCH v2 3/5] ima: update kernel module signing process during build
The kernel build process currently only signs kernel modules when MODULE_SIG is enabled. Also, sign the kernel modules at build time when IMA_APPRAISE_MODSIG is enabled. Signed-off-by: Nayna Jain --- certs/Kconfig | 2 +- init/Kconfig | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/certs/Kconfig b/certs/Kconfig index c94e93d8bccf..48675ad319db 100644 --- a/certs/Kconfig +++ b/certs/Kconfig @@ -4,7 +4,7 @@ menu "Certificates for signature checking" config MODULE_SIG_KEY string "File name or PKCS#11 URI of module signing key" default "certs/signing_key.pem" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Provide the file name of a private key/certificate in PEM format, or a PKCS#11 URI according to RFC7512. The file should contain, or diff --git a/init/Kconfig b/init/Kconfig index 29ad68325028..68147bbda5f9 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2162,7 +2162,7 @@ config MODULE_SIG_FORCE config MODULE_SIG_ALL bool "Automatically sign all modules" default y - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Sign all modules during make modules_install. Without this option, modules must be signed manually, using the scripts/sign-file tool. @@ -2172,7 +2172,7 @@ comment "Do not forget to sign required modules with scripts/sign-file" choice prompt "Which hash algorithm should modules be signed with?" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help This determines which sort of hashing algorithm will be used during signature generation. This algorithm _must_ be built into the kernel @@ -2204,7 +2204,7 @@ endchoice config MODULE_SIG_HASH string - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG default "sha1" if MODULE_SIG_SHA1 default "sha224" if MODULE_SIG_SHA224 default "sha256" if MODULE_SIG_SHA256 -- 2.29.2
[PATCH v2 0/5] ima: kernel build support for loading the kernel module signing key
Kernel modules are currently only signed when CONFIG_MODULE_SIG is enabled.
The kernel module signing key is a self-signed CA only loaded onto the
.builtin_trusted_key keyring. On secure boot enabled systems with an arch
specific IMA policy enabled, but without MODULE_SIG enabled, kernel modules
are not signed, nor is the kernel module signing public key loaded onto the
IMA keyring.
In order to load the the kernel module signing key onto the IMA trusted
keyring ('.ima'), the certificate needs to be signed by a CA key either on
the builtin or secondary keyrings. This series of patches enables IMA
verification of signed kernel modules by:
* Defining a kernel CA key. The CA key signs the kernel module signing key
and is loaded onto the .builtin_trusted_key keyring, only when the kernel
module signing key is loaded onto the .ima keyring.
* Enable module signing at build time for IMA_APPRAISE_MODSIG as well
v2:
* Include feedback from Stefan - corrected the Fixes commit id in Patch 1
and cleaned Patch 5/5.
* Fix the issue reported by kernel test bot.
* Include Jarkko's feedback on patch description.
Nayna Jain (5):
keys: cleanup build time module signing keys
keys: generate self-signed module signing key using CSR
ima: update kernel module signing process during build
keys: define build time generated ephemeral kernel CA key
ima: enable loading of build time generated key on .ima keyring
Makefile | 9 ++--
certs/Kconfig | 2 +-
certs/Makefile| 77 ---
certs/system_certificates.S | 16 +++-
certs/system_keyring.c| 55 +++--
include/keys/system_keyring.h | 9 +++-
init/Kconfig | 6 +--
security/integrity/digsig.c | 4 ++
8 files changed, 150 insertions(+), 28 deletions(-)
--
2.29.2
[PATCH v2 2/5] keys: generate self-signed module signing key using CSR
Loading a key on the IMA trusted keyring requires the key be signed by an existing key on the builtin or secondary trusted keyring. Creating a Certificate Signing Request (CSR) allows the certificate to be self-signed or signed by a CA. Generate a self-signed module signing key using CSR. Signed-off-by: Nayna Jain --- Makefile | 3 ++- certs/Makefile | 15 +++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 004163a4e6b3..a971d4ae40bd 100644 --- a/Makefile +++ b/Makefile @@ -1473,7 +1473,8 @@ MRPROPER_FILES += include/config include/generated \ .config .config.old .version \ Module.symvers \ certs/signing_key.pem certs/signing_key.x509 \ - certs/x509.genkey \ + certs/x509.genkey certs/signing_key.key \ + certs/signing_key.crt certs/signing_key.csr \ vmlinux-gdb.py \ *.spec diff --git a/certs/Makefile b/certs/Makefile index f4c25b67aad9..b2be7eb413d3 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -60,11 +60,18 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "### needs to be run as root, and uses a hardware random" @$(kecho) "### number generator if one is available." @$(kecho) "###" - $(Q)openssl req -new -nodes -utf8 -$(CONFIG_MODULE_SIG_HASH) -days 36500 \ - -batch -x509 -config $(obj)/x509.genkey \ - -outform PEM -out $(obj)/signing_key.pem \ - -keyout $(obj)/signing_key.pem \ + $(Q)openssl req -new -nodes -utf8 \ + -batch -config $(obj)/x509.genkey \ + -outform PEM -out $(obj)/signing_key.csr \ + -keyout $(obj)/signing_key.key -extensions myexts \ $($(quiet)redirect_openssl) + $(Q)openssl x509 -req -days 36500 -in $(obj)/signing_key.csr \ + -outform PEM -out $(obj)/signing_key.crt \ + -signkey $(obj)/signing_key.key \ + -$(CONFIG_MODULE_SIG_HASH) -extensions myexts \ + -extfile $(obj)/x509.genkey \ + $($(quiet)redirect_openssl) + @cat $(obj)/signing_key.key $(obj)/signing_key.crt >> $(obj)/signing_key.pem @$(kecho) "###" @$(kecho) "### Key pair generated." @$(kecho) "###" -- 2.29.2
[PATCH v2 1/5] keys: cleanup build time module signing keys
The "mrproper" target is still looking for build time generated keys in
the old path instead of certs/ directory. Fix the path and remove the
names of the files which are no longer generated.
Fixes: fb1179499134 ("modsign: Use single PEM file for autogenerated key")
Signed-off-by: Nayna Jain
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index de1acaefe87e..004163a4e6b3 100644
--- a/Makefile
+++ b/Makefile
@@ -1472,9 +1472,9 @@ MRPROPER_FILES += include/config include/generated
\
debian snap tar-install \
.config .config.old .version \
Module.symvers \
- signing_key.pem signing_key.priv signing_key.x509 \
- x509.genkey extra_certificates signing_key.x509.keyid \
- signing_key.x509.signer vmlinux-gdb.py \
+ certs/signing_key.pem certs/signing_key.x509 \
+ certs/x509.genkey \
+ vmlinux-gdb.py \
*.spec
# Directories & files removed with 'make distclean'
--
2.29.2
Re: [PATCH 1/5] keys: cleanup build time module signing keys
On 2/11/21 4:57 PM, Stefan Berger wrote:
On 2/11/21 2:54 PM, Nayna Jain wrote:
The "mrproper" target is still looking for build time generated keys
in the old path instead of certs/ directory.
This patch fixes the path as well removes the names of the files which
are no longer generated.
Signed-off-by: Nayna Jain
Fixes: 28a68f828266 ("modsign: Use single PEM file for autogenerated
key")
I was curious about some of the files and how they were created in the
past but couldn't see it in the hostory of the Makefile. The above
Fixes tag seems to give the wrong commit id:
commit 28a68f828266754c2bd64b87873e8099e3f8fe0c
Author: Dave Airlie
Date: Thu Oct 29 13:59:45 2020 +1000
drm/radeon/ttm: use multihop
Thanks Stefan for noticing it. I will fix this in v2.
Thanks & Regards,
- Nayna
[PATCH 5/5] ima: enable loading of build time generated key to .ima keyring
The kernel currently only loads the kernel module signing key onto
the builtin trusted keyring. To support IMA, load the module signing
key selectively either onto builtin or ima keyring based on MODULE_SIG
or MODULE_APPRAISE_MODSIG config respectively; and loads the CA kernel
key onto builtin trusted keyring.
Signed-off-by: Nayna Jain
---
certs/system_keyring.c| 56 +++
include/keys/system_keyring.h | 9 +-
security/integrity/digsig.c | 4 +++
3 files changed, 55 insertions(+), 14 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 798291177186..0bbbe501f8a7 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -26,6 +26,7 @@ static struct key *platform_trusted_keys;
extern __initconst const u8 system_certificate_list[];
extern __initconst const unsigned long system_certificate_list_size;
+extern __initconst const unsigned long module_cert_size;
/**
* restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
@@ -131,19 +132,12 @@ static __init int system_trusted_keyring_init(void)
*/
device_initcall(system_trusted_keyring_init);
-/*
- * Load the compiled-in list of X.509 certificates.
- */
-static __init int load_system_certificate_list(void)
+static __init int load_cert(const u8 *p, const u8 *end, struct key *keyring,
+ unsigned long flags)
{
key_ref_t key;
- const u8 *p, *end;
size_t plen;
- pr_notice("Loading compiled-in X.509 certificates\n");
-
- p = system_certificate_list;
- end = p + system_certificate_list_size;
while (p < end) {
/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
* than 256 bytes in size.
@@ -158,16 +152,15 @@ static __init int load_system_certificate_list(void)
if (plen > end - p)
goto dodgy_cert;
- key = key_create_or_update(make_key_ref(builtin_trusted_keys,
1),
+ key = key_create_or_update(make_key_ref(keyring, 1),
"asymmetric",
NULL,
p,
plen,
((KEY_POS_ALL & ~KEY_POS_SETATTR) |
KEY_USR_VIEW | KEY_USR_READ),
- KEY_ALLOC_NOT_IN_QUOTA |
- KEY_ALLOC_BUILT_IN |
- KEY_ALLOC_BYPASS_RESTRICTION);
+ flags);
+
if (IS_ERR(key)) {
pr_err("Problem loading in-kernel X.509 certificate
(%ld)\n",
PTR_ERR(key));
@@ -185,6 +178,43 @@ static __init int load_system_certificate_list(void)
pr_err("Problem parsing in-kernel X.509 certificate list\n");
return 0;
}
+
+__init int load_module_cert(struct key *keyring, unsigned long flags)
+{
+ const u8 *p, *end;
+
+ if (!IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG))
+ return 0;
+
+ pr_notice("Loading compiled-in module X.509 certificates\n");
+
+ p = system_certificate_list;
+ end = p + module_cert_size;
+ load_cert(p, end, keyring, flags);
+
+ return 0;
+}
+
+/*
+ * Load the compiled-in list of X.509 certificates.
+ */
+static __init int load_system_certificate_list(void)
+{
+ const u8 *p, *end;
+
+ pr_notice("Loading compiled-in X.509 certificates\n");
+
+#ifdef CONFIG_MODULE_SIG
+ p = system_certificate_list;
+#else
+ p = system_certificate_list + module_cert_size;
+#endif
+ end = p + system_certificate_list_size;
+ load_cert(p, end, builtin_trusted_keys, KEY_ALLOC_NOT_IN_QUOTA |
+ KEY_ALLOC_BUILT_IN |
+ KEY_ALLOC_BYPASS_RESTRICTION);
+ return 0;
+}
late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..e91c03376599 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -16,9 +16,16 @@ extern int restrict_link_by_builtin_trusted(struct key
*keyring,
const struct key_type *type,
const union key_payload *payload,
struct key *restriction_key);
-
+extern __init int load_module_cert(struct key *keyring, unsigned long flags);
#else
#define restrict_link_by_builtin_trusted restrict_link_reject
+
+static inline __init int load_module_cert(struct key *keyring,
+
[PATCH 4/5] keys: define build time generated ephemeral kernel CA key
Certificates being loaded onto the IMA trusted keyring must be signed by a key on either the builtin and secondary trusted keyring. This patch creates and includes in the kernel image an ephemeral CA key, at build time when IMA_APPRAISE_MODSIG is enabled. Signed-off-by: Nayna Jain --- Makefile| 2 ++ certs/Makefile | 68 ++--- certs/system_certificates.S | 16 - 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 9c87fdd600d8..a1d4b0a1745e 100644 --- a/Makefile +++ b/Makefile @@ -1475,6 +1475,8 @@ MRPROPER_FILES += include/config include/generated \ certs/signing_key.pem certs/signing_key.x509 \ certs/x509.genkey certs/signing_key.key \ certs/signing_key.crt certs/signing_key.csr \ + certs/ca_signing_key.pem certs/ca_signing_key.x509 \ + certs/ca_signing_key.srl \ vmlinux-gdb.py \ *.spec diff --git a/certs/Makefile b/certs/Makefile index b2be7eb413d3..c3592ba63a05 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -32,6 +32,14 @@ endif # CONFIG_SYSTEM_TRUSTED_KEYRING clean-files := x509_certificate_list .x509.list ifeq ($(CONFIG_MODULE_SIG),y) +SIGN_KEY = y +endif + +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) +SIGN_KEY = y +endif + +ifdef SIGN_KEY ### # # If module signing is requested, say by allyesconfig, but a key has not been @@ -51,6 +59,16 @@ silent_redirect_openssl = 2>/dev/null # external private key, because 'make randconfig' might enable such a # boolean option and we unfortunately can't make it depend on !RANDCONFIG. ifeq ($(CONFIG_MODULE_SIG_KEY),"certs/signing_key.pem") + +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) +# openssl arguments for CA Signed certificate. +CA_KEY = certs/ca_signing_key.pem +SIGNER = -CA $(CA_KEY) -CAkey $(CA_KEY) -CAcreateserial +else +# openssl arguments for Self Signed certificate. +SIGNER = -signkey $(obj)/signing_key.key +endif # CONFIG_IMA_APPRAISE_MODSIG + $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "###" @$(kecho) "### Now generating an X.509 key pair to be used for signing modules." @@ -60,14 +78,23 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "### needs to be run as root, and uses a hardware random" @$(kecho) "### number generator if one is available." @$(kecho) "###" +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) + # Generate kernel build time CA Certificate. + @$(Q)openssl req -new -nodes -utf8 \ + -$(CONFIG_MODULE_SIG_HASH) -days 36500 \ + -subj "/CN=Build time autogenerated kernel CA key" \ + -batch -x509 -config $(obj)/x509.genkey \ + -outform PEM -out $(CA_KEY) \ + -keyout $(CA_KEY) -extensions ca_ext \ + $($(quiet)redirect_openssl) +endif # CONFIG_IMA_APPRAISE_MODSIG $(Q)openssl req -new -nodes -utf8 \ -batch -config $(obj)/x509.genkey \ -outform PEM -out $(obj)/signing_key.csr \ -keyout $(obj)/signing_key.key -extensions myexts \ $($(quiet)redirect_openssl) $(Q)openssl x509 -req -days 36500 -in $(obj)/signing_key.csr \ - -outform PEM -out $(obj)/signing_key.crt \ - -signkey $(obj)/signing_key.key \ + -outform PEM -out $(obj)/signing_key.crt $(SIGNER) \ -$(CONFIG_MODULE_SIG_HASH) -extensions myexts \ -extfile $(obj)/x509.genkey \ $($(quiet)redirect_openssl) @@ -95,19 +122,50 @@ $(obj)/x509.genkey: @echo >>$@ "keyUsage=digitalSignature" @echo >>$@ "subjectKeyIdentifier=hash" @echo >>$@ "authorityKeyIdentifier=keyid" + @echo >>$@ + @echo >>$@ "[ ca_ext ]" + @echo >>$@ "keyUsage=critical,keyCertSign" + @echo >>$@ "basicConstraints=critical,CA:TRUE,pathlen:0" + @echo >>$@ "subjectKeyIdentifier=hash" + @echo >>$@ "authorityKeyIdentifier=keyid" endif # CONFIG_MODULE_SIG_KEY $(eval $(call config_filename,MODULE_SIG_KEY)) +SUBJECT=CN = Build time autogenerated kernel key +ISSUER=$(shell openssl x509 -in certs/signing_key.crt -noout -issuer) # If CONFIG_MODULE_SIG_KEY isn't a PKCS#11 URI, depend on it + +# GCC PR#66871 again. +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) + +# Remove existing keys if it is self-signed. +$(if $(findstring $(SUBJECT),$(ISSUER)),$(shell rm -f certs/signing_key.* certs/x509.genkey)) +CA_KEY = certs/ca_signing_key.pem + +$(obj)/system_certificates.o: $(obj)/ca_signing_key.x509 $(obj)/signing_key.x509 + +targets += ca_sig
[PATCH 3/5] ima: update kernel module signing process during build
The kernel build process currently only signs kernel modules when MODULE_SIG is enabled. Also, sign the kernel modules at build time when IMA_APPRAISE_MODSIG is enabled. Signed-off-by: Nayna Jain --- certs/Kconfig | 2 +- init/Kconfig | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/certs/Kconfig b/certs/Kconfig index c94e93d8bccf..48675ad319db 100644 --- a/certs/Kconfig +++ b/certs/Kconfig @@ -4,7 +4,7 @@ menu "Certificates for signature checking" config MODULE_SIG_KEY string "File name or PKCS#11 URI of module signing key" default "certs/signing_key.pem" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Provide the file name of a private key/certificate in PEM format, or a PKCS#11 URI according to RFC7512. The file should contain, or diff --git a/init/Kconfig b/init/Kconfig index 29ad68325028..68147bbda5f9 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2162,7 +2162,7 @@ config MODULE_SIG_FORCE config MODULE_SIG_ALL bool "Automatically sign all modules" default y - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help Sign all modules during make modules_install. Without this option, modules must be signed manually, using the scripts/sign-file tool. @@ -2172,7 +2172,7 @@ comment "Do not forget to sign required modules with scripts/sign-file" choice prompt "Which hash algorithm should modules be signed with?" - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG help This determines which sort of hashing algorithm will be used during signature generation. This algorithm _must_ be built into the kernel @@ -2204,7 +2204,7 @@ endchoice config MODULE_SIG_HASH string - depends on MODULE_SIG + depends on MODULE_SIG || IMA_APPRAISE_MODSIG default "sha1" if MODULE_SIG_SHA1 default "sha224" if MODULE_SIG_SHA224 default "sha256" if MODULE_SIG_SHA256 -- 2.18.1
[PATCH 2/5] keys: generate self-signed module signing key using CSR
Loading a key on the IMA trusted keyring requires the key be signed by an existing key on the builtin or secondary trusted keyring. Creating a Certificate Signing Request (CSR) allows the certificate to be self-signed or signed by a CA. This patch generates a self-signed module signing key using CSR. Signed-off-by: Nayna Jain --- Makefile | 3 ++- certs/Makefile | 15 +++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index af18aab6bbee..9c87fdd600d8 100644 --- a/Makefile +++ b/Makefile @@ -1473,7 +1473,8 @@ MRPROPER_FILES += include/config include/generated \ .config .config.old .version \ Module.symvers \ certs/signing_key.pem certs/signing_key.x509 \ - certs/x509.genkey \ + certs/x509.genkey certs/signing_key.key \ + certs/signing_key.crt certs/signing_key.csr \ vmlinux-gdb.py \ *.spec diff --git a/certs/Makefile b/certs/Makefile index f4c25b67aad9..b2be7eb413d3 100644 --- a/certs/Makefile +++ b/certs/Makefile @@ -60,11 +60,18 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey @$(kecho) "### needs to be run as root, and uses a hardware random" @$(kecho) "### number generator if one is available." @$(kecho) "###" - $(Q)openssl req -new -nodes -utf8 -$(CONFIG_MODULE_SIG_HASH) -days 36500 \ - -batch -x509 -config $(obj)/x509.genkey \ - -outform PEM -out $(obj)/signing_key.pem \ - -keyout $(obj)/signing_key.pem \ + $(Q)openssl req -new -nodes -utf8 \ + -batch -config $(obj)/x509.genkey \ + -outform PEM -out $(obj)/signing_key.csr \ + -keyout $(obj)/signing_key.key -extensions myexts \ $($(quiet)redirect_openssl) + $(Q)openssl x509 -req -days 36500 -in $(obj)/signing_key.csr \ + -outform PEM -out $(obj)/signing_key.crt \ + -signkey $(obj)/signing_key.key \ + -$(CONFIG_MODULE_SIG_HASH) -extensions myexts \ + -extfile $(obj)/x509.genkey \ + $($(quiet)redirect_openssl) + @cat $(obj)/signing_key.key $(obj)/signing_key.crt >> $(obj)/signing_key.pem @$(kecho) "###" @$(kecho) "### Key pair generated." @$(kecho) "###" -- 2.18.1
[PATCH 1/5] keys: cleanup build time module signing keys
The "mrproper" target is still looking for build time generated keys
in the old path instead of certs/ directory.
This patch fixes the path as well removes the names of the files which
are no longer generated.
Signed-off-by: Nayna Jain
Fixes: 28a68f828266 ("modsign: Use single PEM file for autogenerated key")
---
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index ade44ac4cc2f..af18aab6bbee 100644
--- a/Makefile
+++ b/Makefile
@@ -1472,9 +1472,9 @@ MRPROPER_FILES += include/config include/generated
\
debian snap tar-install \
.config .config.old .version \
Module.symvers \
- signing_key.pem signing_key.priv signing_key.x509 \
- x509.genkey extra_certificates signing_key.x509.keyid \
- signing_key.x509.signer vmlinux-gdb.py \
+ certs/signing_key.pem certs/signing_key.x509 \
+ certs/x509.genkey \
+ vmlinux-gdb.py \
*.spec
# Directories & files removed with 'make distclean'
--
2.18.1
[PATCH 0/5] ima: kernel build support for loading the kernel module signing key
Kernel modules are currently only signed when CONFIG_MODULE_SIG is enabled.
The kernel module signing key is a self-signed CA only loaded onto the
.builtin_trusted_key keyring. On secure boot enabled systems with an arch
specific IMA policy enabled, but without MODULE_SIG enabled, kernel modules
are not signed, nor is the kernel module signing public key loaded onto the
IMA keyring.
In order to load the the kernel module signing key onto the IMA trusted
keyring ('.ima'), the certificate needs to be signed by a CA key either on
the builtin or secondary keyrings. This series of patches enables IMA
verification of signed kernel modules by:
* Defining a kernel CA key. The CA key signs the kernel module signing key
and is loaded onto .builtin_trusted_key keyring, only when the kernel
module signing key is loaded onto the .ima keyring.
* Enable module signing at build time for IMA_APPRAISE_MODSIG as well
Nayna Jain (5):
keys: cleanup build time module signing keys
keys: generate self-signed module signing key using CSR
ima: update kernel module signing process during build
keys: define build time generated ephemeral kernel CA key
ima: enable loading of build time generated key to .ima keyring
Makefile | 9 ++--
certs/Kconfig | 2 +-
certs/Makefile| 77 ---
certs/system_certificates.S | 16 +++-
certs/system_keyring.c| 56 +++--
include/keys/system_keyring.h | 9 +++-
init/Kconfig | 6 +--
security/integrity/digsig.c | 4 ++
8 files changed, 151 insertions(+), 28 deletions(-)
--
2.18.1
Re: [PATCH v5 1/4] certs: Add EFI_CERT_X509_GUID support for dbx entries
On 1/27/21 11:11 PM, Eric Snowberg wrote:
On Jan 27, 2021, at 8:54 PM, Nayna wrote:
On 1/22/21 1:10 PM, Eric Snowberg wrote:
This fixes CVE-2020-26541.
The Secure Boot Forbidden Signature Database, dbx, contains a list of now
revoked signatures and keys previously approved to boot with UEFI Secure
Boot enabled. The dbx is capable of containing any number of
EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and EFI_CERT_X509_GUID
entries.
Currently when EFI_CERT_X509_GUID are contained in the dbx, the entries are
skipped.
Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
is found, it is added as an asymmetrical key to the .blacklist keyring.
Anytime the .platform keyring is used, the keys in the .blacklist keyring
are referenced, if a matching key is found, the key will be rejected.
Signed-off-by: Eric Snowberg
Reviewed-by: Jarkko Sakkinen
Signed-off-by: David Howells
---
v5: Function name changes done by David Howells
---
certs/blacklist.c | 32 +++
certs/blacklist.h | 12 +++
certs/system_keyring.c| 6
include/keys/system_keyring.h | 11 +++
.../platform_certs/keyring_handler.c | 11 +++
5 files changed, 72 insertions(+)
diff --git a/certs/blacklist.c b/certs/blacklist.c
index 6514f9ebc943..a7f021878a4b 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -100,6 +100,38 @@ int mark_hash_blacklisted(const char *hash)
return 0;
}
+int add_key_to_revocation_list(const char *data, size_t size)
+{
+ key_ref_t key;
+
+ key = key_create_or_update(make_key_ref(blacklist_keyring, true),
+ "asymmetric",
+ NULL,
+ data,
+ size,
+ ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
KEY_USR_VIEW),
+ KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
+
+ if (IS_ERR(key)) {
+ pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
+ return PTR_ERR(key);
+ }
+
+ return 0;
+}
+
+int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
+{
+ int ret;
+
+ ret = validate_trust(pkcs7, blacklist_keyring);
+
+ if (ret == 0)
+ return -EKEYREJECTED;
+
+ return -ENOKEY;
+}
+
/**
* is_hash_blacklisted - Determine if a hash is blacklisted
* @hash: The hash to be checked as a binary blob
diff --git a/certs/blacklist.h b/certs/blacklist.h
index 1efd6fa0dc60..420bb7c86e07 100644
--- a/certs/blacklist.h
+++ b/certs/blacklist.h
@@ -1,3 +1,15 @@
#include
+#include
+#include
extern const char __initconst *const blacklist_hashes[];
+
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+#define validate_trust pkcs7_validate_trust
+#else
+static inline int validate_trust(struct pkcs7_message *pkcs7,
+struct key *trust_keyring)
+{
+ return -ENOKEY;
+}
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 798291177186..cc165b359ea3 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -241,6 +241,12 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
pr_devel("PKCS#7 platform keyring is not available\n");
goto error;
}
+
+ ret = is_key_on_revocation_list(pkcs7);
+ if (ret != -ENOKEY) {
+ pr_devel("PKCS#7 platform key is on revocation list\n");
+ goto error;
+ }
}
ret = pkcs7_validate_trust(pkcs7, trusted_keys);
if (ret < 0) {
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..61f98739e8b1 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -31,11 +31,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
#define restrict_link_by_builtin_and_secondary_trusted
restrict_link_by_builtin_trusted
#endif
+extern struct pkcs7_message *pkcs7;
#ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
extern int mark_hash_blacklisted(const char *hash);
+extern int add_key_to_revocation_list(const char *data, size_t size);
extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
const char *type);
extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
+extern int is_key_on_revocation_list(struct pkcs7_message *pkcs7);
#else
static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
const char *type)
@@ -47,6 +50,14 @@ static inline int is_binary_blacklisted(const u8 *hash,
size_t hash_len)
{
return 0;
}
+static inline int add_key_to_revocation_list(const char *data, size_t s
Re: [PATCH v5 1/4] certs: Add EFI_CERT_X509_GUID support for dbx entries
*pkcs7)
+{
+ return -ENOKEY;
+}
#endif
#ifdef CONFIG_IMA_BLACKLIST_KEYRING
diff --git a/security/integrity/platform_certs/keyring_handler.c
b/security/integrity/platform_certs/keyring_handler.c
index c5ba695c10e3..5604bd57c990 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -55,6 +55,15 @@ static __init void uefi_blacklist_binary(const char *source,
uefi_blacklist_hash(source, data, len, "bin:", 4);
}
+/*
+ * Add an X509 cert to the revocation list.
+ */
+static __init void uefi_revocation_list_x509(const char *source,
+const void *data, size_t len)
+{
+ add_key_to_revocation_list(data, len);
+}
In keeping the naming convention with other functions that blacklist
hashes, why can't we call these functions:
* uefi_revocation_list_x509() -> uefi_blacklist_x509_cert()
* add_key_to_revocation_list() -> uefi_blacklist_cert()
* is_key_on_revocation_list() -> is_cert_blacklisted()
Thanks & Regards,
- Nayna
Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
On 1/27/21 10:41 AM, Eric Snowberg wrote: On Jan 27, 2021, at 7:03 AM, Mimi Zohar wrote: [Cc'ing linux-integrity] On Wed, 2021-01-27 at 11:46 +, David Howells wrote: Jarkko Sakkinen wrote: I suppose a user space tool could be created. But wouldn’t what is currently done in the kernel in this area need to be removed? Right. I don't think this was a great idea in the first place to do to the kernel but since it exists, I guess the patch does make sense. This information needs to be loaded from the UEFI tables before the system starts loading any kernel modules or running any programs (if we do verification of such, which I think IMA can do). There needs to a clear distinction between the pre-boot and post-boot keys. UEFI has its own trust model, which should be limited to UEFI. The .platform keyring was upstreamed and limited to verifying the kexec kernel image. Any other usage of the .platform keyring keys is abusing its intended purpose. The cover letter says, "Anytime the .platform keyring is used, the keys in the .blacklist keyring are referenced, if a matching key is found, the key will be rejected." I don't have a problem with loading the UEFI X509 dbx entries as long as its usage is limited to verifying the kexec kernel image. Correct, with my patch, when EFI_CERT_X509_GUID entries are found in the dbx, they will only be used during kexec. I believe the latest dbx file on uefi.org contains three of these entires. Based on my understanding of why the platform keyring was introduced, I intentionally only used these for kexec. I do question the current upstream mainline code though. Currently, when EFI_CERT_X509_SHA256_GUID or EFI_CERT_SHA256_GUID entries are found in the dbx, they are applied everywhere. It seems like there should be a dbx revocation keyring equivalent to the current platform keyring that is only used for pre-boot. If that is a direction you would like to see this go in the future, let me know, I’d be happy to work on it. Yes, as you said, currently blacklist entries from dbx for EFI_CERT_X509_SHA256_GUID or EFI_CERT_SHA256_GUID are applied everywhere, and does not satisfy the trust model for .platform keyring. We should fix this, but changing now might break some existing systems. Probably it should be discussed as separate thread from this patchset. Thanks & Regards, - Nayna
Re: [PATCH v2 0/2] ima: Fix keyrings race condition and other key related bugs
On 8/11/20 3:26 PM, Tyler Hicks wrote:
v2:
- Always return an ERR_PTR from ima_alloc_rule_opt_list() (Nayna)
- Add Lakshmi's Reviewed-by to both patches
- Rebased on commit 3db0d0c276a7 ("integrity: remove redundant
initialization of variable ret") of next-integrity
v1:
https://lore.kernel.org/lkml/20200727140831.64251-1-tyhi...@linux.microsoft.com/
Nayna pointed out that the "keyrings=" option in an IMA policy rule
should only be accepted when CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is
enabled:
https://lore.kernel.org/linux-integrity/336cc947-1f70-0286-6506-6df3d1d23...@linux.vnet.ibm.com/
While fixing this, the compiler warned me about the potential for the
ima_keyrings pointer to be NULL despite it being used, without a check
for NULL, as the destination address for the strcpy() in
ima_match_keyring().
It also became apparent that there was not adequate locking around the
use of the pre-allocated buffer that ima_keyrings points to. The kernel
keyring has a lock (.sem member of struct key) that ensures only one key
can be added to a given keyring at a time but there's no protection
against adding multiple keys to different keyrings at the same time.
The first patch in this series fixes both ima_keyrings related issues by
parsing the list of keyrings in a KEY_CHECK rule at policy load time
rather than deferring the parsing to policy check time. Once that fix is
in place, the second patch can enforce that
CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS must be enabled in order to use
"func=KEY_CHECK" or "keyrings=" options in IMA policy.
The new "keyrings=" value handling is done in a generic manner that can
be reused by other options in the future. This seems to make sense as
"appraise_type=" has similar style (though it doesn't need to be fully
parsed at this time) and using "|" as an alternation delimiter is
becoming the norm in IMA policy.
This series is based on commit 311aa6aafea4 ("ima: move
APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime") in
next-integrity.
Tyler
Tyler Hicks (2):
ima: Pre-parse the list of keyrings in a KEY_CHECK rule
ima: Fail rule parsing when asymmetric key measurement isn't
supportable
security/integrity/ima/ima_policy.c | 142 +++-
1 file changed, 96 insertions(+), 46 deletions(-)
Sorry for delay in responding.
The patches look good. Feel free to add my tag
Reviewed-by: Nayna Jain
Thanks & Regards,
- Nayna
Re: [PATCH 1/2] ima: Pre-parse the list of keyrings in a KEY_CHECK rule
On 7/27/20 10:08 AM, Tyler Hicks wrote:
The ima_keyrings buffer was used as a work buffer for strsep()-based
parsing of the "keyrings=" option of an IMA policy rule. This parsing
was re-performed each time an asymmetric key was added to a kernel
keyring for each loaded policy rule that contained a "keyrings=" option.
An example rule specifying this option is:
measure func=KEY_CHECK keyrings=a|b|c
The rule says to measure asymmetric keys added to any of the kernel
keyrings named "a", "b", or "c". The size of the buffer size was
equal to the size of the largest "keyrings=" value seen in a previously
loaded rule (5 + 1 for the NUL-terminator in the previous example) and
the buffer was pre-allocated at the time of policy load.
The pre-allocated buffer approach suffered from a couple bugs:
1) There was no locking around the use of the buffer so concurrent key
add operations, to two different keyrings, would result in the
strsep() loop of ima_match_keyring() to modify the buffer at the same
time. This resulted in unexpected results from ima_match_keyring()
and, therefore, could cause unintended keys to be measured or keys to
not be measured when IMA policy intended for them to be measured.
2) If the kstrdup() that initialized entry->keyrings in ima_parse_rule()
failed, the ima_keyrings buffer was freed and set to NULL even when a
valid KEY_CHECK rule was previously loaded. The next KEY_CHECK event
would trigger a call to strcpy() with a NULL destination pointer and
crash the kernel.
Remove the need for a pre-allocated global buffer by parsing the list of
keyrings in a KEY_CHECK rule at the time of policy load. The
ima_rule_entry will contain an array of string pointers which point to
the name of each keyring specified in the rule. No string processing
needs to happen at the time of asymmetric key add so iterating through
the list and doing a string comparison is all that's required at the
time of policy check.
In the process of changing how the "keyrings=" policy option is handled,
a couple additional bugs were fixed:
1) The rule parser accepted rules containing invalid "keyrings=" values
such as "a|b||c", "a|b|", or simply "|".
2) The /sys/kernel/security/ima/policy file did not display the entire
"keyrings=" value if the list of keyrings was longer than what could
fit in the fixed size tbuf buffer in ima_policy_show().
Fixes: 5c7bac9fb2c5 ("IMA: pre-allocate buffer to hold keyrings string")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks
---
security/integrity/ima/ima_policy.c | 138 +++-
1 file changed, 93 insertions(+), 45 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 07f033634b27..c328cfa0fc49 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -59,6 +59,11 @@ enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
+struct ima_rule_opt_list {
+ size_t count;
+ char *items[];
+};
+
struct ima_rule_entry {
struct list_head list;
int action;
@@ -78,7 +83,7 @@ struct ima_rule_entry {
int type; /* audit type */
} lsm[MAX_LSM_RULES];
char *fsname;
- char *keyrings; /* Measure keys added to these keyrings */
+ struct ima_rule_opt_list *keyrings; /* Measure keys added to these
keyrings */
struct ima_template_desc *template;
};
@@ -206,10 +211,6 @@ static LIST_HEAD(ima_policy_rules);
static LIST_HEAD(ima_temp_rules);
static struct list_head *ima_rules = _default_rules;
-/* Pre-allocated buffer used for matching keyrings. */
-static char *ima_keyrings;
-static size_t ima_keyrings_len;
-
static int ima_policy __initdata;
static int __init default_measure_policy_setup(char *str)
@@ -253,6 +254,72 @@ static int __init default_appraise_policy_setup(char *str)
}
__setup("ima_appraise_tcb", default_appraise_policy_setup);
+static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src)
+{
+ struct ima_rule_opt_list *opt_list;
+ size_t count = 0;
+ char *src_copy;
+ char *cur, *next;
+ size_t i;
+
+ src_copy = match_strdup(src);
+ if (!src_copy)
+ return NULL;
The caller of this function checks for IS_ERR(..) and not
IS_ERR_OR_NULL(..). Shouldn't it return ERR_PTR(-EINVAL) instead of NULL ?
Thanks & Regards,
- Nayna
Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
On 7/17/20 2:11 PM, Tyler Hicks wrote:
On 2020-07-17 13:40:22, Nayna wrote:
On 7/9/20 2:19 AM, Tyler Hicks wrote:
The "appraise_flag" option is only appropriate for appraise actions
and its "blacklist" value is only appropriate when
CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
only appropriate when "appraise_type=imasig|modsig" is also present.
Make this clear at policy load so that IMA policy authors don't assume
that other uses of "appraise_flag=blacklist" are supported.
Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with
modsig")
Signed-off-by: Tyler Hicks
Cc: Nayna Jain
---
* v3
- New patch
security/integrity/ima/ima_policy.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 81da02071d41..9842e2e0bc6d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry
*entry)
return false;
}
+ /* Ensure that combinations of flags are compatible with each other */
+ if (entry->flags & IMA_CHECK_BLACKLIST &&
+ !(entry->flags & IMA_MODSIG_ALLOWED))
+ return false;
+
return true;
}
@@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
result = -EINVAL;
break;
case Opt_appraise_flag:
+ if (entry->action != APPRAISE) {
+ result = -EINVAL;
+ break;
+ }
+
ima_log_string(ab, "appraise_flag", args[0].from);
- if (strstr(args[0].from, "blacklist"))
+ if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
+ strstr(args[0].from, "blacklist"))
entry->flags |= IMA_CHECK_BLACKLIST;
If IMA_APPRAISE_MODSIG is disabled, it will allow the following rule to
load, which is not as expected.
"appraise func=xxx_CHECK appraise_flag=blacklist appraise_type=imasig"
Missing is the "else" condition to immediately reject the policy rule.
Thanks for the review. You're right. This change is needed:
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 9842e2e0bc6d..cf3ddb38dfa8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1385,6 +1385,8 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
strstr(args[0].from, "blacklist"))
entry->flags |= IMA_CHECK_BLACKLIST;
+ else
+ result = -EINVAL;
break;
case Opt_permit_directio:
entry->flags |= IMA_PERMIT_DIRECTIO;
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain
Thanks & Regards,
- Nayna
Re: [PATCH v6] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
On 7/13/20 12:48 PM, Bruno Meneguele wrote:
The IMA_APPRAISE_BOOTPARAM config allows enabling different "ima_appraise="
modes - log, fix, enforce - at run time, but not when IMA architecture
specific policies are enabled. This prevents properly labeling the
filesystem on systems where secure boot is supported, but not enabled on the
platform. Only when secure boot is actually enabled should these IMA
appraise modes be disabled.
This patch removes the compile time dependency and makes it a runtime
decision, based on the secure boot state of that platform.
Test results as follows:
-> x86-64 with secure boot enabled
[0.015637] Kernel command line: <...> ima_policy=appraise_tcb
ima_appraise=fix
[0.015668] ima: Secure boot enabled: ignoring ima_appraise=fix boot
parameter option
-> powerpc with secure boot disabled
[0.00] Kernel command line: <...> ima_policy=appraise_tcb
ima_appraise=fix
[0.00] Secure boot mode disabled
-> Running the system without secure boot and with both options set:
CONFIG_IMA_APPRAISE_BOOTPARAM=y
CONFIG_IMA_ARCH_POLICY=y
Audit prompts "missing-hash" but still allow execution and, consequently,
filesystem labeling:
type=INTEGRITY_DATA msg=audit(07/09/2020 12:30:27.778:1691) : pid=4976
uid=root auid=root ses=2
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=appraise_data
cause=missing-hash comm=bash name=/usr/bin/evmctl dev="dm-0" ino=493150
res=no
Cc: sta...@vger.kernel.org
Fixes: d958083a8f64 ("x86/ima: define arch_get_ima_policy() for x86")
Signed-off-by: Bruno Meneguele
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain
Thanks & Regards,
- Nayna
Re: [PATCH v3 01/12] ima: Have the LSM free its audit rule
On 7/9/20 2:19 AM, Tyler Hicks wrote: Ask the LSM to free its audit rule rather than directly calling kfree(). Is it to be called audit rule or filter rule ? Likewise in subject line. Thanks & Regards, - Nayna
Re: [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
On 7/9/20 2:19 AM, Tyler Hicks wrote:
The KEY_CHECK function only supports the uid, pcr, and keyrings
conditionals. Make this clear at policy load so that IMA policy authors
don't assume that other conditionals are supported.
Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks
Reviewed-by: Lakshmi Ramasubramanian
---
* v3
- Added Lakshmi's Reviewed-by
- Adjust for the indentation change introduced in patch #4
* v2
- No change
security/integrity/ima/ima_policy.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 1c64bd6f1728..81da02071d41 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry
*entry)
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
+ if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+IMA_KEYRINGS))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
break;
default:
return false;
Should there be a check for IMA_MEASURE_ASYMMETRIC_KEYS in Opt_keyrings
in ima_parse_rule() to return immediately if not enabled ?
Thanks & Regards,
- Nayna
[PATCH v3] powerpc/pseries: detect secure and trusted boot state of the system.
The device-tree property to check secure and trusted boot state is
different for guests(pseries) compared to baremetal(powernv).
This patch updates the existing is_ppc_secureboot_enabled() and
is_ppc_trustedboot_enabled() functions to add support for pseries.
The secureboot and trustedboot state are exposed via device-tree property:
/proc/device-tree/ibm,secure-boot and /proc/device-tree/ibm,trusted-boot
The values of ibm,secure-boot under pseries are interpreted as:
0 - Disabled
1 - Enabled in Log-only mode. This patch interprets this value as
disabled, since audit mode is currently not supported for Linux.
2 - Enabled and enforced.
3-9 - Enabled and enforcing; requirements are at the discretion of the
operating system.
The values of ibm,trusted-boot under pseries are interpreted as:
0 - Disabled
1 - Enabled
Signed-off-by: Nayna Jain
Reviewed-by: Daniel Axtens
---
v3:
* fixed double check. Thanks Daniel for noticing it.
* updated patch description.
v2:
* included Michael Ellerman's feedback.
* added Daniel Axtens's Reviewed-by.
arch/powerpc/kernel/secure_boot.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/secure_boot.c
b/arch/powerpc/kernel/secure_boot.c
index 4b982324d368..118bcb5f79c4 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
static struct device_node *get_ppc_fw_sb_node(void)
{
@@ -23,12 +24,19 @@ bool is_ppc_secureboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ u32 secureboot;
node = get_ppc_fw_sb_node();
enabled = of_property_read_bool(node, "os-secureboot-enforcing");
-
of_node_put(node);
+ if (enabled)
+ goto out;
+
+ if (!of_property_read_u32(of_root, "ibm,secure-boot", ))
+ enabled = (secureboot > 1);
+
+out:
pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
@@ -38,12 +46,19 @@ bool is_ppc_trustedboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ u32 trustedboot;
node = get_ppc_fw_sb_node();
enabled = of_property_read_bool(node, "trusted-enabled");
-
of_node_put(node);
+ if (enabled)
+ goto out;
+
+ if (!of_property_read_u32(of_root, "ibm,trusted-boot", ))
+ enabled = (trustedboot > 0);
+
+out:
pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
--
2.26.2
[PATCH v2] powerpc/pseries: detect secure and trusted boot state of the system.
The device-tree property to check secure and trusted boot state is
different for guests(pseries) compared to baremetal(powernv).
This patch updates the existing is_ppc_secureboot_enabled() and
is_ppc_trustedboot_enabled() function to add support for pseries.
Signed-off-by: Nayna Jain
Reviewed-by: Daniel Axtens
---
v2:
* included Michael Ellerman's feedback.
* added Daniel Axtens's Reviewed-by.
arch/powerpc/kernel/secure_boot.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/secure_boot.c
b/arch/powerpc/kernel/secure_boot.c
index 4b982324d368..efb325cbd42f 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
static struct device_node *get_ppc_fw_sb_node(void)
{
@@ -23,12 +24,21 @@ bool is_ppc_secureboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ u32 secureboot;
node = get_ppc_fw_sb_node();
enabled = of_property_read_bool(node, "os-secureboot-enforcing");
-
of_node_put(node);
+ if (enabled)
+ goto out;
+
+ if (!of_property_read_u32(of_root, "ibm,secure-boot", )) {
+ if (secureboot)
+ enabled = (secureboot > 1) ? true : false;
+ }
+
+out:
pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
@@ -38,12 +48,21 @@ bool is_ppc_trustedboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ u32 trustedboot;
node = get_ppc_fw_sb_node();
enabled = of_property_read_bool(node, "trusted-enabled");
-
of_node_put(node);
+ if (enabled)
+ goto out;
+
+ if (!of_property_read_u32(of_root, "ibm,trusted-boot", )) {
+ if (trustedboot)
+ enabled = (trustedboot > 0) ? true : false;
+ }
+
+out:
pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
--
2.26.2
[PATCH] powerpc/pseries: detect secure and trusted boot state of the system.
The device-tree property to check secure and trusted boot state is
different for guests(pseries) compared to baremetal(powernv).
This patch updates the existing is_ppc_secureboot_enabled() and
is_ppc_trustedboot_enabled() function to add support for pseries.
Signed-off-by: Nayna Jain
---
arch/powerpc/kernel/secure_boot.c | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/secure_boot.c
b/arch/powerpc/kernel/secure_boot.c
index 4b982324d368..43fc6607c7a5 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
static struct device_node *get_ppc_fw_sb_node(void)
{
@@ -23,11 +24,20 @@ bool is_ppc_secureboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ const u32 *secureboot;
- node = get_ppc_fw_sb_node();
- enabled = of_property_read_bool(node, "os-secureboot-enforcing");
+ if (machine_is(powernv)) {
+ node = get_ppc_fw_sb_node();
+ enabled =
+ of_property_read_bool(node, "os-secureboot-enforcing");
+ of_node_put(node);
+ }
- of_node_put(node);
+ if (machine_is(pseries)) {
+ secureboot = of_get_property(of_root, "ibm,secure-boot", NULL);
+ if (secureboot)
+ enabled = (*secureboot > 1) ? true : false;
+ }
pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
@@ -38,11 +48,20 @@ bool is_ppc_trustedboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ const u32 *trustedboot;
- node = get_ppc_fw_sb_node();
- enabled = of_property_read_bool(node, "trusted-enabled");
+ if (machine_is(powernv)) {
+ node = get_ppc_fw_sb_node();
+ enabled = of_property_read_bool(node, "trusted-enabled");
+ of_node_put(node);
+ }
- of_node_put(node);
+ if (machine_is(pseries)) {
+ trustedboot =
+ of_get_property(of_root, "ibm,trusted-boot", NULL);
+ if (trustedboot)
+ enabled = (*trustedboot > 0) ? true : false;
+ }
pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
--
2.18.1
Re: [PATCH v2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
On 6/22/20 1:27 PM, Bruno Meneguele wrote: IMA_APPRAISE_BOOTPARAM has been marked as dependent on !IMA_ARCH_POLICY in compile time, enforcing the appraisal whenever the kernel had the arch policy option enabled. However it breaks systems where the option is actually set but the system wasn't booted in a "secure boot" platform. In this scenario, anytime the an appraisal policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, giving no chance to the user set the 'fix' state (ima_appraise=fix) to actually measure system's files. This patch remove this compile time dependency and move it to a runtime decision, based on the arch policy loading failure/success. Thanks for looking at this. For arch specific policies, kernel signature verification is enabled based on the secure boot state of the system. Perhaps, enforce the appraisal as well based on if secure boot is enabled. Thanks & Regards, - Nayna
[PATCH v2] powerpc/ima: fix secure boot rules in ima arch policy
To prevent verifying the kernel module appended signature twice
(finit_module), once by the module_sig_check() and again by IMA, powerpc
secure boot rules define an IMA architecture specific policy rule
only if CONFIG_MODULE_SIG_FORCE is not enabled. This, unfortunately, does
not take into account the ability of enabling "sig_enforce" on the boot
command line (module.sig_enforce=1).
Including the IMA module appraise rule results in failing the finit_module
syscall, unless the module signing public key is loaded onto the IMA
keyring.
This patch fixes secure boot policy rules to be based on CONFIG_MODULE_SIG
instead.
Fixes: 4238fad366a6 ("powerpc/ima: Add support to initialize ima policy rules")
Signed-off-by: Nayna Jain
---
v2:
* Fixes the patch description to specify the problem more clearly as asked
by Michael Ellerman.
arch/powerpc/kernel/ima_arch.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index e34116255ced..957abd592075 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -19,12 +19,12 @@ bool arch_ima_get_secureboot(void)
* to be stored as an xattr or as an appended signature.
*
* To avoid duplicate signature verification as much as possible, the IMA
- * policy rule for module appraisal is added only if CONFIG_MODULE_SIG_FORCE
+ * policy rule for module appraisal is added only if CONFIG_MODULE_SIG
* is not enabled.
*/
static const char *const secure_rules[] = {
"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
-#ifndef CONFIG_MODULE_SIG_FORCE
+#ifndef CONFIG_MODULE_SIG
"appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#endif
NULL
@@ -50,7 +50,7 @@ static const char *const secure_and_trusted_rules[] = {
"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
"measure func=MODULE_CHECK template=ima-modsig",
"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
-#ifndef CONFIG_MODULE_SIG_FORCE
+#ifndef CONFIG_MODULE_SIG
"appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#endif
NULL
--
2.18.1
Re: [PATCH] sysfs: add BIN_ATTR_WO() macro
On 10/01/2019 02:16 PM, Greg Kroah-Hartman wrote:
On Tue, Oct 01, 2019 at 02:08:53PM -0400, Nayna wrote:
Hi Greg,
On 08/26/2019 11:01 AM, Greg Kroah-Hartman wrote:
This variant was missing from sysfs.h, I guess no one noticed it before.
Turns out the powerpc secure variable code can use it, so add it to the
tree for it, and potentially others to take advantage of, instead of
open-coding it.
Reported-by: Nayna Jain
Signed-off-by: Greg Kroah-Hartman
---
I'll queue this up to my tree for 5.4-rc1, but if you want to take this
in your tree earlier, feel free to do so.
include/linux/sysfs.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 965236795750..5420817ed317 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -196,6 +196,12 @@ struct bin_attribute {
.size = _size,\
}
+#define __BIN_ATTR_WO(_name) { \
+ .attr = { .name = __stringify(_name), .mode = 0200 }, \
+ .store = _name##_store,\
+ .size = _size,\
+}
+
#define __BIN_ATTR_RW(_name, _size) \
__BIN_ATTR(_name, 0644, _name##_read, _name##_write, _size)
@@ -208,6 +214,9 @@ struct bin_attribute bin_attr_##_name = __BIN_ATTR(_name,
_mode, _read, \
#define BIN_ATTR_RO(_name, _size)\
struct bin_attribute bin_attr_##_name = __BIN_ATTR_RO(_name, _size)
+#define BIN_ATTR_WO(_name, _size) \
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_WO(_name, _size)
+
#define BIN_ATTR_RW(_name, _size)\
struct bin_attribute bin_attr_##_name = __BIN_ATTR_RW(_name, _size)
I am sorry. I didn't notice it via inspection but there is a bug in this
macro. When I actually try using it, compilation fails. Here's a likely
patch:
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 5420817ed317..fa7ee503fb76 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -196,9 +196,9 @@ struct bin_attribute {
.size = _size,\
}
-#define __BIN_ATTR_WO(_name) { \
+#define __BIN_ATTR_WO(_name, _size) { \
.attr = { .name = __stringify(_name), .mode = 0200 }, \
- .store = _name##_store,\
+ .write = _name##_write,\
.size = _size,\
}
Heh, good catch. Can you send a real patch for this that I can apply to
give you the proper credit for finding and fixing this?
Sure.. Thanks Greg !!
Thanks & Regards,
- Nayna
Re: [PATCH v6 3/9] powerpc: add support to initialize ima policy rules
On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:
Hello,
Hi,
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index ..39401b67f19e
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+
+#include
+#include
+
+bool arch_ima_get_secureboot(void)
+{
+ return is_powerpc_os_secureboot_enabled();
+}
+
+/* Defines IMA appraise rules for secureboot */
+static const char *const arch_rules[] = {
+ "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if !IS_ENABLED(CONFIG_MODULE_SIG)
+ "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+ NULL
+};
+
+/*
+ * Returns the relevant IMA arch policies based on the system secureboot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+ if (is_powerpc_os_secureboot_enabled())
+ return arch_rules;
+
+ return NULL;
+}
If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
then IMA won't enforce module signature either. x86's
arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
powerpc version need to do that as well?
On the flip side, if module signatures are enforced by the module
subsystem then IMA will verify the signature a second time since there's
no sharing of signature verification results between the module
subsystem and IMA (this was observed by Mimi).
IMHO this is a minor issue, since module loading isn't a hot path and
the duplicate work shouldn't impact anything. But it could be avoided by
having a NULL entry in arch_rules, which arch_get_ima_policy() would
dynamically update with the "appraise func=MODULE_CHECK" rule if
is_module_sig_enforced() is true.
Thanks Thiago for reviewing. I am wondering that this will give two
meanings for NULL. Can we do something like below, there are possibly
two options ?
1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().
OR
2. Let ima_get_action() check for is_module_sig_enforced() when policy
is appraise and func is MODULE_CHECK.
Thanks & Regards,
- Nayna
[PATCH v6 9/9] powerpc/ima: update ima arch policy to check for blacklist
This patch updates the arch specific policies for PowernV systems
to add check against blacklisted hashes before doing the verification.
Signed-off-by: Nayna Jain
---
arch/powerpc/kernel/ima_arch.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index 77c61b142042..3f57433c0824 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -24,9 +24,9 @@ bool arch_ima_get_secureboot(void)
static const char *const arch_rules[] = {
"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
"measure func=MODULE_CHECK template=ima-modsig",
- "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+ "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#if !IS_ENABLED(CONFIG_MODULE_SIG)
- "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+ "appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig",
#endif
NULL
};
--
2.20.1
[PATCH v6 8/9] ima: deprecate permit_directio, instead use appraise_flag
This patch deprecates the existing permit_directio flag, instead adds it as possible value to appraise_flag parameter. For eg. appraise_flag=permit_directio Signed-off-by: Nayna Jain --- Documentation/ABI/testing/ima_policy | 4 ++-- security/integrity/ima/ima_policy.c | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 4c97afcc0f3c..9a2a140dc561 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -24,8 +24,8 @@ Description: [euid=] [fowner=] [fsname=]] lsm:[[subj_user=] [subj_role=] [subj_type=] [obj_user=] [obj_role=] [obj_type=]] - option: [[appraise_type=]] [template=] [permit_directio] - [appraise_flag=[check_blacklist]] + option: [[appraise_type=]] [template=] [permit_directio(deprecated)] + [appraise_flag=[check_blacklist]|[permit_directio]] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index ad3b3af69460..d9df54c75d46 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1177,6 +1177,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) ima_log_string(ab, "appraise_flag", args[0].from); if (strstr(args[0].from, "blacklist")) entry->flags |= IMA_CHECK_BLACKLIST; + if (strstr(args[0].from, "permit_directio")) + entry->flags |= IMA_PERMIT_DIRECTIO; break; case Opt_permit_directio: entry->flags |= IMA_PERMIT_DIRECTIO; -- 2.20.1
Re: [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
On 09/02/2019 07:52 AM, Michael Ellerman wrote:
Hi Nayna,
Hi Michael,
Some more comments below.
Nayna Jain writes:
POWER secure boot relies on the kernel IMA security subsystem to
perform the OS kernel image signature verification.
Again this is just a design choice we've made, it's not specified
anywhere or anything like that. And it only applies to bare metal secure
boot, at least so far. AIUI.
Yes. I will make it consistent to use "PowerNV".
Since each secure
boot mode has different IMA policy requirements, dynamic definition of
the policy rules based on the runtime secure boot mode of the system is
required. On systems that support secure boot, but have it disabled,
only measurement policy rules of the kernel image and modules are
defined.
It's probably worth mentioning that we intend to use this in our
Linux-based boot loader, which uses kexec, and that's one of the reasons
why we're particularly interested in defining the rules for kexec?
Yes. Agreed. I will update patch description to add this.
This patch defines the arch-specific implementation to retrieve the
secure boot mode of the system and accordingly configures the IMA policy
rules.
This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
config is enabled.
Signed-off-by: Nayna Jain
---
arch/powerpc/Kconfig | 2 ++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/ima_arch.c | 50 ++
include/linux/ima.h| 3 +-
4 files changed, 55 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/kernel/ima_arch.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c902a39124dc..42109682b727 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
bool
default n
depends on PPC64
+ depends on IMA
+ depends on IMA_ARCH_POLICY
help
Linux on POWER with firmware secure boot enabled needs to define
security policies to extend secure boot to the OS.This config
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index d310ebb4e526..520b1c814197 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,7 +157,7 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
-obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o
+obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o ima_arch.o
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index ..ac90fac83338
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ *
+ * ima_arch.c
+ * - initialize ima policies for PowerPC Secure Boot
+ */
+
+#include
+#include
+
+bool arch_ima_get_secureboot(void)
+{
+ return get_powerpc_secureboot();
+}
+
+/*
+ * File signature verification is not needed, include only measurements
+ */
+static const char *const default_arch_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK",
+ "measure func=MODULE_CHECK",
+ NULL
+};
The rules above seem fairly self explanatory.
+
+/* Both file signature verification and measurements are needed */
+static const char *const sb_arch_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
+ "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if IS_ENABLED(CONFIG_MODULE_SIG)
+ "measure func=MODULE_CHECK",
+#else
+ "measure func=MODULE_CHECK template=ima-modsig",
+ "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
But these ones are not so obvious, at least to me who knows very little
about IMA.
Can you add a one line comment to each of the ones in here saying what
it does and why we want it?
Sure.
+ NULL
+};
+
+/*
+ * On PowerPC, file measurements are to be added to the IMA measurement list
+ * irrespective of the secure boot state of the system.
Why? Just because we think it's useful? Would be good to provide some
further justification.
Sure. I will clarify this in the next version.
Thanks & Regards,
- Nayna
Re: [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
On 09/02/2019 07:52 AM, Michael Ellerman wrote:
Hi Nayna,
Hi Michael,
Sorry I've taken so long to get to this series, there's just too many
patches that need reviewing :/
No problem. I understand. Thanks for reviewing.
Nayna Jain writes:
Secure boot on POWER defines different IMA policies based on the secure
boot state of the system.
The terminology throughout is a bit vague, we have POWER, PowerPC, Linux
on POWER etc.
What this patch is talking about is a particular implemention of secure
boot on some OpenPOWER machines running bare metal - am I right?
So saying "Secure boot on POWER defines different IMA policies" is a bit
broad I think. Really we've just decided that a way to implement secure
boot is to use IMA policies.
I think the idea was to convey that the same design can be reused or
extended as needed.
But I agree for now it is currently only OpenPOWER machines running on
bare metal, I will fix the wordings to use "PowerNV" consistently.
This patch defines a function to detect the secure boot state of the
system.
The PPC_SECURE_BOOT config represents the base enablement of secureboot
on POWER.
Signed-off-by: Nayna Jain
---
arch/powerpc/Kconfig | 11 +
arch/powerpc/include/asm/secboot.h | 27
arch/powerpc/kernel/Makefile | 2 +
arch/powerpc/kernel/secboot.c | 71 ++
4 files changed, 111 insertions(+)
create mode 100644 arch/powerpc/include/asm/secboot.h
create mode 100644 arch/powerpc/kernel/secboot.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 77f6ebf97113..c902a39124dc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -912,6 +912,17 @@ config PPC_MEM_KEYS
If unsure, say y.
+config PPC_SECURE_BOOT
+ prompt "Enable PowerPC Secure Boot"
How about "Enable secure boot support"
Yes. Sounds better.
+ bool
+ default n
The default is 'n', so you don't need that default line.
Sure.
+ depends on PPC64
Should it just depend on POWERNV for now? AFAIK there's nothing in here
that's necessarily going to be shared with the guest secure boot code is
there?
Yes. sounds good.
+ help
+ Linux on POWER with firmware secure boot enabled needs to define
+ security policies to extend secure boot to the OS.This config
+ allows user to enable OS Secure Boot on PowerPC systems that
+ have firmware secure boot support.
Again POWER vs PowerPC.
I think something like:
"Enable support for secure boot on some systems that have firmware
support for it. If in doubt say N."
Sure.
diff --git a/arch/powerpc/include/asm/secboot.h
b/arch/powerpc/include/asm/secboot.h
secure_boot.h would be fine.
Sure.
new file mode 100644
index ..e726261bb00b
--- /dev/null
+++ b/arch/powerpc/include/asm/secboot.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PowerPC secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
I prefer to not have email addresses in copyright headers, as they just
bit rot. Your email is in the git log.
Sure.
+ *
+ */
+#ifndef POWERPC_SECBOOT_H
+#define POWERPC_SECBOOT_H
We usually do _ASM_POWERPC_SECBOOT_H (or _ASM_POWERPC_SECURE_BOOT_H).
Sure.
+#ifdef CONFIG_PPC_SECURE_BOOT
+extern struct device_node *is_powerpc_secvar_supported(void);
+extern bool get_powerpc_secureboot(void);
You don't need 'extern' for functions in headers.
Yes. will fix.
+#else
+static inline struct device_node *is_powerpc_secvar_supported(void)
+{
+ return NULL;
+}
+
+static inline bool get_powerpc_secureboot(void)
+{
+ return false;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index ea0c69236789..d310ebb4e526 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,6 +157,8 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
+obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o
+
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
KCOV_INSTRUMENT_prom_init.o := n
diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
new file mode 100644
index ..5ea0d52d64ef
--- /dev/null
+++ b/arch/powerpc/kernel/secboot.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ *
+ * secboot.c
+ * - util function to get powerpc secboot state
That's not really necessary.
Sure.
+ */
+#include
+#include
+#include
+
+struct device_node *is_powerpc_secvar_supported(void)
This is a pretty weird signature. The "is_" implies it will return a
bool, but then it actually returns a device node *.
Yes. Agree. Will fix.
+{
+ struct device_n
[PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
POWER secure boot relies on the kernel IMA security subsystem to
perform the OS kernel image signature verification. Since each secure
boot mode has different IMA policy requirements, dynamic definition of
the policy rules based on the runtime secure boot mode of the system is
required. On systems that support secure boot, but have it disabled,
only measurement policy rules of the kernel image and modules are
defined.
This patch defines the arch-specific implementation to retrieve the
secure boot mode of the system and accordingly configures the IMA policy
rules.
This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
config is enabled.
Signed-off-by: Nayna Jain
---
arch/powerpc/Kconfig | 2 ++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/ima_arch.c | 50 ++
include/linux/ima.h| 3 +-
4 files changed, 55 insertions(+), 2 deletions(-)
create mode 100644 arch/powerpc/kernel/ima_arch.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c902a39124dc..42109682b727 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
bool
default n
depends on PPC64
+ depends on IMA
+ depends on IMA_ARCH_POLICY
help
Linux on POWER with firmware secure boot enabled needs to define
security policies to extend secure boot to the OS.This config
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index d310ebb4e526..520b1c814197 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,7 +157,7 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST)+= kvm.o kvm_emul.o
-obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o
+obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o ima_arch.o
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index ..ac90fac83338
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ *
+ * ima_arch.c
+ * - initialize ima policies for PowerPC Secure Boot
+ */
+
+#include
+#include
+
+bool arch_ima_get_secureboot(void)
+{
+ return get_powerpc_secureboot();
+}
+
+/*
+ * File signature verification is not needed, include only measurements
+ */
+static const char *const default_arch_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK",
+ "measure func=MODULE_CHECK",
+ NULL
+};
+
+/* Both file signature verification and measurements are needed */
+static const char *const sb_arch_rules[] = {
+ "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
+ "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if IS_ENABLED(CONFIG_MODULE_SIG)
+ "measure func=MODULE_CHECK",
+#else
+ "measure func=MODULE_CHECK template=ima-modsig",
+ "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+ NULL
+};
+
+/*
+ * On PowerPC, file measurements are to be added to the IMA measurement list
+ * irrespective of the secure boot state of the system. Signature verification
+ * is conditionally enabled based on the secure boot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+ if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
+ return sb_arch_rules;
+ return default_arch_rules;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..10af09b5b478 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
+#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
+ || defined(CONFIG_PPC_SECURE_BOOT)
extern bool arch_ima_get_secureboot(void);
extern const char * const *arch_get_ima_policy(void);
#else
--
2.20.1
[PATCH v5 1/2] powerpc: detect the secure boot mode of the system
Secure boot on POWER defines different IMA policies based on the secure
boot state of the system.
This patch defines a function to detect the secure boot state of the
system.
The PPC_SECURE_BOOT config represents the base enablement of secureboot
on POWER.
Signed-off-by: Nayna Jain
---
arch/powerpc/Kconfig | 11 +
arch/powerpc/include/asm/secboot.h | 27
arch/powerpc/kernel/Makefile | 2 +
arch/powerpc/kernel/secboot.c | 71 ++
4 files changed, 111 insertions(+)
create mode 100644 arch/powerpc/include/asm/secboot.h
create mode 100644 arch/powerpc/kernel/secboot.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 77f6ebf97113..c902a39124dc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -912,6 +912,17 @@ config PPC_MEM_KEYS
If unsure, say y.
+config PPC_SECURE_BOOT
+ prompt "Enable PowerPC Secure Boot"
+ bool
+ default n
+ depends on PPC64
+ help
+ Linux on POWER with firmware secure boot enabled needs to define
+ security policies to extend secure boot to the OS.This config
+ allows user to enable OS Secure Boot on PowerPC systems that
+ have firmware secure boot support.
+
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/secboot.h
b/arch/powerpc/include/asm/secboot.h
new file mode 100644
index ..e726261bb00b
--- /dev/null
+++ b/arch/powerpc/include/asm/secboot.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PowerPC secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ *
+ */
+#ifndef POWERPC_SECBOOT_H
+#define POWERPC_SECBOOT_H
+
+#ifdef CONFIG_PPC_SECURE_BOOT
+extern struct device_node *is_powerpc_secvar_supported(void);
+extern bool get_powerpc_secureboot(void);
+#else
+static inline struct device_node *is_powerpc_secvar_supported(void)
+{
+ return NULL;
+}
+
+static inline bool get_powerpc_secureboot(void)
+{
+ return false;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index ea0c69236789..d310ebb4e526 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,6 +157,8 @@ endif
obj-$(CONFIG_EPAPR_PARAVIRT) += epapr_paravirt.o epapr_hcalls.o
obj-$(CONFIG_KVM_GUEST)+= kvm.o kvm_emul.o
+obj-$(CONFIG_PPC_SECURE_BOOT) += secboot.o
+
# Disable GCOV, KCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
KCOV_INSTRUMENT_prom_init.o := n
diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
new file mode 100644
index ..5ea0d52d64ef
--- /dev/null
+++ b/arch/powerpc/kernel/secboot.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ *
+ * secboot.c
+ * - util function to get powerpc secboot state
+ */
+#include
+#include
+#include
+
+struct device_node *is_powerpc_secvar_supported(void)
+{
+ struct device_node *np;
+ int status;
+
+ np = of_find_node_by_name(NULL, "ibm,secureboot");
+ if (!np) {
+ pr_info("secureboot node is not found\n");
+ return NULL;
+ }
+
+ status = of_device_is_compatible(np, "ibm,secureboot-v3");
+ if (!status) {
+ pr_info("Secure variables are not supported by this
firmware\n");
+ return NULL;
+ }
+
+ return np;
+}
+
+bool get_powerpc_secureboot(void)
+{
+ struct device_node *np;
+ struct device_node *secvar_np;
+ const u64 *psecboot;
+ u64 secboot = 0;
+
+ np = is_powerpc_secvar_supported();
+ if (!np)
+ goto disabled;
+
+ /* Fail-safe for any failure related to secvar */
+ secvar_np = of_get_child_by_name(np, "secvar");
+ if (!secvar_np) {
+ pr_err("Expected secure variables support, fail-safe\n");
+ goto enabled;
+ }
+
+ if (!of_device_is_available(secvar_np)) {
+ pr_err("Secure variables support is in error state,
fail-safe\n");
+ goto enabled;
+ }
+
+ psecboot = of_get_property(secvar_np, "secure-mode", NULL);
+ if (!psecboot)
+ goto enabled;
+
+ secboot = be64_to_cpup((__be64 *)psecboot);
+ if (!(secboot & (~0x0)))
+ goto disabled;
+
+enabled:
+ pr_info("secureboot mode enabled\n");
+ return true;
+
+disabled:
+ pr_info("secureboot mode disabled\n");
+ return false;
+}
--
2.20.1
[PATCH v5 0/2] powerpc: Enabling IMA arch specific secure boot policies
IMA subsystem supports custom, built-in, arch-specific policies to define the files to be measured and appraised. These policies are honored based on the priority where arch-specific policies is the highest and custom is the lowest. OpenPOWER systems rely on IMA for signature verification of the kernel. This patchset adds support for powerpc specific arch policies that are defined based on system's OS secureboot state. The OS secureboot state of the system is determined via device-tree entry. Changelog: v5: * secureboot state is now read via device tree entry rather than OPAL secure variables * ima arch policies are updated to use policy based template for measurement rules v4: * Fixed the build issue as reported by Satheesh Rajendran. v3: * OPAL APIs in Patch 1 are updated to provide generic interface based on key/keylen. This patchset updates kernel OPAL APIs to be compatible with generic interface. * Patch 2 is cleaned up to use new OPAL APIs. * Since OPAL can support different types of backend which can vary in the variable interpretation, the Patch 2 is updated to add a check for the backend version * OPAL API now expects consumer to first check the supported backend version before calling other secvar OPAL APIs. This check is now added in patch 2. * IMA policies in Patch 3 is updated to specify appended signature and per policy template. * The patches now are free of any EFIisms. v2: * Removed Patch 1: powerpc/include: Override unneeded early ioremap functions * Updated Subject line and patch description of the Patch 1 of this series * Removed dependency of OPAL_SECVAR on EFI, CPU_BIG_ENDIAN and UCS2_STRING * Changed OPAL APIs from static to non-static. Added opal-secvar.h for the same * Removed EFI hooks from opal_secvar.c * Removed opal_secvar_get_next(), opal_secvar_enqueue() and opal_query_variable_info() function * get_powerpc_sb_mode() in secboot.c now directly calls OPAL Runtime API rather than via EFI hooks. * Fixed log messages in get_powerpc_sb_mode() function. * Added dependency for PPC_SECURE_BOOT on configs PPC64 and OPAL_SECVAR * Replaced obj-$(CONFIG_IMA) with obj-$(CONFIG_PPC_SECURE_BOOT) in arch/powerpc/kernel/Makefile Nayna Jain (2): powerpc: detect the secure boot mode of the system powerpc: Add support to initialize ima policy rules arch/powerpc/Kconfig | 13 ++ arch/powerpc/include/asm/secboot.h | 27 arch/powerpc/kernel/Makefile | 2 + arch/powerpc/kernel/ima_arch.c | 50 + arch/powerpc/kernel/secboot.c | 71 ++ include/linux/ima.h| 3 +- 6 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 arch/powerpc/include/asm/secboot.h create mode 100644 arch/powerpc/kernel/ima_arch.c create mode 100644 arch/powerpc/kernel/secboot.c -- 2.20.1
Re: [PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
Hi Jarkko,
On 07/09/2019 12:38 PM, Jarkko Sakkinen wrote:
On Mon, Jul 08, 2019 at 03:43:04PM -0700, Christoph Hellwig wrote:
On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote:
static int tpm_get_pcr_allocation(struct tpm_chip *chip)
{
int rc;
rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
tpm2_get_pcr_allocation(chip) :
tpm1_get_pcr_allocation(chip);
return rc > 0 ? -ENODEV : rc;
}
This addresses the issue that Stefan also pointed out. You have to
deal with the TPM error codes.
Hm, in the past I was told by Christoph not to use the ternary
operator. Have things changed? Other than removing the comment, the
only other difference is the return.
In the end it is a matter of personal preference, but I find the
quote version above using the ternary horribly obsfucated.
I fully agree that the return statement is an obsfucated mess and
not a good place at all for using ternary operator.
I have posted the v3 version that includes the suggested corrections by
you and Stefan. Sorry for some delay.
Michal and Sachin, I would appreciate if you can test the v3 version,
please ?
Thanks & Regards,
- Nayna
[PATCH v3] tpm: tpm_ibm_vtpm: Fix unallocated banks
The nr_allocated_banks and allocated banks are initialized as part of
tpm_chip_register. Currently, this is done as part of auto startup
function. However, some drivers, like the ibm vtpm driver, do not run
auto startup during initialization. This results in uninitialized memory
issue and causes a kernel panic during boot.
This patch moves the pcr allocation outside the auto startup function
into tpm_chip_register. This ensures that allocated banks are initialized
in any case.
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
PCR read")
Reported-by: Michal Suchanek
Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Tested-by: Sachin Sant
Tested-by: Michal Suchánek
---
Changelog:
v3:
* Includes Stefan's feedback correctly:
* Fixed handling of rc > 0 error
* Includes Jarkko's feedback related to comment and the function.
v2:
* Includes Jarkko's feedbacks
* fixes the function name to tpm_get_pcr_allocation()
* adds new function tpm1_get_pcr_allocation()
* updates patch summary line
* fixes alignment
* adds Reported-by: Michal Suchanek
* Includes Stefan's feedbacks
* Fixes overwriting of return code
* Fixes misplacing of tpm_chip_stop()
* Adds Reviewed-by, Tested-by
drivers/char/tpm/tpm-chip.c | 20
drivers/char/tpm/tpm.h | 2 ++
drivers/char/tpm/tpm1-cmd.c | 36
drivers/char/tpm/tpm2-cmd.c | 6 +-
4 files changed, 47 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..5a0396d6560d 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -550,6 +550,20 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
return hwrng_register(>hwrng);
}
+static int tpm_get_pcr_allocation(struct tpm_chip *chip)
+{
+ int rc;
+
+ rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
+tpm2_get_pcr_allocation(chip) :
+tpm1_get_pcr_allocation(chip);
+
+ if (rc > 0)
+ return -ENODEV;
+
+ return rc;
+}
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -569,6 +583,12 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
return rc;
rc = tpm_auto_startup(chip);
+ if (rc) {
+ tpm_chip_stop(chip);
+ return rc;
+ }
+
+ rc = tpm_get_pcr_allocation(chip);
tpm_chip_stop(chip);
if (rc)
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2cce072f25b5..d571df3694c3 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -399,6 +399,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8
*res_buf);
ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
+int tpm1_get_pcr_allocation(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
@@ -454,6 +455,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
int tpm2_auto_startup(struct tpm_chip *chip);
void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 85dcf2654d11..260a3917f0fe 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -696,18 +696,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
goto out;
}
- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks) {
- rc = -ENOMEM;
- goto out;
- }
-
- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
- chip->nr_allocated_banks = 1;
-
return rc;
out:
if (rc > 0)
@@ -776,3 +764,27 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32
tpm_suspend_pcr)
return rc;
}
+/**
+ * tpm1_get_pcr_allocation() - initialize the allocated bank
+ * @chip: TPM chip to use.
+ *
+ * The function initializes the SHA1 allocated bank to extend PCR
+ *
+ * Return:
+ * * 0 on success,
+ * * < 0 on error.
+ */
+int tpm1_get_pcr_allocation(struct tpm_chip *chip)
+{
+ chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
+
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On 07/05/2019 01:50 PM, Jarkko Sakkinen wrote:
On Fri, 2019-07-05 at 11:32 -0400, Nayna wrote:
I am not sure of the purpose of tpm_stop_chip(), so I have left it as it
is. Jarkko, what do you think about the change ?
Stefan right. Your does not work, or will randomly work or not work
depending on the chip.
You need to turn the TPM on with tpm_chip_start() and turn it off with
tpm_chip_stop() once you are done. This is done in tpm_chip_register()
before calling tpm_auto_startup().
TPM power management was once in tpm_transmit() but not anymore after my
patch set that removed nested tpm_transmit() calls.
While you're on it please take into account my earlier feedback.
Also, short summary could be "tpm: tpm_ibm_vtpm: Fix unallocated banks"
Some oddballs in your patch that I have to ask.
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
rc = tpm2_get_pcr_allocation(chip);
if (rc)
goto out;
}
chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
GFP_KERNEL);
if (!chip->allocated_banks) {
rc = -ENOMEM;
goto out;
}
Why you don't return on site and instead jump somewhere? Also the
2nd line for kcalloc() is misaligned.
out:
if (rc < 0)
rc = -ENODEV;
This will cause a new regression i.e. you let TPM error codes
through.
To summarize this patch fixes one regression and introduces two
completely new ones...
Thanks Jarkko. I just now posted the v2 version that includes your and
Stefan's feedbacks.
Thanks & Regards,
- Nayna
[PATCH v2] tpm: tpm_ibm_vtpm: Fix unallocated banks
The nr_allocated_banks and allocated banks are initialized as part of
tpm_chip_register. Currently, this is done as part of auto startup
function. However, some drivers, like the ibm vtpm driver, do not run
auto startup during initialization. This results in uninitialized memory
issue and causes a kernel panic during boot.
This patch moves the pcr allocation outside the auto startup function
into tpm_chip_register. This ensures that allocated banks are initialized
in any case.
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
PCR read")
Reported-by: Michal Suchanek
Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Tested-by: Sachin Sant
Tested-by: Michal Suchánek
---
Changelog:
v2:
* Includes Jarkko's feedbacks
* fixes the function name to tpm_get_pcr_allocation()
* adds new function tpm1_get_pcr_allocation()
* updates patch summary line
* fixes alignment
* adds Reported-by: Michal Suchanek
* Includes Stefan's feedbacks
* Fixes overwriting of return code
* Fixes misplacing of tpm_chip_stop()
* Adds Reviewed-by, Tested-by
drivers/char/tpm/tpm-chip.c | 22 ++
drivers/char/tpm/tpm.h | 2 ++
drivers/char/tpm/tpm1-cmd.c | 36
drivers/char/tpm/tpm2-cmd.c | 6 +-
4 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..6589291df355 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -550,6 +550,22 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
return hwrng_register(>hwrng);
}
+/*
+ * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs
+ * @chip: TPM chip to use.
+ */
+static int tpm_get_pcr_allocation(struct tpm_chip *chip)
+{
+ int rc;
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ rc = tpm2_get_pcr_allocation(chip);
+ else
+ rc = tpm1_get_pcr_allocation(chip);
+
+ return rc;
+}
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -569,6 +585,12 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
return rc;
rc = tpm_auto_startup(chip);
+ if (rc) {
+ tpm_chip_stop(chip);
+ return rc;
+ }
+
+ rc = tpm_get_pcr_allocation(chip);
tpm_chip_stop(chip);
if (rc)
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2cce072f25b5..d571df3694c3 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -399,6 +399,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8
*res_buf);
ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
+int tpm1_get_pcr_allocation(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
@@ -454,6 +455,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
int tpm2_auto_startup(struct tpm_chip *chip);
void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 85dcf2654d11..260a3917f0fe 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -696,18 +696,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
goto out;
}
- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks) {
- rc = -ENOMEM;
- goto out;
- }
-
- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
- chip->nr_allocated_banks = 1;
-
return rc;
out:
if (rc > 0)
@@ -776,3 +764,27 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32
tpm_suspend_pcr)
return rc;
}
+/**
+ * tpm1_get_pcr_allocation() - initialize the allocated bank
+ * @chip: TPM chip to use.
+ *
+ * The function initializes the SHA1 allocated bank to extend PCR
+ *
+ * Return:
+ * * 0 on success,
+ * * < 0 on error.
+ */
+int tpm1_get_pcr_allocation(struct tpm_chip *chip)
+{
+ chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
+ GFP_KERNEL);
+ if (!chip->allocated_banks)
+ r
Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
On 07/05/2019 10:13 AM, Stefan Berger wrote:
On 7/3/19 11:32 PM, Nayna Jain wrote:
The nr_allocated_banks and allocated banks are initialized as part of
tpm_chip_register. Currently, this is done as part of auto startup
function. However, some drivers, like the ibm vtpm driver, do not run
auto startup during initialization. This results in uninitialized memory
issue and causes a kernel panic during boot.
This patch moves the pcr allocation outside the auto startup function
into tpm_chip_register. This ensures that allocated banks are
initialized
in any case.
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms
with
PCR read")
Signed-off-by: Nayna Jain
---
drivers/char/tpm/tpm-chip.c | 37 +
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm1-cmd.c | 12
drivers/char/tpm/tpm2-cmd.c | 6 +-
4 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..958508bb8379 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
return hwrng_register(>hwrng);
}
+/*
+ * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs
+ */
+static int tpm_pcr_allocation(struct tpm_chip *chip)
+{
+ int rc = 0;
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ rc = tpm2_get_pcr_allocation(chip);
+ if (rc)
+ goto out;
+ }
+
+ /* Initialize TPM 1.2 */
+ chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
+ GFP_KERNEL);
+ if (!chip->allocated_banks) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
+ chip->allocated_banks[0].digest_size =
hash_digest_size[HASH_ALGO_SHA1];
+ chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
+ chip->nr_allocated_banks = 1;
+
+ return 0;
+out:
+ if (rc < 0)
+ rc = -ENODEV;
The old code where you lifted this from said:
out:
if (rc > 0)
rc = -ENODEV;
return rc;
It would not overwrite -ENOMEM with -ENODEV but yours does.
I think the correct fix would be to use:
if (rc > 0)
rc = -ENODEV;
Yes. I think I misread it. Thanks Stefan. Will fix this..
+ return rc;
+}
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -573,6 +606,10 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
return rc;
Above this is tpm_chip_stop(chip) because (afaik) none of the
following function calls in tpm_chip_register() needed the TPM, but
now with tpm_pcr_allocation() you will need to send a command to the
TPM. So I would say you should move the tpm_chip_stop() into the error
branch visible above and also after the tpm_pcr_allocation().
+ rc = tpm_pcr_allocation(chip);
tpm_chip_stop(chip);
I am not sure of the purpose of tpm_stop_chip(), so I have left it as it
is. Jarkko, what do you think about the change ?
Thanks & Regards,
- Nayna
Re: [PATCH] Revert "tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()"
Hi Tyler, On 07/04/2019 03:58 PM, Tyler Hicks wrote: Hey Mimi! On 2019-07-04 11:46:41, Mimi Zohar wrote: Hi Jarkko, On Thu, 2019-07-04 at 07:48 -0400, Mimi Zohar wrote: On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid following crash: Thank you. I think this the right choice for the moment. I fixed a trivial checkpatch.pl error and added the mandatory tags. Can you check quickly v2 (just posted)? I already made it available in my master and next. Could you please wait few days? I would prefer to fix this issue instead of reverting the whole patch. Nayna posted a patch late yesterday titled "tpm: fixes uninitialized allocated banks for IBM vtpm driver", which addresses this bug. Now with my review, and with Sachin Sant's and Michal Suchánek testing, instead of reverting this patch could you pick up Nayna's patch instead? It looks to me like the revert would also fix a bug that is keeping the eCryptfs module from loading when the TPM is in an "inactive" state: https://bugzilla.kernel.org/show_bug.cgi?id=203953 I just noticed that it was recently discussed here, too: https://lore.kernel.org/linux-integrity/1562244125.6165.95.ca...@linux.ibm.com/T/#t I believe that the revert would fix it because the call to init_digests()/tpm_get_random() would no longer be in the path of loading ecryptfs.ko (which depends on encrypted-keys.ko, which depends on trusted.ko). If the revert isn't used, we'll need a different fix for bug 203953. It should be an easy fix but I don't want it to be forgotten. I think if TPM is inactive/disabled, it needs to be handled during tpm_chip_register() itself. However, probably that needs more analysis and discussion. For now, in context of the trusted.ko module, it seems init_trusted() should "put_device", but continue even if init_digests() fails, that will fix the issue. Thanks & Regards, - Nayna
[PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver
The nr_allocated_banks and allocated banks are initialized as part of
tpm_chip_register. Currently, this is done as part of auto startup
function. However, some drivers, like the ibm vtpm driver, do not run
auto startup during initialization. This results in uninitialized memory
issue and causes a kernel panic during boot.
This patch moves the pcr allocation outside the auto startup function
into tpm_chip_register. This ensures that allocated banks are initialized
in any case.
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
PCR read")
Signed-off-by: Nayna Jain
---
drivers/char/tpm/tpm-chip.c | 37 +
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm1-cmd.c | 12
drivers/char/tpm/tpm2-cmd.c | 6 +-
4 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..958508bb8379 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
return hwrng_register(>hwrng);
}
+/*
+ * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs
+ */
+static int tpm_pcr_allocation(struct tpm_chip *chip)
+{
+ int rc = 0;
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ rc = tpm2_get_pcr_allocation(chip);
+ if (rc)
+ goto out;
+ }
+
+ /* Initialize TPM 1.2 */
+ chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
+ GFP_KERNEL);
+ if (!chip->allocated_banks) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
+ chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+ chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
+ chip->nr_allocated_banks = 1;
+
+ return 0;
+out:
+ if (rc < 0)
+ rc = -ENODEV;
+ return rc;
+}
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -573,6 +606,10 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
return rc;
+ rc = tpm_pcr_allocation(chip);
+ if (rc)
+ return rc;
+
tpm_sysfs_add_device(chip);
rc = tpm_bios_log_setup(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2cce072f25b5..eabe6b755fa6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -454,6 +454,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
int tpm2_auto_startup(struct tpm_chip *chip);
void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 85dcf2654d11..ec5f3693c096 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -696,18 +696,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
goto out;
}
- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks) {
- rc = -ENOMEM;
- goto out;
- }
-
- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
- chip->nr_allocated_banks = 1;
-
return rc;
out:
if (rc > 0)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e74c5b7b64bf..b4384d0e3741 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -841,7 +841,7 @@ struct tpm2_pcr_selection {
u8 pcr_select[3];
} __packed;
-static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
{
struct tpm2_pcr_selection pcr_selection;
struct tpm_buf buf;
@@ -1041,10 +1041,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
goto out;
}
- rc = tpm2_get_pcr_allocation(chip);
- if (rc)
- goto out;
-
rc = tpm2_get_cc_attrs_tbl(chip);
out:
--
2.20.1
Re: [PATCH] integrity: Fix __integrity_init_keyring() section mismatch
On 06/17/2019 03:44 AM, Geert Uytterhoeven wrote:
With gcc-4.6.3:
WARNING: vmlinux.o(.text.unlikely+0x24c64): Section mismatch in reference
from the function __integrity_init_keyring() to the function
.init.text:set_platform_trusted_keys()
The function __integrity_init_keyring() references
the function __init set_platform_trusted_keys().
This is often because __integrity_init_keyring lacks a __init
annotation or the annotation of set_platform_trusted_keys is wrong.
Indeed, if the compiler decides not to inline __integrity_init_keyring(),
a warning is issued.
Fix this by adding the missing __init annotation.
Fixes: 9dc92c45177ab70e ("integrity: Define a trusted platform keyring")
Signed-off-by: Geert Uytterhoeven
Thanks for fixing it.
Reviewed-by: Nayna Jain
Thanks & Regards,
- Nayna
[PATCH] x86/ima: fix the Kconfig dependency for IMA_ARCH_POLICY
If enabled, ima arch specific policies always adds the measurements rules, this makes it dependent on CONFIG_IMA. CONFIG_IMA_APPRAISE implicitly takes care of this, however it is needed explicitly for CONFIG_KEXEC_VERIFY_SIG. This patch adds the CONFIG_IMA dependency in combination with CONFIG_KEXEC_VERIFY_SIG for CONFIG_IMA_ARCH_POLICY Fixes: d958083a8f640 (x86/ima: define arch_get_ima_policy() for x86) Signed-off-by: Nayna Jain Cc: Eric Biederman Cc: Dave Young --- security/integrity/ima/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index a18f8c6d13b5..df65d2d41905 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -159,7 +159,8 @@ config IMA_APPRAISE config IMA_ARCH_POLICY bool "Enable loading an IMA architecture specific policy" -depends on KEXEC_VERIFY_SIG || IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS +depends on (KEXEC_VERIFY_SIG && IMA) || IMA_APPRAISE \ + && INTEGRITY_ASYMMETRIC_KEYS default n help This option enables loading an IMA architecture specific policy -- 2.17.1
Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic
On 04/25/2019 01:19 PM, prsriva wrote: On 2019-04-25 4:48 a.m., Nayna wrote: On 04/23/2019 08:15 PM, Prakhar Srivastava wrote: From: Prakhar Srivastava Signed-off-by: Prakhar Srivastava --- The v2 version has to be on top of the HEAD of the repository itself, and not on the v1 version. Only the final reviewed and tested version makes to the upstream. Btw, which repository and its branch are you using ? I am basing my changes off IMA branch: git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git Ok. Please use either next-integrity branch or James Morris next-general or next-testing. Thanks & Regards, - Nayna
Re: [PATCH v2 2/5 RFC] use event name instead of enum to make the call generic
On 04/23/2019 08:15 PM, Prakhar Srivastava wrote:
From: Prakhar Srivastava
Signed-off-by: Prakhar Srivastava
---
Currently for soft reboot(kexec_file_load) the kernel file and
signature is measured by IMA. The cmdline args used to load the kernel
is not measured.
The boot aggregate that gets calculated will have no change since the
EFI loader has not been triggered.
Adding the kexec cmdline args measure and kernel version will add some
attestable criteria.
Any reason for including the whole commit message after "---"
Anything after "---" is not included in the patch description when patch
is applied.
This comment applies to all the patches in this patchset.
remove enums to control type of buffers entries, instead pass the event name to
be used.
Is the last statement meant to be a Changelog from v1-> v2 ? Only the
changelog has to be after "---"
Also, If posting more than one patch, it is preferrable to add a
cover-letter.
include/linux/ima.h | 10 ++
kernel/kexec_file.c | 3 +++
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_main.c | 30 ++
4 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 733d0cb9dedc..5e41507c57e5 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -14,12 +14,6 @@
#include
struct linux_binprm;
-enum __buffer_id {
- KERNEL_VERSION,
- KEXEC_CMDLINE,
- MAX_BUFFER_ID = KEXEC_CMDLINE
-} buffer_id;
-
Is the v2 version created on top of the v1 version that was posted ?
The v2 version has to be on top of the HEAD of the repository itself,
and not on the v1 version. Only the final reviewed and tested version
makes to the upstream.
Btw, which repository and its branch are you using ?
Thanks & Regards,
- Nayna
#ifdef CONFIG_IMA
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_file_check(struct file *file, int mask, int opened);
@@ -29,7 +23,7 @@ extern int ima_read_file(struct file *file, enum
kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
-extern void ima_buffer_check(const void *buff, int size, enum buffer_id id);
+extern void ima_buffer_check(const void *buff, int size, char *eventname);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
@@ -72,7 +66,7 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
}
static inline void ima_buffer_check(const void *buff, int size,
- enum buffer_id id)
+ char *eventname)
{
return;
}
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b118735fea9d..2a5234eb4b28 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -182,6 +182,9 @@ kimage_file_prepare_segments(struct kimage *image, int
kernel_fd, int initrd_fd,
ret = -EINVAL;
goto out;
}
+
+ ima_buffer_check(image->cmdline_buf, cmdline_len - 1,
+ "kexec_cmdline");
}
/* Call arch image load handlers */
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b71f2f6f7421..fcade3c103ed 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -181,8 +181,8 @@ enum ima_hooks {
FIRMWARE_CHECK,
KEXEC_KERNEL_CHECK,
KEXEC_INITRAMFS_CHECK,
- BUFFER_CHECK,
POLICY_CHECK,
+ BUFFER_CHECK,
MAX_CHECK
};
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 6408cadaadbb..da82c705a5ed 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -160,8 +160,7 @@ void ima_file_free(struct file *file)
* (Instead of using the file hash the buffer hash is used).
* @buff - The buffer that needs to be added to the log
* @size - size of buffer(in bytes)
- * @id - buffer id, this is differentiator for the various buffers
- * that can be measured.
+ * @id - eventname, event name to be used for buffer measurement.
*
* The buffer passed is added to the ima logs.
* If the sig template is used, then the sig field contains the buffer.
@@ -170,7 +169,7 @@ void ima_file_free(struct file *file)
* On error cases surface errors from ima calls.
*/
static int process_buffer_measurement(const void *buff, int size,
- enum buffer_id id)
+ char *eventname)
{
int ret = -EINVAL;
struct ima_template_entry *entry = NULL;
@@ -185,23 +184,13 @@ static int process_buffer_measurement(const void *buff,
int size,
int violation = 0;
int pcr = CONF
Re: [PATCH] x86/ima: require signed kernel modules
On 01/31/2019 02:18 PM, Mimi Zohar wrote: Require signed kernel modules on systems with secure boot mode enabled. To coordinate between appended kernel module signatures and IMA signatures, only define an IMA MODULE_CHECK policy rule if CONFIG_MODULE_SIG is not enabled. This patch defines a function named set_module_sig_required() and renames is_module_sig_enforced() to is_module_sig_enforced_or_required(). The call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY being enabled. Signed-off-by: Mimi Zohar --- Reviewed-by: Nayna Jain Thanks & Regards, - Nayna
Re: [PATCH v4 1/2] integrity, KEYS: add a reference to platform keyring
On 01/18/2019 04:17 AM, Kairui Song wrote:
commit 9dc92c45177a ('integrity: Define a trusted platform keyring')
introduced a .platform keyring for storing preboot keys, used for
verifying kernel images' signature. Currently only IMA-appraisal is able
to use the keyring to verify kernel images that have their signature
stored in xattr.
This patch exposes the .platform keyring, making it accessible for
verifying PE signed kernel images as well.
Suggested-by: Mimi Zohar
Signed-off-by: Kairui Song
Reviewed-by: Mimi Zohar
Tested-by: Mimi Zohar
---
certs/system_keyring.c| 9 +
include/keys/system_keyring.h | 5 +
security/integrity/digsig.c | 6 ++
3 files changed, 20 insertions(+)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 81728717523d..4690ef9cda8a 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -24,6 +24,9 @@ static struct key *builtin_trusted_keys;
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
static struct key *secondary_trusted_keys;
#endif
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+static struct key *platform_trusted_keys;
+#endif
extern __initconst const u8 system_certificate_list[];
extern __initconst const unsigned long system_certificate_list_size;
@@ -265,4 +268,10 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+void __init set_platform_trusted_keys(struct key *keyring) {
+ platform_trusted_keys = keyring;
+}
+#endif
+
#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 359c2f936004..9e1b7849b6aa 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -61,5 +61,10 @@ static inline struct key *get_ima_blacklist_keyring(void)
}
#endif /* CONFIG_IMA_BLACKLIST_KEYRING */
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+
+extern void __init set_platform_trusted_keys(struct key* keyring);
+
+#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
#endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index f45d6edecf99..bfabc2a8111d 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -89,6 +89,12 @@ static int __integrity_init_keyring(const unsigned int id,
key_perm_t perm,
keyring[id] = NULL;
}
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+ if (id == INTEGRITY_KEYRING_PLATFORM) {
Shouldn't it also check that keyring[id] is not NULL ?
Thanks & Regards,
- Nayna
+ set_platform_trusted_keys(keyring[id]);
+ }
+#endif
+
return err;
}
Re: [RFC PATCH 2/2] kexec, KEYS: Make use of platform keyring for signature verify
On 2019-01-14 21:42, Dave Young wrote: On 01/14/19 at 11:10am, Mimi Zohar wrote: On Sun, 2019-01-13 at 09:39 +0800, Dave Young wrote: > Hi, > > On 01/11/19 at 11:13am, Mimi Zohar wrote: > > On Fri, 2019-01-11 at 21:43 +0800, Dave Young wrote: > > [snip] > > > > > Personally I would like to see platform key separated from integrity. > > > But for the kexec_file part I think it is good at least it works with > > > this fix. > > > > > > Acked-by: Dave Young > > > > The original "platform" keyring patches that Nayna posted multiple > > times were in the certs directory, but nobody commented/responded. So > > she reworked the patches, moving them to the integrity directory and > > posted them (cc'ing the kexec mailing list). It's a bit late to be > > asking to move it, isn't it? > > Hmm, apologize for being late, I did not get chance to have a look the > old series. Since we have the needs now, it should be still fine > > Maybe Kairui can check Nayna's old series, see if he can do something > again? Whether the platform keyring is defined in certs/ or in integrity/ the keyring id needs to be accessible to the other, without making the keyring id global. Moving where the platform keyring is defined is not the problem. Agreed, but just feel kexec depends on IMA sounds not good. The platform keyring is not dependent on IMA, it is dependent on "integrity" - CONFIG_INTEGRITY_ASYMMETRIC_KEYS. Other CONFIGS which it needs are CONFIG_SYSTEM_BLACKLIST_KEYRING, CONFIG_EFI. Thanks & Regards, - Nayna
[PATCH v2a 5/7] efi: Import certificates from UEFI Secure Boot
From: Josh Boyer
Secure Boot stores a list of allowed certificates in the 'db' variable.
This patch imports those certificates into the platform keyring. The shim
UEFI bootloader has a similar certificate list stored in the 'MokListRT'
variable. We import those as well.
Secure Boot also maintains a list of disallowed certificates in the 'dbx'
variable. We load those certificates into the system blacklist keyring
and forbid any kernel signed with those from loading.
[zo...@linux.ibm.com: dropped Josh's original patch description]
Signed-off-by: Josh Boyer
Signed-off-by: David Howells
Signed-off-by: Nayna Jain
Acked-by: Serge Hallyn
Signed-off-by: Mimi Zohar
---
Changelog:
v2a:
- refactored uefi_blacklist_x509_tbs() and uefi_blacklist_binary()
v2:
- Fixed the checkpatch.pl warnings
v0:
- This patch replaces the loading of certificates onto the secondary
keyring with platform keyring
- removed the CONFIG LOAD_UEFI_KEYS
- moved the file load_uefi.o from certs to security/integrity/platform_certs
security/integrity/Makefile | 5 +-
security/integrity/platform_certs/load_uefi.c | 169 ++
2 files changed, 173 insertions(+), 1 deletion(-)
create mode 100644 security/integrity/platform_certs/load_uefi.c
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 6ee9058866cd..86df9aba8c0f 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -10,7 +10,10 @@ integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) +=
platform_certs/platform_keyring.o \
- platform_certs/efi_parser.o
+ platform_certs/efi_parser.o \
+ platform_certs/load_uefi.o
+obj-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/load_uefi.o
+$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
diff --git a/security/integrity/platform_certs/load_uefi.c
b/security/integrity/platform_certs/load_uefi.c
new file mode 100644
index ..8ceafa58d98c
--- /dev/null
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../integrity.h"
+
+static efi_guid_t efi_cert_x509_guid __initdata = EFI_CERT_X509_GUID;
+static efi_guid_t efi_cert_x509_sha256_guid __initdata =
+ EFI_CERT_X509_SHA256_GUID;
+static efi_guid_t efi_cert_sha256_guid __initdata = EFI_CERT_SHA256_GUID;
+
+/*
+ * Get a certificate list blob from the named EFI variable.
+ */
+static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
+ unsigned long *size)
+{
+ efi_status_t status;
+ unsigned long lsize = 4;
+ unsigned long tmpdb[4];
+ void *db;
+
+ status = efi.get_variable(name, guid, NULL, , );
+ if (status != EFI_BUFFER_TOO_SMALL) {
+ pr_err("Couldn't get size: 0x%lx\n", status);
+ return NULL;
+ }
+
+ db = kmalloc(lsize, GFP_KERNEL);
+ if (!db)
+ return NULL;
+
+ status = efi.get_variable(name, guid, NULL, , db);
+ if (status != EFI_SUCCESS) {
+ kfree(db);
+ pr_err("Error reading db var: 0x%lx\n", status);
+ return NULL;
+ }
+
+ *size = lsize;
+ return db;
+}
+
+/*
+ * Blacklist a hash.
+ */
+static __init void uefi_blacklist_hash(const char *source, const void *data,
+ size_t len, const char *type,
+ size_t type_len)
+{
+ char *hash, *p;
+
+ hash = kmalloc(type_len + len * 2 + 1, GFP_KERNEL);
+ if (!hash)
+ return;
+ p = memcpy(hash, type, type_len);
+ p += type_len;
+ bin2hex(p, data, len);
+ p += len * 2;
+ *p = 0;
+
+ mark_hash_blacklisted(hash);
+ kfree(hash);
+}
+
+/*
+ * Blacklist an X509 TBS hash.
+ */
+static __init void uefi_blacklist_x509_tbs(const char *source,
+ const void *data, size_t len)
+{
+ uefi_blacklist_hash(source, data, len, "tbs:", 4);
+}
+
+/*
+ * Blacklist the hash of an executable.
+ */
+static __init void uefi_blacklist_binary(const char *source,
+const void *data, size_t len)
+{
+ uefi_blacklist_hash(source, data, len, "bin:", 4);
+}
+
+/*
+ * Return the appropriate handler for particular signature list types found in
+ * the UEFI db and MokListRT tables.
+ */
+static __init efi_ele
Re: [PATCH v2 5/7] efi: Import certificates from UEFI Secure Boot
On 12/12/2018 12:17 AM, James Morris wrote:
On Sun, 9 Dec 2018, Nayna Jain wrote:
+/*
+ * Blacklist an X509 TBS hash.
+ */
+static __init void uefi_blacklist_x509_tbs(const char *source,
+ const void *data, size_t len)
+{
+ char *hash, *p;
+
+ hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
+ if (!hash)
+ return;
+ p = memcpy(hash, "tbs:", 4);
+ p += 4;
+ bin2hex(p, data, len);
+ p += len * 2;
+ *p = 0;
+
+ mark_hash_blacklisted(hash);
+ kfree(hash);
+}
+
+/*
+ * Blacklist the hash of an executable.
+ */
+static __init void uefi_blacklist_binary(const char *source,
+const void *data, size_t len)
+{
+ char *hash, *p;
+
+ hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
+ if (!hash)
+ return;
+ p = memcpy(hash, "bin:", 4);
+ p += 4;
+ bin2hex(p, data, len);
+ p += len * 2;
+ *p = 0;
+
+ mark_hash_blacklisted(hash);
+ kfree(hash);
+}
These could be refactored into one function.
Thanks James for reviewing.
Yes, the code should be refactored. However, I think making it a single
function would require adding a new field to the function callback
definitions as well. Probably, a simpler approach would be to define a
common function uefi_blacklist_hash(...) which can then be used by the
two functions uefi_blacklist_x509_tbs(...) and
uefi_blacklist_binary(...). These two functions now act as wrapper
functions. Below is the example code:
+/*
+ * Blacklist a hash.
+ */
+static __init void uefi_blacklist_hash(const char *source, const void
*data,
+ size_t len, char *type, size_t type_len)
+{
+ char *hash, *p;
+
+ hash = kmalloc(type_len + len * 2 + 1, GFP_KERNEL);
+ if (!hash)
+ return;
+ p = memcpy(hash, type, type_len);
+ p += type_len;
+ bin2hex(p, data, len);
+ p += len * 2;
+ *p = 0;
+
+ mark_hash_blacklisted(hash);
+ kfree(hash);
+}
+
+/*
+ * Blacklist an X509 TBS hash.
+ */
+static __init void uefi_blacklist_x509_tbs(const char *source,
+ const void *data, size_t len)
+{
+ uefi_blacklist_hash(source, data, len, "tbs:" , 4);
+}
+
+/*
+ * Blacklist the hash of an executable.
+ */
+static __init void uefi_blacklist_binary(const char *source,
+ const void *data, size_t len)
+{
+ uefi_blacklist_hash(source, data, len, "bin:" , 4);
+}
Thanks & Regards,
- Nayna
Re: [PATCH v6 2/7] tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2
On 12/04/2018 01:51 PM, Roberto Sassu wrote: TCG defines two structures, TCG_EfiSpecIDEventStruct and TCG_PCR_EVENT2, which contain variable-sized arrays in the middle of the definition. Since these structures are not suitable for type casting, this patch removes structure members after the variable-sized arrays and adds the _head suffix to the structure name, to indicate that the renamed structures do not contain all fields defined by TCG. Lastly, given that variable-sized arrays are now in the last position, and given that the size of the arrays cannot be determined in advance, this patch also sets the size of those arrays to zero and removes the definition of TPM2_ACTIVE_PCR_BANKS. Signed-off-by: Roberto Sassu Tested-by: Nayna Jain Thanks & Regards, - Nayna
Re: [PATCH v6 4/7] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
On 12/05/2018 05:10 AM, Jarkko Sakkinen wrote:
On Tue, Dec 04, 2018 at 09:21:35AM +0100, Roberto Sassu wrote:
Currently the TPM driver allows other kernel subsystems to read only the
SHA1 PCR bank. This patch modifies the parameters of tpm_pcr_read() and
tpm2_pcr_read() to pass a tpm_digest structure, which contains the desired
hash algorithm. Also, since commit 125a22105410 ("tpm: React correctly to
RC_TESTING from TPM 2.0 self tests") removed the call to tpm2_pcr_read(),
the new parameter is expected to be always not NULL.
Due to the API change, IMA functions have been modified.
Signed-off-by: Roberto Sassu
Acked-by: Mimi Zohar
Reviewed-by: Jarkko Sakkinen
Mimi, Nayna, can you help with testing this (because of the IMA change)?
Tested-by: Nayna Jain
Thanks & Regards,
- Nayna
/Jarkko
[PATCH v2 1/7] integrity: Define a trusted platform keyring
On secure boot enabled systems, a verified kernel may need to kexec
additional kernels. For example, it may be used as a bootloader needing
to kexec a target kernel or it may need to kexec a crashdump kernel. In
such cases, it may want to verify the signature of the next kernel
image.
It is further possible that the kernel image is signed with third party
keys which are stored as platform or firmware keys in the 'db' variable.
The kernel, however, can not directly verify these platform keys, and an
administrator may therefore not want to trust them for arbitrary usage.
In order to differentiate platform keys from other keys and provide the
necessary separation of trust, the kernel needs an additional keyring to
store platform keys.
This patch creates the new keyring called ".platform" to isolate keys
provided by platform from keys by kernel. These keys are used to
facilitate signature verification during kexec. Since the scope of this
keyring is only the platform/firmware keys, it cannot be updated from
userspace.
This keyring can be enabled by setting CONFIG_INTEGRITY_PLATFORM_KEYRING.
Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Acked-by: Serge Hallyn
---
security/integrity/Kconfig | 11 +
security/integrity/Makefile| 1 +
security/integrity/digsig.c| 48 +++---
security/integrity/integrity.h | 3 +-
.../integrity/platform_certs/platform_keyring.c| 35
5 files changed, 83 insertions(+), 15 deletions(-)
create mode 100644 security/integrity/platform_certs/platform_keyring.c
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..4b4d2aeef539 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -51,6 +51,17 @@ config INTEGRITY_TRUSTED_KEYRING
.evm keyrings be signed by a key on the system trusted
keyring.
+config INTEGRITY_PLATFORM_KEYRING
+bool "Provide keyring for platform/firmware trusted keys"
+depends on INTEGRITY_ASYMMETRIC_KEYS
+depends on SYSTEM_BLACKLIST_KEYRING
+depends on EFI
+help
+ Provide a separate, distinct keyring for platform trusted keys, which
+ the kernel automatically populates during initialization from values
+ provided by the platform for verifying the kexec'ed kerned image
+ and, possibly, the initramfs signature.
+
config INTEGRITY_AUDIT
bool "Enables integrity auditing support "
depends on AUDIT
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 04d6e462b079..046ffc1bb42d 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -9,6 +9,7 @@ integrity-y := iint.o
integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
+integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) +=
platform_certs/platform_keyring.o
subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 5eacba858e4b..fef2a858300c 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -35,6 +35,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX]
= {
".ima",
#endif
"_module",
+ ".platform",
};
#ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
@@ -73,12 +74,39 @@ int integrity_digsig_verify(const unsigned int id, const
char *sig, int siglen,
return -EOPNOTSUPP;
}
-int __init integrity_init_keyring(const unsigned int id)
+static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
+ struct key_restriction *restriction)
{
const struct cred *cred = current_cred();
+ int err = 0;
+
+ keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
+ KGIDT_INIT(0), cred, perm,
+ KEY_ALLOC_NOT_IN_QUOTA,
+ restriction, NULL);
+ if (IS_ERR(keyring[id])) {
+ err = PTR_ERR(keyring[id]);
+ pr_info("Can't allocate %s keyring (%d)\n",
+ keyring_name[id], err);
+ keyring[id] = NULL;
+ }
+
+ return err;
+}
+
+int __init integrity_init_keyring(const unsigned int id)
+{
struct key_restriction *restriction;
+ key_perm_t perm;
int err = 0;
+ if (id == INTEGRITY_KEYRING_PLATFORM) {
+ restriction = NULL;
+ perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW
+ | KEY_USR_READ | KEY_USR_SEARCH;
+ goto out;
+ }
+
if (!IS_ENABLED(CONFIG
[PATCH v2 3/7] efi: Add EFI signature data types
From: Dave Howells
Add the data types that are used for containing hashes, keys and
certificates for cryptographic verification along with their corresponding
type GUIDs.
Signed-off-by: David Howells
Acked-by: Nayna Jain
Acked-by: Serge Hallyn
---
Changelog:
v0:
- No changes
include/linux/efi.h | 25 +
1 file changed, 25 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 845174e113ce..214516b29b36 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -663,6 +663,10 @@ void efi_native_runtime_setup(void);
#define EFI_IMAGE_SECURITY_DATABASE_GUID EFI_GUID(0xd719b2cb, 0x3d3a,
0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
#define EFI_SHIM_LOCK_GUID EFI_GUID(0x605dab50, 0xe046,
0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+#define EFI_CERT_SHA256_GUID EFI_GUID(0xc1c41626, 0x504c,
0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28)
+#define EFI_CERT_X509_GUID EFI_GUID(0xa5c059a1, 0x94e4,
0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72)
+#define EFI_CERT_X509_SHA256_GUID EFI_GUID(0x3bd2a492, 0x96c0,
0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
+
/*
* This GUID is used to pass to the kernel proper the struct screen_info
* structure that was populated by the stub based on the GOP protocol instance
@@ -934,6 +938,27 @@ typedef struct {
efi_memory_desc_t entry[0];
} efi_memory_attributes_table_t;
+typedef struct {
+ efi_guid_t signature_owner;
+ u8 signature_data[];
+} efi_signature_data_t;
+
+typedef struct {
+ efi_guid_t signature_type;
+ u32 signature_list_size;
+ u32 signature_header_size;
+ u32 signature_size;
+ u8 signature_header[];
+ /* efi_signature_data_t signatures[][] */
+} efi_signature_list_t;
+
+typedef u8 efi_sha256_hash_t[32];
+
+typedef struct {
+ efi_sha256_hash_t to_be_signed_hash;
+ efi_time_t time_of_revocation;
+} efi_cert_x509_sha256_t;
+
/*
* All runtime access to EFI goes through this structure:
*/
--
2.13.6
[PATCH v2 4/7] efi: Add an EFI signature blob parser
From: Dave Howells
Add a function to parse an EFI signature blob looking for elements of
interest. A list is made up of a series of sublists, where all the
elements in a sublist are of the same type, but sublists can be of
different types.
For each sublist encountered, the function pointed to by the
get_handler_for_guid argument is called with the type specifier GUID and
returns either a pointer to a function to handle elements of that type or
NULL if the type is not of interest.
If the sublist is of interest, each element is passed to the handler
function in turn.
Signed-off-by: David Howells
Signed-off-by: Nayna Jain
Acked-by: Serge Hallyn
---
Changelog:
v0:
- removed the CONFIG EFI_SIGNATURE_LIST_PARSER
- moved efi_parser.c from certs to security/integrity/platform_certs
directory
v2:
- Fixed the checkpatch.pl warnings
include/linux/efi.h| 9 +++
security/integrity/Makefile| 3 +-
security/integrity/platform_certs/efi_parser.c | 108 +
3 files changed, 119 insertions(+), 1 deletion(-)
create mode 100644 security/integrity/platform_certs/efi_parser.c
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 214516b29b36..c3206b134137 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1141,6 +1141,15 @@ extern int efi_memattr_apply_permissions(struct
mm_struct *mm,
char * __init efi_md_typeattr_format(char *buf, size_t size,
const efi_memory_desc_t *md);
+
+typedef void (*efi_element_handler_t)(const char *source,
+ const void *element_data,
+ size_t element_size);
+extern int __init parse_efi_signature_list(
+ const char *source,
+ const void *data, size_t size,
+ efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *));
+
/**
* efi_range_is_wc - check the WC bit on an address range
* @start: starting kvirt address
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 046ffc1bb42d..6ee9058866cd 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -9,7 +9,8 @@ integrity-y := iint.o
integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
-integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) +=
platform_certs/platform_keyring.o
+integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) +=
platform_certs/platform_keyring.o \
+ platform_certs/efi_parser.o
subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
diff --git a/security/integrity/platform_certs/efi_parser.c
b/security/integrity/platform_certs/efi_parser.c
new file mode 100644
index ..18f01f36fe6a
--- /dev/null
+++ b/security/integrity/platform_certs/efi_parser.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* EFI signature/key/certificate list parser
+ *
+ * Copyright (C) 2012, 2016 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowe...@redhat.com)
+ */
+
+#define pr_fmt(fmt) "EFI: "fmt
+#include
+#include
+#include
+#include
+
+/**
+ * parse_efi_signature_list - Parse an EFI signature list for certificates
+ * @source: The source of the key
+ * @data: The data blob to parse
+ * @size: The size of the data blob
+ * @get_handler_for_guid: Get the handler func for the sig type (or NULL)
+ *
+ * Parse an EFI signature list looking for elements of interest. A list is
+ * made up of a series of sublists, where all the elements in a sublist are of
+ * the same type, but sublists can be of different types.
+ *
+ * For each sublist encountered, the @get_handler_for_guid function is called
+ * with the type specifier GUID and returns either a pointer to a function to
+ * handle elements of that type or NULL if the type is not of interest.
+ *
+ * If the sublist is of interest, each element is passed to the handler
+ * function in turn.
+ *
+ * Error EBADMSG is returned if the list doesn't parse correctly and 0 is
+ * returned if the list was parsed correctly. No error can be returned from
+ * the @get_handler_for_guid function or the element handler function it
+ * returns.
+ */
+int __init parse_efi_signature_list(
+ const char *source,
+ const void *data, size_t size,
+ efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *))
+{
+ efi_element_handler_t handler;
+ unsigned int offs = 0;
+
+ pr_devel("-->%s(,%zu)\n", __func__, size);
+
+ while (size > 0) {
+ const efi_signature_data_t *elem;
+ efi_signature_list_t list;
+ size_t lsize, esize, hsize, elsize;
+
+ if (size < sizeof(list))
+ return -EBADMSG;
+
+ memcpy(, data, sizeof(list));
+
[PATCH v2 6/7] efi: Allow the "db" UEFI variable to be suppressed
From: Josh Boyer
If a user tells shim to not use the certs/hashes in the UEFI db variable
for verification purposes, shim will set a UEFI variable called
MokIgnoreDB. Have the uefi import code look for this and ignore the db
variable if it is found.
Signed-off-by: Josh Boyer
Signed-off-by: David Howells
Acked-by: Nayna Jain
Acked-by: Serge Hallyn
---
Changelog:
v0:
- No changes
v2:
- Fixed the checkpatch.pl warnings
security/integrity/platform_certs/load_uefi.c | 45 +--
1 file changed, 35 insertions(+), 10 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c
b/security/integrity/platform_certs/load_uefi.c
index acd9db90dde7..8bd2e9b421e1 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -16,6 +16,26 @@ static efi_guid_t efi_cert_x509_sha256_guid __initdata =
static efi_guid_t efi_cert_sha256_guid __initdata = EFI_CERT_SHA256_GUID;
/*
+ * Look to see if a UEFI variable called MokIgnoreDB exists and return true if
+ * it does.
+ *
+ * This UEFI variable is set by the shim if a user tells the shim to not use
+ * the certs/hashes in the UEFI db variable for verification purposes. If it
+ * is set, we should ignore the db variable also and the true return indicates
+ * this.
+ */
+static __init bool uefi_check_ignore_db(void)
+{
+ efi_status_t status;
+ unsigned int db = 0;
+ unsigned long size = sizeof(db);
+ efi_guid_t guid = EFI_SHIM_LOCK_GUID;
+
+ status = efi.get_variable(L"MokIgnoreDB", , NULL, , );
+ return status == EFI_SUCCESS;
+}
+
+/*
* Get a certificate list blob from the named EFI variable.
*/
static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
@@ -116,7 +136,9 @@ static __init efi_element_handler_t
get_handler_for_dbx(const efi_guid_t *
}
/*
- * Load the certs contained in the UEFI databases
+ * Load the certs contained in the UEFI databases into the secondary trusted
+ * keyring and the UEFI blacklisted X.509 cert SHA256 hashes into the blacklist
+ * keyring.
*/
static int __init load_uefi_certs(void)
{
@@ -132,15 +154,18 @@ static int __init load_uefi_certs(void)
/* Get db, MokListRT, and dbx. They might not exist, so it isn't
* an error if we can't get them.
*/
- db = get_cert_list(L"db", _var, );
- if (!db) {
- pr_err("Couldn't get UEFI db list\n");
- } else {
- rc = parse_efi_signature_list("UEFI:db",
- db, dbsize, get_handler_for_db);
- if (rc)
- pr_err("Couldn't parse db signatures: %d\n", rc);
- kfree(db);
+ if (!uefi_check_ignore_db()) {
+ db = get_cert_list(L"db", _var, );
+ if (!db) {
+ pr_err("MODSIGN: Couldn't get UEFI db list\n");
+ } else {
+ rc = parse_efi_signature_list("UEFI:db",
+ db, dbsize, get_handler_for_db);
+ if (rc)
+ pr_err("Couldn't parse db signatures: %d\n",
+ rc);
+ kfree(db);
+ }
}
mok = get_cert_list(L"MokListRT", _var, );
--
2.13.6
[PATCH v2 5/7] efi: Import certificates from UEFI Secure Boot
From: Josh Boyer
New Patch Description:
==
Secure Boot stores a list of allowed certificates in the 'db' variable.
This patch imports those certificates into the platform keyring. The shim
UEFI bootloader has a similar certificate list stored in the 'MokListRT'
variable. We import those as well.
Secure Boot also maintains a list of disallowed certificates in the 'dbx'
variable. We load those certificates into the system blacklist keyring
and forbid any kernel signed with those from loading.
Original Patch Description:
Secure Boot stores a list of allowed certificates in the 'db' variable.
This imports those certificates into the system trusted keyring. This
allows for a third party signing certificate to be used in conjunction
with signed modules. By importing the public certificate into the 'db'
variable, a user can allow a module signed with that certificate to
load. The shim UEFI bootloader has a similar certificate list stored
in the 'MokListRT' variable. We import those as well.
Secure Boot also maintains a list of disallowed certificates in the 'dbx'
variable. We load those certificates into the newly introduced system
blacklist keyring and forbid any module signed with those from loading and
forbid the use within the kernel of any key with a matching hash.
This facility is enabled by setting CONFIG_LOAD_UEFI_KEYS.
Signed-off-by: Josh Boyer
Signed-off-by: David Howells
Signed-off-by: Nayna Jain
Acked-by: Serge Hallyn
---
Changelog:
v0:
- This patch replaces the loading of certificates onto the secondary
keyring with platform keyring
- removed the CONFIG LOAD_UEFI_KEYS
- moved the file load_uefi.o from certs to security/integrity/platform_certs
v2:
- Fixed the checkpatch.pl warnings
security/integrity/Makefile | 5 +-
security/integrity/platform_certs/load_uefi.c | 171 ++
2 files changed, 175 insertions(+), 1 deletion(-)
create mode 100644 security/integrity/platform_certs/load_uefi.c
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 6ee9058866cd..86df9aba8c0f 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -10,7 +10,10 @@ integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) +=
platform_certs/platform_keyring.o \
- platform_certs/efi_parser.o
+ platform_certs/efi_parser.o \
+ platform_certs/load_uefi.o
+obj-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/load_uefi.o
+$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
diff --git a/security/integrity/platform_certs/load_uefi.c
b/security/integrity/platform_certs/load_uefi.c
new file mode 100644
index ..acd9db90dde7
--- /dev/null
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../integrity.h"
+
+static efi_guid_t efi_cert_x509_guid __initdata = EFI_CERT_X509_GUID;
+static efi_guid_t efi_cert_x509_sha256_guid __initdata =
+ EFI_CERT_X509_SHA256_GUID;
+static efi_guid_t efi_cert_sha256_guid __initdata = EFI_CERT_SHA256_GUID;
+
+/*
+ * Get a certificate list blob from the named EFI variable.
+ */
+static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
+ unsigned long *size)
+{
+ efi_status_t status;
+ unsigned long lsize = 4;
+ unsigned long tmpdb[4];
+ void *db;
+
+ status = efi.get_variable(name, guid, NULL, , );
+ if (status != EFI_BUFFER_TOO_SMALL) {
+ pr_err("Couldn't get size: 0x%lx\n", status);
+ return NULL;
+ }
+
+ db = kmalloc(lsize, GFP_KERNEL);
+ if (!db)
+ return NULL;
+
+ status = efi.get_variable(name, guid, NULL, , db);
+ if (status != EFI_SUCCESS) {
+ kfree(db);
+ pr_err("Error reading db var: 0x%lx\n", status);
+ return NULL;
+ }
+
+ *size = lsize;
+ return db;
+}
+
+/*
+ * Blacklist an X509 TBS hash.
+ */
+static __init void uefi_blacklist_x509_tbs(const char *source,
+ const void *data, size_t len)
+{
+ char *hash, *p;
+
+ hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
+ if (!hash)
+ return;
+ p = memcpy(hash, "tbs:", 4);
+ p += 4;
+ bin2hex(p, data, len);
+ p += len * 2;
+ *p = 0;
+
+ mark_hash_blacklisted(hash);
+ kfre
[PATCH v2 7/7] ima: Support platform keyring for kernel appraisal
On secure boot enabled systems, the bootloader verifies the kernel
image and possibly the initramfs signatures based on a set of keys. A
soft reboot(kexec) of the system, with the same kernel image and
initramfs, requires access to the original keys to verify the
signatures.
This patch allows IMA-appraisal access to those original keys, now
loaded on the platform keyring, needed for verifying the kernel image
and initramfs signatures.
Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Acked-by: Serge Hallyn
- replace 'rc' with 'xattr_len' when calling integrity_digsig_verify()
with INTEGRITY_KEYRING_IMA for readability
Suggested-by: Serge Hallyn
---
Changelog:
v2:
- replace 'rc' with 'xattr_len' when calling integrity_digsig_verify()
with INTEGRITY_KEYRING_IMA for readability
security/integrity/ima/ima_appraise.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index deec1804a00a..e8f520450895 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -289,12 +289,21 @@ int ima_appraise_measurement(enum ima_hooks func,
case EVM_IMA_XATTR_DIGSIG:
set_bit(IMA_DIGSIG, >atomic_flags);
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
-(const char *)xattr_value, rc,
+(const char *)xattr_value,
+xattr_len,
iint->ima_hash->digest,
iint->ima_hash->length);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
- } else if (rc) {
+ break;
+ }
+ if (rc && func == KEXEC_KERNEL_CHECK)
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_PLATFORM,
+(const char *)xattr_value,
+xattr_len,
+iint->ima_hash->digest,
+iint->ima_hash->length);
+ if (rc) {
cause = "invalid-signature";
status = INTEGRITY_FAIL;
} else {
--
2.13.6
[PATCH v2 2/7] integrity: Load certs to the platform keyring
The patch refactors integrity_load_x509(), making it a wrapper for a new
function named integrity_add_key(). This patch also defines a new
function named integrity_load_cert() for loading the platform keys.
Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Acked-by: Serge Hallyn
---
security/integrity/digsig.c| 71 ++
security/integrity/integrity.h | 20 ++
.../integrity/platform_certs/platform_keyring.c| 23 +++
3 files changed, 90 insertions(+), 24 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index b5b180ff1434..4ba167255225 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -82,8 +82,7 @@ static int __integrity_init_keyring(const unsigned int id,
key_perm_t perm,
keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
KGIDT_INIT(0), cred, perm,
- KEY_ALLOC_NOT_IN_QUOTA,
- restriction, NULL);
+ KEY_ALLOC_NOT_IN_QUOTA, restriction, NULL);
if (IS_ERR(keyring[id])) {
err = PTR_ERR(keyring[id]);
pr_info("Can't allocate %s keyring (%d)\n",
@@ -124,16 +123,38 @@ int __init integrity_init_keyring(const unsigned int id)
return err;
}
-int __init integrity_load_x509(const unsigned int id, const char *path)
+int __init integrity_add_key(const unsigned int id, const void *data,
+off_t size, key_perm_t perm)
{
key_ref_t key;
- void *data;
- loff_t size;
- int rc;
+ int rc = 0;
if (!keyring[id])
return -EINVAL;
+ key = key_create_or_update(make_key_ref(keyring[id], 1), "asymmetric",
+ NULL, data, size, perm,
+ KEY_ALLOC_NOT_IN_QUOTA);
+ if (IS_ERR(key)) {
+ rc = PTR_ERR(key);
+ pr_err("Problem loading X.509 certificate %d\n", rc);
+ } else {
+ pr_notice("Loaded X.509 cert '%s'\n",
+ key_ref_to_ptr(key)->description);
+ key_ref_put(key);
+ }
+
+ return rc;
+
+}
+
+int __init integrity_load_x509(const unsigned int id, const char *path)
+{
+ void *data;
+ loff_t size;
+ int rc;
+ key_perm_t perm;
+
rc = kernel_read_file_from_path(path, , , 0,
READING_X509_CERTIFICATE);
if (rc < 0) {
@@ -141,23 +162,25 @@ int __init integrity_load_x509(const unsigned int id,
const char *path)
return rc;
}
- key = key_create_or_update(make_key_ref(keyring[id], 1),
- "asymmetric",
- NULL,
- data,
- size,
- ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
- KEY_USR_VIEW | KEY_USR_READ),
- KEY_ALLOC_NOT_IN_QUOTA);
- if (IS_ERR(key)) {
- rc = PTR_ERR(key);
- pr_err("Problem loading X.509 certificate (%d): %s\n",
- rc, path);
- } else {
- pr_notice("Loaded X.509 cert '%s': %s\n",
- key_ref_to_ptr(key)->description, path);
- key_ref_put(key);
- }
+ perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ;
+
+ pr_info("Loading X.509 certificate: %s\n", path);
+ rc = integrity_add_key(id, (const void *)data, size, perm);
+
vfree(data);
- return 0;
+ return rc;
+}
+
+int __init integrity_load_cert(const unsigned int id, const char *source,
+ const void *data, size_t len, key_perm_t perm)
+{
+ int rc;
+
+ if (!data)
+ return -EINVAL;
+
+ pr_info("Loading X.509 certificate: %s\n", source);
+ rc = integrity_add_key(id, data, len, perm);
+
+ return rc;
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index c2332a44799e..3517d2852a07 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -154,6 +154,8 @@ int integrity_digsig_verify(const unsigned int id, const
char *sig, int siglen,
int __init integrity_init_keyring(const unsigned int id);
int __init integrity_load_x509(const unsigned int id, const char *path);
+int __init integrity_load_cert(const unsigned int id, const char *source,
+ const void *data, size_t len, key_perm_t perm);
#else
static inline int integrity_digsig_verify(const unsigned int id,
@@ -167,6 +169,14 @@ static inline int integrity_init_keyring(const unsi
[PATCH v2 0/7] add platform/firmware keys support for kernel verification by IMA
On secure boot enabled systems, a verified kernel may need to kexec additional kernels. For example, it may be used as a bootloader needing to kexec a target kernel or it may need to kexec a crashdump kernel. In such cases, it may want to verify the signature of the next kernel image. It is possible that the new kernel image is signed with third party keys which are stored as platform or firmware keys in the 'db' variable. The kernel, however, can not directly verify these platform keys, and an administrator may therefore not want to trust them for arbitrary usage. In order to differentiate platform keys from other keys and provide the necessary separation of trust the kernel needs an additional keyring to store platform/firmware keys. The secure boot key database is expected to store the keys as EFI Signature List(ESL). The patch set uses David Howells and Josh Boyer's patch to access and parse the ESL to extract the certificates and load them onto the platform keyring. The last patch in this patch set adds support for IMA-appraisal to verify the kexec'ed kernel image based on keys stored in the platform keyring. Changelog: v0: - The original patches loaded the certificates onto the secondary trusted keyring. This patch set defines a new keyring named ".platform" and adds the certificates to this new keyring - removed CONFIG EFI_SIGNATURE_LIST_PARSER and LOAD_UEFI_KEYS - moved files from certs/ to security/integrity/platform_certs/ v2: - fixed the checkpatch warnings and other formatting as suggested by Mimi Zohar - fixed coding style as suggested by Serge Hallyn in Patch "ima: Support platform keyring for kernel appraisal" Dave Howells (2): efi: Add EFI signature data types efi: Add an EFI signature blob parser Josh Boyer (2): efi: Import certificates from UEFI Secure Boot efi: Allow the "db" UEFI variable to be suppressed Nayna Jain (3): integrity: Define a trusted platform keyring integrity: Load certs to the platform keyring ima: Support platform keyring for kernel appraisal include/linux/efi.h| 34 security/integrity/Kconfig | 11 ++ security/integrity/Makefile| 5 + security/integrity/digsig.c| 115 security/integrity/ima/ima_appraise.c | 13 +- security/integrity/integrity.h | 23 ++- security/integrity/platform_certs/efi_parser.c | 108 security/integrity/platform_certs/load_uefi.c | 196 + .../integrity/platform_certs/platform_keyring.c| 62 +++ 9 files changed, 528 insertions(+), 39 deletions(-) create mode 100644 security/integrity/platform_certs/efi_parser.c create mode 100644 security/integrity/platform_certs/load_uefi.c create mode 100644 security/integrity/platform_certs/platform_keyring.c -- 2.13.6
[PATCH v2 1/7] integrity: Define a trusted platform keyring
On secure boot enabled systems, a verified kernel may need to kexec
additional kernels. For example, it may be used as a bootloader needing
to kexec a target kernel or it may need to kexec a crashdump kernel. In
such cases, it may want to verify the signature of the next kernel
image.
It is further possible that the kernel image is signed with third party
keys which are stored as platform or firmware keys in the 'db' variable.
The kernel, however, can not directly verify these platform keys, and an
administrator may therefore not want to trust them for arbitrary usage.
In order to differentiate platform keys from other keys and provide the
necessary separation of trust, the kernel needs an additional keyring to
store platform keys.
This patch creates the new keyring called ".platform" to isolate keys
provided by platform from keys by kernel. These keys are used to
facilitate signature verification during kexec. Since the scope of this
keyring is only the platform/firmware keys, it cannot be updated from
userspace.
This keyring can be enabled by setting CONFIG_INTEGRITY_PLATFORM_KEYRING.
Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
Acked-by: Serge Hallyn
---
security/integrity/Kconfig | 11 +
security/integrity/Makefile| 1 +
security/integrity/digsig.c| 48 +++---
security/integrity/integrity.h | 3 +-
.../integrity/platform_certs/platform_keyring.c| 39 ++
5 files changed, 87 insertions(+), 15 deletions(-)
create mode 100644 security/integrity/platform_certs/platform_keyring.c
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..4b4d2aeef539 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -51,6 +51,17 @@ config INTEGRITY_TRUSTED_KEYRING
.evm keyrings be signed by a key on the system trusted
keyring.
+config INTEGRITY_PLATFORM_KEYRING
+bool "Provide keyring for platform/firmware trusted keys"
+depends on INTEGRITY_ASYMMETRIC_KEYS
+depends on SYSTEM_BLACKLIST_KEYRING
+depends on EFI
+help
+ Provide a separate, distinct keyring for platform trusted keys, which
+ the kernel automatically populates during initialization from values
+ provided by the platform for verifying the kexec'ed kerned image
+ and, possibly, the initramfs signature.
+
config INTEGRITY_AUDIT
bool "Enables integrity auditing support "
depends on AUDIT
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 04d6e462b079..046ffc1bb42d 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -9,6 +9,7 @@ integrity-y := iint.o
integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
+integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) +=
platform_certs/platform_keyring.o
subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 5eacba858e4b..b5b180ff1434 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -35,6 +35,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX]
= {
".ima",
#endif
"_module",
+ ".platform",
};
#ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
@@ -73,12 +74,40 @@ int integrity_digsig_verify(const unsigned int id, const
char *sig, int siglen,
return -EOPNOTSUPP;
}
-int __init integrity_init_keyring(const unsigned int id)
+static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
+ struct key_restriction *restriction)
{
const struct cred *cred = current_cred();
+ int err = 0;
+
+ keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
+ KGIDT_INIT(0), cred, perm,
+ KEY_ALLOC_NOT_IN_QUOTA,
+ restriction, NULL);
+ if (IS_ERR(keyring[id])) {
+ err = PTR_ERR(keyring[id]);
+ pr_info("Can't allocate %s keyring (%d)\n",
+ keyring_name[id], err);
+ keyring[id] = NULL;
+ }
+
+ return err;
+}
+
+int __init integrity_init_keyring(const unsigned int id)
+{
struct key_restriction *restriction;
+ key_perm_t perm;
int err = 0;
+ perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW
+ | KEY_USR_READ | KEY_USR_SEARCH;
+
+ if (id == INTEGRITY_KEYRING_PLATFORM) {
+ restriction = NULL;
+ goto out;
+ }
+
if (!IS_ENABLED(CONFIG_INTEGR
Re: [PATCH v6 4/7] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
On 12/05/2018 05:10 AM, Jarkko Sakkinen wrote:
On Tue, Dec 04, 2018 at 09:21:35AM +0100, Roberto Sassu wrote:
Currently the TPM driver allows other kernel subsystems to read only the
SHA1 PCR bank. This patch modifies the parameters of tpm_pcr_read() and
tpm2_pcr_read() to pass a tpm_digest structure, which contains the desired
hash algorithm. Also, since commit 125a22105410 ("tpm: React correctly to
RC_TESTING from TPM 2.0 self tests") removed the call to tpm2_pcr_read(),
the new parameter is expected to be always not NULL.
Due to the API change, IMA functions have been modified.
Signed-off-by: Roberto Sassu
Acked-by: Mimi Zohar
Reviewed-by: Jarkko Sakkinen
Mimi, Nayna, can you help with testing this (because of the IMA change)?
Sure, I will try to do by end of my day tomorrow,
Thanks & Regards,
- Nayna
/Jarkko
Re: [PATCH v6 4/7] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
On 12/05/2018 05:10 AM, Jarkko Sakkinen wrote:
On Tue, Dec 04, 2018 at 09:21:35AM +0100, Roberto Sassu wrote:
Currently the TPM driver allows other kernel subsystems to read only the
SHA1 PCR bank. This patch modifies the parameters of tpm_pcr_read() and
tpm2_pcr_read() to pass a tpm_digest structure, which contains the desired
hash algorithm. Also, since commit 125a22105410 ("tpm: React correctly to
RC_TESTING from TPM 2.0 self tests") removed the call to tpm2_pcr_read(),
the new parameter is expected to be always not NULL.
Due to the API change, IMA functions have been modified.
Signed-off-by: Roberto Sassu
Acked-by: Mimi Zohar
Reviewed-by: Jarkko Sakkinen
Mimi, Nayna, can you help with testing this (because of the IMA change)?
Sure, I will try to do by end of my day tomorrow,
Thanks & Regards,
- Nayna
/Jarkko
Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
On 11/07/2018 03:11 PM, Roberto Sassu wrote:
On 11/7/2018 7:14 AM, Nayna Jain wrote:
On 11/06/2018 08:31 PM, Roberto Sassu wrote:
This patch removes the hard-coded limit of the active_banks array size.
The hard-coded limit in static array active_banks[] represents the
maximum possible banks.
A TPM might have three banks, but only one bank may be active.
To confirm my understanding, is the idea for this patch is to
dynamically identify the number of possible banks or the number of
active banks ?
The idea is to dynamically identify the number of active banks.
In the TPM Commands specification (section 30.2.1), I found:
TPM_CAP_PCRS – Returns the current allocation of PCR in a
TPML_PCR_SELECTION.
You mentioned:
#TPM_RC_SIZE response code when count is greater
than the possible number of banks
but TPML_PCR_SELECTION is provided by the TPM.
Based on a discussion with Ken, the count in the TPML_PCR_SELECTION
returns the number of possible algorithms supported. In the example
below, two possible algorithms - SHA1 and SHA256 - are returned.
# /usr/local/bin/tssgetcapability -cap 5
2 PCR selections
hash TPM_ALG_SHA1
TPMS_PCR_SELECTION length 3
ff ff ff
hash TPM_ALG_SHA256
TPMS_PCR_SELECTION length 3
00 00 00
The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for
the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while
the SHA256 bank is not enabled.
The current code works, but it unnecessarily extends some banks. Instead
of basing the number of active banks on the number of algorithms
returned, it should be based on the pcr_select field.
- Mimi & Nayna
Roberto
It stores in the tpm_chip structure the number of active PCR banks,
determined in tpm2_get_pcr_allocation(), and replaces the static array
with a pointer to a dynamically allocated array.
As a consequence of the introduction of nr_active_banks,
tpm_pcr_extend()
does not check anymore if the algorithm stored in tpm_chip is equal to
zero. The active_banks array always contains valid algorithms.
Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
PCR banks")
Signed-off-by: Roberto Sassu
---
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm-interface.c | 19 ---
drivers/char/tpm/tpm.h | 3 ++-
drivers/char/tpm/tpm2-cmd.c | 17 -
4 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..2a9e8b744436 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
kfree(chip->log.bios_event_log);
kfree(chip->work_space.context_buf);
kfree(chip->work_space.session_buf);
+ kfree(chip->active_banks);
kfree(chip);
}
diff --git a/drivers/char/tpm/tpm-interface.c
b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..ba7ca6b3e664 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip
*chip, int pcr_idx, const u8 *hash,
int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8
*hash)
{
int rc;
- struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
- u32 count = 0;
+ struct tpm2_digest *digest_list;
int i;
chip = tpm_find_get_ops(chip);
@@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip,
int pcr_idx, const u8 *hash)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
- memset(digest_list, 0, sizeof(digest_list));
+ digest_list = kmalloc_array(chip->nr_active_banks,
+ sizeof(*digest_list), GFP_KERNEL);
+ if (!digest_list)
+ return -ENOMEM;
- for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
- chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+ memset(digest_list, 0,
+ chip->nr_active_banks * sizeof(*digest_list));
+
+ for (i = 0; i < chip->nr_active_banks; i++) {
digest_list[i].alg_id = chip->active_banks[i];
memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
- count++;
}
- rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+ rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
+ digest_list);
+ kfree(digest_list);
tpm_put_ops(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..98368c3a6ff7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -248,7 +248,8 @@ struct tpm_chip {
const struct attribute_group *groups[3];
unsigned int groups_cnt;
- u16 active_banks[7];
+ u32 nr_active_banks;
+ u16 *a
Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
On 11/07/2018 03:11 PM, Roberto Sassu wrote:
On 11/7/2018 7:14 AM, Nayna Jain wrote:
On 11/06/2018 08:31 PM, Roberto Sassu wrote:
This patch removes the hard-coded limit of the active_banks array size.
The hard-coded limit in static array active_banks[] represents the
maximum possible banks.
A TPM might have three banks, but only one bank may be active.
To confirm my understanding, is the idea for this patch is to
dynamically identify the number of possible banks or the number of
active banks ?
The idea is to dynamically identify the number of active banks.
In the TPM Commands specification (section 30.2.1), I found:
TPM_CAP_PCRS – Returns the current allocation of PCR in a
TPML_PCR_SELECTION.
You mentioned:
#TPM_RC_SIZE response code when count is greater
than the possible number of banks
but TPML_PCR_SELECTION is provided by the TPM.
Based on a discussion with Ken, the count in the TPML_PCR_SELECTION
returns the number of possible algorithms supported. In the example
below, two possible algorithms - SHA1 and SHA256 - are returned.
# /usr/local/bin/tssgetcapability -cap 5
2 PCR selections
hash TPM_ALG_SHA1
TPMS_PCR_SELECTION length 3
ff ff ff
hash TPM_ALG_SHA256
TPMS_PCR_SELECTION length 3
00 00 00
The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for
the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while
the SHA256 bank is not enabled.
The current code works, but it unnecessarily extends some banks. Instead
of basing the number of active banks on the number of algorithms
returned, it should be based on the pcr_select field.
- Mimi & Nayna
Roberto
It stores in the tpm_chip structure the number of active PCR banks,
determined in tpm2_get_pcr_allocation(), and replaces the static array
with a pointer to a dynamically allocated array.
As a consequence of the introduction of nr_active_banks,
tpm_pcr_extend()
does not check anymore if the algorithm stored in tpm_chip is equal to
zero. The active_banks array always contains valid algorithms.
Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
PCR banks")
Signed-off-by: Roberto Sassu
---
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm-interface.c | 19 ---
drivers/char/tpm/tpm.h | 3 ++-
drivers/char/tpm/tpm2-cmd.c | 17 -
4 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..2a9e8b744436 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
kfree(chip->log.bios_event_log);
kfree(chip->work_space.context_buf);
kfree(chip->work_space.session_buf);
+ kfree(chip->active_banks);
kfree(chip);
}
diff --git a/drivers/char/tpm/tpm-interface.c
b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..ba7ca6b3e664 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip
*chip, int pcr_idx, const u8 *hash,
int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8
*hash)
{
int rc;
- struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
- u32 count = 0;
+ struct tpm2_digest *digest_list;
int i;
chip = tpm_find_get_ops(chip);
@@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip,
int pcr_idx, const u8 *hash)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
- memset(digest_list, 0, sizeof(digest_list));
+ digest_list = kmalloc_array(chip->nr_active_banks,
+ sizeof(*digest_list), GFP_KERNEL);
+ if (!digest_list)
+ return -ENOMEM;
- for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
- chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+ memset(digest_list, 0,
+ chip->nr_active_banks * sizeof(*digest_list));
+
+ for (i = 0; i < chip->nr_active_banks; i++) {
digest_list[i].alg_id = chip->active_banks[i];
memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
- count++;
}
- rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+ rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
+ digest_list);
+ kfree(digest_list);
tpm_put_ops(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..98368c3a6ff7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -248,7 +248,8 @@ struct tpm_chip {
const struct attribute_group *groups[3];
unsigned int groups_cnt;
- u16 active_banks[7];
+ u32 nr_active_banks;
+ u16 *a
Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
_banks = be32_to_cpup(
(__be32 *)[TPM_HEADER_SIZE + 5]);
As per my understanding, the count in the TPML_PCR_SELECTION represent
the number of possible banks and not the number of active banks.
TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation
of #TPM_RC_SIZE.
Thanks & Regards,
- Nayna
- if (count > ARRAY_SIZE(chip->active_banks)) {
- rc = -ENODEV;
+ chip->active_banks = kmalloc_array(chip->nr_active_banks,
+ sizeof(*chip->active_banks),
+ GFP_KERNEL);
+ if (!chip->active_banks) {
+ rc = -ENOMEM;
goto out;
}
@@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip
*chip)
rsp_len = be32_to_cpup((__be32 *)[2]);
end = [rsp_len];
- for (i = 0; i < count; i++) {
+ for (i = 0; i < chip->nr_active_banks; i++) {
pcr_select_offset = marker +
offsetof(struct tpm2_pcr_selection, size_of_select);
if (pcr_select_offset >= end) {
@@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip
*chip)
}
out:
- if (i < ARRAY_SIZE(chip->active_banks))
- chip->active_banks[i] = TPM2_ALG_ERROR;
-
tpm_buf_destroy();
return rc;
Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
_banks = be32_to_cpup(
(__be32 *)[TPM_HEADER_SIZE + 5]);
As per my understanding, the count in the TPML_PCR_SELECTION represent
the number of possible banks and not the number of active banks.
TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation
of #TPM_RC_SIZE.
Thanks & Regards,
- Nayna
- if (count > ARRAY_SIZE(chip->active_banks)) {
- rc = -ENODEV;
+ chip->active_banks = kmalloc_array(chip->nr_active_banks,
+ sizeof(*chip->active_banks),
+ GFP_KERNEL);
+ if (!chip->active_banks) {
+ rc = -ENOMEM;
goto out;
}
@@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip
*chip)
rsp_len = be32_to_cpup((__be32 *)[2]);
end = [rsp_len];
- for (i = 0; i < count; i++) {
+ for (i = 0; i < chip->nr_active_banks; i++) {
pcr_select_offset = marker +
offsetof(struct tpm2_pcr_selection, size_of_select);
if (pcr_select_offset >= end) {
@@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip
*chip)
}
out:
- if (i < ARRAY_SIZE(chip->active_banks))
- chip->active_banks[i] = TPM2_ALG_ERROR;
-
tpm_buf_destroy();
return rc;
Re: [PATCH] tpm: tpm_i2c_nuvoton: use correct command duration for TPM 2.x
On 10/17/2018 10:02 PM, Tomas Winkler wrote:
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
b/drivers/char/tpm/tpm_i2c_nuvoton.c
index caa86b19c76d..f74f451baf6a 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -369,6 +369,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf,
size_t len)
struct device *dev = chip->dev.parent;
struct i2c_client *client = to_i2c_client(dev);
u32 ordinal;
+ unsigned long duration;
size_t count = 0;
int burst_count, bytes2write, retries, rc = -EIO;
@@ -455,10 +456,12 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8
*buf, size_t len)
return rc;
}
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- rc = i2c_nuvoton_wait_for_data_avail(chip,
-tpm_calc_ordinal_duration(chip,
- ordinal),
->read_queue);
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ duration = tpm2_calc_ordinal_duration(chip, ordinal);
+ else
+ duration = tpm_calc_ordinal_duration(chip, ordinal);
+
+ rc = i2c_nuvoton_wait_for_data_avail(chip, duration, >read_queue);
if (rc) {
dev_err(dev, "%s() timeout command duration\n", __func__);
i2c_nuvoton_ready(chip);
I only have Nuvoton TPM 2.0, tested for that.
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain (For TPM 2.0)
Thanks & Regards,
- Nayna
Re: [PATCH] tpm: tpm_i2c_nuvoton: use correct command duration for TPM 2.x
On 10/17/2018 10:02 PM, Tomas Winkler wrote:
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
b/drivers/char/tpm/tpm_i2c_nuvoton.c
index caa86b19c76d..f74f451baf6a 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -369,6 +369,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf,
size_t len)
struct device *dev = chip->dev.parent;
struct i2c_client *client = to_i2c_client(dev);
u32 ordinal;
+ unsigned long duration;
size_t count = 0;
int burst_count, bytes2write, retries, rc = -EIO;
@@ -455,10 +456,12 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8
*buf, size_t len)
return rc;
}
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- rc = i2c_nuvoton_wait_for_data_avail(chip,
-tpm_calc_ordinal_duration(chip,
- ordinal),
->read_queue);
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ duration = tpm2_calc_ordinal_duration(chip, ordinal);
+ else
+ duration = tpm_calc_ordinal_duration(chip, ordinal);
+
+ rc = i2c_nuvoton_wait_for_data_avail(chip, duration, >read_queue);
if (rc) {
dev_err(dev, "%s() timeout command duration\n", __func__);
i2c_nuvoton_ready(chip);
I only have Nuvoton TPM 2.0, tested for that.
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain (For TPM 2.0)
Thanks & Regards,
- Nayna
Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
On 10/17/2018 05:54 PM, Winkler, Tomas wrote: ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); - rc = i2c_nuvoton_wait_for_data_avail(chip, -tpm_calc_ordinal_duration(chip, - ordinal), ->read_queue); + duration = tpm1_calc_ordinal_duration(chip, ordinal); This version of the patch didn't address my previous comment - "The original code in the nuvoton driver does not differentiate between TPM 1.2 and TPM 2.0 as it does in tpm_tis_core.c. Before making any changes, I would first fix it, so that it could easily be backported. Only then do the refactoring." This patch doesn't change the original behavior, just change the name of the function, so there is no regression. I would suggest there is another bug in those drivers/devices that is orthogonal to this refactoring and should not block this from merging. The problem is that you are inadvertently fixing a bug without realizing it - [Patch 04/20]. Bug fixes should be addressed independently of this change, so that they can be backported properly. According to what you say it can call just tpm_calc_oridnal_duration() instead of tpm1_calc_ordinal_duration(chip, ordinal), but I prefer that someone that has those devices will do that change on top of this series as I cannot test it. The problem is: 1. This patch calls tpm1_calc_ordinal_duration for both the TPM 1.2 and TPM 2.0. 2. In the next patch, it adds a new function tpm_calc_ordinal_duration as a wrapper for both the TPM 1.2 and TPM 2.0. After this change when it calls tpm_calc_ordinal_duration(), it now calls different functions for TPM 1.2 and TPM 2.0. This is a change in behavior. Thanks & Regards, - Nayna Thanks Tomas
Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
On 10/17/2018 05:54 PM, Winkler, Tomas wrote: ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); - rc = i2c_nuvoton_wait_for_data_avail(chip, -tpm_calc_ordinal_duration(chip, - ordinal), ->read_queue); + duration = tpm1_calc_ordinal_duration(chip, ordinal); This version of the patch didn't address my previous comment - "The original code in the nuvoton driver does not differentiate between TPM 1.2 and TPM 2.0 as it does in tpm_tis_core.c. Before making any changes, I would first fix it, so that it could easily be backported. Only then do the refactoring." This patch doesn't change the original behavior, just change the name of the function, so there is no regression. I would suggest there is another bug in those drivers/devices that is orthogonal to this refactoring and should not block this from merging. The problem is that you are inadvertently fixing a bug without realizing it - [Patch 04/20]. Bug fixes should be addressed independently of this change, so that they can be backported properly. According to what you say it can call just tpm_calc_oridnal_duration() instead of tpm1_calc_ordinal_duration(chip, ordinal), but I prefer that someone that has those devices will do that change on top of this series as I cannot test it. The problem is: 1. This patch calls tpm1_calc_ordinal_duration for both the TPM 1.2 and TPM 2.0. 2. In the next patch, it adds a new function tpm_calc_ordinal_duration as a wrapper for both the TPM 1.2 and TPM 2.0. After this change when it calls tpm_calc_ordinal_duration(), it now calls different functions for TPM 1.2 and TPM 2.0. This is a change in behavior. Thanks & Regards, - Nayna Thanks Tomas
Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
On 10/17/2018 12:15 PM, Tomas Winkler wrote:
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
b/drivers/char/tpm/tpm_i2c_nuvoton.c
index caa86b19c76d..5d20e98b844f 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -370,6 +370,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf,
size_t len)
struct i2c_client *client = to_i2c_client(dev);
u32 ordinal;
size_t count = 0;
+ unsigned long duration;
int burst_count, bytes2write, retries, rc = -EIO;
for (retries = 0; retries < TPM_RETRY; retries++) {
@@ -455,12 +456,11 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8
*buf, size_t len)
return rc;
}
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- rc = i2c_nuvoton_wait_for_data_avail(chip,
-tpm_calc_ordinal_duration(chip,
- ordinal),
->read_queue);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
This version of the patch didn't address my previous comment -
"The original code in the nuvoton driver does not differentiate between
TPM 1.2 and TPM 2.0 as it does in tpm_tis_core.c.
Before making any changes, I would first fix it, so that it could easily
be backported. Only then do the refactoring."
Thanks & Regards,
- Nayna
+ rc = i2c_nuvoton_wait_for_data_avail(chip, duration, >read_queue);
if (rc) {
- dev_err(dev, "%s() timeout command duration\n", __func__);
+ dev_err(dev, "%s() timeout command duration %ld\n",
+ __func__, duration);
i2c_nuvoton_ready(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2345d9fd7b5..14c332104de4 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -476,7 +476,7 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const
u8 *buf, size_t len)
if (chip->flags & TPM_CHIP_FLAG_TPM2)
dur = tpm2_calc_ordinal_duration(chip, ordinal);
else
- dur = tpm_calc_ordinal_duration(chip, ordinal);
+ dur = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat
(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index b150f87f38f5..77097229bf49 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -164,7 +164,7 @@ static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t
count)
notify_remote_via_evtchn(priv->evtchn);
ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
- duration = tpm_calc_ordinal_duration(chip, ordinal);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
>read_queue, true) < 0) {
Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
On 10/17/2018 12:15 PM, Tomas Winkler wrote:
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
b/drivers/char/tpm/tpm_i2c_nuvoton.c
index caa86b19c76d..5d20e98b844f 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -370,6 +370,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf,
size_t len)
struct i2c_client *client = to_i2c_client(dev);
u32 ordinal;
size_t count = 0;
+ unsigned long duration;
int burst_count, bytes2write, retries, rc = -EIO;
for (retries = 0; retries < TPM_RETRY; retries++) {
@@ -455,12 +456,11 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8
*buf, size_t len)
return rc;
}
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- rc = i2c_nuvoton_wait_for_data_avail(chip,
-tpm_calc_ordinal_duration(chip,
- ordinal),
->read_queue);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
This version of the patch didn't address my previous comment -
"The original code in the nuvoton driver does not differentiate between
TPM 1.2 and TPM 2.0 as it does in tpm_tis_core.c.
Before making any changes, I would first fix it, so that it could easily
be backported. Only then do the refactoring."
Thanks & Regards,
- Nayna
+ rc = i2c_nuvoton_wait_for_data_avail(chip, duration, >read_queue);
if (rc) {
- dev_err(dev, "%s() timeout command duration\n", __func__);
+ dev_err(dev, "%s() timeout command duration %ld\n",
+ __func__, duration);
i2c_nuvoton_ready(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2345d9fd7b5..14c332104de4 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -476,7 +476,7 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const
u8 *buf, size_t len)
if (chip->flags & TPM_CHIP_FLAG_TPM2)
dur = tpm2_calc_ordinal_duration(chip, ordinal);
else
- dur = tpm_calc_ordinal_duration(chip, ordinal);
+ dur = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat
(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index b150f87f38f5..77097229bf49 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -164,7 +164,7 @@ static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t
count)
notify_remote_via_evtchn(priv->evtchn);
ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
- duration = tpm_calc_ordinal_duration(chip, ordinal);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
>read_queue, true) < 0) {
Re: [PATCH v5 20/21] tpm1: reimplement tpm1_continue_selftest() using tpm_buf
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
Reimplement tpm1_continue_selftest() using tpm_buf structure.
This is the last command using the old tpm_cmd_t structure
and now the structure can be removed.
Cc: Nayna Jain
Signed-off-by: Tomas Winkler
Reviewed-by: Jarkko Sakkinen
Tested-by: Jarkko Sakkinen
---
V3: New in the series.
V4: Resend.
V5: Fix -> buf.data in tpm1_continue_selftest()
drivers/char/tpm/tpm.h | 9 -
drivers/char/tpm/tpm1-cmd.c | 21 ++---
2 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2f06740f993d..7ada00f067f1 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -377,15 +377,6 @@ enum tpm_sub_capabilities {
TPM_CAP_PROP_TIS_DURATION = 0x120,
};
-typedef union {
- struct tpm_input_header in;
- struct tpm_output_header out;
-} tpm_cmd_header;
-
-struct tpm_cmd_t {
- tpm_cmd_header header;
-} __packed;
-
/* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
* bytes, but 128 is still a relatively large number of random bytes and
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index d418a27a75e0..6b04648f8184 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -602,15 +602,8 @@ int tpm1_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
}
#define TPM_ORD_CONTINUE_SELFTEST 83
-#define CONTINUE_SELFTEST_RESULT_SIZE 10
-static const struct tpm_input_header continue_selftest_header = {
- .tag = cpu_to_be16(TPM_TAG_RQU_COMMAND),
- .length = cpu_to_be32(10),
- .ordinal = cpu_to_be32(TPM_ORD_CONTINUE_SELFTEST),
-};
-
/**
- * tpm_continue_selftest -- run TPM's selftest
+ * tpm_continue_selftest() - run TPM's selftest
* @chip: TPM chip to use
*
* Returns 0 on success, < 0 in case of fatal error or a value > 0
representing
@@ -618,12 +611,18 @@ static const struct tpm_input_header
continue_selftest_header = {
*/
static int tpm1_continue_selftest(struct tpm_chip *chip)
{
+ struct tpm_buf buf;
int rc;
- struct tpm_cmd_t cmd;
- cmd.header.in = continue_selftest_header;
- rc = tpm_transmit_cmd(chip, NULL, , CONTINUE_SELFTEST_RESULT_SIZE,
+ rc = tpm_buf_init(, TPM_TAG_RQU_COMMAND, TPM_ORD_CONTINUE_SELFTEST);
+ if (rc)
+ return rc;
+
+ rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
0, 0, "continue selftest");
+
+ tpm_buf_destroy();
+
return rc;
}
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain
Thanks & Regards,
- Nayna
Re: [PATCH v5 20/21] tpm1: reimplement tpm1_continue_selftest() using tpm_buf
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
Reimplement tpm1_continue_selftest() using tpm_buf structure.
This is the last command using the old tpm_cmd_t structure
and now the structure can be removed.
Cc: Nayna Jain
Signed-off-by: Tomas Winkler
Reviewed-by: Jarkko Sakkinen
Tested-by: Jarkko Sakkinen
---
V3: New in the series.
V4: Resend.
V5: Fix -> buf.data in tpm1_continue_selftest()
drivers/char/tpm/tpm.h | 9 -
drivers/char/tpm/tpm1-cmd.c | 21 ++---
2 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2f06740f993d..7ada00f067f1 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -377,15 +377,6 @@ enum tpm_sub_capabilities {
TPM_CAP_PROP_TIS_DURATION = 0x120,
};
-typedef union {
- struct tpm_input_header in;
- struct tpm_output_header out;
-} tpm_cmd_header;
-
-struct tpm_cmd_t {
- tpm_cmd_header header;
-} __packed;
-
/* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
* bytes, but 128 is still a relatively large number of random bytes and
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index d418a27a75e0..6b04648f8184 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -602,15 +602,8 @@ int tpm1_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
}
#define TPM_ORD_CONTINUE_SELFTEST 83
-#define CONTINUE_SELFTEST_RESULT_SIZE 10
-static const struct tpm_input_header continue_selftest_header = {
- .tag = cpu_to_be16(TPM_TAG_RQU_COMMAND),
- .length = cpu_to_be32(10),
- .ordinal = cpu_to_be32(TPM_ORD_CONTINUE_SELFTEST),
-};
-
/**
- * tpm_continue_selftest -- run TPM's selftest
+ * tpm_continue_selftest() - run TPM's selftest
* @chip: TPM chip to use
*
* Returns 0 on success, < 0 in case of fatal error or a value > 0
representing
@@ -618,12 +611,18 @@ static const struct tpm_input_header
continue_selftest_header = {
*/
static int tpm1_continue_selftest(struct tpm_chip *chip)
{
+ struct tpm_buf buf;
int rc;
- struct tpm_cmd_t cmd;
- cmd.header.in = continue_selftest_header;
- rc = tpm_transmit_cmd(chip, NULL, , CONTINUE_SELFTEST_RESULT_SIZE,
+ rc = tpm_buf_init(, TPM_TAG_RQU_COMMAND, TPM_ORD_CONTINUE_SELFTEST);
+ if (rc)
+ return rc;
+
+ rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
0, 0, "continue selftest");
+
+ tpm_buf_destroy();
+
return rc;
}
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain
Thanks & Regards,
- Nayna
Re: [PATCH v5 06/21] tpm: move tpm1_pcr_extend to tpm1-cmd.c
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
Move tpm1_pcr_extend to tpm1-cmd.c and remove
unused pcrextend_header structure and
EXTEND_PCR_RESULT_SIZE and EXTEND_PCR_RESULT_BODY_SIZE
defines.
Fixes warning:
drivers/char/tpm/tpm-interface.c:609:38: warning: ‘pcrextend_header’ defined
but not used [-Wunused-const-variable=]
static const struct tpm_input_header pcrextend_header = {
^~~~
Signed-off-by: Tomas Winkler
Reviewed-by: Jarkko Sakkinen
---
V2-V3: Rebase
V4: Remove defines.
V5: Resend.
drivers/char/tpm/tpm-interface.c | 28
drivers/char/tpm/tpm.h | 2 ++
drivers/char/tpm/tpm1-cmd.c | 21 +
3 files changed, 23 insertions(+), 28 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1fa0300f3829..ac73e6ac3d83 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -614,34 +614,6 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
}
EXPORT_SYMBOL_GPL(tpm_pcr_read);
-#define TPM_ORD_PCR_EXTEND 20
-#define EXTEND_PCR_RESULT_SIZE 34
-#define EXTEND_PCR_RESULT_BODY_SIZE 20
-static const struct tpm_input_header pcrextend_header = {
- .tag = cpu_to_be16(TPM_TAG_RQU_COMMAND),
- .length = cpu_to_be32(34),
- .ordinal = cpu_to_be32(TPM_ORD_PCR_EXTEND)
-};
-
-static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
- char *log_msg)
-{
- struct tpm_buf buf;
- int rc;
-
- rc = tpm_buf_init(, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(, pcr_idx);
- tpm_buf_append(, hash, TPM_DIGEST_SIZE);
-
- rc = tpm_transmit_cmd(chip, NULL, buf.data, EXTEND_PCR_RESULT_SIZE,
- EXTEND_PCR_RESULT_BODY_SIZE, 0, log_msg);
- tpm_buf_destroy();
- return rc;
-}
-
/**
* tpm_pcr_extend - extend a PCR value in SHA1 bank.
* @chip: a tpm_chip instance, %NULL for the default chip
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a97d72fcda5b..3fb268f43955 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -549,6 +549,8 @@ int tpm_do_selftest(struct tpm_chip *chip);
int tpm1_auto_startup(struct tpm_chip *chip);
int tpm1_get_timeouts(struct tpm_chip *chip);
unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
+int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
+ const char *log_msg);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 978946748ea3..ec242397e6dc 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -413,3 +413,24 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
return 0;
}
+
+#define TPM_ORD_PCR_EXTEND 20
+int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
+ const char *log_msg)
+{
+ struct tpm_buf buf;
+ int rc;
+
+ rc = tpm_buf_init(, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(, pcr_idx);
+ tpm_buf_append(, hash, TPM_DIGEST_SIZE);
+
+ rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+ TPM_DIGEST_SIZE, 0, log_msg);
+
+ tpm_buf_destroy();
+ return rc;
+}
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain
Sorry for bit delay in testing.
Thanks & Regards,
- Nayna
Re: [PATCH v5 06/21] tpm: move tpm1_pcr_extend to tpm1-cmd.c
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
Move tpm1_pcr_extend to tpm1-cmd.c and remove
unused pcrextend_header structure and
EXTEND_PCR_RESULT_SIZE and EXTEND_PCR_RESULT_BODY_SIZE
defines.
Fixes warning:
drivers/char/tpm/tpm-interface.c:609:38: warning: ‘pcrextend_header’ defined
but not used [-Wunused-const-variable=]
static const struct tpm_input_header pcrextend_header = {
^~~~
Signed-off-by: Tomas Winkler
Reviewed-by: Jarkko Sakkinen
---
V2-V3: Rebase
V4: Remove defines.
V5: Resend.
drivers/char/tpm/tpm-interface.c | 28
drivers/char/tpm/tpm.h | 2 ++
drivers/char/tpm/tpm1-cmd.c | 21 +
3 files changed, 23 insertions(+), 28 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1fa0300f3829..ac73e6ac3d83 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -614,34 +614,6 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8
*res_buf)
}
EXPORT_SYMBOL_GPL(tpm_pcr_read);
-#define TPM_ORD_PCR_EXTEND 20
-#define EXTEND_PCR_RESULT_SIZE 34
-#define EXTEND_PCR_RESULT_BODY_SIZE 20
-static const struct tpm_input_header pcrextend_header = {
- .tag = cpu_to_be16(TPM_TAG_RQU_COMMAND),
- .length = cpu_to_be32(34),
- .ordinal = cpu_to_be32(TPM_ORD_PCR_EXTEND)
-};
-
-static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
- char *log_msg)
-{
- struct tpm_buf buf;
- int rc;
-
- rc = tpm_buf_init(, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(, pcr_idx);
- tpm_buf_append(, hash, TPM_DIGEST_SIZE);
-
- rc = tpm_transmit_cmd(chip, NULL, buf.data, EXTEND_PCR_RESULT_SIZE,
- EXTEND_PCR_RESULT_BODY_SIZE, 0, log_msg);
- tpm_buf_destroy();
- return rc;
-}
-
/**
* tpm_pcr_extend - extend a PCR value in SHA1 bank.
* @chip: a tpm_chip instance, %NULL for the default chip
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a97d72fcda5b..3fb268f43955 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -549,6 +549,8 @@ int tpm_do_selftest(struct tpm_chip *chip);
int tpm1_auto_startup(struct tpm_chip *chip);
int tpm1_get_timeouts(struct tpm_chip *chip);
unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
+int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
+ const char *log_msg);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 978946748ea3..ec242397e6dc 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -413,3 +413,24 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
return 0;
}
+
+#define TPM_ORD_PCR_EXTEND 20
+int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
+ const char *log_msg)
+{
+ struct tpm_buf buf;
+ int rc;
+
+ rc = tpm_buf_init(, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(, pcr_idx);
+ tpm_buf_append(, hash, TPM_DIGEST_SIZE);
+
+ rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+ TPM_DIGEST_SIZE, 0, log_msg);
+
+ tpm_buf_destroy();
+ return rc;
+}
Reviewed-by: Nayna Jain
Tested-by: Nayna Jain
Sorry for bit delay in testing.
Thanks & Regards,
- Nayna
Re: [PATCH v5 05/21] tpm: factor out tpm_get_timeouts()
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 73511cd89bef..a97d72fcda5b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -544,8 +544,10 @@ int tpm_startup(struct tpm_chip *chip);
ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm_get_timeouts(struct tpm_chip *);
-int tpm1_auto_startup(struct tpm_chip *chip);
int tpm_do_selftest(struct tpm_chip *chip);
+
+int tpm1_auto_startup(struct tpm_chip *chip);
What is different in this tpm1_auto_startup(...) and the original one ?
Is this needed ?
Thanks & Regards,
- Nayna
+int tpm1_get_timeouts(struct tpm_chip *chip);
unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
@@ -585,6 +587,7 @@ static inline u32 tpm2_rc_value(u32 rc)
return (rc & BIT(7)) ? rc & 0xff : rc;
}
+int tpm2_get_timeouts(struct tpm_chip *chip);
int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
struct tpm2_digest *digests);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index dfbe9c60cbcf..978946748ea3 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -307,3 +307,109 @@ unsigned long tpm1_calc_ordinal_duration(struct tpm_chip
*chip, u32 ordinal)
else
return duration;
}
+
+int tpm1_get_timeouts(struct tpm_chip *chip)
+{
+ cap_t cap;
+ unsigned long timeout_old[4], timeout_chip[4], timeout_eff[4];
+ ssize_t rc;
+
+ rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, , NULL,
+ sizeof(cap.timeout));
+ if (rc == TPM_ERR_INVALID_POSTINIT) {
+ if (tpm_startup(chip))
+ return rc;
+
+ rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, ,
+ "attempting to determine the timeouts",
+ sizeof(cap.timeout));
+ }
+
+ if (rc) {
+ dev_err(>dev, "A TPM error (%zd) occurred attempting to
determine the timeouts\n",
+ rc);
+ return rc;
+ }
+
+ timeout_old[0] = jiffies_to_usecs(chip->timeout_a);
+ timeout_old[1] = jiffies_to_usecs(chip->timeout_b);
+ timeout_old[2] = jiffies_to_usecs(chip->timeout_c);
+ timeout_old[3] = jiffies_to_usecs(chip->timeout_d);
+ timeout_chip[0] = be32_to_cpu(cap.timeout.a);
+ timeout_chip[1] = be32_to_cpu(cap.timeout.b);
+ timeout_chip[2] = be32_to_cpu(cap.timeout.c);
+ timeout_chip[3] = be32_to_cpu(cap.timeout.d);
+ memcpy(timeout_eff, timeout_chip, sizeof(timeout_eff));
+
+ /*
+* Provide ability for vendor overrides of timeout values in case
+* of misreporting.
+*/
+ if (chip->ops->update_timeouts)
+ chip->timeout_adjusted =
+ chip->ops->update_timeouts(chip, timeout_eff);
+
+ if (!chip->timeout_adjusted) {
+ /* Restore default if chip reported 0 */
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(timeout_eff); i++) {
+ if (timeout_eff[i])
+ continue;
+
+ timeout_eff[i] = timeout_old[i];
+ chip->timeout_adjusted = true;
+ }
+
+ if (timeout_eff[0] != 0 && timeout_eff[0] < 1000) {
+ /* timeouts in msec rather usec */
+ for (i = 0; i != ARRAY_SIZE(timeout_eff); i++)
+ timeout_eff[i] *= 1000;
+ chip->timeout_adjusted = true;
+ }
+ }
+
+ /* Report adjusted timeouts */
+ if (chip->timeout_adjusted) {
+ dev_info(>dev, HW_ERR "Adjusting reported timeouts: A %lu->%luus B
%lu->%luus C %lu->%luus D %lu->%luus\n",
+timeout_chip[0], timeout_eff[0],
+timeout_chip[1], timeout_eff[1],
+timeout_chip[2], timeout_eff[2],
+timeout_chip[3], timeout_eff[3]);
+ }
+
+ chip->timeout_a = usecs_to_jiffies(timeout_eff[0]);
+ chip->timeout_b = usecs_to_jiffies(timeout_eff[1]);
+ chip->timeout_c = usecs_to_jiffies(timeout_eff[2]);
+ chip->timeout_d = usecs_to_jiffies(timeout_eff[3]);
+
+ rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, ,
+ "attempting to determine the durations",
+ sizeof(cap.duration));
+ if (r
Re: [PATCH v5 05/21] tpm: factor out tpm_get_timeouts()
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 73511cd89bef..a97d72fcda5b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -544,8 +544,10 @@ int tpm_startup(struct tpm_chip *chip);
ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm_get_timeouts(struct tpm_chip *);
-int tpm1_auto_startup(struct tpm_chip *chip);
int tpm_do_selftest(struct tpm_chip *chip);
+
+int tpm1_auto_startup(struct tpm_chip *chip);
What is different in this tpm1_auto_startup(...) and the original one ?
Is this needed ?
Thanks & Regards,
- Nayna
+int tpm1_get_timeouts(struct tpm_chip *chip);
unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
@@ -585,6 +587,7 @@ static inline u32 tpm2_rc_value(u32 rc)
return (rc & BIT(7)) ? rc & 0xff : rc;
}
+int tpm2_get_timeouts(struct tpm_chip *chip);
int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
struct tpm2_digest *digests);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index dfbe9c60cbcf..978946748ea3 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -307,3 +307,109 @@ unsigned long tpm1_calc_ordinal_duration(struct tpm_chip
*chip, u32 ordinal)
else
return duration;
}
+
+int tpm1_get_timeouts(struct tpm_chip *chip)
+{
+ cap_t cap;
+ unsigned long timeout_old[4], timeout_chip[4], timeout_eff[4];
+ ssize_t rc;
+
+ rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, , NULL,
+ sizeof(cap.timeout));
+ if (rc == TPM_ERR_INVALID_POSTINIT) {
+ if (tpm_startup(chip))
+ return rc;
+
+ rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, ,
+ "attempting to determine the timeouts",
+ sizeof(cap.timeout));
+ }
+
+ if (rc) {
+ dev_err(>dev, "A TPM error (%zd) occurred attempting to
determine the timeouts\n",
+ rc);
+ return rc;
+ }
+
+ timeout_old[0] = jiffies_to_usecs(chip->timeout_a);
+ timeout_old[1] = jiffies_to_usecs(chip->timeout_b);
+ timeout_old[2] = jiffies_to_usecs(chip->timeout_c);
+ timeout_old[3] = jiffies_to_usecs(chip->timeout_d);
+ timeout_chip[0] = be32_to_cpu(cap.timeout.a);
+ timeout_chip[1] = be32_to_cpu(cap.timeout.b);
+ timeout_chip[2] = be32_to_cpu(cap.timeout.c);
+ timeout_chip[3] = be32_to_cpu(cap.timeout.d);
+ memcpy(timeout_eff, timeout_chip, sizeof(timeout_eff));
+
+ /*
+* Provide ability for vendor overrides of timeout values in case
+* of misreporting.
+*/
+ if (chip->ops->update_timeouts)
+ chip->timeout_adjusted =
+ chip->ops->update_timeouts(chip, timeout_eff);
+
+ if (!chip->timeout_adjusted) {
+ /* Restore default if chip reported 0 */
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(timeout_eff); i++) {
+ if (timeout_eff[i])
+ continue;
+
+ timeout_eff[i] = timeout_old[i];
+ chip->timeout_adjusted = true;
+ }
+
+ if (timeout_eff[0] != 0 && timeout_eff[0] < 1000) {
+ /* timeouts in msec rather usec */
+ for (i = 0; i != ARRAY_SIZE(timeout_eff); i++)
+ timeout_eff[i] *= 1000;
+ chip->timeout_adjusted = true;
+ }
+ }
+
+ /* Report adjusted timeouts */
+ if (chip->timeout_adjusted) {
+ dev_info(>dev, HW_ERR "Adjusting reported timeouts: A %lu->%luus B
%lu->%luus C %lu->%luus D %lu->%luus\n",
+timeout_chip[0], timeout_eff[0],
+timeout_chip[1], timeout_eff[1],
+timeout_chip[2], timeout_eff[2],
+timeout_chip[3], timeout_eff[3]);
+ }
+
+ chip->timeout_a = usecs_to_jiffies(timeout_eff[0]);
+ chip->timeout_b = usecs_to_jiffies(timeout_eff[1]);
+ chip->timeout_c = usecs_to_jiffies(timeout_eff[2]);
+ chip->timeout_d = usecs_to_jiffies(timeout_eff[3]);
+
+ rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, ,
+ "attempting to determine the durations",
+ sizeof(cap.duration));
+ if (r
Re: [PATCH v5 03/21] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
+unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
+{
+ int duration_idx = TPM_UNDEFINED;
+ int duration = 0;
+
+ /*
+* We only have a duration table for protected commands, where the upper
+* 16 bits are 0. For the few other ordinals the fallback will be used.
+*/
+ if (ordinal < TPM_MAX_ORDINAL)
+ duration_idx = tpm1_ordinal_duration[ordinal];
+
+ if (duration_idx != TPM_UNDEFINED)
+ duration = chip->duration[duration_idx];
+ if (duration <= 0)
+ return 2 * 60 * HZ;
+ else
+ return duration;
+}
+EXPORT_SYMBOL_GPL(tpm1_calc_ordinal_duration);
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
b/drivers/char/tpm/tpm_i2c_nuvoton.c
index caa86b19c76d..5d20e98b844f 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -370,6 +370,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf,
size_t len)
struct i2c_client *client = to_i2c_client(dev);
u32 ordinal;
size_t count = 0;
+ unsigned long duration;
int burst_count, bytes2write, retries, rc = -EIO;
for (retries = 0; retries < TPM_RETRY; retries++) {
@@ -455,12 +456,11 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8
*buf, size_t len)
return rc;
}
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- rc = i2c_nuvoton_wait_for_data_avail(chip,
-tpm_calc_ordinal_duration(chip,
- ordinal),
->read_queue);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
The original code in the nuvoton driver does not differentiate between
TPM 1.2 and TPM 2.0 as it does in tpm_tis_core.c. Before making any
changes, I would first fix it, so that it could easily be backported.
Only then do the refactoring
Thanks & Regards,
- Nayna
+ rc = i2c_nuvoton_wait_for_data_avail(chip, duration, >read_queue);
if (rc) {
- dev_err(dev, "%s() timeout command duration\n", __func__);
+ dev_err(dev, "%s() timeout command duration %ld\n",
+ __func__, duration);
i2c_nuvoton_ready(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2345d9fd7b5..14c332104de4 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -476,7 +476,7 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const
u8 *buf, size_t len)
if (chip->flags & TPM_CHIP_FLAG_TPM2)
dur = tpm2_calc_ordinal_duration(chip, ordinal);
else
- dur = tpm_calc_ordinal_duration(chip, ordinal);
+ dur = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat
(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index b150f87f38f5..77097229bf49 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -164,7 +164,7 @@ static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t
count)
notify_remote_via_evtchn(priv->evtchn);
ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
- duration = tpm_calc_ordinal_duration(chip, ordinal);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
>read_queue, true) < 0) {
Re: [PATCH v5 03/21] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
On 09/29/2018 04:00 AM, Tomas Winkler wrote:
+unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
+{
+ int duration_idx = TPM_UNDEFINED;
+ int duration = 0;
+
+ /*
+* We only have a duration table for protected commands, where the upper
+* 16 bits are 0. For the few other ordinals the fallback will be used.
+*/
+ if (ordinal < TPM_MAX_ORDINAL)
+ duration_idx = tpm1_ordinal_duration[ordinal];
+
+ if (duration_idx != TPM_UNDEFINED)
+ duration = chip->duration[duration_idx];
+ if (duration <= 0)
+ return 2 * 60 * HZ;
+ else
+ return duration;
+}
+EXPORT_SYMBOL_GPL(tpm1_calc_ordinal_duration);
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
b/drivers/char/tpm/tpm_i2c_nuvoton.c
index caa86b19c76d..5d20e98b844f 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -370,6 +370,7 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf,
size_t len)
struct i2c_client *client = to_i2c_client(dev);
u32 ordinal;
size_t count = 0;
+ unsigned long duration;
int burst_count, bytes2write, retries, rc = -EIO;
for (retries = 0; retries < TPM_RETRY; retries++) {
@@ -455,12 +456,11 @@ static int i2c_nuvoton_send(struct tpm_chip *chip, u8
*buf, size_t len)
return rc;
}
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
- rc = i2c_nuvoton_wait_for_data_avail(chip,
-tpm_calc_ordinal_duration(chip,
- ordinal),
->read_queue);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
The original code in the nuvoton driver does not differentiate between
TPM 1.2 and TPM 2.0 as it does in tpm_tis_core.c. Before making any
changes, I would first fix it, so that it could easily be backported.
Only then do the refactoring
Thanks & Regards,
- Nayna
+ rc = i2c_nuvoton_wait_for_data_avail(chip, duration, >read_queue);
if (rc) {
- dev_err(dev, "%s() timeout command duration\n", __func__);
+ dev_err(dev, "%s() timeout command duration %ld\n",
+ __func__, duration);
i2c_nuvoton_ready(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2345d9fd7b5..14c332104de4 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -476,7 +476,7 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const
u8 *buf, size_t len)
if (chip->flags & TPM_CHIP_FLAG_TPM2)
dur = tpm2_calc_ordinal_duration(chip, ordinal);
else
- dur = tpm_calc_ordinal_duration(chip, ordinal);
+ dur = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat
(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index b150f87f38f5..77097229bf49 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -164,7 +164,7 @@ static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t
count)
notify_remote_via_evtchn(priv->evtchn);
ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
- duration = tpm_calc_ordinal_duration(chip, ordinal);
+ duration = tpm1_calc_ordinal_duration(chip, ordinal);
if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
>read_queue, true) < 0) {
[PATCH v6 3/5] ima: refactor ima_init_policy()
From: Nayna Jain
This patch removes the code duplication in ima_init_policy() by defining
a new function named add_rules(). The new function adds the rules to the
initial IMA policy, the custom policy or both based on the policy mask
(IMA_DEFAULT_POLICY, IMA_CUSTOM_POLICY).
Signed-off-by: Nayna Jain
---
security/integrity/ima/ima_policy.c | 97 +
1 file changed, 56 insertions(+), 41 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 8c9499867c91..1e30d09a56db 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -58,6 +58,8 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE,
LSM_OBJ_TYPE,
enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
+enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
+
struct ima_rule_entry {
struct list_head list;
int action;
@@ -473,6 +475,32 @@ static int ima_appraise_flag(enum ima_hooks func)
return 0;
}
+static void add_rules(struct ima_rule_entry *entries, int count,
+ enum policy_rule_list policy_rule)
+{
+ int i = 0;
+
+ for (i = 0; i < count; i++) {
+ struct ima_rule_entry *entry;
+
+ if (policy_rule & IMA_DEFAULT_POLICY)
+ list_add_tail([i].list, _default_rules);
+
+ if (policy_rule & IMA_CUSTOM_POLICY) {
+ entry = kmemdup([i], sizeof(*entry),
+ GFP_KERNEL);
+ if (!entry)
+ continue;
+
+ list_add_tail(>list, _policy_rules);
+ }
+ if (entries[i].action == APPRAISE)
+ temp_ima_appraise |= ima_appraise_flag(entries[i].func);
+ if (entries[i].func == POLICY_CHECK)
+ temp_ima_appraise |= IMA_APPRAISE_POLICY;
+ }
+}
+
/**
* ima_init_policy - initialize the default measure rules.
*
@@ -481,28 +509,23 @@ static int ima_appraise_flag(enum ima_hooks func)
*/
void __init ima_init_policy(void)
{
- int i, measure_entries, appraise_entries, secure_boot_entries;
-
- /* if !ima_policy set entries = 0 so we load NO default rules */
- measure_entries = ima_policy ? ARRAY_SIZE(dont_measure_rules) : 0;
- appraise_entries = ima_use_appraise_tcb ?
-ARRAY_SIZE(default_appraise_rules) : 0;
- secure_boot_entries = ima_use_secure_boot ?
- ARRAY_SIZE(secure_boot_rules) : 0;
+ int build_appraise_entries;
- for (i = 0; i < measure_entries; i++)
- list_add_tail(_measure_rules[i].list, _default_rules);
+ /* if !ima_policy, we load NO default rules */
+ if (ima_policy)
+ add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
+ IMA_DEFAULT_POLICY);
switch (ima_policy) {
case ORIGINAL_TCB:
- for (i = 0; i < ARRAY_SIZE(original_measurement_rules); i++)
- list_add_tail(_measurement_rules[i].list,
- _default_rules);
+ add_rules(original_measurement_rules,
+ ARRAY_SIZE(original_measurement_rules),
+ IMA_DEFAULT_POLICY);
break;
case DEFAULT_TCB:
- for (i = 0; i < ARRAY_SIZE(default_measurement_rules); i++)
- list_add_tail(_measurement_rules[i].list,
- _default_rules);
+ add_rules(default_measurement_rules,
+ ARRAY_SIZE(default_measurement_rules),
+ IMA_DEFAULT_POLICY);
default:
break;
}
@@ -511,38 +534,30 @@ void __init ima_init_policy(void)
* Insert the builtin "secure_boot" policy rules requiring file
* signatures, prior to any other appraise rules.
*/
- for (i = 0; i < secure_boot_entries; i++) {
- list_add_tail(_boot_rules[i].list, _default_rules);
- temp_ima_appraise |=
- ima_appraise_flag(secure_boot_rules[i].func);
- }
+ if (ima_use_secure_boot)
+ add_rules(secure_boot_rules, ARRAY_SIZE(secure_boot_rules),
+ IMA_DEFAULT_POLICY);
/*
* Insert the build time appraise rules requiring file signatures
* for both the initial and custom policies, prior to other appraise
-* rules.
+* rules. As the secure boot rules includes all of the build time
+* rules, include either one or the other set of rules, but not both.
*/
- for (i = 0; i < ARRAY_SIZE(build_appraise_rules); i++) {
- struct ima_rule_entry *entry;
-
- if (!secure_boot_entries
[PATCH v6 3/5] ima: refactor ima_init_policy()
From: Nayna Jain
This patch removes the code duplication in ima_init_policy() by defining
a new function named add_rules(). The new function adds the rules to the
initial IMA policy, the custom policy or both based on the policy mask
(IMA_DEFAULT_POLICY, IMA_CUSTOM_POLICY).
Signed-off-by: Nayna Jain
---
security/integrity/ima/ima_policy.c | 97 +
1 file changed, 56 insertions(+), 41 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 8c9499867c91..1e30d09a56db 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -58,6 +58,8 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE,
LSM_OBJ_TYPE,
enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
+enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
+
struct ima_rule_entry {
struct list_head list;
int action;
@@ -473,6 +475,32 @@ static int ima_appraise_flag(enum ima_hooks func)
return 0;
}
+static void add_rules(struct ima_rule_entry *entries, int count,
+ enum policy_rule_list policy_rule)
+{
+ int i = 0;
+
+ for (i = 0; i < count; i++) {
+ struct ima_rule_entry *entry;
+
+ if (policy_rule & IMA_DEFAULT_POLICY)
+ list_add_tail([i].list, _default_rules);
+
+ if (policy_rule & IMA_CUSTOM_POLICY) {
+ entry = kmemdup([i], sizeof(*entry),
+ GFP_KERNEL);
+ if (!entry)
+ continue;
+
+ list_add_tail(>list, _policy_rules);
+ }
+ if (entries[i].action == APPRAISE)
+ temp_ima_appraise |= ima_appraise_flag(entries[i].func);
+ if (entries[i].func == POLICY_CHECK)
+ temp_ima_appraise |= IMA_APPRAISE_POLICY;
+ }
+}
+
/**
* ima_init_policy - initialize the default measure rules.
*
@@ -481,28 +509,23 @@ static int ima_appraise_flag(enum ima_hooks func)
*/
void __init ima_init_policy(void)
{
- int i, measure_entries, appraise_entries, secure_boot_entries;
-
- /* if !ima_policy set entries = 0 so we load NO default rules */
- measure_entries = ima_policy ? ARRAY_SIZE(dont_measure_rules) : 0;
- appraise_entries = ima_use_appraise_tcb ?
-ARRAY_SIZE(default_appraise_rules) : 0;
- secure_boot_entries = ima_use_secure_boot ?
- ARRAY_SIZE(secure_boot_rules) : 0;
+ int build_appraise_entries;
- for (i = 0; i < measure_entries; i++)
- list_add_tail(_measure_rules[i].list, _default_rules);
+ /* if !ima_policy, we load NO default rules */
+ if (ima_policy)
+ add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
+ IMA_DEFAULT_POLICY);
switch (ima_policy) {
case ORIGINAL_TCB:
- for (i = 0; i < ARRAY_SIZE(original_measurement_rules); i++)
- list_add_tail(_measurement_rules[i].list,
- _default_rules);
+ add_rules(original_measurement_rules,
+ ARRAY_SIZE(original_measurement_rules),
+ IMA_DEFAULT_POLICY);
break;
case DEFAULT_TCB:
- for (i = 0; i < ARRAY_SIZE(default_measurement_rules); i++)
- list_add_tail(_measurement_rules[i].list,
- _default_rules);
+ add_rules(default_measurement_rules,
+ ARRAY_SIZE(default_measurement_rules),
+ IMA_DEFAULT_POLICY);
default:
break;
}
@@ -511,38 +534,30 @@ void __init ima_init_policy(void)
* Insert the builtin "secure_boot" policy rules requiring file
* signatures, prior to any other appraise rules.
*/
- for (i = 0; i < secure_boot_entries; i++) {
- list_add_tail(_boot_rules[i].list, _default_rules);
- temp_ima_appraise |=
- ima_appraise_flag(secure_boot_rules[i].func);
- }
+ if (ima_use_secure_boot)
+ add_rules(secure_boot_rules, ARRAY_SIZE(secure_boot_rules),
+ IMA_DEFAULT_POLICY);
/*
* Insert the build time appraise rules requiring file signatures
* for both the initial and custom policies, prior to other appraise
-* rules.
+* rules. As the secure boot rules includes all of the build time
+* rules, include either one or the other set of rules, but not both.
*/
- for (i = 0; i < ARRAY_SIZE(build_appraise_rules); i++) {
- struct ima_rule_entry *entry;
-
- if (!secure_boot_entries
Re: [PATCH v5 06/21] tpm: move tpm1_pcr_extend to tpm1-cmd.c
On 10/02/2018 06:12 AM, Jarkko Sakkinen wrote:
On Sat, Sep 29, 2018 at 01:30:20AM +0300, Tomas Winkler wrote:
Move tpm1_pcr_extend to tpm1-cmd.c and remove
unused pcrextend_header structure and
EXTEND_PCR_RESULT_SIZE and EXTEND_PCR_RESULT_BODY_SIZE
defines.
Fixes warning:
drivers/char/tpm/tpm-interface.c:609:38: warning: ‘pcrextend_header’ defined
but not used [-Wunused-const-variable=]
static const struct tpm_input_header pcrextend_header = {
^~~~
Signed-off-by: Tomas Winkler
Reviewed-by: Jarkko Sakkinen
Would need help with this from someone with a working IMA setup
on testing.
My test system is down for now.. I think it will be up later in this
week. I can test it then.
Thanks & Regards,
- Nayna
/Jarkko
Re: [PATCH v5 06/21] tpm: move tpm1_pcr_extend to tpm1-cmd.c
On 10/02/2018 06:12 AM, Jarkko Sakkinen wrote:
On Sat, Sep 29, 2018 at 01:30:20AM +0300, Tomas Winkler wrote:
Move tpm1_pcr_extend to tpm1-cmd.c and remove
unused pcrextend_header structure and
EXTEND_PCR_RESULT_SIZE and EXTEND_PCR_RESULT_BODY_SIZE
defines.
Fixes warning:
drivers/char/tpm/tpm-interface.c:609:38: warning: ‘pcrextend_header’ defined
but not used [-Wunused-const-variable=]
static const struct tpm_input_header pcrextend_header = {
^~~~
Signed-off-by: Tomas Winkler
Reviewed-by: Jarkko Sakkinen
Would need help with this from someone with a working IMA setup
on testing.
My test system is down for now.. I think it will be up later in this
week. I can test it then.
Thanks & Regards,
- Nayna
/Jarkko
Re: [PATCH v4 03/21] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 90 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED, /* 95 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_MEDIUM, /* 100 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 105 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 110 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT, /* 115 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_LONG, /* 120 */
+ TPM_LONG,
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_SHORT, /* 125 */
+ TPM_SHORT,
+ TPM_LONG,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT, /* 130 */
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_MEDIUM,
+ TPM_UNDEFINED, /* 135 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 140 */
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 145 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 150 */
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED, /* 155 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 160 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 165 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_LONG, /* 170 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 175 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_MEDIUM, /* 180 */
+ TPM_SHORT,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_MEDIUM, /* 185 */
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 190 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 195 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 200 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_SHORT, /* 205 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_MEDIUM, /* 210 */
+ TPM_UNDEFINED,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_UNDEFINED, /* 215 */
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_SHORT, /* 220 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED, /* 225 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 230 */
+ TPM_LONG,
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 235 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 240 */
+ TPM_UNDEFINED,
+ TPM_MEDIUM,
+};
+
+/**
+ * tpm1_calc_ordinal_duration() - returns the maximum amount of time
+ * the chip could take to return the result for a particular ordinal
+ * in jiffies.
+ *
+ * @chip:TPM chip to use.
+ * @ordinal: TPM command ordinal.
+ *
+ * Return: A maxiaml duration time for an ordinal in jiffies.
typo *maximal". It seems the typo got carried over to all
tpm*_calc_ordinal_duration functions.
Thanks & Regards,
- Nayna
+ */
+unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
+{
+ int duration_idx = TPM_UNDEFINED;
+ int duration = 0;
+
+ /*
+* We only have a duration table for protected commands, where the upper
+* 16 bits are 0. For the few other ordinals the fallback will be used.
+*/
+ if (ordinal < TPM_MAX_ORDINAL)
+ duration_idx = tpm1_ordinal_duration[ordinal];
+
+ if (duration_idx != TPM_UNDEFINED)
+ duration = chip->duration[duratio
Re: [PATCH v4 03/21] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 90 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED, /* 95 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_MEDIUM, /* 100 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 105 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 110 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT, /* 115 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_LONG, /* 120 */
+ TPM_LONG,
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_SHORT, /* 125 */
+ TPM_SHORT,
+ TPM_LONG,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT, /* 130 */
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_MEDIUM,
+ TPM_UNDEFINED, /* 135 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 140 */
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 145 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 150 */
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED, /* 155 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 160 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 165 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_LONG, /* 170 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 175 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_MEDIUM, /* 180 */
+ TPM_SHORT,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_MEDIUM, /* 185 */
+ TPM_SHORT,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 190 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 195 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 200 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_SHORT, /* 205 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_MEDIUM, /* 210 */
+ TPM_UNDEFINED,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_MEDIUM,
+ TPM_UNDEFINED, /* 215 */
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT,
+ TPM_SHORT, /* 220 */
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_SHORT,
+ TPM_UNDEFINED, /* 225 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 230 */
+ TPM_LONG,
+ TPM_MEDIUM,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED, /* 235 */
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_UNDEFINED,
+ TPM_SHORT, /* 240 */
+ TPM_UNDEFINED,
+ TPM_MEDIUM,
+};
+
+/**
+ * tpm1_calc_ordinal_duration() - returns the maximum amount of time
+ * the chip could take to return the result for a particular ordinal
+ * in jiffies.
+ *
+ * @chip:TPM chip to use.
+ * @ordinal: TPM command ordinal.
+ *
+ * Return: A maxiaml duration time for an ordinal in jiffies.
typo *maximal". It seems the typo got carried over to all
tpm*_calc_ordinal_duration functions.
Thanks & Regards,
- Nayna
+ */
+unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
+{
+ int duration_idx = TPM_UNDEFINED;
+ int duration = 0;
+
+ /*
+* We only have a duration table for protected commands, where the upper
+* 16 bits are 0. For the few other ordinals the fallback will be used.
+*/
+ if (ordinal < TPM_MAX_ORDINAL)
+ duration_idx = tpm1_ordinal_duration[ordinal];
+
+ if (duration_idx != TPM_UNDEFINED)
+ duration = chip->duration[duratio
