Re: [PATCH] implement in-kernel keys & keyring management
From: Andrew Morton (akpm_at_osdl.org)
Date: 08/07/04
- Previous message: William Lee Irwin III: "Re: Hugetlb demanding paging for -mm tree"
- In reply to: David Howells: "[PATCH] implement in-kernel keys & keyring management"
- Next in thread: David Howells: "[PATCH] implement in-kernel keys & keyring management [try #2]"
- Reply: David Howells: "[PATCH] implement in-kernel keys & keyring management [try #2]"
- Reply: Greg KH: "Re: [PATCH] implement in-kernel keys & keyring management"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Sat, 7 Aug 2004 01:17:58 -0700 To: David Howells <dhowells@redhat.com>
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/
- Previous message: William Lee Irwin III: "Re: Hugetlb demanding paging for -mm tree"
- In reply to: David Howells: "[PATCH] implement in-kernel keys & keyring management"
- Next in thread: David Howells: "[PATCH] implement in-kernel keys & keyring management [try #2]"
- Reply: David Howells: "[PATCH] implement in-kernel keys & keyring management [try #2]"
- Reply: Greg KH: "Re: [PATCH] implement in-kernel keys & keyring management"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
