Re: [PATCH] implement in-kernel keys & keyring management

[Andrew Morton <akpm@osdl.org>]

David Howells <dhowells@redhat.com> wrote:
>
> I've made available a patch that does a better job of key and keyring
>  management for authentication, cryptography, etc.. I've added a good bit of
>  documentation and I've commented the code more thoroughly.

Nicely presented patch, thanks.

- Why have a separate key_user structure, rather than adding stuff to
  struct user_struct?  This appears to be feasible.

- I had a hard time working out the locking which protects key->flags. 
  It looks racy.

- Are the various system-wide locks going to end up hurting on big SMP?

- user_describe_key() appears to leak a key ref if kmalloc() fails. 
  user_describe_key() appears to leak a key ref if copy_to_user() faults. 
  The one-return-statement-per-function rule rules.

- Various people are likely to get upset about this:

  asmlinkage long sys_keyctl(int option, unsigned long arg2, unsigned long arg3,
		   unsigned long arg4, unsigned long arg5)

  I guess the pure way to do it is to add 13 new syscalls....

- I really hesitate to stick my nose into this perennial, but
  keyring_hash() is a bit lame.  I once read that

	hash = hash * 33 + *p++;

  works as well as pretty much anything else.

- All that keyring tree management (keyring_detect_cycle) looks tricky. 
  Do we really need such complexity?

- __key_link() does

	/* dispose of the old keyring list */
	if (klist)
		kfree(klist);

  but elsewhere you freely do kfree(0)

- Why does the proc code do spin_lock_irq(&key_user_lock)?  Other
  process-context code just does spin_lock().

- In key_create_or_update():

	mode = 0100;
	if (ktype == &key_type_keyring)
		mode = 0700;
	else if (ktype == &key_type_user)
		mode = 0700;
	else if (ktype->update)
		mode |= 0300;

  you've used the stat.h symbols elsewhere.

- How come install_process_keyring() and friends take task_lock() around
  the key_put()?  It's not done elsewhere.

  Please update the what-it-locks comment over task_lock()

- Documentation/CodingStyle doesn't mention

		if (clone_flags & CLONE_THREAD) {
			atomic_inc(&tsk->process_keyring->usage);
		}
		else {

- join_session_keyring() leaks the memory at `name'.  Trivial DoS hole. 
  Please audit the whole patch.

- request_key_construction() leaks `key' if __call_request_key() fails. 
  Multiple return statements again...

- request_key() appears to leak a ref to the key if key_user_lookup()
  fails.  Multiple return statements.

- In user_instantiate():

	if (!key->payload.data) {
		key_put(key->payload.data);

  the key_put() can be removed.


It's a lot of code, isn't it?