This patch prevents the following oops:
[ 10.771813] BUG: kernel NULL pointer dereference, address: 0000000000000 [...] [ 10.779790] RIP: 0010:ima_match_policy+0xf7/0xb80 [...] [ 10.798576] Call Trace: [ 10.798993] ? ima_lsm_policy_change+0x2b0/0x2b0 [ 10.799753] ? inode_init_owner+0x1a0/0x1a0 [ 10.800484] ? _raw_spin_lock+0x7a/0xd0 [ 10.801592] ima_must_appraise.part.0+0xb6/0xf0 [ 10.802313] ? ima_fix_xattr.isra.0+0xd0/0xd0 [ 10.803167] ima_must_appraise+0x4f/0x70 [ 10.804004] ima_post_path_mknod+0x2e/0x80 [ 10.804800] do_mknodat+0x396/0x3c0
It occurs when there is a failure during IMA initialization, and ima_init_policy() is not called. IMA hooks still call ima_match_policy() but ima_rules is NULL. This patch prevents the crash by directly assigning the ima_default_policy pointer to ima_rules when ima_rules is defined. This wouldn't alter the existing behavior, as ima_rules is always set at the end of ima_init_policy().
Cc: stable@vger.kernel.org # 3.7.x Fixes: 07f6a79415d7d ("ima: add appraise action keywords and default rules") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- security/integrity/ima/ima_policy.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index ef7f68cc935e..e493063a3c34 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -204,7 +204,7 @@ static struct ima_rule_entry *arch_policy_entry __ro_after_init; static LIST_HEAD(ima_default_rules); static LIST_HEAD(ima_policy_rules); static LIST_HEAD(ima_temp_rules); -static struct list_head *ima_rules; +static struct list_head *ima_rules = &ima_default_rules;
/* Pre-allocated buffer used for matching keyrings. */ static char *ima_keyrings; @@ -768,7 +768,6 @@ void __init ima_init_policy(void) ARRAY_SIZE(default_appraise_rules), IMA_DEFAULT_POLICY);
- ima_rules = &ima_default_rules; ima_update_policy_flag(); }
If the template field 'd' is chosen and the digest to be added to the measurement entry was not calculated with SHA1 or MD5, it is recalculated with SHA1, by using the passed file descriptor. However, this cannot be done for boot_aggregate, because there is no file descriptor.
This patch adds a call to ima_calc_boot_aggregate() in ima_eventdigest_init(), so that the digest can be recalculated also for the boot_aggregate entry.
Cc: stable@vger.kernel.org # 3.13.x Fixes: 3ce1217d6cd5d ("ima: define template fields library and new helpers") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- security/integrity/ima/ima.h | 3 ++- security/integrity/ima/ima_crypto.c | 6 +++--- security/integrity/ima/ima_init.c | 2 +- security/integrity/ima/ima_template_lib.c | 18 ++++++++++++++++++ 4 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 02796473238b..df93ac258e01 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -57,6 +57,7 @@ extern int ima_hash_algo_idx __ro_after_init; extern int ima_extra_slots __ro_after_init; extern int ima_appraise; extern struct tpm_chip *ima_tpm_chip; +extern const char boot_aggregate_name[];
/* IMA event related data */ struct ima_event_data { @@ -144,7 +145,7 @@ int ima_calc_buffer_hash(const void *buf, loff_t len, struct ima_digest_data *hash); int ima_calc_field_array_hash(struct ima_field_data *field_data, struct ima_template_entry *entry); -int __init ima_calc_boot_aggregate(struct ima_digest_data *hash); +int ima_calc_boot_aggregate(struct ima_digest_data *hash); void ima_add_violation(struct file *file, const unsigned char *filename, struct integrity_iint_cache *iint, const char *op, const char *cause); diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index f3a7f4eb1fc1..ba5cc3264240 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -806,8 +806,8 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d) * hash algorithm for reading the TPM PCRs as for calculating the boot * aggregate digest as stored in the measurement list. */ -static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id, - struct crypto_shash *tfm) +static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id, + struct crypto_shash *tfm) { struct tpm_digest d = { .alg_id = alg_id, .digest = {0} }; int rc; @@ -835,7 +835,7 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id, return rc; }
-int __init ima_calc_boot_aggregate(struct ima_digest_data *hash) +int ima_calc_boot_aggregate(struct ima_digest_data *hash) { struct crypto_shash *tfm; u16 crypto_id, alg_id; diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index fc1e1002b48d..4902fe7bd570 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -19,7 +19,7 @@ #include "ima.h"
/* name for boot aggregate entry */ -static const char boot_aggregate_name[] = "boot_aggregate"; +const char boot_aggregate_name[] = "boot_aggregate"; struct tpm_chip *ima_tpm_chip;
/* Add the boot aggregate to the IMA measurement list and extend diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 9cd1e50f3ccc..635c6ac05050 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -286,6 +286,24 @@ int ima_eventdigest_init(struct ima_event_data *event_data, goto out; }
+ if ((const char *)event_data->filename == boot_aggregate_name) { + if (ima_tpm_chip) { + hash.hdr.algo = HASH_ALGO_SHA1; + result = ima_calc_boot_aggregate(&hash.hdr); + + /* algo can change depending on available PCR banks */ + if (!result && hash.hdr.algo != HASH_ALGO_SHA1) + result = -EINVAL; + + if (result < 0) + memset(&hash, 0, sizeof(hash)); + } + + cur_digest = hash.hdr.digest; + cur_digestsize = hash_digest_size[HASH_ALGO_SHA1]; + goto out; + } + if (!event_data->file) /* missing info to re-calculate the digest */ return -EINVAL;
Hi Roberto,
On Wed, 2020-06-03 at 17:08 +0200, Roberto Sassu wrote:
If the template field 'd' is chosen and the digest to be added to the measurement entry was not calculated with SHA1 or MD5, it is recalculated with SHA1, by using the passed file descriptor. However, this cannot be done for boot_aggregate, because there is no file descriptor.
This patch adds a call to ima_calc_boot_aggregate() in ima_eventdigest_init(), so that the digest can be recalculated also for the boot_aggregate entry.
Cc: stable@vger.kernel.org # 3.13.x Fixes: 3ce1217d6cd5d ("ima: define template fields library and new helpers") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Thanks, Roberto.
I've pushed both patches out to the next-integrity branch and would appreciate some additional testing.
thanks,
Mimi
On Wed, Jun 03, 2020 at 06:03:35PM -0400, Mimi Zohar wrote:
Hi Roberto,
On Wed, 2020-06-03 at 17:08 +0200, Roberto Sassu wrote:
If the template field 'd' is chosen and the digest to be added to the measurement entry was not calculated with SHA1 or MD5, it is recalculated with SHA1, by using the passed file descriptor. However, this cannot be done for boot_aggregate, because there is no file descriptor.
This patch adds a call to ima_calc_boot_aggregate() in ima_eventdigest_init(), so that the digest can be recalculated also for the boot_aggregate entry.
Cc: stable@vger.kernel.org # 3.13.x Fixes: 3ce1217d6cd5d ("ima: define template fields library and new helpers") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Thanks, Roberto.
I've pushed both patches out to the next-integrity branch and would appreciate some additional testing.
thanks,
Mimi
Hi Mimi and Roberto,
FWIW, I've tested this patch manually and things went fine, with no unexpected behavior or results.
However, wouldn't it be worth add a note in kmsg about the ima_calc_boot_aggregate() being called with an algo different from the system's default? Just to let the user know he won't find a sha256 when check the measurement. But that's something we can add later too.
Thanks.
On Thu, 2020-06-04 at 16:12 -0300, Bruno Meneguele wrote:
On Wed, Jun 03, 2020 at 06:03:35PM -0400, Mimi Zohar wrote:
Hi Roberto,
On Wed, 2020-06-03 at 17:08 +0200, Roberto Sassu wrote:
If the template field 'd' is chosen and the digest to be added to the measurement entry was not calculated with SHA1 or MD5, it is recalculated with SHA1, by using the passed file descriptor. However, this cannot be done for boot_aggregate, because there is no file descriptor.
This patch adds a call to ima_calc_boot_aggregate() in ima_eventdigest_init(), so that the digest can be recalculated also for the boot_aggregate entry.
Cc: stable@vger.kernel.org # 3.13.x Fixes: 3ce1217d6cd5d ("ima: define template fields library and new helpers") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Thanks, Roberto.
I've pushed both patches out to the next-integrity branch and would appreciate some additional testing.
thanks,
Mimi
Hi Mimi and Roberto,
FWIW, I've tested this patch manually and things went fine, with no unexpected behavior or results.
Thanks, Bruno!
However, wouldn't it be worth add a note in kmsg about the ima_calc_boot_aggregate() being called with an algo different from the system's default? Just to let the user know he won't find a sha256 when check the measurement. But that's something we can add later too.
There's no guarantees that the IMA default crypto algorithm will be used for calculating the boot_aggregate. The algorithm is dependent on the TPM. For example, the default IMA algorithm could be sha256, but on a system with TPM 1.2, the boot_aggregate would have to be sha1.
This patch addresses a mismatch between the template format field ('d' field) and the larger digest. We could require the "ima_template_fmt" specified on the boot command line define an appropriate format, but Roberto decided to support the situation where both "d" and "d-ng" are defined.
Mimi
On Thu, Jun 04, 2020 at 03:35:20PM -0400, Mimi Zohar wrote:
On Thu, 2020-06-04 at 16:12 -0300, Bruno Meneguele wrote:
On Wed, Jun 03, 2020 at 06:03:35PM -0400, Mimi Zohar wrote:
Hi Roberto,
On Wed, 2020-06-03 at 17:08 +0200, Roberto Sassu wrote:
If the template field 'd' is chosen and the digest to be added to the measurement entry was not calculated with SHA1 or MD5, it is recalculated with SHA1, by using the passed file descriptor. However, this cannot be done for boot_aggregate, because there is no file descriptor.
This patch adds a call to ima_calc_boot_aggregate() in ima_eventdigest_init(), so that the digest can be recalculated also for the boot_aggregate entry.
Cc: stable@vger.kernel.org # 3.13.x Fixes: 3ce1217d6cd5d ("ima: define template fields library and new helpers") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Thanks, Roberto.
I've pushed both patches out to the next-integrity branch and would appreciate some additional testing.
thanks,
Mimi
Hi Mimi and Roberto,
FWIW, I've tested this patch manually and things went fine, with no unexpected behavior or results.
Thanks, Bruno!
However, wouldn't it be worth add a note in kmsg about the ima_calc_boot_aggregate() being called with an algo different from the system's default? Just to let the user know he won't find a sha256 when check the measurement. But that's something we can add later too.
There's no guarantees that the IMA default crypto algorithm will be used for calculating the boot_aggregate. The algorithm is dependent on the TPM. For example, the default IMA algorithm could be sha256, but on a system with TPM 1.2, the boot_aggregate would have to be sha1.
Ah Indeed.. it makes sense.
This patch addresses a mismatch between the template format field ('d' field) and the larger digest. We could require the "ima_template_fmt" specified on the boot command line define an appropriate format, but Roberto decided to support the situation where both "d" and "d-ng" are defined.
Yes, personally I also prefer to "fail earlier" or to be more stricter on user definitions, but I also understand going the other way allowing both d & d-ng.
Mimi
thanks Mimi.
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag fixing commit: 3ce1217d6cd5 ("ima: define template fields library and new helpers").
The bot has tested the following trees: v5.6.15, v5.4.43, v4.19.125, v4.14.182, v4.9.225, v4.4.225.
v5.6.15: Failed to apply! Possible dependencies: 6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate") 7ca79645a1f8 ("ima: Store template digest directly in ima_template_entry")
v5.4.43: Failed to apply! Possible dependencies: 6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate") 7ca79645a1f8 ("ima: Store template digest directly in ima_template_entry")
v4.19.125: Failed to apply! Possible dependencies: 100b16a6f290 ("tpm: sort objects in the Makefile") 1ad6640cd614 ("tpm: move tpm1_pcr_extend to tpm1-cmd.c") 6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate") 70a3199a7101 ("tpm: factor out tpm_get_timeouts()") 7ca79645a1f8 ("ima: Store template digest directly in ima_template_entry") 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read") 899102bc4518 ("tpm2: add new tpm2 commands according to TCG 1.36") 95adc6b410b7 ("tpm: use u32 instead of int for PCR index") b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c") c82a330ceced ("tpm: factor out tpm 1.x pm suspend flow into tpm1-cmd.c") d4a317563207 ("tpm: move tpm 1.x selftest code from tpm-interface.c tpm1-cmd.c") d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
v4.14.182: Failed to apply! Possible dependencies: 0bfb23746052 ("tpm: Move eventlog files to a subdirectory") 100b16a6f290 ("tpm: sort objects in the Makefile") 58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table") 5c2a640aff73 ("ima: Use tpm_default_chip() and call TPM functions with a tpm_chip") 67cb8e113ecd ("tpm: rename event log provider files") 6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate") 7ca79645a1f8 ("ima: Store template digest directly in ima_template_entry") 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read") 95adc6b410b7 ("tpm: use u32 instead of int for PCR index") 9b01b5356629 ("tpm: Move shared eventlog functions to common.c") aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()") b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c") c82a330ceced ("tpm: factor out tpm 1.x pm suspend flow into tpm1-cmd.c") d4a317563207 ("tpm: move tpm 1.x selftest code from tpm-interface.c tpm1-cmd.c") fd3ec3663718 ("tpm: move tpm_eventlog.h outside of drivers folder")
v4.9.225: Failed to apply! Possible dependencies: 02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime") 2528a64664f8 ("tpm: define a generic open() method for ascii & bios measurements") 37f4915fef05 ("tpm: use idr_find(), not idr_find_slowpath()") 5c2a640aff73 ("ima: Use tpm_default_chip() and call TPM functions with a tpm_chip") 62bfdacbac4c ("tpm: Do not print an error message when doing TPM auto startup") 745b361e989a ("tpm: infrastructure for TPM spaces") 748935eeb72c ("tpm: have event log use the tpm_chip") 7518a21a9da3 ("tpm: drop tpm1_chip_register(/unregister)") 84fda15286d1 ("tpm: sanitize constant expressions") aaa6f7f6c8bf ("tpm: Clean up reading of timeout and duration capabilities") aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()") b1a9b7b602c5 ("tpm: replace symbolic permission with octal for securityfs files") c1f92b4b04ad ("tpm: enhance TPM 2.0 PCR extend to support multiple banks") c659af78eb7b ("tpm: Check size of response before accessing data") ca6d45802201 ("tpm: place kdoc just above tpm_pcr_extend") cd9b7631a888 ("tpm: replace dynamically allocated bios_dir with a static array") f865c196856d ("tpm: add kdoc for tpm_transmit and tpm_transmit_cmd")
v4.4.225: Failed to apply! Possible dependencies: 062807f20e3f ("tpm: Remove all uses of drvdata from the TPM Core") 23d06ff700f5 ("tpm: drop tpm_atmel specific fields from tpm_vendor_specific") 25112048cd59 ("tpm: rework tpm_get_timeouts()") 37f4915fef05 ("tpm: use idr_find(), not idr_find_slowpath()") 4eea703caaac ("tpm: drop 'iobase' from struct tpm_vendor_specific") 570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific") 5c2a640aff73 ("ima: Use tpm_default_chip() and call TPM functions with a tpm_chip") 62bfdacbac4c ("tpm: Do not print an error message when doing TPM auto startup") 6e599f6f261f ("tpm: drop 'read_queue' from struct tpm_vendor_specific") 7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code") 84fda15286d1 ("tpm: sanitize constant expressions") aaa6f7f6c8bf ("tpm: Clean up reading of timeout and duration capabilities") aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()") af782f339a5d ("tpm: Move tpm_vendor_specific data related with PTP specification to tpm_chip") c1f92b4b04ad ("tpm: enhance TPM 2.0 PCR extend to support multiple banks") c659af78eb7b ("tpm: Check size of response before accessing data") d4956524f1b0 ("tpm: drop manufacturer_id from struct tpm_vendor_specific") e3837e74a06d ("tpm_tis: Refactor the interrupt setup") ee1779840d09 ("tpm: drop 'base' from struct tpm_vendor_specific") f865c196856d ("tpm: add kdoc for tpm_transmit and tpm_transmit_cmd")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
On Wed, 2020-06-03 at 17:08 +0200, Roberto Sassu wrote:
This patch prevents the following oops:
[ 10.771813] BUG: kernel NULL pointer dereference, address: 0000000000000 [...] [ 10.779790] RIP: 0010:ima_match_policy+0xf7/0xb80 [...] [ 10.798576] Call Trace: [ 10.798993] ? ima_lsm_policy_change+0x2b0/0x2b0 [ 10.799753] ? inode_init_owner+0x1a0/0x1a0 [ 10.800484] ? _raw_spin_lock+0x7a/0xd0 [ 10.801592] ima_must_appraise.part.0+0xb6/0xf0 [ 10.802313] ? ima_fix_xattr.isra.0+0xd0/0xd0 [ 10.803167] ima_must_appraise+0x4f/0x70 [ 10.804004] ima_post_path_mknod+0x2e/0x80 [ 10.804800] do_mknodat+0x396/0x3c0
It occurs when there is a failure during IMA initialization, and ima_init_policy() is not called. IMA hooks still call ima_match_policy() but ima_rules is NULL. This patch prevents the crash by directly assigning the ima_default_policy pointer to ima_rules when ima_rules is defined. This wouldn't alter the existing behavior, as ima_rules is always set at the end of ima_init_policy().
Cc: stable@vger.kernel.org # 3.7.x Fixes: 07f6a79415d7d ("ima: add appraise action keywords and default rules") Reported-by: Takashi Iwai tiwai@suse.de Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Thanks, Roberto!
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag fixing commit: 07f6a79415d7 ("ima: add appraise action keywords and default rules").
The bot has tested the following trees: v5.6.15, v5.4.43, v4.19.125, v4.14.182, v4.9.225, v4.4.225.
v5.6.15: Build OK! v5.4.43: Build OK! v4.19.125: Build OK! v4.14.182: Failed to apply! Possible dependencies: Unable to calculate
v4.9.225: Failed to apply! Possible dependencies: Unable to calculate
v4.4.225: Failed to apply! Possible dependencies: 38d859f991f3 ("IMA: policy can now be updated multiple times") 95ee08fa373b ("ima: require signed IMA policy")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
linux-stable-mirror@lists.linaro.org
-
Bruno Meneguele -
Mimi Zohar -
Roberto Sassu -
Sasha Levin