Hi Roberto,
thanks a lot for your quick feedback.
On 22/01/24 09:20, Roberto Sassu wrote:
> 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.
Your are absolutely right, there is no need of two loops.
>> + 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.
Sure.
>> + 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.
ima_ascii_measurements_files and ima_binary_measurements_files are allocated
just in the case a tpm chip is detected. What you are suggesting is to allocate
in any case these lists, with at least one element, and creating the legacy file
always as symbolic links? Or to define them as symbolic links only in the case a
tpm chip is detected, otherwise creating them as usual?
>> +
>> + 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.
Yes these lines are definitely not necessary.
Thanks a lot,
Enrico
>
>> +
>> + 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
