Re: [PATCH 3/6] integrity: IMA as an integrity service provider

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Wed, 2008-12-03 at 13:17 -0500, Mimi Zohar wrote:
> On Tue, 2008-12-02 at 15:35 -0800, Dave Hansen wrote: 
> > > +config IMA_MEASURE_PCR_IDX
> > > +	int "PCR for Aggregate (8 <= Index <= 14)"
> > > +	depends on IMA
> > > +	range 8 14
> > > +	default 10
> > > +	help
> > > +	  IMA_MEASURE_PCR_IDX determines the TPM PCR register index
> > > +	  that IMA uses to maintain the integrity aggregate of the
> > > +	  measurement list.  If unsure, use the default 10.
> > 
> > Why would you want to change this?  Can it be done at runtime instead of
> > compile time?  I don't know what a PCR is.
> 
> The only reason to change it would be if in the future, TCG decides on a
> standard PCR for IMA, other than 10, or if they pick 10 for something
> else. We really don't need a runtime variable for this, but kconfig
> makes it easy to change once if necessary in the future.

OK.  Could you take out the prompt for now?  You can use Kconfig for
values that don't give user prompts.  I just don't think it is something
that people need to see.

in mm/Kconfig, for instance:

config NR_QUICK
        int
        depends on QUICKLIST
        default "2" if SUPERH || AVR32
        default "1"

> > > +int ima_iint_insert(struct inode *inode)
> > > +{
> > > +	struct ima_iint_cache *iint;
> > > +	int rc = 0;
> > > +
> > > +	iint = kzalloc(sizeof(*iint), GFP_KERNEL);
> > 
> > Does this basically get done for every inode, or only special ones?  I
> > just wonder if having a dedicated slab with a constructor to do
> > redundant things like mutex_init() would be helpful.
> 
> every inode, except those allocated before init_latecall.

I'd be willing to bet that you'll see a measurable performance
improvement if you decide to use a slab here.  All of the inodes for the
different fs's use slabs and these are at least as common as any single
fs's inode.  Also, using the con/destructors will save some work at each
object creation.

> > > +static void ima_add_boot_aggregate(void)
> > > +{
> > > +	struct ima_inode_measure_entry measure_entry;
> > > +	struct ima_store_template_data template = {
> > > +		.name = "ima",
> > > +		.len = sizeof(measure_entry),
> > > +		.data = (char *)&measure_entry,
> > > +	};
> > > +	int namelen, result;
> > > +
> > > +	memset(&measure_entry, 0, sizeof measure_entry);
> > > +	namelen = strlen(boot_aggregate_name);
> > > +	if (namelen > IMA_EVENT_NAME_LEN_MAX)
> > > +		namelen = IMA_EVENT_NAME_LEN_MAX;
> > > +	memcpy(measure_entry.file_name, boot_aggregate_name, namelen);
> > > +
> > > +	if (ima_used_chip) {
> > > +		int i;
> > > +		u8 pcr_i[IMA_DIGEST_SIZE];
> > > +		struct hash_desc desc;
> > > +		struct crypto_hash *tfm;
> > > +		struct scatterlist sg;
> > 
> > All of this stack stuff with very important, large sounding names makes
> > me nervous.  Can you reassure me?
> 
> The crypto code here will be moved to ima_crypto.c and will be
> refactored, cleaning up the code. Both measure_entry and template could
> be allocated/freed each time, but does that make sense?

That's reassuring, thanks. :)

-- Dave