On Tue, Nov 15, 2005 at 09:09:05AM -0500, David Safford wrote:
> + * Revision history:
> + *
> + * 2003/10/29 initial implementation Seiji Munetoh
> + * Adding experimental EventLog support code.
> + * Requirements
> + * - Kernel was configured to support ACPI (CONFIG_ACPI=y)
> + * Other ACPI config options should be 'n'.
> + *
> + * EventLog Initialization
> + * Post-O/S EventLog is stored in the ACPI table. At first we retrive this
> + * table with ACPI driver's help.
> + *
> + * Size of theEventLog
> + * ACPI table size is 0x10000(64K) bytes.
> + * Minimum size of event is 32 bytes, so this hold max 2048 events.
> + * For a while, this is enough size!?
> + * 'tcg_eventlog' holds the copy of ACPI table and new EventLogs.
> + *
> + * Stefan Berger 07/2005
> + * - adapt it to IMA
> + *
> + * Reiner Sailer 08/2005
> + * - major rewrite, move to seq file / security fs
Revision history does not belong in the file itself, that's what the
ChangeLog is for in the git repository.
> +#include <linux/config.h>
> +
> +#ifdef CONFIG_ACPI
Just refuse to build this file if CONFIG_ACPI is not enabled. That
logic belongs in the Makefile, not in the .c file.
> +#include <linux/version.h>
Do you really need version.h?
> +/*
> +
> + TCPA/TCG Eventlog
> +
> + EventLog is stored in ACPI table. Current linux ACPI driver
> + does not support the TCPA table under RSDT. and I/F functions
> +
> + driver/acpi/tables/tbxfroot.c
> + *****************************************************************************
> + *
> + * FUNCTION: Acpi_get_firmware_table
> + *
> + * PARAMETERS: Signature - Any ACPI table signature
> + * Instance - the non zero instance of the table, allows
> + * support for multiple tables of the same type
> + * Flags - 0: Physical/Virtual support
> + * Ret_buffer - pointer to a structure containing a buffer to
> + * receive the table
> + *
> + * RETURN: Status
> + *
> + * DESCRIPTION: This function is called to get an ACPI table. The caller
> + * supplies an Out_buffer large enough to contain the entire ACPI
> + * table. Upon completion
> + * the Out_buffer->Length field will indicate the number of bytes
> + * copied into the Out_buffer->Buf_ptr buffer. This table will be
> + * a complete table including the header.
> + *
> + ******************************************************************************/
Oh my. Please read the proper way to document a kernel function using
kernel-doc (hint, it's not this way...)
> +#define ACPI_TCPA_SIG "TCPA" /* 0x41504354 /'TCPA' */
> +
> +#define PACK __attribute__ ((__packed__))
No, just spell it out where it is used please.
> +struct tcpa_event {
> + u32 PCRIndex;
> + u32 eventType;
> + u8 PCRValue[20]; /* SHA1 */
> + u32 eventSize;
> + /* (eventSize) bytes of event data follows */
Then use a u8 event_data[0]; type thing to show that.
> +/* (Binary) bios measurement buffer */
> +static void *tcg_eventlog = NULL;
> +static void *tcg_eventlog_addr_limit = NULL; /* MAX */
Don't initialize NULL variables...
> +static u32 be_decode_u32(u8 * buf)
> +{
> + u32 val = buf[3];
> + val = (val << 8) | (u8) buf[2];
> + val = (val << 8) | (u8) buf[1];
> + val = (val << 8) | (u8) buf[0];
> + return val;
> +}
We have bit operations for endian issues, please don't roll your own.
> +#ifdef CONFIG_ACPI
> +extern struct file_operations ima_ascii_bios_measurements_ops;
> +#endif
This should be in a .h file instead of a .c
> + out:
> + securityfs_remove(call_count);
> + out1:
Labels belong in the first collumn, not 8 spaces? in. And use tabs, not
spaces as per Documentation/CodingStyle.
> +#ifndef __LINUX_IMA_H
> +#define __LINUX_IMA_H
> +
> +#include <linux/types.h>
> +#include <linux/fs.h>
> +#include <linux/major.h>
What do you need major.h for?
> +/*
> + * used to protect h_table and sha_table
> + */
> +extern spinlock_t ima_queue_lock;
> +
> +struct h_table {
> + atomic_t len; /* number of stored measurements in the list */
> + atomic_t call_count; /* # measure requests (calls into ima) */
> + atomic_t changed_files; /* times dirty marked entry really changed */
> + atomic_t violations;
If you have a lock, why use atomic_t then? They are _slow_.
> + unsigned int max_htable_size;
> + struct hlist_head queue[MEASURE_HTABLE_SIZE];
> + atomic_t queue_len[MEASURE_HTABLE_SIZE];
> +};
> +extern struct h_table htable;
That's not the nicest global variable name :(
> +/* protos */
> +void invalidate_pcr(char *);
Neither is that.
> +int ima_add_measure_entry(struct measure_entry *entry);
> +extern void ima_measure(const unsigned char *func, const unsigned char *name,
> + struct inode *inode, int mask, char *hash);
Why switch between the "extern" and "not-extern" style in the same file?
Consistancy please...
> + if ( tpm_pcr_extend( IMA_TPM, CONFIG_IMA_MEASURE_PCR_IDX, hash ) != 0 )
Should be:
if (tpm_pcr_extend(IMA_TPM, CONFIG_IMA_MEASURE_PCR_IDX, hash) != 0)
You do this in other places too.
> + * File: ima_init.c
> + * init functions to start up IBM IMA
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/linkage.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/fcntl.h>
> +#include <asm/uaccess.h>
> +#include <linux/file.h>
> +#include <linux/slab.h>
> +#include <linux/errno.h>
> +#include <linux/crypto.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include "ima.h"
asm/ files at the end of the list please.
> +/* These identify the driver base version and may not be removed. */
Why not, what happens then? :)
> +static const char version[] = "v5.5 08/15/2005";
> +static const char illegal_pcr[20] =
> + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> +int ima_used_chip = 0;
Again with the initialized data...
> +int _ima_init(void)
> +{
> + printk(KERN_INFO
> + "IBM Integrity Measurement Architecture (IBM IMA %s).\n",
> + version);
Why announce it to the world? We are trying to be quieter on boot.
> +
> + ima_used_chip = 0;
> + if (tpm_pcr_read(IMA_TPM, 0, NULL, 0) == 0)
> + ima_used_chip = 1;
> +
> + if (!ima_used_chip)
> + printk(KERN_INFO
> + " IMA (TPM/BYPASS - no TPM chip found)\n");
What's the spaces for? Shouldn't you error out here?
> +
> + create_htable(); /* for measurements */
> +
> + /* boot aggregate must be very first entry */
> + ima_add_boot_aggregate();
> +
> + ima_fs_init();
> + return 0;
> +}
> +
> +/*
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Reiner Sailer <sailer@private>");
> +MODULE_DESCRIPTION("Run-time LSM-based IBM Integrity Measurement Architecture");
> +*/
Why are these lines commented out?
> +/* Add a file that is scheduled for measurement */
> +int
> +_ima_measure(const unsigned char *func, const unsigned char *name,
> + struct inode *inode, int mask, int hash_len, char *hash)
static?
Have you run all of this code through sparse?
> + /* create new entry and add to measurement list */
> + entry = kmalloc(sizeof(struct measure_entry), GFP_KERNEL);
> + if (!entry) {
> + invalidate_pcr("error allocating new measurement entry");
> + err = -ENOMEM;
> + goto out; /* invalidate pcr */
> + }
> +
> + memset(entry, sizeof(struct measure_entry), 0);
kzalloc() instead.
> +#menu "TPM-based Integrity Measurement Architecture"
> +
> +config IMA_MEASURE
> + bool "TCG run-time Integrity Measurement Architecture"
> + depends on SECURITY && (CRYPTO_SHA1=y) && SECURITY_SLIM
> + help
> + To measure executable code running on this
> + system, say Y.
> + If unsure, say N.
> +
> +config IMA_MEASURE_PCR_IDX
> + int "PCR for Aggregate (8<= Index <= 15)"
> + depends on IMA_MEASURE
> + range 8 15
> + default 10
> + help
> + This determines the PCR index used for aggregating the
> + measurement list into the TPM hardware.
> + If unsure, use the default 10.
You need _much_ better descriptions for these options here.
> diff -rupN linux-2.6.14.2.orig/security/evm/slim/ima.h linux-2.6.14.2.tc/security/evm/slim/ima.h
> --- linux-2.6.14.2.orig/security/evm/slim/ima.h 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.14.2.tc/security/evm/slim/ima.h 2005-11-11 14:20:28.000000000 -0500
> @@ -0,0 +1,32 @@
> +/*
> + IMA
> + */
> +
> +int ima_init(void);
> +void _ima_init(void);
_ functions should not be global.
> +extern unsigned int slm_enable_ima;
> +
> +#ifdef CONFIG_IMA_MEASURE
> +int ima_init()
> +{
> + printk(KERN_INFO "%s: slm_enable_ima %d\n",
> + __FUNCTION__, slm_enable_ima);
> + if (slm_enable_ima)
> + _ima_init();
> + return 0;
> +}
> +#else
> +int ima_init()
> +{
> + slm_enable_ima = 0;
> + return 0;
> +}
> +#endif
Huh? Not inline?
thanks,
greg k-h
This archive was generated by hypermail 2.1.3 : Wed Nov 16 2005 - 10:01:08 PST