On Sun, 2024-01-21 at 17:16 +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 algorothms corresponding to all the PCR banks
> configured in the TPM.
>
> This patch introduce 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
> for the PCR banks of the TPM. Each new file has the name with the following
> structure:
>
> {binary, ascii}_runtime_measurements_<hash_algo_name>
>
> except for sha1 which it remains associated with the standard file names.
> The <hash_algo_name> is used to select the template data hash algorithm to show
> in ima_ascii_measurements_show() and in ima_measurements_show().
>
> As example, in the case sha1 and sha256 are the configured PCR banks, the
> listing of the security/ima directory is the following:
>
> -r--r----- 1 root root 0 gen 20 15:06 ascii_runtime_measurements
> -r--r----- 1 root root 0 gen 20 15:06 ascii_runtime_measurements_sha256
> -r--r----- 1 root root 0 gen 20 15:06 binary_runtime_measurements
> -r--r----- 1 root root 0 gen 20 15:06 binary_runtime_measurements_sha256
> --w------- 1 root root 0 gen 20 15:06 policy
> -r--r----- 1 root root 0 gen 20 15:06 runtime_measurements_count
> -r--r----- 1 root root 0 gen 20 15:06 violations
>
> v2:
> - Changed the behaviour 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)
>
> Signed-off-by: Enrico Bravi <enrico.bravi@xxxxxxxxx>
> Signed-off-by: Silvia Sisinni <silvia.sisinni@xxxxxxxxx>
>
> ---
> security/integrity/ima/ima_fs.c | 164 ++++++++++++++++++++++++++++++--
> 1 file changed, 157 insertions(+), 7 deletions(-)
>
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index cd1683dad3bf..db57edeb112d 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -118,7 +118,7 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
>
> /* 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 +130,37 @@ int ima_measurements_show(struct seq_file *m, void *v)
> struct ima_queue_entry *qe = v;
> struct ima_template_entry *e;
> char *template_name;
> + const char *filename;
> + char algo_name[16];
> u32 pcr, namelen, template_data_len; /* temporary fields */
> bool is_ima_template = false;
> - int i;
> + int i, rc, algo_idx;
> + enum hash_algo algo;
> +
> + filename = m->file->f_path.dentry->d_name.name;
> + rc = sscanf(filename, "binary_runtime_measurements%s", algo_name);
> +
> + if (rc != 0) {
> + for (i = 0; i < HASH_ALGO__LAST; i++) {
> + if (!strcmp(algo_name + 1, hash_algo_name[i])) {
> + algo = i;
> + break;
> + }
> + }
> + if (i == HASH_ALGO__LAST)
> + algo = HASH_ALGO_SHA1;
> +
> + for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
> + if (algo == ima_tpm_chip->allocated_banks[i].crypto_id) {
> + algo_idx = i;
> + break;
> + }
> + }
> + }
Hi Enrico, Silvia
I would find more efficient if you create an array of dentries in the
same order as ima_tpm_chip->allocated_banks, so that you can compare
dentry addresses and find already the right index.
> + else {
> + algo_idx = ima_sha1_idx;
> + algo = HASH_ALGO_SHA1;
> + }
>
> /* get entry */
> e = qe->entry;
> @@ -151,7 +179,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 +248,35 @@ 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;
> + const char *filename;
> + char algo_name[16];
> + int i, algo_idx, rc;
> + enum hash_algo algo;
> +
> + filename = m->file->f_path.dentry->d_name.name;
> + rc = sscanf(filename, "ascii_runtime_measurements%s", algo_name);
> +
> + if (rc != 0) {
> + for (i = 0; i < HASH_ALGO__LAST; i++) {
> + if (!strcmp(algo_name + 1, hash_algo_name[i])) {
> + algo = i;
> + break;
> + }
> + }
> + if (i == HASH_ALGO__LAST)
> + algo = HASH_ALGO_SHA1;
> +
> + for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
> + if (algo == ima_tpm_chip->allocated_banks[i].crypto_id) {
> + algo_idx = i;
> + break;
> + }
> + }
> + }
Same.
> + else {
> + algo_idx = ima_sha1_idx;
> + algo = HASH_ALGO_SHA1;
> + }
>
> /* get entry */
> e = qe->entry;
> @@ -233,8 +289,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);
> @@ -363,6 +419,8 @@ static struct dentry *ascii_runtime_measurements;
> static struct dentry *runtime_measurements_count;
> static struct dentry *violations;
> static struct dentry *ima_policy;
> +static struct dentry **ima_ascii_measurements_files;
> +static struct dentry **ima_binary_measurements_files;
>
> enum ima_fs_flags {
> IMA_FS_BUSY,
> @@ -379,6 +437,31 @@ static const struct seq_operations ima_policy_seqops = {
> };
> #endif
>
> +/*
> + * Remove the securityfs files created for each hash algo configured
> + * in the TPM, excluded ascii_runtime_measurements and
> + * binary_runtime_measurements.
> + */
> +static void remove_measurements_list_files(void)
> +{
> + int i;
> +
> + for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
> + if (ima_ascii_measurements_files[i]) {
> + securityfs_remove(ima_ascii_measurements_files[i]);
> + kfree(ima_ascii_measurements_files[i]);
> + }
> +
> + if (ima_binary_measurements_files[i]) {
> + securityfs_remove(ima_binary_measurements_files[i]);
> + kfree(ima_binary_measurements_files[i]);
> + }
> + }
> +
> + kfree(ima_ascii_measurements_files);
> + kfree(ima_binary_measurements_files);
Oh, you actually put them in a array and order the elements by PCR
bank.
> +}
> +
> /*
> * ima_open_policy: sequentialize access to the policy file
> */
> @@ -452,7 +535,10 @@ static const struct file_operations ima_measure_policy_ops = {
>
> int __init ima_fs_init(void)
> {
> - int ret;
> + int ret, i;
> + u16 algo;
> + char file_name[50];
> + struct dentry *dfile;
>
> ima_dir = securityfs_create_dir("ima", integrity_dir);
> if (IS_ERR(ima_dir))
> @@ -483,6 +569,69 @@ int __init ima_fs_init(void)
> goto out;
> }
>
> + /*
> + * Allocate a file in the securityfs for each hash algo configured
> + * in the TPM but sha1 (for ascii and binary output).
> + */
> + if (ima_tpm_chip) {
> +
> + ima_ascii_measurements_files = kmalloc_array(NR_BANKS(ima_tpm_chip),
> + sizeof(struct dentry *), GFP_KERNEL);
Since you added a function for freeing the arrays, I would do the same
for adding.
> + if(ima_ascii_measurements_files == NULL) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ima_binary_measurements_files = kmalloc_array(NR_BANKS(ima_tpm_chip),
> + sizeof(struct dentry *), GFP_KERNEL);
> + if(ima_binary_measurements_files == NULL) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
> + algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> +
> + /* Skip sha1 */
> + if (algo == HASH_ALGO_SHA1)
> + continue;
I would go ahead, create also the dentry for SHA1 and add a symbolic
link for the legacy files.
> +
> + dfile = kmalloc(sizeof(struct dentry), GFP_KERNEL);
> + if (!dfile) {
> + ret = -ENOMEM;
> + goto out;
> + }
I don't remember if the lines above are really necessary. You actually
overwrite the pointer below.
Thanks
Roberto
> +
> + sprintf(file_name, "ascii_runtime_measurements_%s",
> + hash_algo_name[algo]);
> + dfile = securityfs_create_file(file_name,
> + S_IRUSR | S_IRGRP, ima_dir, NULL,
> + &ima_ascii_measurements_ops);
> + if (IS_ERR(dfile)) {
> + ret = PTR_ERR(dfile);
> + goto out;
> + }
> + ima_ascii_measurements_files[i] = dfile;
> +
> + dfile = kmalloc(sizeof(struct dentry), GFP_KERNEL);
> + if (!dfile) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + 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)) {
> + ret = PTR_ERR(dfile);
> + goto out;
> + }
> + ima_binary_measurements_files[i] = dfile;
> + }
> + }
> +
> runtime_measurements_count =
> securityfs_create_file("runtime_measurements_count",
> S_IRUSR | S_IRGRP, ima_dir, NULL,
> @@ -515,6 +664,7 @@ 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();
> securityfs_remove(ima_symlink);
> securityfs_remove(ima_dir);
>
> base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
