From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759369AbcDAP4W (ORCPT ); Fri, 1 Apr 2016 11:56:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53541 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751450AbcDAP4T convert rfc822-to-8bit (ORCPT ); Fri, 1 Apr 2016 11:56:19 -0400 Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells In-Reply-To: <1458607577-25578-1-git-send-email-k.marinushkin@gmail.com> References: <1458607577-25578-1-git-send-email-k.marinushkin@gmail.com> To: Kirill Marinushkin Cc: dhowells@redhat.com, linux-kernel@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org Subject: Re: [PATCH] Security: Keys: Added derived keytype MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <31459.1459526175.1@warthog.procyon.org.uk> Content-Transfer-Encoding: 8BIT Date: Fri, 01 Apr 2016 16:56:15 +0100 Message-ID: <31460.1459526175@warthog.procyon.org.uk> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Kirill Marinushkin 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