On Mon, 2024-01-22 at 18:41 +0100, Enrico Bravi wrote:
> 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 <[email protected]>
> > > Signed-off-by: Silvia Sisinni <[email protected]>
> > >
> > > ---
> > > 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?
Hi Enrico
I would keep the same scheme, even if there is no TPM chip. So SHA1
files, plus symbolic links in this case.
Thanks
Roberto
> > > +
> > > + 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
