Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

[Chris Wright <chrisw@osdl.org>]

* David Howells (dhowells@redhat.com) wrote:

Might be useful to take one step back and define the actions that users
can take on keys and show that each is mediated by label check.  Looks
like key_permission does most of this, but key_type has:

  ->instantiate
  ->duplicate
  ->update
  ->match
  ->describe
  ->read

So the hooks you added control search/read/write via permission, and
then special case instantiate and update.  Does this mean you expect the
security module to inspect the key data and set a label based on data?
Does duplicate need to update label to a new context, or should it get
identical lable, and does it need a hook for that?

> diff -uNrp linux-2.6.14-rc3-keys/include/linux/key.h linux-2.6.14-rc3-keys-lsm/include/linux/key.h
> --- linux-2.6.14-rc3-keys/include/linux/key.h	2005-10-05 11:50:22.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/include/linux/key.h	2005-10-05 17:06:42.000000000 +0100
> @@ -23,6 +23,8 @@
>  
>  #ifdef __KERNEL__
>  
> +#undef KEY_DEBUGGING
> +
>  /* key handle serial number */
>  typedef int32_t key_serial_t;
>  
> @@ -31,9 +33,39 @@ typedef uint32_t key_perm_t;
>  
>  struct key;
>  
> +/*****************************************************************************/
> +/*
> + * key reference with possession attribute handling
> + *
> + * NOTE! key_ref_t is a typedef'd pointer to a type that is not actually
> + * defined. This is because we abuse the bottom bit of the reference to carry a
> + * flag to indicate whether the calling process possesses that key in one of
> + * its keyrings.
> + *
> + * the key_ref_t has been made a separate type so that the compiler can reject
> + * attempts to dereference it without proper conversion.
> + *
> + * the three functions are used to assemble and disassemble references
> + */
> +typedef struct __key_reference_with_attributes *key_ref_t;


> +
>  #ifdef CONFIG_KEYS
>  
> -#undef KEY_DEBUGGING
> +static inline key_ref_t make_key_ref(const struct key *key,
> +				     unsigned long possession)
> +{
> +	return (key_ref_t) ((unsigned long) key | possession);
> +}
> +
> +static inline struct key *key_ref_to_ptr(const key_ref_t key_ref)
> +{
> +	return (struct key *) ((unsigned long) key_ref & ~1UL);
> +}
> +
> +static inline unsigned long is_key_possessed(const key_ref_t key_ref)
> +{
> +	return (unsigned long) key_ref & 1UL;
> +}
>  
>  #define KEY_POS_VIEW	0x01000000	/* possessor can view a key's attributes */
>  #define KEY_POS_READ	0x02000000	/* possessor can read key payload / view keyring */
> @@ -74,38 +106,6 @@ struct keyring_name;
>  
>  /*****************************************************************************/
>  /*
> - * key reference with possession attribute handling
> - *
> - * NOTE! key_ref_t is a typedef'd pointer to a type that is not actually
> - * defined. This is because we abuse the bottom bit of the reference to carry a
> - * flag to indicate whether the calling process possesses that key in one of
> - * its keyrings.
> - *
> - * the key_ref_t has been made a separate type so that the compiler can reject
> - * attempts to dereference it without proper conversion.
> - *
> - * the three functions are used to assemble and disassemble references
> - */
> -typedef struct __key_reference_with_attributes *key_ref_t;
> -
> -static inline key_ref_t make_key_ref(const struct key *key,
> -				     unsigned long possession)
> -{
> -	return (key_ref_t) ((unsigned long) key | possession);
> -}
> -
> -static inline struct key *key_ref_to_ptr(const key_ref_t key_ref)
> -{
> -	return (struct key *) ((unsigned long) key_ref & ~1UL);
> -}
> -
> -static inline unsigned long is_key_possessed(const key_ref_t key_ref)
> -{
> -	return (unsigned long) key_ref & 1UL;
> -}
> -
> -/*****************************************************************************/

This move of key_ref_t handling is either unrelated or simple prep work,
yes?  Just clutters the patch.

> -/*
>   * authentication token / access credential / keyring
>   * - types of key include:
>   *   - keyrings
> @@ -119,6 +119,7 @@ struct key {
>  	struct key_type		*type;		/* type of key */
>  	struct rw_semaphore	sem;		/* change vs change sem */
>  	struct key_user		*user;		/* owner of this key */
> +	void			*security;	/* security data for this key */
>  	time_t			expiry;		/* time at which key expires (or 0) */
>  	uid_t			uid;
>  	gid_t			gid;
> diff -uNrp linux-2.6.14-rc3-keys/include/linux/key-ui.h linux-2.6.14-rc3-keys-lsm/include/linux/key-ui.h
> --- linux-2.6.14-rc3-keys/include/linux/key-ui.h	2005-10-03 10:48:38.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/include/linux/key-ui.h	2005-10-05 15:02:55.000000000 +0100
> @@ -38,97 +38,16 @@ struct keyring_list {
>  	struct key	*keys[0];
>  };
>  
> +extern int key_task_permission(const key_ref_t key_ref,
> +			       struct task_struct *context,
> +			       key_perm_t perm);
>  
>  /*
>   * check to see whether permission is granted to use a key in the desired way
>   */
>  static inline int key_permission(const key_ref_t key_ref, key_perm_t perm)
>  {
> -	struct key *key = key_ref_to_ptr(key_ref);
> -	key_perm_t kperm;
> -
> -	if (is_key_possessed(key_ref))
> -		kperm = key->perm >> 24;
> -	else if (key->uid == current->fsuid)
> -		kperm = key->perm >> 16;
> -	else if (key->gid != -1 &&
> -		 key->perm & KEY_GRP_ALL &&
> -		 in_group_p(key->gid)
> -		 )
> -		kperm = key->perm >> 8;
> -	else
> -		kperm = key->perm;
> -
> -	kperm = kperm & perm & KEY_ALL;
> -
> -	return kperm == perm;
> -}
> -
> -/*
> - * check to see whether permission is granted to use a key in at least one of
> - * the desired ways
> - */
> -static inline int key_any_permission(const key_ref_t key_ref, key_perm_t perm)
> -{
> -	struct key *key = key_ref_to_ptr(key_ref);
> -	key_perm_t kperm;
> -
> -	if (is_key_possessed(key_ref))
> -		kperm = key->perm >> 24;
> -	else if (key->uid == current->fsuid)
> -		kperm = key->perm >> 16;
> -	else if (key->gid != -1 &&
> -		 key->perm & KEY_GRP_ALL &&
> -		 in_group_p(key->gid)
> -		 )
> -		kperm = key->perm >> 8;
> -	else
> -		kperm = key->perm;
> -
> -	kperm = kperm & perm & KEY_ALL;
> -
> -	return kperm != 0;
> -}

Again patch clutter, since this is just removing unused code AFAICT
(which should definitely be removed).  I mention only because it makes it
harder to review since I don't know the key infrastructure as well as you.

> -static inline int key_task_groups_search(struct task_struct *tsk, gid_t gid)
> -{
> -	int ret;
> -
> -	task_lock(tsk);
> -	ret = groups_search(tsk->group_info, gid);
> -	task_unlock(tsk);
> -	return ret;
> -}
> -
> -static inline int key_task_permission(const key_ref_t key_ref,
> -				      struct task_struct *context,
> -				      key_perm_t perm)
> -{
> -	struct key *key = key_ref_to_ptr(key_ref);
> -	key_perm_t kperm;
> -
> -	if (is_key_possessed(key_ref)) {
> -		kperm = key->perm >> 24;
> -	}
> -	else if (key->uid == context->fsuid) {
> -		kperm = key->perm >> 16;
> -	}
> -	else if (key->gid != -1 &&
> -		 key->perm & KEY_GRP_ALL && (
> -			 key->gid == context->fsgid ||
> -			 key_task_groups_search(context, key->gid)
> -			 )
> -		 ) {
> -		kperm = key->perm >> 8;
> -	}
> -	else {
> -		kperm = key->perm;
> -	}
> -
> -	kperm = kperm & perm & KEY_ALL;
> -
> -	return kperm == perm;
> -
> +	return key_task_permission(key_ref, current, perm);

Just curious, from quick look, appears that key_task_permission is
basically always called with current.  I found one (which appears to be
a recursive call) that uses rka->context in search_process_keyrings.
What case causes context != current?

>  }
>  
>  extern key_ref_t lookup_user_key(struct task_struct *context,
> diff -uNrp linux-2.6.14-rc3-keys/include/linux/security.h linux-2.6.14-rc3-keys-lsm/include/linux/security.h
> --- linux-2.6.14-rc3-keys/include/linux/security.h	2005-10-03 10:48:39.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/include/linux/security.h	2005-10-05 16:10:53.000000000 +0100
> @@ -30,6 +30,7 @@
>  #include <linux/shm.h>
>  #include <linux/msg.h>
>  #include <linux/sched.h>
> +#include <linux/key.h>
>  
>  struct ctl_table;
>  
> @@ -785,6 +786,54 @@ struct swap_info_struct;
>   * @sk_free_security:
>   *	Deallocate security structure.
>   *
> + * Security hooks affecting all Key Management operations
> + *
> + * @key_alloc:
> + *	Permit allocation a key and assign security data. Note that key does
> + *	not have a serial number assigned at this point.
> + *	@key points to the key.
> + *	Return 0 if permission is granted, -ve error otherwise.
> + * @key_post_alloc:
> + *	Notification of key publication; key has serial number.
> + *	@key points to the key.
> + *	No return.
> + * @key_free:
> + *	Notification of destruction; free security data.
> + *	@key points to the key.
> + *	No return.
> + * @key_set_security:
> + *	Apply security data to a key.
> + *	@key points to the key.
> + *	@name points to the name of the attribute.
> + *	@data points to the data (may be NULL).
> + *	@dlen indicates the size of the data (may be 0).
> + *	Return 0 if successful, -ve error otherwise.
> + * @key_get_security:
> + *	Retrieve security data from a key.
> + *	@key points to the key.
> + *	@name points to the name of the attribute.
> + *	@buffer points to the buffer (may be NULL if size wanted).
> + *	@blen indicates the size of the buffer (may be 0 if buffer is NULL).
> + *	Load buffer with up to maximum of blen bytes upon successful return.
> + *	Return number of bytes retrievable if successful, -ve error otherwise.
> + * @key_permission:
> + *	See whether a specific operational right is granted to a process on a
> + *      key.
> + *	@key_ref refers to the key (key pointer + possession attribute bit).
> + *	@context points to the process to provide the context against which to
> + *       evaluate the security data on the key.
> + *	@perm describes the combination of permissions required of this key.
> + *	Return 1 if permission granted, 0 if permission denied and -ve it the
> + *      normal permissions model should be effected.
> + * @key_set_data:
> + *	Test whether a key may be set to the given data, and adjust the
> + *      security information accordingly.
> + *	@key points to the key being altered.
> + *	@instkey points to the instantiation key if the key is being
> + *       instantiated, and NULL if being updated.
> + *	@data points to the data.
> + *	@dlen indicates the size of the data.
> + *
>   * Security hooks affecting all System V IPC operations.
>   *
>   * @ipc_permission:
> @@ -1213,6 +1262,28 @@ struct security_operations {
>  	int (*sk_alloc_security) (struct sock *sk, int family, int priority);
>  	void (*sk_free_security) (struct sock *sk);
>  #endif	/* CONFIG_SECURITY_NETWORK */
> +
> +	/* key management security hooks */
> +#ifdef CONFIG_SECURITY_KEYS
> +	int (*key_alloc)(struct key *key);
> +	void (*key_post_alloc)(struct key *key);
> +	void (*key_free)(struct key *key);
> +	long (*key_set_security)(struct key *key, const char __user *name,
> +				 const void __user *data, size_t dlen);
> +	long (*key_get_security)(struct key *key, const char __user *name,
> +				 void __user *buffer, size_t blen);
> +
> +	int (*key_permission)(key_ref_t key_ref,
> +			      struct task_struct *context,
> +			      key_perm_t perm);
> +
> +	int (*key_set_data)(struct key *key,
> +			    struct key *instkey,
> +			    const void *data,
> +			    size_t dlen);
> +
> +#endif	/* CONFIG_SECURITY_KEYS */
> +
>  };
>  
>  /* global variables */
> @@ -2763,5 +2834,101 @@ static inline void security_sk_free(stru
>  }
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
> +#ifdef CONFIG_SECURITY_KEYS
> +
> +static inline int security_key_alloc(struct key *key)
> +{
> +	return security_ops->key_alloc(key);
> +}
> +
> +static inline void security_key_post_alloc(struct key *key)
> +{
> +	security_ops->key_post_alloc(key);
> +}
> +
> +static inline void security_key_free(struct key *key)
> +{
> +	security_ops->key_free(key);
> +}
> +
> +static inline long security_key_set_security(struct key *key,
> +					     const char __user *name,
> +					     const void __user *data,
> +					     size_t dlen)
> +{
> +	return security_ops->key_set_security(key, name, data, dlen);
> +}
> +
> +static inline long security_key_get_security(struct key *key,
> +					     const char __user *name,
> +					     void __user *buffer,
> +					     size_t blen)
> +{
> +	return security_ops->key_get_security(key, name, buffer, blen);
> +}
> +
> +static inline int security_key_permission(key_ref_t key_ref,
> +					  struct task_struct *context,
> +					  key_perm_t perm)
> +{
> +	return security_ops->key_permission(key_ref, context, perm);
> +}
> +
> +static inline int security_key_set_data(struct key *key,
> +					struct key *instkey,
> +					const void *data,
> +					size_t dlen)
> +{
> +	return security_ops->key_set_data(key, instkey, data, dlen);
> +}
> +
> +#else
> +
> +static inline int security_key_alloc(struct key *key)
> +{
> +	return 0;
> +}
> +
> +static inline void security_key_post_alloc(struct key *key)
> +{
> +}
> +
> +static inline void security_key_free(struct key *key)
> +{
> +}
> +
> +static inline long security_key_set_security(struct key *key,
> +					     const char __user *name,
> +					     const void __user *data,
> +					     size_t dlen)
> +{
> +	return 0;
> +}
> +
> +static inline long security_key_get_security(struct key *key,
> +					     const char __user *name,
> +					     void __user *buffer,
> +					     size_t blen)
> +{
> +	return -ENODATA;
> +}
> +
> +static inline int security_key_permission(key_ref_t key_ref,
> +					  struct task_struct *context,
> +					  key_perm_t perm)
> +{
> +	return -1;
> +}
> +
> +static inline int security_key_set_data(struct key *key,
> +					struct key *instkey,
> +					const void *data,
> +					size_t dlen)
> +{
> +	return 0;
> +}
> +
> +#endif
> +
>  #endif /* ! __LINUX_SECURITY_H */
>  
> diff -uNrp linux-2.6.14-rc3-keys/security/dummy.c linux-2.6.14-rc3-keys-lsm/security/dummy.c
> --- linux-2.6.14-rc3-keys/security/dummy.c	2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/dummy.c	2005-10-05 16:11:38.000000000 +0100
> @@ -803,6 +803,50 @@ static int dummy_setprocattr(struct task
>  	return -EINVAL;
>  }
>  
> +static inline int dummy_key_alloc(struct key *key)
> +{
> +	return 0;
> +}
> +
> +static inline void dummy_key_post_alloc(struct key *key)
> +{
> +}
> +
> +static inline void dummy_key_free(struct key *key)
> +{
> +}
> +
> +static inline long dummy_key_set_security(struct key *key,
> +					  const char __user *name,
> +					  const void __user *data,
> +					  size_t dlen)
> +{
> +	return 0;
> +}
> +
> +static inline long dummy_key_get_security(struct key *key,
> +					  const char __user *name,
> +					  void __user *buffer,
> +					  size_t blen)
> +{
> +	return -ENODATA;
> +}
> +
> +static inline int dummy_key_permission(key_ref_t key_ref,
> +				       struct task_struct *context,
> +				       key_perm_t perm)
> +{
> +	return -1;
> +}
> +
> +static inline int dummy_key_set_data(struct key *key,
> +				     struct key *instkey,
> +				     const void *data,
> +				     size_t dlen)
> +{
> +	return 0;
> +}
> +
>  
>  struct security_operations dummy_security_ops;
>  
> @@ -954,5 +998,15 @@ void security_fixup_ops (struct security
>  	set_to_dummy_if_null(ops, sk_alloc_security);
>  	set_to_dummy_if_null(ops, sk_free_security);
>  #endif	/* CONFIG_SECURITY_NETWORK */
> +#ifdef CONFIG_SECURITY_KEYS
> +	set_to_dummy_if_null(ops, key_alloc);
> +	set_to_dummy_if_null(ops, key_post_alloc);
> +	set_to_dummy_if_null(ops, key_free);
> +	set_to_dummy_if_null(ops, key_set_security);
> +	set_to_dummy_if_null(ops, key_get_security);
> +	set_to_dummy_if_null(ops, key_permission);
> +	set_to_dummy_if_null(ops, key_set_data);
> +#endif	/* CONFIG_SECURITY_KEYS */
> +
>  }
>  
> diff -uNrp linux-2.6.14-rc3-keys/security/Kconfig linux-2.6.14-rc3-keys-lsm/security/Kconfig
> --- linux-2.6.14-rc3-keys/security/Kconfig	2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/Kconfig	2005-10-05 13:59:45.000000000 +0100
> @@ -74,6 +74,15 @@ config SECURITY_ROOTPLUG
>  	  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_KEYS
> +	bool "Key Management Security Hooks"
> +	depends on SECURITY && KEYS
> +	help
> +	  This enables the key management security hooks.
> +	  If enabled, a security module can use these hooks to
> +	  implement access controls on keys.
> +	  If you are unsure how to answer this question, answer N.
> +
>  config SECURITY_SECLVL
>  	tristate "BSD Secure Levels"
>  	depends on SECURITY
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/compat.c linux-2.6.14-rc3-keys-lsm/security/keys/compat.c
> --- linux-2.6.14-rc3-keys/security/keys/compat.c	2005-08-30 13:56:44.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/compat.c	2005-10-05 16:30:38.000000000 +0100
> @@ -74,6 +74,14 @@ asmlinkage long compat_sys_keyctl(u32 op
>  	case KEYCTL_SET_REQKEY_KEYRING:
>  		return keyctl_set_reqkey_keyring(arg2);
>  
> +	case KEYCTL_SET_SECURITY:
> +		return keyctl_set_security(arg2, compat_ptr(arg3),
> +					   compat_ptr(arg4), arg5);
> +
> +	case KEYCTL_GET_SECURITY:
> +		return keyctl_get_security(arg2, compat_ptr(arg3),
> +					   compat_ptr(arg4), arg5);
> +
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/internal.h linux-2.6.14-rc3-keys-lsm/security/keys/internal.h
> --- linux-2.6.14-rc3-keys/security/keys/internal.h	2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/internal.h	2005-10-05 16:29:39.000000000 +0100
> @@ -12,6 +12,7 @@
>  #ifndef _INTERNAL_H
>  #define _INTERNAL_H
>  
> +#include <linux/security.h>

This looks unnecessary.  I'd include it where it's used.

>  #include <linux/key.h>
>  #include <linux/key-ui.h>
>  
> @@ -137,7 +138,10 @@ extern long keyctl_instantiate_key(key_s
>  				   size_t, key_serial_t);
>  extern long keyctl_negate_key(key_serial_t, unsigned, key_serial_t);
>  extern long keyctl_set_reqkey_keyring(int);
> -
> +extern long keyctl_set_security(key_serial_t, const char __user *,
> +				const void __user *, size_t);
> +extern long keyctl_get_security(key_serial_t, const char __user *,
> +				void __user *, size_t);
>  
>  /*
>   * debugging key validation
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/key.c linux-2.6.14-rc3-keys-lsm/security/keys/key.c
> --- linux-2.6.14-rc3-keys/security/keys/key.c	2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/key.c	2005-10-05 15:33:29.000000000 +0100
> @@ -1,6 +1,6 @@
>  /* key.c: basic authentication token and access key management
>   *
> - * Copyright (C) 2004-5 Red Hat, Inc. All Rights Reserved.
> + * Copyright (C) 2004 Red Hat, Inc. All Rights Reserved.
>   * Written by David Howells (dhowells@redhat.com)
>   *
>   * This program is free software; you can redistribute it and/or
> @@ -253,6 +253,7 @@ struct key *key_alloc(struct key_type *t
>  	struct key_user *user = NULL;
>  	struct key *key;
>  	size_t desclen, quotalen;
> +	int ret;
>  
>  	key = ERR_PTR(-EINVAL);
>  	if (!desc || !*desc)
> @@ -305,6 +306,7 @@ struct key *key_alloc(struct key_type *t
>  	key->flags = 0;
>  	key->expiry = 0;
>  	key->payload.data = NULL;
> +	key->security = NULL;
>  
>  	if (!not_in_quota)
>  		key->flags |= 1 << KEY_FLAG_IN_QUOTA;
> @@ -315,16 +317,37 @@ struct key *key_alloc(struct key_type *t
>  	key->magic = KEY_DEBUG_MAGIC;
>  #endif
>  
> +	/* do a final security check before publishing the key */
> +	ret = security_key_alloc(key);

This may simply be allocating space for the label (and possibly labelling)
not necessarily a security check.

> +	if (ret < 0)
> +		goto security_error;
> +
>  	/* publish the key by giving it a serial number */
>  	atomic_inc(&user->nkeys);
>  	key_alloc_serial(key);
>  
> - error:
> +	/* let the security module know the key has been published */
> +	security_key_post_alloc(key);

This is odd, esp since nothing could have failed between alloc and
publish.  Only state change is serial number.  Would you expect the
security module to update a label based on serial number?

> +
> +error:
>  	return key;
>  
> - no_memory_3:
> +security_error:
> +	kfree(key->description);
> +	kmem_cache_free(key_jar, key);
> +	if (!not_in_quota) {
> +		spin_lock(&user->lock);
> +		user->qnkeys--;
> +		user->qnbytes -= quotalen;
> +		spin_unlock(&user->lock);
> +	}
> +	key_user_put(user);
> +	key = ERR_PTR(ret);
> +	goto error;
> +
> +no_memory_3:
>  	kmem_cache_free(key_jar, key);
> - no_memory_2:
> +no_memory_2:
>  	if (!not_in_quota) {
>  		spin_lock(&user->lock);
>  		user->qnkeys--;
> @@ -332,11 +355,11 @@ struct key *key_alloc(struct key_type *t
>  		spin_unlock(&user->lock);
>  	}
>  	key_user_put(user);
> - no_memory_1:
> +no_memory_1:
>  	key = ERR_PTR(-ENOMEM);
>  	goto error;
>  
> - no_quota:
> +no_quota:
>  	spin_unlock(&user->lock);
>  	key_user_put(user);
>  	key = ERR_PTR(-EDQUOT);
> @@ -406,6 +429,11 @@ static int __key_instantiate_and_link(st
>  
>  	/* can't instantiate twice */
>  	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
> +		/* consult the security modules */
> +		ret = security_key_set_data(key, instkey, data, datalen);
> +		if (ret < 0)
> +			goto error;
> +
>  		/* instantiate the key */
>  		ret = key->type->instantiate(key, data, datalen);

I assume you don't check key_permission here because it's in context
creating the key?

> @@ -427,6 +455,7 @@ static int __key_instantiate_and_link(st
>  		}
>  	}
>  
> +error:
>  	up_write(&key_construction_sem);
>  
>  	/* wake up anyone waiting for a key to be constructed */
> @@ -556,6 +585,8 @@ static void key_cleanup(void *data)
>  
>  	key_check(key);
>  
> +	security_key_free(key);
> +
>  	/* deal with the user's key tracking and quota */
>  	if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
>  		spin_lock(&key->user->lock);
> @@ -710,11 +741,14 @@ static inline key_ref_t __key_update(key
>  
>  	down_write(&key->sem);
>  
> -	ret = key->type->update(key, payload, plen);
> +	ret = security_key_set_data(key, NULL, payload, plen);
> +	if (ret == 0) {
> +		ret = key->type->update(key, payload, plen);
>  
> -	if (ret == 0)
> -		/* updating a negative key instantiates it */
> -		clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> +		if (ret == 0)
> +			/* updating a negative key instantiates it */
> +			clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> +	}
>  
>  	up_write(&key->sem);
>  
> @@ -848,11 +882,15 @@ int key_update(key_ref_t key_ref, const 
>  	ret = -EOPNOTSUPP;
>  	if (key->type->update) {
>  		down_write(&key->sem);
> -		ret = key->type->update(key, payload, plen);
>  
> -		if (ret == 0)
> -			/* updating a negative key instantiates it */
> -			clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> +		ret = security_key_set_data(key, NULL, payload, plen);
> +		if (ret == 0) {
> +			ret = key->type->update(key, payload, plen);
> +
> +			if (ret == 0)
> +				/* updating a negative key instantiates it */
> +				clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> +		}
>  
>  		up_write(&key->sem);
>  	}
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/keyctl.c linux-2.6.14-rc3-keys-lsm/security/keys/keyctl.c
> --- linux-2.6.14-rc3-keys/security/keys/keyctl.c	2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/keyctl.c	2005-10-05 16:25:58.000000000 +0100
> @@ -964,6 +964,82 @@ long keyctl_set_reqkey_keyring(int reqke
>  
>  /*****************************************************************************/
>  /*
> + * apply security data to a key
> + */
> +long keyctl_set_security(key_serial_t id,
> +			 const char __user *name,
> +			 const void __user *data,
> +			 size_t dlen)
> +{
> +	struct key *key;
> +	key_ref_t key_ref;
> +	long ret;
> +
> +	ret = -EINVAL;
> +	if (!name)
> +		goto error;
> +
> +	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
> +	if (IS_ERR(key_ref)) {
> +		ret = PTR_ERR(key_ref);
> +		goto error;
> +	}
> +
> +	key = key_ref_to_ptr(key_ref);
> +
> +	/* make the changes with the locks held to prevent races */
> +	ret = -EACCES;
> +	down_write(&key->sem);
> +
> +	/* if we're not the sysadmin, we can only change a key that we own */
> +	if (capable(CAP_SYS_ADMIN) || key->uid == current->fsuid)
> +		ret = security_key_set_security(key, name, data, dlen);

Are you sure this is right?  Normally I'd expect users can _not_ set the
security labels of their own keys.  But perhaps I've missed the point
of this one, could you give a use case?

> +	up_write(&key->sem);
> +	key_put(key);
> +error:
> +	return ret;
> +
> +} /* end keyctl_set_security() */
> +
> +/*****************************************************************************/
> +/*
> + * retrieve security data from a key
> + */
> +long keyctl_get_security(key_serial_t id,
> +			 const char __user *name,
> +			 void __user *buffer,
> +			 size_t blen)
> +{
> +	struct key *key;
> +	key_ref_t key_ref;
> +	long ret;
> +
> +	ret = -EINVAL;
> +	if (!name)
> +		goto error;
> +
> +	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
> +	if (IS_ERR(key_ref)) {
> +		ret = PTR_ERR(key_ref);
> +		goto error;
> +	}
> +
> +	key = key_ref_to_ptr(key_ref);
> +
> +	/* make the changes with the locks held to prevent races */
> +	down_read(&key->sem);
> +	ret = security_key_get_security(key, name, buffer, blen);

This would be a whole lot easier if keys were available in keyfs ;-)
/me ducks and runs
Again, maybe I've lost you on the keyctl interface, but what's this for?
How will users benefit from read/write of the security label on a key?

> +	up_read(&key->sem);
> +
> +	key_put(key);
> +error:
> +	return ret;
> +
> +} /* end keyctl_get_security() */
> +
> +/*****************************************************************************/
> +/*
>   * the key control system call
>   */
>  asmlinkage long sys_keyctl(int option, unsigned long arg2, unsigned long arg3,
> @@ -1035,6 +1111,18 @@ asmlinkage long sys_keyctl(int option, u
>  	case KEYCTL_SET_REQKEY_KEYRING:
>  		return keyctl_set_reqkey_keyring(arg2);
>  
> +	case KEYCTL_SET_SECURITY:
> +		return keyctl_set_security((key_serial_t) arg2,
> +					   (const char __user *) arg3,
> +					   (const void __user *) arg4,
> +					   (size_t) arg5);
> +
> +	case KEYCTL_GET_SECURITY:
> +		return keyctl_get_security((key_serial_t) arg2,
> +					   (const char __user *) arg3,
> +					   (void __user *) arg4,
> +					   (size_t) arg5);
> +
>  	default:
>  		return -EOPNOTSUPP;
>  	}

> diff -uNrp linux-2.6.14-rc3-keys/security/keys/permission.c linux-2.6.14-rc3-keys-lsm/security/keys/permission.c
> --- linux-2.6.14-rc3-keys/security/keys/permission.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/permission.c	2005-10-05 15:36:22.000000000 +0100
> @@ -0,0 +1,81 @@
> +/* permission.c: key permission determination
> + *
> + * Copyright (C) 2005 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include "internal.h"
> +
> +/*****************************************************************************/
> +/*
> + * check to see whether permission is granted to use a key in the desired way,
> + * but permit the security modules to override
> + */
> +int key_task_permission(const key_ref_t key_ref,
> +			struct task_struct *context,
> +			key_perm_t perm)
> +{
> +	struct key *key;
> +	key_perm_t kperm;
> +	int ret;
> +
> +	/* let the security module have first say
> +	 * - it should return:
> +	 *	+ve to grant access
> +	 *	0   to deny access
> +	 *	-ve to fall back to normal permission checking
> +	 */
> +	ret = security_key_permission(key_ref, context, perm);
> +	if (ret >= 0)
> +		return ret;

This is not right.  Do normal permissions check first.  Iff they pass,
then allow security module to check labels.  And simply return 0 on
success and -ERR on error.

> +	/* normal permissions checking */
> +	key = key_ref_to_ptr(key_ref);
> +
> +	/* use the top 8-bits of permissions for keys the caller possesses */
> +	if (is_key_possessed(key_ref)) {
> +		kperm = key->perm >> 24;
> +		goto use_these_perms;
> +	}
> +
> +	/* use the second 8-bits of permissions for keys the caller owns */
> +	if (key->uid == context->fsuid) {
> +		kperm = key->perm >> 16;
> +		goto use_these_perms;
> +	}
> +
> +	/* use the third 8-bits of permissions for keys the caller has a group
> +	 * membership in common with */
> +	if (key->gid != -1 && key->perm & KEY_GRP_ALL) {
> +		if (key->gid == context->fsgid) {
> +			kperm = key->perm >> 8;
> +			goto use_these_perms;
> +		}
> +
> +		task_lock(context);
> +		ret = groups_search(context->group_info, key->gid);
> +		task_unlock(context);
> +
> +		if (ret) {
> +			kperm = key->perm >> 8;
> +			goto use_these_perms;
> +		}
> +	}
> +
> +	/* otherwise use the least-significant 8-bits */
> +	kperm = key->perm;
> +
> +use_these_perms:
> +	kperm = kperm & perm & KEY_ALL;
> +
> +	return kperm == perm;
> +
> +} /* end key_task_permission() */
> +
> +EXPORT_SYMBOL(key_task_permission);

EXPORT_SYMBOL_GPL looks more appropriate here.

thanks,
-chris