Re: [PATCH] Keys: Add possessor permissions to keys

[Andrew Morton <akpm@osdl.org>]

David Howells <dhowells@redhat.com> wrote:
>
> 
> The attached patch adds extra permission grants to keys for the possessor of a
> key in addition to the owner, group and other permissions bits. This makes
> SUID binaries easier to support without going as far as labelling keys and key
> targets using the LSM facilities.
> 
> This patch adds a second "pointer type" to key structures (struct key_ref *)
> that can have the bottom bit of the address set to indicate the possession of
> a key. This is propagated through searches from the keyring to the discovered
> key. It has been made a separate type so that the compiler can spot attempts
> to dereference a potentially incorrect pointer.

The above bit needs to be captured in a code comment.  Because:

> ...
>  /*
> + * key reference with possession flag handling
> + */
> +static inline struct key_ref *key_mkref(const struct key *key, unsigned long possession)
> +{
> +	return (struct key_ref *) ((unsigned long) key | possession);
> +}

Is hair-raising and makes people want to come after you with a stick ;)

(And an 80-col xterm)

ugh, I see.  `struct key_ref' doesn't actually exist anywhere.  The code
only ever deals with pointers to this non-existent structure and they are
munged `struct key *'s.

Did this _have_ to happen?

> +	}
> +	else if (key->uid == context->fsuid) {

Documentation/CodingStyle?

> +		       "%s;%d;%d;%08x;%s",
> +		       key_deref(key)->type->name,
> +		       key_deref(key)->uid,
> +		       key_deref(key)->gid,
> +		       key_deref(key)->perm,
> +		       key_deref(key)->description ? key_deref(key)->description : ""
>  		       );

This doesn't actually make things clear.

> +			if (PTR_ERR(key_ref) != -EAGAIN) {
> +				if (IS_ERR(key_ref))
> +					key = key_deref(key_ref);
> +				else
> +					key = ERR_PTR(PTR_ERR(key_ref));
> +				break;
> +			}
> +		}

That's getting a bit intimate with how IS_ERR and PTR_ERR are implemented
but I guess we're unlikely to change that.

This all seems quite inappropriate to -rc2?