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

[David Howells <dhowells@redhat.com>]

James Morris <jmorris@redhat.com> wrote:
> Here's some more feedback:
> 
>  typedef int32_t key_serial_t;
> 
> Why is this signed?

So I can have special values that are negative. I suppose it doesn't really
matter - they could be small positive numbers or something, but then if I want
to add one later, you get the possibility of overlap on a userspace that
supports one running with a kernel that doesn't.

> And does this really need to be a typedef? (Do you forsee it ever changing
> from 32-bit?).

No... but then 640KB of memory is enough for anyone, right? :-)

> For consistency, request_key(), validate_key() and lookup_key() should 
> probably be of the form key_request() etc.  There are other similar 
> cases throughout the code.

Maybe. Though I think request_key() should follow the form of similar
functions inside the kernel, such as request_firmware().

> I would suggest that the /sbin/request-key interface be done via Netlink
> messaging instead.

Other people argued the exact opposite first.

> 
>   #define sys_keyctl(o,b,c,d,e)          (-EINVAL)
> 
> This should probably be -ENOSYS.

If it becomes a real syscall rather than being a subset of prctl(), then yes.

> -                   capable(CAP_SETGID))
> +                   capable(CAP_SETGID)) {
>                         new_egid = egid;
> +               }
> 
> This looks superfluous.

Yes. I had added an additional statement into there at one point.

> We need to look at the implications for LSM, e.g. keys have Unix style
> access control information attached, and LSM apps may want to extend this
> to other security models.  Some of the user interface calls may also need
> to be mediated via LSM.

True. I don't know much about LSM though.

David