Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <23ac7b16909bd9b86e7a65ade6a526bf46bdb734.camel@linux.ibm.com>
Subject: Re: [PATCH v6 3/5] ima: permit fsverity's file digests in the IMA
 measurement list
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Stefan Berger <stefanb@linux.ibm.com>,
        linux-integrity@vger.kernel.org
Cc:     Eric Biggers <ebiggers@kernel.org>, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Mon, 21 Mar 2022 15:54:57 -0400
In-Reply-To: <1e9263cb-8a3e-a3d8-01c8-0893934f8235@linux.ibm.com>
References: <20220318182151.100847-1-zohar@linux.ibm.com>
         <20220318182151.100847-4-zohar@linux.ibm.com>
         <1e9263cb-8a3e-a3d8-01c8-0893934f8235@linux.ibm.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2022-03-21 at 08:59 -0400, Stefan Berger wrote:

> > +/*
> > + * Make sure the policy rule and template format are in sync.
> If they are not in sync I need to update my policy rule?

It doesn't prevent loading the policy, if they're not in sync, but
simply issues a warning.

> 
> > + */
> > +static void check_template_field(const struct ima_template_desc *template,
> > +				 const char *field, const char *msg)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < template->num_fields; i++)
> > +		if (!strcmp(template->fields[i]->field_id, field))
> > +			return;
> > +
> > +	pr_notice_once("%s", msg)
> > +}
> > +
> >   static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   {
> >   	/* Ensure that the action is set and is compatible with the flags */
> > @@ -1215,7 +1232,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   				     IMA_INMASK | IMA_EUID | IMA_PCR |
> >   				     IMA_FSNAME | IMA_GID | IMA_EGID |
> >   				     IMA_FGROUP | IMA_DIGSIG_REQUIRED |
> > -				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS))
> > +				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS |
> > +				     IMA_VERITY_REQUIRED))
> >   			return false;
> >   
> >   		break;
> > @@ -1708,6 +1726,13 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >   						   LSM_SUBJ_TYPE,
> >   						   AUDIT_SUBJ_TYPE);
> >   			break;
> > +		case Opt_digest_type:
> > +			ima_log_string(ab, "digest_type", args[0].from);
> > +			if ((strcmp(args[0].from, "verity")) == 0)
> > +				entry->flags |= IMA_VERITY_REQUIRED;
> > +			else
> > +				result = -EINVAL;
> > +			break;
> >   		case Opt_appraise_type:
> >   			ima_log_string(ab, "appraise_type", args[0].from);
> >   			if ((strcmp(args[0].from, "imasig")) == 0)
> > @@ -1798,6 +1823,15 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >   		check_template_modsig(template_desc);
> >   	}
> >   
> > +	/* d-ngv2 template field recommended for unsigned fs-verity digests */
> > +	if (!result && entry->action == MEASURE &&
> > +	    entry->flags & IMA_VERITY_REQUIRED) {
> > +		template_desc = entry->template ? entry->template :
> > +						  ima_template_desc_current();
> > +		check_template_field(template_desc, "d-ngv2",
> > +				     "verity rules should include d-ngv2");
> > +	}
> > +
> >   	audit_log_format(ab, "res=%d", !result);
> >   	audit_log_end(ab);
> >   	return result;
> > @@ -2155,6 +2189,8 @@ int ima_policy_show(struct seq_file *m, void *v)
> >   		else
> >   			seq_puts(m, "appraise_type=imasig ");
> >   	}
> > +	if (entry->flags & IMA_VERITY_REQUIRED)
> > +		seq_puts(m, "digest_type=verity ");
> >   	if (entry->flags & IMA_CHECK_BLACKLIST)
> >   		seq_puts(m, "appraise_flag=check_blacklist ");
> >   	if (entry->flags & IMA_PERMIT_DIRECTIO)
> > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> > index bd95864a5f6f..0cff6658a4c2 100644
> > --- a/security/integrity/ima/ima_template_lib.c
> > +++ b/security/integrity/ima/ima_template_lib.c
> > @@ -31,7 +31,7 @@ enum data_formats {
> >   };
> >   
> >   #define DIGEST_TYPE_MAXLEN 16	/* including NULL */
> > -static const char * const digest_type_name[] = {"ima"};
> > +static const char * const digest_type_name[] = {"ima", "verity"};
> >   static int digest_type_size = ARRAY_SIZE(digest_type_name);
> 
> if this static needs to exist at all, and I dn't think it should, it 
> should probably be called digest_type_array_size. Though I would just 
> use ARRAY_SIZE() where needed.

Ok.

thanks,

Mimi