Re: [PATCH] Security: Keys: Added derived keytype

From: David Howells
Date: Fri Apr 01 2016 - 11:56:26 EST


Kirill Marinushkin <k.marinushkin@xxxxxxxxx> wrote:

> For details see
> Documentation/security/keys-derived.txt

Please include at least a summary in the patch description, not just a pointer
to the documentation file.

> + Derived Keys
> +
> +Derived is a keytype of the kernel keyring facility.
> +The key secret is derived from the secret value given by user.

I'm not keen on the type name "derived" as it's totally non-obvious. How
about "secret", "shared-secret" or "salted" or something like that.

> + i=, iterations= - number of itaretions,

"iterations"

> +#ifndef INCLUDE_KEYS_DERIVED_TYPE_H_
> +#define INCLUDE_KEYS_DERIVED_TYPE_H_

I would drop the initial "INCLUDE_" from that.

> +extern int derived_instantiate(struct key *key,
> + struct key_preparsed_payload *prep);
> +extern int derived_update(struct key *key,
> + struct key_preparsed_payload *prep);
> +extern long derived_read(const struct key *key,
> + char __user *buffer, size_t buflen);
> +extern void derived_revoke(struct key *key);
> +extern void derived_destroy(struct key *key);

Is there a reason you're exporting all the methods?

> +struct derived_key_payload {

Should this struct go in your type header?

> + struct rcu_head rcu; /* RCU destructor */
> + char *alg_name; /* null-terminated digest algorithm name */
> + char *rng_name; /* null-terminated random generator algorithm name */
> + u64 iter; /* number of iterations */

Isn't the max value for this 0x000FFFFF? If so, why is it u64?

> + unsigned int saltlen; /* length of salt */
> + unsigned char *salt; /* salt */
> + unsigned int datalen; /* length of derived data */
> + unsigned char *data; /* derived data */

Reorder these to put saltlen and datalen next to each other, thereby
eliminating two holes in the struct on a 64-bit machine.


> +static int gen_random(const char *rnd_name, u8 *buf, unsigned int len)

Prefix with "derived_" please.

> + case OPT_FORMAT_RAND:
> + if (kstrtouint(v[i].b->data, 0, &tempu)
> + || tempu == 0
> + || tempu > RAND_MAX_SIZE) {
> + pr_err(PREFIX "invalid random size");
> + return -EINVAL;
> + }
> + v[i].b->data = kmalloc(tempu, GFP_KERNEL);
> + if (!v[i].b->data) {
> + pr_err(PREFIX "random data alloc failed");
> + return -ENOMEM;
> + }
> + *v[i].b->lenp = tempu;
> + ret = gen_random(payload->rng_name, v[i].b->data, *v[i].b->lenp);

I would move the kmalloc() inside the gen_random() function.

> +static void free_payload_content(struct derived_key_payload *payload)
> +{
> + if (payload->alg_name)
> + kzfree(payload->alg_name);
> + if (payload->rng_name)
> + kzfree(payload->rng_name);
> + if (payload->data)
> + kzfree(payload->data);
> + if (payload->salt)
> + kzfree(payload->salt);
> +}

kzfree() can handle a NULL pointer. You've got more instances of this.

Your functions should all be prefixed with "derived_".

> + sdesc = kzalloc(sizeof(struct shash_desc) + crypto_shash_descsize(sh), GFP_KERNEL);

Do you need some wrappers on this to get the alignment correct?

> + if (!sdesc) {
> + pr_err(PREFIX "sdesc alloc failed");

Don't print an error here.

> + ret = -ENOMEM;
> + goto out;
> + }

You should stick a label about four lines below "out:" and go there instead.
Then you can get rid of the conditionalisation in the following:

+ if (!IS_ERR(sh))
+ crypto_free_shash(sh);

> + payload = kzalloc(sizeof(*payload), GFP_KERNEL);
> + if (!payload) {
> + pr_err(PREFIX "payload alloc failed");
> + return -ENOMEM;
> + }
> +
> + /* fill payload */
> + ret = fill_payload(payload, prep);

Move the kzalloc() call into fill_payload().

> +int derived_update(struct key *key, struct key_preparsed_payload *prep)
> +{
> + int ret = -EINVAL;
> + struct derived_key_payload *payload =
> + (struct derived_key_payload *)key->payload.data;
> +
> + /* free current payload */
> + free_payload_content(payload);
> + memset(payload, 0x00, sizeof(*payload));
> +
> + ret = fill_payload(payload, prep);
> + if (!ret)
> + ret = reserve_derived_payload(key, payload);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(derived_update);

This is *not* RCU safe.

You should implement the ->preparse() method and do the argument parsing and
creation and filling in of struct derived_key_payload there. Take a look at
user_preparse(). I would start by renaming fill_payload() to
derived_preparse() - it's almost exactly what you want.

You will also need to implement ->free_preparse().

You can then get rid of reserve_derived_payload() and just put the quota
amount into prep->quotalen and the payload into prep->payload.data[0].

derived_instantiate() can then be replaced with generic_key_instantiate.

Since derived_preparse() would be called prior to derived_update(), the latter
can just replace where prep->payload.data[0] points using
rcu_assign_keypointer() and then call_rcu() on the old payload.

David