lkml.org 
[lkml]   [2004]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 14:05    [W:0.438 / U:0.820 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site