| Message ID | 20240308104953.743847-1-enrico.bravi@polito.it (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] ima: add crypto agility support for template-hash algorithm | expand |
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@polito.it> > Signed-off-by: Silvia Sisinni <silvia.sisinni@polito.it> > > --- > > 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
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. > > 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@polito.it> > Signed-off-by: Silvia Sisinni <silvia.sisinni@polito.it> > > --- > > 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; The variable naming isn't quite right. It's defined as a 'struct dentry', but the name is '*_files'. Why not just name the variables 'ima_{ascii, binary} _measurements'? Variables that shouldn't change after '__init' should be annotated as __ro_after_init; > +static int ima_measurements_files_count; Removing '_files', in this case, from the variable name could be confused with the number of measurements. Perhaps rename the variable to ima_measurement_list_count or ima_securityfs_measurement_list_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; > + } > + } > + } > > /* 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) And remove '_files' from the function name. Perhaps rename it remove_measurement_lists or remove_securityfs_measurement_lists. > +{ > + 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) And remove '_files' from the function name. Perhaps rename it to create_measurement_lists or create_securityfs_measurement_lists. > +{ > + int i; > + u16 algo; > + char file_name[NAME_MAX+1]; > + struct dentry *dfile; Use 'dentry', if needed. > + 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); > + 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]); > + 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) The new functions should be annotated with '__init' as well. > 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); Please use scripts/checkpatch.pl to check formatting. > 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( And here ... thanks, Mimi > + "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); > securityfs_remove(ima_symlink); > securityfs_remove(ima_dir); > > base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
On 18/03/24 09:25, Roberto Sassu wrote: > 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. Hi Roberto, thanks for the correction. (more below) >> 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@polito.it> >> Signed-off-by: Silvia Sisinni <silvia.sisinni@polito.it> >> >> --- >> >> 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(). Thank you for the suggestion, we will wrap that logic in a function. >> /* 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. Thank you, we will fix the alignment. > >> + 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? Yes, the porting of the list seems to work fine. I put an example with the following configuration: - TPM 2.0 with only SHA256 bank configured - ima hash algo before kexec: sha1 - ima hash algo after kexec: sha384 Here it is the dump of the ascii_runtime_measurements_sha256 list: 10 8328[..]a07e ima-ng sha256:065c[..]8023 boot_aggregate 10 afd6[..]3123 ima-ng sha1:5a49[..]9566 /init 10 9932[..]6353 ima-ng sha1:8c87[..]d8c7 /usr/bin/sh ... 10 58e3[..]9996 ima-ng sha1:f969[..]ad47 /etc/pam.d/sudo 10 f85f[..]a2f2 ima-ng sha1:8525[..]a168 /usr/sbin/kexec 10 01bb[..]e97f ima-ng sha1:235a[..]8ddb /boot/vmlinuz-6.8.0-dirty 10 64c9[..]e5c8 ima-ng sha1:7341[..]cd48 /boot/initrd.img-6.8.0-dirty 10 8328[..]a07e ima-ng sha256:065c[..]8023 boot_aggregate 10 93ca[..]0de0 ima-ng sha384:4d80[..]f38b /init 10 a240[..]7f27 ima-ng sha384:8dd5[..]6f31 /usr/bin/sh ... > Thanks Thank you for your feedback and suggestions. Enrico > Roberto > >> securityfs_remove(ima_symlink); >> securityfs_remove(ima_dir); >> >> base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
On 18/03/24 14:05, Mimi Zohar wrote: > 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. >> >> 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@polito.it> >> Signed-off-by: Silvia Sisinni <silvia.sisinni@polito.it> >> >> --- >> >> 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; > > The variable naming isn't quite right. It's defined as a 'struct dentry', but > the name is '*_files'. Why not just name the variables 'ima_{ascii, binary} > _measurements'? Hi Mimi, thank you for pointing that out. What do you think of naming them 'ima_{ascii, binary}_securityfs_measurement_lists', to have also coherence with the names of the new functions defined. > Variables that shouldn't change after '__init' should be annotated as > __ro_after_init; Thanks we will fix it. >> +static int ima_measurements_files_count; > > Removing '_files', in this case, from the variable name could be confused with > the number of measurements. Perhaps rename the variable to > ima_measurement_list_count or ima_securityfs_measurement_list_count. I think that 'ima_securityfs_measurement_list_count' is better, just to point out that is the number of entries in the securityfs. >> + >> /* 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; >> + } >> + } >> + } >> >> /* 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) > > And remove '_files' from the function name. Perhaps rename it > remove_measurement_lists or remove_securityfs_measurement_lists. > >> +{ >> + 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) > > And remove '_files' from the function name. Perhaps rename it to > create_measurement_lists or create_securityfs_measurement_lists. I think that keeping this structure for the names 'remove_securityfs_measurement_lists' and 'create_securityfs_measurement_lists' makes sense. >> +{ >> + int i; >> + u16 algo; >> + char file_name[NAME_MAX+1]; >> + struct dentry *dfile; > > Use 'dentry', if needed.> >> + 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); >> + 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]); >> + 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) > > The new functions should be annotated with '__init' as well. Yes you are right, we will add it. >> 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); > > Please use scripts/checkpatch.pl to check formatting. Yes thank you, we will fix the format. >> 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( > > And here ... > > thanks, Thanks to you for your valuable feedback. Enrico > Mimi > >> + "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); >> securityfs_remove(ima_symlink); >> securityfs_remove(ima_dir); >> >> base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
> > > 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; > > > > The variable naming isn't quite right. It's defined as a 'struct dentry', > > but > > the name is '*_files'. Why not just name the variables 'ima_{ascii, binary} > > _measurements'? > > Hi Mimi, Hi Enrico, > thank you for pointing that out. What do you think of naming them 'ima_{ascii, > binary}_securityfs_measurement_lists', to have also coherence with the names > of > the new functions defined. As these are static variables, prefixing them with 'ima_' isn't necessary. Either way is fine. > > > +static void remove_measurements_list_files(struct dentry **files) > > > > And remove '_files' from the function name. Perhaps rename it > > remove_measurement_lists or remove_securityfs_measurement_lists. > > > > > +{ > > > + 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) > > > > And remove '_files' from the function name. Perhaps rename it to > > create_measurement_lists or create_securityfs_measurement_lists. > > I think that keeping this structure for the names > 'remove_securityfs_measurement_lists' and > 'create_securityfs_measurement_lists' > makes sense. Agreed. thanks, Mimi
On 20/03/24 13:07, Mimi Zohar wrote: > >>>> 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; >>> >>> The variable naming isn't quite right. It's defined as a 'struct dentry', >>> but >>> the name is '*_files'. Why not just name the variables 'ima_{ascii, binary} >>> _measurements'? >> >> Hi Mimi, > > Hi Enrico, > >> thank you for pointing that out. What do you think of naming them 'ima_{ascii, >> binary}_securityfs_measurement_lists', to have also coherence with the names >> of >> the new functions defined. > > As these are static variables, prefixing them with 'ima_' isn't necessary. > Either way is fine. Hi Mimi, perfect, in this way they would be even shorter. Thank you, Enrico >>>> +static void remove_measurements_list_files(struct dentry **files) >>> >>> And remove '_files' from the function name. Perhaps rename it >>> remove_measurement_lists or remove_securityfs_measurement_lists. >>> >>>> +{ >>>> + 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) >>> >>> And remove '_files' from the function name. Perhaps rename it to >>> create_measurement_lists or create_securityfs_measurement_lists. >> >> I think that keeping this structure for the names >> 'remove_securityfs_measurement_lists' and >> 'create_securityfs_measurement_lists' >> makes sense. > > Agreed. > > thanks, > > Mimi
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; + } + } + } /* 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); + 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]); + 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); securityfs_remove(ima_symlink); securityfs_remove(ima_dir);
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. 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@polito.it> Signed-off-by: Silvia Sisinni <silvia.sisinni@polito.it> --- 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(-) base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b