Re: [RFC][PATCH 0/9] encrypted keys & key control op

[Mimi Zohar]

On Mon, 2013-11-11 at 22:34 +0000, David Howells wrote:
> Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > > Further, the existence of encrypted_update() means that add_key() will
> > > sometimes get things wrong with encrypted keys (add_key() will call
> > > ->update() if a matching key already exists rather than creating a new
> > > key).
> > 
> > I see.  The key_type structure defines a number of methods,
> > including .instantiate and .update.  I would have thought that
> > only .update would be allowed to update an existing key.
> 
> That is correct.  ->instantiate() is only called for a new key and ->update()
> for an existing key.
> 
> > Instantiating a new key should create a new key or fail, not update a key.
> 
> That is subjective and depends on how you want your interface to work.  If you
> look upon it as the "key struct" is a box with some content and the content is
> switchable, then does it matter?

I think it does.  (Example below)

> And, yes, ->update() should probably have been called something like replace
> or reinstantiate.  The benefits of hindsight...

I don't have a problem with the existing name.

> > I'm sure there is/was a good reason for add_key() to do both.
> 
> Yes.  No race.
> 
> > > But you can't pre-search for the existence of a key and mould the payload
> > > accordingly because that means you can race against both add_key() and
> > > keyctl_unlink().
> > 
> > Would this still be the case, if you differentiated between
> > instantiating and updating a key?
> 
> Yes.  Imagine, you try to add a key and it gets rejected because the key
> already exists.  You then try to update the existing key, but that gets
> rejected because someone unlinked the key in the meantime.  So you try and add
> it again, but this now fails because someone added a new key.  Repeat.

A counter example would be two processes, having nothing to do with each
other, attempt to a create a key with the same name.  Instead of each
process getting its own key, they land up sharing the same key.
Not only are they sharing the same key, but neither process knows that
there is another process using the same key.  I would think this is a
bigger problem.

Failing to create/update a key, at least to me, seems safer than having
two apps trying to create a key with same name, but instead land up
using the same key.

> Or add_key() could immediately displace a key someone else just added, leaving
> them with a key ID that disappeared as soon as it was returned due to an
> add/add race.

This is a separate issue.  If a key/keyring exists, a new key/keyring,
with the same name, should not be created replacing the existing
key/keyring.  It should simply fail.  (Removing a key/keyring first,
before creating a key/keyring of the same name, is different.)

Mimi

> The right behaviour can be argued in different ways.

> > > My solution is to add a new keyctl function that allows the caller to
> > > perform a type-specific operation on a key:
> > > 
> > > 	long keyctl_control(key_serial_t key_id,
> > > 			    const char *command,
> > > 			    char *reply_buffer,
> > > 			    size_t reply_size);
> > 
> > > This would then take a NUL-terminated string indicating the command and
> > > arguments and potentially return a reply up to the buffer length.
> > 
> > What is the usecase scenario that would require a reply_buffer?
> 
> I don't want to have to add a keyctl_control2() down the line that has a
> reply_buffer if this one doesn't when someone finds a use it if it's easy
> enough and small enough to provide the facility here.  This is aimed at being
> a general interface rather than being specifically for encrypted keys.
> 
> 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/