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