Re: [RFC][PATCH 1/3] EVM

From: Greg KH (greg@private)
Date: Tue Nov 15 2005 - 15:59:14 PST


On Tue, Nov 15, 2005 at 09:08:53AM -0500, David Safford wrote:
> EVM securityfs files:
>        /sys/kernel/security/evm/config       - configuration input
>        /sys/kernel/security/evm/debug/cache  - debugging control
>        /sys/kernel/security/evm/debug/xattr  - debugging control
>        /sys/kernel/security/evm/debug/crypto - debugging control

Why not use debugfs for your debugging stuff?  That's the proper place
for it.

> +extern int sandbox_avail; /* can't use MODULE_STATE_GOING notification */

That's a pretty horrible global symbol name.  What exactly are you
trying to test for here?

> +typedef enum {
> +	EVM_PERMIT = 0, EVM_SANDBOX = -1, EVM_DENY = -2, EVM_NOLABEL = -3
> +} evm_status;

No new typedefs please.

> +extern evm_status evm_analyze_cacheinfo(struct dentry *dentry);
> +extern int evm_idx;
> +extern int evm_setxattr (struct dentry *d, char *name, void *value,
> +                               size_t size, int flags);
> +extern int evm_update_hmac(struct dentry *dentry, int flags);
> +extern int mem2hex(char *mem, char *buffer, int count);

Heh, anything wrong with sprintf()?

> +	char hashStr[MD5_STR_SIZE + 1];	/* MD5 MAC as a string */

Wrong coding style for variable names.  Please read
Documentation/CodingStyle again...

> +static int evm_config_datasize = 0;	/* number of extended attributes */
> +static int evm_config_version;	/* using a timestamp as a config version no. */

Do not initialize static variables to 0, it's redundant and takes up
more memory.

> +int evm_get_isec_size(void)
> +{
> +	return evm_isec_size;
> +}

I've seen some foolish wrapper functions before, but...


> +int evm_get_config_info(struct evm_xattr_config **head, int *size,
> +			time_t * version)
> +{
> +	*head = evm_config_data;
> +	*size = evm_config_datasize;
> +	*version = evm_config_version;
> +	return 0;
> +}

Same here, why is this needed?

> diff -rpuN linux-2.6.14.2.orig/security/evm/evm.h linux-2.6.14.2.tc/security/evm/evm.h
> --- linux-2.6.14.2.orig/security/evm/evm.h	1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.14.2.tc/security/evm/evm.h	2005-11-11 13:45:18.000000000 -0500
> @@ -0,0 +1,133 @@
> +/*
> + *  evm.h - extended verification module
> + *
> +*/
> +#include <linux/security.h>
> +#include <linux/version.h>
> +#include <linux/security-stack.h>
> +
> +#define CONFIG_SECURITY_STACKER 1
> +
> +#define SHA1_STR_SIZE 40 	/* String value  */
> +#define SHA1_DIGEST_SIZE 20 	/* SHA1 is 160-bits */
> +#define MD5_STR_SIZE 32 	/* String value  */
> +#define MD5_DIGEST_SIZE 16 	/* MD5 is 128-bits */
> +#define EVM_XATTR_SIZE  256	/* XATTR_SIZE_MAX causes kernel to crash */
> +
> +/*
> + * EVM configuration policies as defined in /etc/evm.config are
> + * stored in struct evm_config *evm_config_data.
> + */
> +typedef enum {
> +	EVM_TYPE_ENUM = 1, EVM_TYPE_HMAC, EVM_TYPE_MD5, EVM_TYPE_SHA1,
> +		EVM_TYPE_STRING
> +} evm_configtype_t;
> +typedef enum {
> +	EVM_VALUE_NOLABEL = '0', EVM_VALUE_FAIL = '1', EVM_VALUE_PASS = '2',
> +		EVM_VALUE_EXPIRED = '3'
> +} evm_configvalue_t;

No new typedefs please.

> +extern void evm_init_secfs(void);
> +void evm_cleanup_secfs(void);

Why switch between the "extern" and "no extern" style here?

> +#define dprintk(level, format, a...) \
> +	if (evm_debug & level) \
> +		printk(KERN_INFO format, ##a)

Will not build on older versions of gcc :(

> +/*
> + * Replacing module_use with the global sandbox_avail,
> + * since we don't get MODULE_STATE_GOING notification,
> + * however using a global creates a dependency on EVM for other modules.
> + */
> +int sandbox_avail = 0;

I still don't understand what you are trying to do here.

See above comment about initializing stuff to 0 not being needed...

> +	int xattrFlags = 0;

StudlyCapsIsNotTheLinuxKernelVariableStyle.

thanks,

greg k-h



This archive was generated by hypermail 2.1.3 : Tue Nov 15 2005 - 16:16:47 PST