[lkml]   [2008]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [RFC][Patch 5/5]integrity: IMA as an integrity service provider
    On Fri, 23 May 2008 11:05:45 -0400 Mimi Zohar <> 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.

     \ /
      Last update: 2008-05-28 10:31    [W:0.030 / U:4.520 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site