| Message ID | 153936863308.55836.2972520178944977338.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] libnvdimm: fix updating of kernel key during nvdimm key update | expand |
On Fri, Oct 12, 2018 at 11:23 AM Dave Jiang <dave.jiang@intel.com> wrote: > > There are several issues WRT kernel key update when we are doing nvdimm > security key update. > > 1. The kernel key created needs to have proper permission for update > 2. We need to check key_update() return value and make sure it didn't fail > 3. We need to not hold the key->sem when calling key_update() since it will > call down_write() when doing modification to the key. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/nvdimm/security.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c > index 776c440a02ef..ef83bdf47c31 100644 > --- a/drivers/nvdimm/security.c > +++ b/drivers/nvdimm/security.c > @@ -27,7 +27,7 @@ static struct key *make_kernel_key(struct key *key) > > new_key = key_alloc(&key_type_logon, key->description, > GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(), > - KEY_POS_SEARCH, KEY_ALLOC_NOT_IN_QUOTA, NULL); > + KEY_POS_ALL, KEY_ALLOC_NOT_IN_QUOTA, NULL); Should that be (KEY_POS_ALL & ~KEY_POS_SETATTR)? I don't see a reason why we would ever change key attributes. > if (IS_ERR(new_key)) > return NULL; > > @@ -419,11 +419,19 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm, > dev_warn(dev, "key update failed: %d\n", rc); > > if (old_key) { > + up_read(&old_key->sem); > /* copy new payload to old payload */ Let's delete that comment and replace it with one that talks about the locking rules. Namely that we are done with the old_key payload after ->change_key() returns and that key_update() will take the write lock on the payload to update it. > - if (rc == 0) > - key_update(make_key_ref(old_key, 1), new_data, > + if (rc == 0) { > + rc = key_update(make_key_ref(old_key, 1), new_data, > old_key->datalen); > - up_read(&old_key->sem); > + if (rc < 0) { > + dev_warn(dev, > + "kernel key update failed: %d\n", rc); > + key_invalidate(old_key); > + key_put(old_key); I added a local 'key_destroy()' helper for that invalidate+put pattern.
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c index 776c440a02ef..ef83bdf47c31 100644 --- a/drivers/nvdimm/security.c +++ b/drivers/nvdimm/security.c @@ -27,7 +27,7 @@ static struct key *make_kernel_key(struct key *key) new_key = key_alloc(&key_type_logon, key->description, GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(), - KEY_POS_SEARCH, KEY_ALLOC_NOT_IN_QUOTA, NULL); + KEY_POS_ALL, KEY_ALLOC_NOT_IN_QUOTA, NULL); if (IS_ERR(new_key)) return NULL; @@ -419,11 +419,19 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm, dev_warn(dev, "key update failed: %d\n", rc); if (old_key) { + up_read(&old_key->sem); /* copy new payload to old payload */ - if (rc == 0) - key_update(make_key_ref(old_key, 1), new_data, + if (rc == 0) { + rc = key_update(make_key_ref(old_key, 1), new_data, old_key->datalen); - up_read(&old_key->sem); + if (rc < 0) { + dev_warn(dev, + "kernel key update failed: %d\n", rc); + key_invalidate(old_key); + key_put(old_key); + nvdimm->key = NULL; + } + } } up_read(&key->sem);
There are several issues WRT kernel key update when we are doing nvdimm security key update. 1. The kernel key created needs to have proper permission for update 2. We need to check key_update() return value and make sure it didn't fail 3. We need to not hold the key->sem when calling key_update() since it will call down_write() when doing modification to the key. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/nvdimm/security.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)