Re: [RFC][Patch 5/5]integrity: IMA as an integrity service provider

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 28 May 2008 23:17:26 -0400 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Wed, 2008-05-28 at 01:22 -0700, Andrew Morton wrote: 
> > On Fri, 23 May 2008 11:05:45 -0400 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > This is a re-release of Integrity Measurement Architecture(IMA) as an
> > > independent Linunx Integrity Module(LIM) service provider, which implements
> > > the new LIM must_measure(), collect_measurement(), store_measurement(), and
> > > display_template() API calls. The store_measurement() call supports two 
> > > types of data, IMA (i.e. file data) and generic template data.
> > > 
> > > When store_measurement() is called for the IMA type of data, the file 
> > > measurement and the file name hint are used to form an IMA template.
> > > IMA then calculates the IMA template measurement(hash) and submits it 
> > > to the TPM chip for inclusion in one of the chip's Platform Configuration 
> > > Registers (PCR).  
> > > 
> > > When store_measurement() is called for generic template data, IMA 
> > > calculates the measurement(hash) of the template data, and submits 
> > > the template measurement to the TPM chip for inclusion in one of the
> > > chip's Platform Configuration Registers(PCR).
> > > 
> > > In order to view the contents of template data through securityfs, the
> > > template_display() function must be defined in the registered 
> > > template_operations.  In the case of the IMA template, the list of 
> > > file names and files hashes submitted can be viewed through securityfs. 
> > > 
> > > IMA can be included or excluded in the kernel configuration.  If
> > > included in the kernel and the IMA_BOOTPARAM is selected, IMA can 
> > > also be enabled/disabled on the kernel command line with 'ima='.
> > > 
> > 
> > - I see lots of user file I/O being done from within the kernel. 
> >   This makes eyebrows raise.  Also some other eyebrow-raising
> >   file-related things in there.
> 
> The amount of I/O is dependent on the number of files being measured.
> The default policy measures a whole lot.  An LSM specific integrity 
> policy would cut down on the number of files being measured. For now,
> either remove the third rule in default_rules or replace the default
> rules with a new policy. To load a new policy execute:
> 	./integrity_load < policy

The problem is that the code is doing in-kernel user file I/O *at all*.
It's a red flag.

Look who else is using kernel_read(): just the exec code.  Plus
something in v9fs which I'd better not look at.

>
> ...
>
> > - timespec_set() is unneeeded - just use struct assignment (ie: "=")
> 
> Am confused.  timespec_set is doing an assignment. Should I
> replace timespec_set with a memcpy?

	struct timespec a, b;

	a = b;
> 
> > - All the games with mtimes should be described in the changelog too.
> 
> Ok. The timespec_recent and mtime issues are part of the same problem
> of detecting when a file has been modified.

Can't use inode.i_version?

> 
> > - ima_fixup_inodes looks like it will race and crash against a
> >   well-timed unmount.  I expect you will need to bump s_count before
> >   dropping sb_lock.  See writeback_inodes() for an example.
> 
> ima_fixup_inodes() is called once at initialization.  

What is "initialisation"?  During initcalls?  Are there even any files
in cache at that time?  I bet we can arrange for the answer to become
"no".