| Message ID | 153549644916.4089.12258485183075906901.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adding security support for nvdimm | expand |
Dave Jiang <dave.jiang@intel.com> wrote: > +static void nvdimm_unregister_keyring(void) > +{ > + key_revoke(nvdimm_keyring); > +} You need key_put() as well otherwise you'll leak a ref. David
On Tue, Aug 28, 2018 at 3:47 PM Dave Jiang <dave.jiang@intel.com> wrote: > > Prepping the libnvdimm to support security management by adding a keyring > in order to provide passphrase management through the kernel key management > APIs. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/nvdimm/core.c | 7 ++++++- > drivers/nvdimm/dimm_devs.c | 29 ++++++++++++++++++++++++++++- > drivers/nvdimm/nd-core.h | 1 + > include/linux/libnvdimm.h | 3 +++ > kernel/cred.c | 1 + > 5 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c > index acce050856a8..3cd33d5c7cf0 100644 > --- a/drivers/nvdimm/core.c > +++ b/drivers/nvdimm/core.c > @@ -437,9 +437,12 @@ static __init int libnvdimm_init(void) > { > int rc; > > - rc = nvdimm_bus_init(); > + rc = nvdimm_devs_init(); > if (rc) > return rc; > + rc = nvdimm_bus_init(); > + if (rc) > + goto err_bus; > rc = nvdimm_init(); > if (rc) > goto err_dimm; > @@ -454,6 +457,8 @@ static __init int libnvdimm_init(void) > nvdimm_exit(); > err_dimm: > nvdimm_bus_exit(); > + err_bus: > + nvdimm_devs_exit(); > return rc; > } > > diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c > index 8d348b22ba45..586432450c82 100644 > --- a/drivers/nvdimm/dimm_devs.c > +++ b/drivers/nvdimm/dimm_devs.c > @@ -18,12 +18,15 @@ > #include <linux/io.h> > #include <linux/fs.h> > #include <linux/mm.h> > +#include <linux/key.h> > +#include <linux/init_task.h> > #include "nd-core.h" > #include "label.h" > #include "pmem.h" > #include "nd.h" > > static DEFINE_IDA(dimm_ida); > +static struct key *nvdimm_keyring; > > /* > * Retrieve bus and dimm handle and return if this bus supports > @@ -668,7 +671,31 @@ int nvdimm_bus_check_dimm_count(struct nvdimm_bus *nvdimm_bus, int dimm_count) > } > EXPORT_SYMBOL_GPL(nvdimm_bus_check_dimm_count); > > -void __exit nvdimm_devs_exit(void) > +static int nvdimm_register_keyring(void) > { > + nvdimm_keyring = keyring_alloc(".nvdimm", > + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, &init_cred, [..] > diff --git a/kernel/cred.c b/kernel/cred.c > index ecf03657e71c..3a161b78a0a4 100644 > --- a/kernel/cred.c > +++ b/kernel/cred.c > @@ -64,6 +64,7 @@ struct cred init_cred = { > .user_ns = &init_user_ns, > .group_info = &init_groups, > }; > +EXPORT_SYMBOL(init_cred); I think you want to use prepare_kernel_cred(), not export init_cred.
Dave Jiang <dave.jiang@intel.com> wrote: > + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, &init_cred, Hmmm... I wonder if current_cred() would suffice since you must be called from something that has the ability to load modules. Further, I wonder if module loading should be wrapped in an override with init_cred in the core. > + (KEY_USR_ALL & ~KEY_USR_SETATTR), Did you really want to give the user write access, btw? David
Dan Williams <dan.j.williams@intel.com> wrote:
> I think you want to use prepare_kernel_cred(), not export init_cred.
That only works if the searching is done with the creds generated by
prepare_kernel_cred(). He probably does want init_cred, or at least
current_cred() if module loading overrides the creds with init_cred whilst
initialising the module.
David
On 09/24/2018 02:04 PM, David Howells wrote: > Dan Williams <dan.j.williams@intel.com> wrote: > >> I think you want to use prepare_kernel_cred(), not export init_cred. > > That only works if the searching is done with the creds generated by > prepare_kernel_cred(). He probably does want init_cred, or at least > current_cred() if module loading overrides the creds with init_cred whilst > initialising the module. > > David > Can I pass in NULL or is some sort of cred is necessary?
On 09/24/2018 02:02 PM, David Howells wrote: > Dave Jiang <dave.jiang@intel.com> wrote: > >> + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, &init_cred, > > Hmmm... I wonder if current_cred() would suffice since you must be called > from something that has the ability to load modules. Further, I wonder if > module loading should be wrapped in an override with init_cred in the core. > >> + (KEY_USR_ALL & ~KEY_USR_SETATTR), > > Did you really want to give the user write access, btw? Hmmm....maybe I don't want user access at all since this is a kernel key ring? > > David >
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c index acce050856a8..3cd33d5c7cf0 100644 --- a/drivers/nvdimm/core.c +++ b/drivers/nvdimm/core.c @@ -437,9 +437,12 @@ static __init int libnvdimm_init(void) { int rc; - rc = nvdimm_bus_init(); + rc = nvdimm_devs_init(); if (rc) return rc; + rc = nvdimm_bus_init(); + if (rc) + goto err_bus; rc = nvdimm_init(); if (rc) goto err_dimm; @@ -454,6 +457,8 @@ static __init int libnvdimm_init(void) nvdimm_exit(); err_dimm: nvdimm_bus_exit(); + err_bus: + nvdimm_devs_exit(); return rc; } diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c index 8d348b22ba45..586432450c82 100644 --- a/drivers/nvdimm/dimm_devs.c +++ b/drivers/nvdimm/dimm_devs.c @@ -18,12 +18,15 @@ #include <linux/io.h> #include <linux/fs.h> #include <linux/mm.h> +#include <linux/key.h> +#include <linux/init_task.h> #include "nd-core.h" #include "label.h" #include "pmem.h" #include "nd.h" static DEFINE_IDA(dimm_ida); +static struct key *nvdimm_keyring; /* * Retrieve bus and dimm handle and return if this bus supports @@ -668,7 +671,31 @@ int nvdimm_bus_check_dimm_count(struct nvdimm_bus *nvdimm_bus, int dimm_count) } EXPORT_SYMBOL_GPL(nvdimm_bus_check_dimm_count); -void __exit nvdimm_devs_exit(void) +static int nvdimm_register_keyring(void) { + nvdimm_keyring = keyring_alloc(".nvdimm", + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, &init_cred, + (KEY_POS_ALL & ~KEY_POS_SETATTR) | + (KEY_USR_ALL & ~KEY_USR_SETATTR), + KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL); + if (IS_ERR(nvdimm_keyring)) + return PTR_ERR(nvdimm_keyring); + + return 0; +} + +static void nvdimm_unregister_keyring(void) +{ + key_revoke(nvdimm_keyring); +} + +int __init nvdimm_devs_init(void) +{ + return nvdimm_register_keyring(); +} + +void nvdimm_devs_exit(void) +{ + nvdimm_unregister_keyring(); ida_destroy(&dimm_ida); } diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h index 79274ead54fb..2af0f89c4010 100644 --- a/drivers/nvdimm/nd-core.h +++ b/drivers/nvdimm/nd-core.h @@ -76,6 +76,7 @@ static inline bool is_memory(struct device *dev) struct nvdimm_bus *walk_to_nvdimm_bus(struct device *nd_dev); int __init nvdimm_bus_init(void); void nvdimm_bus_exit(void); +int nvdimm_devs_init(void); void nvdimm_devs_exit(void); void nd_region_devs_exit(void); void nd_region_probe_success(struct nvdimm_bus *nvdimm_bus, struct device *dev); diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h index 472171af7f60..51084043915c 100644 --- a/include/linux/libnvdimm.h +++ b/include/linux/libnvdimm.h @@ -155,6 +155,9 @@ static inline struct nd_blk_region_desc *to_blk_region_desc( } +#define NVDIMM_PASSPHRASE_LEN 32 +#define NVDIMM_KEY_DESC_LEN 22 + void badrange_init(struct badrange *badrange); int badrange_add(struct badrange *badrange, u64 addr, u64 length); void badrange_forget(struct badrange *badrange, phys_addr_t start, diff --git a/kernel/cred.c b/kernel/cred.c index ecf03657e71c..3a161b78a0a4 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -64,6 +64,7 @@ struct cred init_cred = { .user_ns = &init_user_ns, .group_info = &init_groups, }; +EXPORT_SYMBOL(init_cred); static inline void set_cred_subscribers(struct cred *cred, int n) {