On Fri, 2024-03-08 at 11:49 +0100, Enrico Bravi wrote:
> The template hash showed by the ascii_runtime_measurements and
> binary_runtime_measurements is the one calculated using sha1 and there is
> no possibility to change this value, despite the fact that the template
> hash is calculated using the hash algorithms corresponding to all the PCR
> banks configured in the TPM.
>
> Add the support to retrieve the ima log with the template data hash
> calculated with a specific hash algorithm.
> Add a new file in the securityfs ima directory for each hash algo
> configured in a PCR bank of the TPM. Each new file has the name with
> the following structure:
>
> {binary, ascii}_runtime_measurements_<hash_algo_name>
>
> Legacy files are kept, to avoid breaking existing applications, but as
> symbolic links which point to {binary, ascii}_runtime_measurements_sha1
> files. These two files are created even if a TPM chip is not detected or
> the sha1 bank is not configure in the TPM.
Configured.
> As example, in the case a TPM chip is present and sha256 is the only
> configured PCR bank, the listing of the security/ima directory is the
> following:
>
> lr--r--r-- [...] ascii_runtime_measurements -> ascii_runtime_measurements_sha1
> -r--r----- [...] ascii_runtime_measurements_sha1
> -r--r----- [...] ascii_runtime_measurements_sha256
> lr--r--r-- [...] binary_runtime_measurements -> binary_runtime_measurements_sha1
> -r--r----- [...] binary_runtime_measurements_sha1
> -r--r----- [...] binary_runtime_measurements_sha256
> --w------- [...] policy
> -r--r----- [...] runtime_measurements_count
> -r--r----- [...] violations
>
> Signed-off-by: Enrico Bravi <enrico.bravi@xxxxxxxxx>
> Signed-off-by: Silvia Sisinni <silvia.sisinni@xxxxxxxxx>
>
> ---
>
> v4:
> - Added NULL check on m->file for measurements list dump called by
> ima_dump_measurement_list() on kexec.
> - Exported ima_algo_array and struct ima_algo_desc declaration from
> ima_crypto.c to access this information in ima_fs.c.
> - Added ima_measurements_files_count global variable to avoid extra
> logic each time the number of measurements file is needed.
>
> v3:
> - Added create_measurements_list_files function for measurements files creation.
> - Parameterized the remove_measurements_list_files function and add NULL
> check before freeing files' list.
> - Removed algorithm selection based on file name during ima_measurements_show
> and ima_ascii_measurements_show, and selecting it comparing dentry address.
> - Allocate also sha1 file following the schema
> {binary, ascii}_runtime_measurements_<hash_algo_name> and keep legacy
> files as symbolic links to those files.
> - Allocate measurements files lists even if a TPM chip is not detected,
> adding only sha1 files.
>
> v2:
> - Changed the behavior of configuring at boot time the template data hash
> algorithm.
> - Removed template data hash algo name prefix.
> - Removed ima_template_hash command line option.
> - Introducing a new file in the securityfs ima subdir for each PCR banks
> algorithm configured in the TPM.
> (suggested by Roberto)
>
>
> security/integrity/ima/ima.h | 9 ++
> security/integrity/ima/ima_crypto.c | 7 +-
> security/integrity/ima/ima_fs.c | 135 +++++++++++++++++++++++++---
> 3 files changed, 131 insertions(+), 20 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..81318e294175 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -54,6 +54,15 @@ extern int ima_hash_algo __ro_after_init;
> extern int ima_sha1_idx __ro_after_init;
> extern int ima_hash_algo_idx __ro_after_init;
> extern int ima_extra_slots __ro_after_init;
> +
> +/* export hash algorithms configured in ima */
> +struct ima_algo_desc {
> + struct crypto_shash *tfm;
> + enum hash_algo algo;
> +};
> +
> +extern struct ima_algo_desc *ima_algo_array;
> +
> extern int ima_appraise;
> extern struct tpm_chip *ima_tpm_chip;
> extern const char boot_aggregate_name[];
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 51ad29940f05..d42ea0d350a1 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -57,11 +57,6 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
> static struct crypto_shash *ima_shash_tfm;
> static struct crypto_ahash *ima_ahash_tfm;
>
> -struct ima_algo_desc {
> - struct crypto_shash *tfm;
> - enum hash_algo algo;
> -};
> -
> int ima_sha1_idx __ro_after_init;
> int ima_hash_algo_idx __ro_after_init;
> /*
> @@ -70,7 +65,7 @@ int ima_hash_algo_idx __ro_after_init;
> */
> int ima_extra_slots __ro_after_init;
>
> -static struct ima_algo_desc *ima_algo_array;
> +struct ima_algo_desc *ima_algo_array;
>
> static int __init ima_init_ima_crypto(void)
> {
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index cd1683dad3bf..475ab368e32f 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -116,9 +116,13 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
> seq_putc(m, *(char *)data++);
> }
>
> +static struct dentry **ima_ascii_measurements_files;
> +static struct dentry **ima_binary_measurements_files;
> +static int ima_measurements_files_count;
> +
> /* print format:
> * 32bit-le=pcr#
> - * char[20]=template digest
> + * char[n]=template digest
> * 32bit-le=template name size
> * char[n]=template name
> * [eventdata length]
> @@ -130,9 +134,26 @@ int ima_measurements_show(struct seq_file *m, void *v)
> struct ima_queue_entry *qe = v;
> struct ima_template_entry *e;
> char *template_name;
> + struct dentry *dentry;
> u32 pcr, namelen, template_data_len; /* temporary fields */
> bool is_ima_template = false;
> - int i;
> + int i, algo_idx;
> + enum hash_algo algo;
> +
> + algo_idx = ima_sha1_idx;
> + algo = HASH_ALGO_SHA1;
> +
> + if (m->file != NULL) {
> + dentry = file_dentry(m->file);
> +
> + for (i = 0; i < ima_measurements_files_count; i++) {
> + if (dentry == ima_binary_measurements_files[i]) {
> + algo_idx = i;
> + algo = ima_algo_array[i].algo;
> + break;
> + }
> + }
> + }
Since you duplicate the same code below, I would put it in a separate
function, like lookup_algo_by_dentry().
> /* get entry */
> e = qe->entry;
> @@ -151,7 +172,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
> ima_putc(m, &pcr, sizeof(e->pcr));
>
> /* 2nd: template digest */
> - ima_putc(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
> + ima_putc(m, e->digests[algo_idx].digest, hash_digest_size[algo]);
>
> /* 3rd: template name size */
> namelen = !ima_canonical_fmt ? strlen(template_name) :
> @@ -220,7 +241,24 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
> struct ima_queue_entry *qe = v;
> struct ima_template_entry *e;
> char *template_name;
> - int i;
> + struct dentry *dentry;
> + int i, algo_idx;
> + enum hash_algo algo;
> +
> + algo_idx = ima_sha1_idx;
> + algo = HASH_ALGO_SHA1;
> +
> + if (m->file != NULL) {
> + dentry = file_dentry(m->file);
> +
> + for (i = 0; i < ima_measurements_files_count; i++) {
> + if (dentry == ima_ascii_measurements_files[i]) {
> + algo_idx = i;
> + algo = ima_algo_array[i].algo;
> + break;
> + }
> + }
> + }
>
> /* get entry */
> e = qe->entry;
> @@ -233,8 +271,8 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
> /* 1st: PCR used (config option) */
> seq_printf(m, "%2d ", e->pcr);
>
> - /* 2nd: SHA1 template hash */
> - ima_print_digest(m, e->digests[ima_sha1_idx].digest, TPM_DIGEST_SIZE);
> + /* 2nd: template hash */
> + ima_print_digest(m, e->digests[algo_idx].digest, hash_digest_size[algo]);
>
> /* 3th: template name */
> seq_printf(m, " %s", template_name);
> @@ -379,6 +417,71 @@ static const struct seq_operations ima_policy_seqops = {
> };
> #endif
>
> +static void remove_measurements_list_files(struct dentry **files)
> +{
> + int i;
> +
> + if (files) {
> + for (i = 0; i < ima_measurements_files_count; i++)
> + securityfs_remove(files[i]);
> +
> + kfree(files);
> + }
> +}
> +
> +static int create_measurements_list_files(void)
> +{
> + int i;
> + u16 algo;
> + char file_name[NAME_MAX+1];
> + struct dentry *dfile;
> + enum hash_algo sha1_algo_idx = -1;
> +
> + ima_measurements_files_count = NR_BANKS(ima_tpm_chip);
> +
> + if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
> + ima_measurements_files_count++;
> +
> + ima_ascii_measurements_files = kcalloc(ima_measurements_files_count,
> + sizeof(struct dentry *), GFP_KERNEL);
Second line not aligned?
Try scripts/checkpatch.pl --strict
It should tell about these issues.
> + if (!ima_ascii_measurements_files)
> + return -ENOMEM;
> +
> + ima_binary_measurements_files = kcalloc(ima_measurements_files_count,
> + sizeof(struct dentry *), GFP_KERNEL);
> + if (!ima_binary_measurements_files)
> + return -ENOMEM;
> +
> + for (i = 0; i < ima_measurements_files_count; i++) {
> + algo = ima_algo_array[i].algo;
> +
> + if (algo == HASH_ALGO_SHA1)
> + sha1_algo_idx = i;
> +
> + sprintf(file_name, "ascii_runtime_measurements_%s",
> + hash_algo_name[algo]);
Alignment.
> + dfile = securityfs_create_file(file_name,
> + S_IRUSR | S_IRGRP, ima_dir, NULL,
> + &ima_ascii_measurements_ops);
> + if (IS_ERR(dfile))
> + return PTR_ERR(dfile);
> +
> + ima_ascii_measurements_files[i] = dfile;
> +
> + sprintf(file_name, "binary_runtime_measurements_%s",
> + hash_algo_name[algo]);
> + dfile = securityfs_create_file(file_name,
> + S_IRUSR | S_IRGRP, ima_dir, NULL,
> + &ima_measurements_ops);
> + if (IS_ERR(dfile))
> + return PTR_ERR(dfile);
> +
> + ima_binary_measurements_files[i] = dfile;
> + }
> +
> + return 0;
> +}
> +
> /*
> * ima_open_policy: sequentialize access to the policy file
> */
> @@ -465,19 +568,21 @@ int __init ima_fs_init(void)
> goto out;
> }
>
> - binary_runtime_measurements =
> - securityfs_create_file("binary_runtime_measurements",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> - &ima_measurements_ops);
> + ret = create_measurements_list_files();
> + if (ret != 0)
> + goto out;
> +
> + binary_runtime_measurements = securityfs_create_symlink(
> + "binary_runtime_measurements", ima_dir,
> + "binary_runtime_measurements_sha1", NULL);
> if (IS_ERR(binary_runtime_measurements)) {
> ret = PTR_ERR(binary_runtime_measurements);
> goto out;
> }
>
> - ascii_runtime_measurements =
> - securityfs_create_file("ascii_runtime_measurements",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> - &ima_ascii_measurements_ops);
> + ascii_runtime_measurements = securityfs_create_symlink(
> + "ascii_runtime_measurements", ima_dir,
> + "ascii_runtime_measurements_sha1", NULL);
> if (IS_ERR(ascii_runtime_measurements)) {
> ret = PTR_ERR(ascii_runtime_measurements);
> goto out;
> @@ -515,6 +620,8 @@ int __init ima_fs_init(void)
> securityfs_remove(runtime_measurements_count);
> securityfs_remove(ascii_runtime_measurements);
> securityfs_remove(binary_runtime_measurements);
> + remove_measurements_list_files(ima_ascii_measurements_files);
> + remove_measurements_list_files(ima_binary_measurements_files);
Other than the issues above, looks good.
Did you try to dump non-SHA1 measurements after kexec?
Thanks
Roberto
> securityfs_remove(ima_symlink);
> securityfs_remove(ima_dir);
>
> base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
