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

[David Howells]

Chris Wright <chrisw@xxxxxxxx> wrote:
> Here's a few comments/questions from first pass-thru.  Looks good so
> far.  I have yet to fully digest it all, or run your current
> patchset/toolset to play with it.
> 
> it's still tough to walk away from the idea that this is really close to
> a filesystem interface.

It doesn't map especially well into a filesystem. I'm trying to implement a
keyfs, but there are some problems with doing so. Some of these problems can
be avoided by making keys into inodes in keyfs, but I want to keep memory
usage down. Inodes are way overkill. Basically doing that would make it into a
ramfs of sorts.

> sys_keyctl prototype declaration in key.h?  maybe in syscalls.h?
> also, the #else /* !CONFIG_KEYS */ for doesn't look right.  i think
> the syscall should always be there, instead use cond_syscall.

Currently sys_keyctl() is called from prctl() rather than having a syscall of
its own, or fanning the various functions out into separate syscalls. This
means I don't need to modify glibc, all the kernel arch dirs, and I don't need
to provide 32->64 bit conversion, since what's already done for prctl() is
sufficient.

If Andrew is shows a willingness to take the patch, I'd be willing to break
out the syscalls at that point if it's deemed necessary. I suppose I could do
that now, but only provide it on x86, and leave syscall addition to the
various arch maintainers.

Actually, doing that might be a good idea anyway... If I break key creation
out, at least, I can clean up its interface so that I don't have to store the
payload length in with the data.

> prctl case statement.  are case ranges supported in all gcc versions
> that are valid for 2.6 kernel compilation?

Shhh! :-)

I don't know, but Andrew hasn't complained yet, so I guess it must be:-)

> key_euid_changed/key_egid_changed will change the process_keyring even
> if it's just a thread which did the setuid/gid.  this doesn't sound
> right.

I know. If I don't do it, then a single-threaded process will lose access to
its process keyring if it changes process UID/GID; it could change its keyring
ownership manually, and it probably shouldn't be allowed to do that if it
doesn't have root privileges.

> i'm a little confused by suid_keys().  it's sprinkled in various spots,
> yet the function does nothing.  what's the intention there (esp. w.r.t.
> key_euid_changed)?  e.g.  placement in compute_creds is before the actual
> process uid updates are done.

It originally set up new keyrings for or cleared the thread, process and
session keyrings. However, it seems that this isn't a good idea. I've left the
hooks in in case someone comes up with a good strategy.

> why check pid ==1 and uid ==0 in exec_keys?

Well, the init process starts off without any keyrings attached. If it doesn't
have a session keyring attached and it's the init process exec'ing itself, it
automatically gets subscribed to the root keyring.

An alternative way of doing things that might work better, is to start off by
giving init no session keyring, and only force subscription to one when an
attempt is made to use it; and at that point subscribe the process to the
default session ring for its owning user.

> why not switch_uid_keyring for non root_session_keyrings?

I'd like login to pick up the user's keyring automatically if the user isn't
root and the process's current keyring is root's default without having to
bother PAM.

If I implement what I suggested above, this would get fixed too, I think.

> in alloc_uid_keyring, keyring_alloc failure for session_keyring leaks
> uid_keyring allocation.

Hmmm... you're right. I'd made the assumption that the caller would clean up,
but that isn't true, is it?

I've fixed that in my tree.

> key_user can't be squished into user_struct?  seems like the quota stuff
> could become setable from userspace (rlimit-like).

No:

 (1) A key has to pin the quota tracking struct in memory so that a user can't
     gain more quota (a key or keyring contributing to a user's quota could
     have been linked to by someone else).

 (2) The quota tracking information has to persist as long as any keys that
     contribute to it do.

 (3) user_struct pins two keyrings. These keyrings, and possibly keys they
     contain pin the quota tracking struct.

 (4) If user_struct was the quota tracking struct, you'd end up with a
     reference cycle, meaning that the user_struct and its associated baggage
     could never be discarded.

I could make it so that if a user_struct is discarded, then every key owned by
that user is withdrawn from the system, but that's not necessarily easy or
desirable.

It might make sense to split the user_struct into two: a statistics record and
a struct full of pointers that can pin things, including the statistics
record.

> /sbin/request-key should probably path configurable like /sbin/hotplug.

Yeah... That's an optimisation for later, I think. If Andrew or Linus takes
that patch, I'll fix that up.

David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/