Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH] Security: Keys: Added derived keytype | Date | Fri, 01 Apr 2016 16:56:15 +0100 |
| |
Kirill Marinushkin <k.marinushkin@gmail.com> 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
| |