Messages in this thread | | | Date | Sat, 7 Aug 2004 01:17:58 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] implement in-kernel keys & keyring management |
| |
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? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |