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

From: Andrew Morton
Date: Wed May 28 2008 - 04:29:10 EST


On Fri, 23 May 2008 11:05:45 -0400 Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> 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.

- A complicated-looking in-kernel string parser which is implementing
an new and apparently-undocumented user->kernel ABI.

- Some GFP_ATOMICs which can hopefully become GFP_KERNEL.

- timespec_set() is unneeeded - just use struct assignment (ie: "=")

- timespec_recent() looks a bit hacky. The problems which are being
solved here should be described in the changelog. Perhaps we can
think of a better way, but first we have to know about it.

- shouldn't ima_inode_init() initialise tv_usec too?

- All the games with mtimes should be described in the changelog too.

- All the `static struct integrity_operations' instances could be
made const. And lots of other foo_operations too, I expect.

That will lead to a constification chase all over the place, but
it's probably for the best. This is after all a "security" feature
and there is perhaps some benefit in getting your eminently
hijackable function pointers into read-only memory.

- 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.

- bug: ima_fixup_inodes() does a GFP_KERNEL allocation inside
inode->i_lock. This bug shouldn't have got this far. Please always
enable all kernel debugging features when testing code.
Documentation/SubmitChecklist has useful things.

- inode.i_lock is defined as an innermost lock which is used for
protecting data internal to the inode. You appear to be using it for
way too much stuff in here.

- It would be useful to add a comment explaining why
late_initcall(init_ima) is using late_initcall() rather than plain
old module_init(). Because it is impossible for the reader to
determine this information from the implementation.

- mutex_init(&ima_extend_list_mutex) is unneeded.

- Does ima_add_digest_entry() need to use the unreliable GFP_ATOMIC?

This matters. This is a security feature and if that
kmalloc(GFP_ATOMIC) fails (as it easily can do) then I expect the
system will either be insecure or will outright malfunction.

- Why does CONFIG_IMA_BOOTPARAM exist, and can it be removed (ie:
made unconditional)?

- Similarly CONFIG_IMA_BOOTPARAM_VALUE. Let's be decisive here -
distributors only get one shot at setting these things.

- mode_setup() will identify itself as "mode_setup" in its KERN_INFO
printk. That's a bit unhelpful. I'd suggest that all/most printks
here be prefixed with "integrity:".

- GFP_ATOMICs everywhere :(

- As ima_htable.violations "can overflow", atomic_long_t might be a
better choice of type.

- skip_measurement(): the hard-coded test for PROC_SUPER_MAGIC,
SYSFS_MAGIC etc is quite unpleasant. Surely there is a better way.

-
+/**
+ * ima_must_measure - measure decision based on policy.
+ * @d - pointer to struct ima_data containing ima_args_data

So if we know the type of d, did we _have_ to make it void*? It's
much better to use the C yype system if at all possible.

- ditto ima_collect_measurement()

Generally: the code is all moderately intrusive into the VFS and this
sort of thing does need careful explanation and justification, please.
Once we have some understanding of what you're trying to achieve here
we will inevitably ask "can't that be done in userspace". So it would
be best if your description were to preemptively answer all that.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/