On Thu, 2024-01-25 at 09:30 +0100, Roberto Sassu wrote:
> On Wed, 2024-01-24 at 20:32 +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.
>
> Hi Enrico
>
> the missing part is that we should not modify existing files, to avoid
> breaking existing applications.
Forgot:
please use scripts/get_maintainer.pl to find the correct recipients of
the patch (I think you only need to include maintainers and reviewers).
Thanks
Roberto
> > 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
> > for the PCR banks of the TPM. Each new file has the name with the following
> > structure:
> >
> > {binary, ascii}_runtime_measurements_<hash_algo_name>
> >
> > 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().
> > Legacy files are kept but as sysmbolic links which point to
>
> Typo. Please use codespell on the patch.
>
> > {binary, ascii}_runtime_measurements_sha1 files. These two files are created
> > even if a TPM chip is not detected.
> >
> > As example, in the case a TPM chip is present and sha1 and sha256 are the
> > configured PCR banks, 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
>
> Ok, great.
>
> > Signed-off-by: Enrico Bravi <enrico.bravi@xxxxxxxxx>
> > Signed-off-by: Silvia Sisinni <silvia.sisinni@xxxxxxxxx>
> >
> > ---
> >
> > v3:
> > - Added create_measurements_list_files function for measurements files creation.
> > - Parametrized 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_mesurements_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 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)
> >
> > security/integrity/ima/ima_fs.c | 145 +++++++++++++++++++++++++++++---
> > 1 file changed, 131 insertions(+), 14 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index cd1683dad3bf..fb65ba9426a1 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -116,9 +116,12 @@ 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;
> > +
> > /* 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 +133,25 @@ 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;
> > +
> > + dentry = m->file->f_path.dentry;
>
> I like more file_dentry(m->file).
>
> > + algo_idx = ima_sha1_idx;
> > + algo = HASH_ALGO_SHA1;
> > +
> > + if (ima_tpm_chip) {
> > + for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
> > + if (dentry == ima_binary_measurements_files[i]) {
> > + algo_idx = i;
> > + algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> > + break;
> > + }
> > + }
> > + }
> >
> > /* get entry */
> > e = qe->entry;
> > @@ -151,7 +170,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 +239,23 @@ 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;
> > +
> > + dentry = m->file->f_path.dentry;
>
> Same.
>
> > + algo_idx = ima_sha1_idx;
> > + algo = HASH_ALGO_SHA1;
> > +
> > + if (ima_tpm_chip) {
> > + for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
> > + if (dentry == ima_ascii_measurements_files[i]) {
>
> Uhm, why not iterating over ima_ascii_measurements_files?
>
> But I have another comment on this.
>
> Have a look at ima_crypto.c:
>
> if (ima_sha1_idx < 0) {
> ima_sha1_idx = NR_BANKS(ima_tpm_chip) + ima_extra_slots++;
> if (ima_hash_algo == HASH_ALGO_SHA1)
> ima_hash_algo_idx = ima_sha1_idx;
> }
>
> This is done because not necessarily the TPM has a SHA1 bank.
>
> ima_extra_slots is already exported, you could use that to determine if
> you need more slots than NR_BANKS(ima_tpm_chip).
>
> > + algo_idx = i;
> > + algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> > + break;
> > + }
> > + }
> > + }
> >
> > /* get entry */
> > e = qe->entry;
> > @@ -233,8 +268,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 +414,84 @@ 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.
> > + */
>
> It does not hurt following the kernel-doc format.
>
> > +static void remove_measurements_list_files(struct dentry **files)
> > +{
> > + int i, len;
> > + len = ima_tpm_chip ? NR_BANKS(ima_tpm_chip) : 1;
>
> It is much better having a global variable with the number of array
> elements.
>
> > +
> > + if (files)
> > + {
>
> This bracket should be one line up.
>
> > + for (i = 0; i < len; i++) {
> > + if (files[i]) {
> > + securityfs_remove(files[i]);
> > + }
> > + }
> > +
> > + kfree(files);
> > + }
> > +}
> > +
> > +/*
> > + * Allocate a file in the securityfs for each hash algo configured
> > + * in the TPM (for ascii and binary output). In case no TPM chip is
> > + * detected only sha1 files are created.
> > + */
> > +static int create_measurements_list_files(void)
> > +{
> > + int i, len;
> > + u16 algo;
> > + char file_name[NAME_MAX+1];
> > + struct dentry *dfile;
> > +
> > + /*
> > + * Allocate NR_BANKS(ima_tpm_chip) files in case a tpm chip is detected,
> > + * otherwise allocate just one file for sha1.
> > + */
> > + len = ima_tpm_chip ? NR_BANKS(ima_tpm_chip) : 1;
>
> See the comment above regarding ima_extra_slots.
>
> If you export len (static), you can always use that without extra
> logic.
>
> > +
> > + ima_ascii_measurements_files = kmalloc_array(len,
> > + sizeof(struct dentry *), GFP_KERNEL);
> > + if(!ima_ascii_measurements_files)
> > + return -ENOMEM;
> > +
> > + ima_binary_measurements_files = kmalloc_array(len,
> > + sizeof(struct dentry *), GFP_KERNEL);
> > + if(!ima_binary_measurements_files)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < len; i++) {
> > + algo = ima_tpm_chip ? ima_tpm_chip->allocated_banks[i].crypto_id :
> > + HASH_ALGO_SHA1;
>
> I'm starting to think that maybe we should export ima_algo_array
> instead, and follow that.
>
> > +
> > + 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 +578,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 +630,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);
>
> The rest is ok.
>
> Thanks
>
> Roberto
>
> > securityfs_remove(ima_symlink);
> > securityfs_remove(ima_dir);
> >
> > base-commit: 88035e5694a86a7167d490bb95e9df97a9bb162b
