| Message ID | 153186087803.27463.7423668214880824595.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dave Jiang <dave.jiang@intel.com> wrote: > Add support for setting and/or updating passphrase on the Intel nvdimms. > The passphrase is pulled from userspace through the kernel key management. > We trigger the update via writing "update" to the sysfs attribute > "security". The state of the security can also be read via the "security" > attribute. libnvdimm will generically support the key_change API call. Btw, could you use a logon-type key rather than spinning your own? Do you specifically not want it to be updateable? > +static int intel_dimm_security_update_passphrase( > + struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, > + struct nvdimm_key_data *old_data, > + struct nvdimm_key_data *new_data) You might want to mark old_data and new_data as const here. At least, they don't appear to be changed. Also, can you stick a banner comment on the function to say what it actually does? Is it changing the key stored in h/w or is it meant to be changing what's stored in the key payload? > + /* request new key from userspace */ > + key = nvdimm_request_key(dev); > + if (!key) { > + dev_dbg(dev, "%s: failed to acquire new key\n", __func__); > + return -ENXIO; > + } > + > + dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial); > + > + if (key->datalen == NVDIMM_PASSPHRASE_LEN) { > + old_data = NULL; > + new_data = key->payload.data[0]; > + } else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) { > + new_data = key->payload.data[0]; > + old_data = new_data + NVDIMM_PASSPHRASE_LEN; > + } else { > + key_invalidate(key); > + key_put(key); > + dev_warn(dev, "Incorrect key payload size for update: %u\n", > + key->datalen); > + return -EINVAL; > + } > + > + down_read(&key->sem); > + rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data, > + new_data); Why do you need to get a read lock on key->sem and not a write-lock? > + if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) > + memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN); You appear to be changing the key content here. If that's the case, don't you need to write-lock key->sem? > + up_read(&key->sem); David
Dave Jiang <dave.jiang@intel.com> wrote: > + /* look for a key from keyring if exists and remove */ > + old_key = nvdimm_search_key(dev); > + if (old_key) { > + dev_dbg(dev, "%s: killing old key: %#x\n", > + __func__, old_key->serial); > + key_invalidate(old_key); > + key_put_sync(old_key); > + } Why don't you unlink the key from the .nvdimm keyring here? David
On 07/18/2018 04:14 AM, David Howells wrote: > Dave Jiang <dave.jiang@intel.com> wrote: > >> Add support for setting and/or updating passphrase on the Intel nvdimms. >> The passphrase is pulled from userspace through the kernel key management. >> We trigger the update via writing "update" to the sysfs attribute >> "security". The state of the security can also be read via the "security" >> attribute. libnvdimm will generically support the key_change API call. > > Btw, could you use a logon-type key rather than spinning your own? Do you > specifically not want it to be updateable? Thanks for reviewing this David. I'll take a look and see if I can. Basically the code is acting as the proxy for the hardware and passing through the passphrase. What would be the best way to update the passphrase if I need to retrieve the new passphrase from the userspace? The hardware still needs the old passphrase in order to verify. There has to be a nice way to do this but I'm not familiar with key management API to know better. > >> +static int intel_dimm_security_update_passphrase( >> + struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, >> + struct nvdimm_key_data *old_data, >> + struct nvdimm_key_data *new_data) > > You might want to mark old_data and new_data as const here. At least, they > don't appear to be changed. Also, can you stick a banner comment on the > function to say what it actually does? Is it changing the key stored in h/w > or is it meant to be changing what's stored in the key payload? > >> + /* request new key from userspace */ >> + key = nvdimm_request_key(dev); >> + if (!key) { >> + dev_dbg(dev, "%s: failed to acquire new key\n", __func__); >> + return -ENXIO; >> + } >> + >> + dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial); >> + >> + if (key->datalen == NVDIMM_PASSPHRASE_LEN) { >> + old_data = NULL; >> + new_data = key->payload.data[0]; >> + } else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) { >> + new_data = key->payload.data[0]; >> + old_data = new_data + NVDIMM_PASSPHRASE_LEN; >> + } else { >> + key_invalidate(key); >> + key_put(key); >> + dev_warn(dev, "Incorrect key payload size for update: %u\n", >> + key->datalen); >> + return -EINVAL; >> + } >> + >> + down_read(&key->sem); >> + rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data, >> + new_data); > > Why do you need to get a read lock on key->sem and not a write-lock? > >> + if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) >> + memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN); > > You appear to be changing the key content here. If that's the case, don't you > need to write-lock key->sem? > >> + up_read(&key->sem); > > David >
On 07/18/2018 09:05 AM, Dave Jiang wrote: > > > On 07/18/2018 04:14 AM, David Howells wrote: >> Dave Jiang <dave.jiang@intel.com> wrote: >> >>> Add support for setting and/or updating passphrase on the Intel nvdimms. >>> The passphrase is pulled from userspace through the kernel key management. >>> We trigger the update via writing "update" to the sysfs attribute >>> "security". The state of the security can also be read via the "security" >>> attribute. libnvdimm will generically support the key_change API call. >> >> Btw, could you use a logon-type key rather than spinning your own? Do you >> specifically not want it to be updateable? > > Thanks for reviewing this David. I'll take a look and see if I can. > Basically the code is acting as the proxy for the hardware and passing > through the passphrase. What would be the best way to update the > passphrase if I need to retrieve the new passphrase from the userspace? > The hardware still needs the old passphrase in order to verify. There > has to be a nice way to do this but I'm not familiar with key management > API to know better. A thought occurred to me. For password update, would it make sense to do this instead: 1. get the existing key by: request_key("nvdimm:xxxxxxxx") 2. get the new key by: request_key("nvdimm.update:xxxxxxxx") 3. verify key with hardware on success, copy new payload to existing key payload 4. invalidate "nvdimm.update" key This way then we won't have to mess with needing the invalidated key to be garbage collected. Thoughts? > >> >>> +static int intel_dimm_security_update_passphrase( >>> + struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, >>> + struct nvdimm_key_data *old_data, >>> + struct nvdimm_key_data *new_data) >> >> You might want to mark old_data and new_data as const here. At least, they >> don't appear to be changed. Also, can you stick a banner comment on the >> function to say what it actually does? Is it changing the key stored in h/w >> or is it meant to be changing what's stored in the key payload? >> >>> + /* request new key from userspace */ >>> + key = nvdimm_request_key(dev); >>> + if (!key) { >>> + dev_dbg(dev, "%s: failed to acquire new key\n", __func__); >>> + return -ENXIO; >>> + } >>> + >>> + dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial); >>> + >>> + if (key->datalen == NVDIMM_PASSPHRASE_LEN) { >>> + old_data = NULL; >>> + new_data = key->payload.data[0]; >>> + } else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) { >>> + new_data = key->payload.data[0]; >>> + old_data = new_data + NVDIMM_PASSPHRASE_LEN; >>> + } else { >>> + key_invalidate(key); >>> + key_put(key); >>> + dev_warn(dev, "Incorrect key payload size for update: %u\n", >>> + key->datalen); >>> + return -EINVAL; >>> + } >>> + >>> + down_read(&key->sem); >>> + rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data, >>> + new_data); >> >> Why do you need to get a read lock on key->sem and not a write-lock? >> >>> + if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) >>> + memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN); >> >> You appear to be changing the key content here. If that's the case, don't you >> need to write-lock key->sem? >> >>> + up_read(&key->sem); >> >> David >> > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm >
Dave Jiang <dave.jiang@intel.com> wrote: > A thought occurred to me. For password update, would it make sense to do > this instead: > 1. get the existing key by: request_key("nvdimm:xxxxxxxx") > 2. get the new key by: request_key("nvdimm.update:xxxxxxxx") > 3. verify key with hardware > on success, copy new payload to existing key payload > 4. invalidate "nvdimm.update" key > > This way then we won't have to mess with needing the invalidated key to > be garbage collected. Thoughts? Can you tell me at what points you actually access the key? David
On 07/18/2018 01:41 PM, David Howells wrote: > Dave Jiang <dave.jiang@intel.com> wrote: > >> A thought occurred to me. For password update, would it make sense to do >> this instead: >> 1. get the existing key by: request_key("nvdimm:xxxxxxxx") >> 2. get the new key by: request_key("nvdimm.update:xxxxxxxx") >> 3. verify key with hardware >> on success, copy new payload to existing key payload >> 4. invalidate "nvdimm.update" key >> >> This way then we won't have to mess with needing the invalidated key to >> be garbage collected. Thoughts? > > Can you tell me at what points you actually access the key? When we unlock the DIMM, disable security, update/enable passphrase, and secure erase. Unlock is called by the kernel during initialization of NVDIMMs. The rest are triggered through a knob in sysfs. i.e. echo "erase" > /sys/devices/..../nmem0/security
On 07/18/2018 04:14 AM, David Howells wrote: > Dave Jiang <dave.jiang@intel.com> wrote: > >> Add support for setting and/or updating passphrase on the Intel nvdimms. >> The passphrase is pulled from userspace through the kernel key management. >> We trigger the update via writing "update" to the sysfs attribute >> "security". The state of the security can also be read via the "security" >> attribute. libnvdimm will generically support the key_change API call. > > Btw, could you use a logon-type key rather than spinning your own? Do you > specifically not want it to be updateable? Ok stupid question David. I'm attempting to use the logon-type key. I have added this line to the request-key.conf: create logon nvdimm* * /usr/sbin/nvdimm-upcall %k But I can't seem to get /sbin/request-key to trigger this line and call my upcall app. On the kernel side it seems ok? ==> search_nested_keyrings({17212177},{logon,nvdimm.update:cdab-0a-07e0-ffffffff}) <== call_sbin_request_key() = -126 <== construct_key() = -126 cons failed What am I doing wrong? > >> +static int intel_dimm_security_update_passphrase( >> + struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, >> + struct nvdimm_key_data *old_data, >> + struct nvdimm_key_data *new_data) > > You might want to mark old_data and new_data as const here. At least, they > don't appear to be changed. Also, can you stick a banner comment on the > function to say what it actually does? Is it changing the key stored in h/w > or is it meant to be changing what's stored in the key payload? > >> + /* request new key from userspace */ >> + key = nvdimm_request_key(dev); >> + if (!key) { >> + dev_dbg(dev, "%s: failed to acquire new key\n", __func__); >> + return -ENXIO; >> + } >> + >> + dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial); >> + >> + if (key->datalen == NVDIMM_PASSPHRASE_LEN) { >> + old_data = NULL; >> + new_data = key->payload.data[0]; >> + } else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) { >> + new_data = key->payload.data[0]; >> + old_data = new_data + NVDIMM_PASSPHRASE_LEN; >> + } else { >> + key_invalidate(key); >> + key_put(key); >> + dev_warn(dev, "Incorrect key payload size for update: %u\n", >> + key->datalen); >> + return -EINVAL; >> + } >> + >> + down_read(&key->sem); >> + rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data, >> + new_data); > > Why do you need to get a read lock on key->sem and not a write-lock? > >> + if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) >> + memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN); > > You appear to be changing the key content here. If that's the case, don't you > need to write-lock key->sem? > >> + up_read(&key->sem); > > David >
Dave Jiang <dave.jiang@intel.com> wrote: > Ok stupid question David. I'm attempting to use the logon-type key. I > have added this line to the request-key.conf: > create logon nvdimm* * /usr/sbin/nvdimm-upcall %k Can you show me the whole file? Let me ask a stupid question too: Why do you need to call request_key()? As I understand it, you poke an attribute file in sysfs by writing "update" to it and this triggers a request_key() call. The kernel then links the key it found across to the internal keyring. You could instead require that the key be specified directly, ie. you write "update <keyid>" to the attribute file. The driver can then call key_lookup() to get the key - or, better still, we should make lookup_user_key() available so that you can call that - which will do a security check. Another advantage of doing this is that the old key is still available in the internal keyring until it gets replaced. So you can do your password change if you want to do it this way. On the other hand, requiring both the old and the new passwords to be supplied is probably better from a security point of view, so you could require them both to be included in the key. David
On 07/19/2018 01:22 AM, David Howells wrote: > Dave Jiang <dave.jiang@intel.com> wrote: > >> Ok stupid question David. I'm attempting to use the logon-type key. I >> have added this line to the request-key.conf: >> create logon nvdimm* * /usr/sbin/nvdimm-upcall %k > > Can you show me the whole file? https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html I don't believe /sbin/request-key is calling my upcall app. > > Let me ask a stupid question too: Why do you need to call request_key()? > > As I understand it, you poke an attribute file in sysfs by writing "update" to > it and this triggers a request_key() call. The kernel then links the key it > found across to the internal keyring. Correct. And when there isn't a key, it needs to fetch from userspace and construct the key. > > You could instead require that the key be specified directly, ie. you write > "update <keyid>" to the attribute file. The driver can then call key_lookup() > to get the key - or, better still, we should make lookup_user_key() available > so that you can call that - which will do a security check. I can attach the keyid to the nvdimm when I initially get a success key and the update can look it up easily enough. I'm not groking the lookup_user_key() comment. What determines a key to be a user key or a kernel key? I think right now the issue I have is trying to get logon key type working with creating the initial key from userspace and/or update the key with new passphrase, which needs a payload from userspace. > > Another advantage of doing this is that the old key is still available in the > internal keyring until it gets replaced. So you can do your password change > if you want to do it this way. > > On the other hand, requiring both the old and the new passwords to be supplied > is probably better from a security point of view, so you could require them > both to be included in the key. > > David >
On 07/19/2018 02:28 PM, Dave Jiang wrote: > > > On 07/19/2018 01:22 AM, David Howells wrote: >> Dave Jiang <dave.jiang@intel.com> wrote: >> >>> Ok stupid question David. I'm attempting to use the logon-type key. I >>> have added this line to the request-key.conf: >>> create logon nvdimm* * /usr/sbin/nvdimm-upcall %k >> >> Can you show me the whole file? > https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html > > I don't believe /sbin/request-key is calling my upcall app. Ok I got this part working. I didn't realize that request-key looks for <key type>.conf and I needed to add my line to /etc/request-key.conf and /etc/request-key.d/nvdimm.conf no longer works. > >> >> Let me ask a stupid question too: Why do you need to call request_key()? >> >> As I understand it, you poke an attribute file in sysfs by writing "update" to >> it and this triggers a request_key() call. The kernel then links the key it >> found across to the internal keyring. > > Correct. And when there isn't a key, it needs to fetch from userspace > and construct the key. > >> >> You could instead require that the key be specified directly, ie. you write >> "update <keyid>" to the attribute file. The driver can then call key_lookup() >> to get the key - or, better still, we should make lookup_user_key() available >> so that you can call that - which will do a security check. > > I can attach the keyid to the nvdimm when I initially get a success key > and the update can look it up easily enough. I'm not groking the > lookup_user_key() comment. What determines a key to be a user key or a > kernel key? > > I think right now the issue I have is trying to get logon key type > working with creating the initial key from userspace and/or update the > key with new passphrase, which needs a payload from userspace. > >> >> Another advantage of doing this is that the old key is still available in the >> internal keyring until it gets replaced. So you can do your password change >> if you want to do it this way. >> >> On the other hand, requiring both the old and the new passwords to be supplied >> is probably better from a security point of view, so you could require them >> both to be included in the key. >> >> David >> > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm >
Dave Jiang <dave.jiang@intel.com> wrote: > > Can you show me the whole file? > https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html Is that exactly what's in your /etc/request-key.conf? > > As I understand it, you poke an attribute file in sysfs by writing > > "update" to it and this triggers a request_key() call. The kernel then > > links the key it found across to the internal keyring. > > Correct. And when there isn't a key, it needs to fetch from userspace > and construct the key. I think I've missed a step. Am I right in thinking that the "update" command is just for changing the password, and is not the primary way that passwords get into the kernel to, say, unlock the device? > > You could instead require that the key be specified directly, ie. you > > write "update <keyid>" to the attribute file. The driver can then call > > key_lookup() to get the key - or, better still, we should make > > lookup_user_key() available so that you can call that - which will do a > > security check. > > I can attach the keyid to the nvdimm when I initially get a success key > and the update can look it up easily enough. I'm not groking the > lookup_user_key() comment. What determines a key to be a user key or a > kernel key? lookup_user_key() converts a key serial number into a key applies all the security checks to make sure that userspace is allowed to use that key for some purpose. key_lookup(), on the other hand, has no security checks whatsoever. So if you're using a key ID given to you by userspace, you have to use lookup_user_key(). Whereas if you want to find a key by description - and potentially create it - you use request_key(). request_key() also does all the appropriate security checks. So I would be tempted to make the update command take an explicit key ID rather than calling request_key() - particularly as you want to control what the password gets changed to. Possibly even make it take *two* explicit key IDs: the key holding the new password and the key holding the old one. Then you can have security checks on both and you can compare the descriptions to make sure they're the same. David
On 07/20/2018 08:40 AM, David Howells wrote: > Dave Jiang <dave.jiang@intel.com> wrote: > >>> Can you show me the whole file? >> https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html > > Is that exactly what's in your /etc/request-key.conf? No, I realized that I had to do update when converted to logon key. This is what I added to /etc/request-key.conf: create logon nvdimm* * /usr/sbin/nvdimm-upcall %k I do have it working now. > >>> As I understand it, you poke an attribute file in sysfs by writing >>> "update" to it and this triggers a request_key() call. The kernel then >>> links the key it found across to the internal keyring. >> >> Correct. And when there isn't a key, it needs to fetch from userspace >> and construct the key. > > I think I've missed a step. Am I right in thinking that the "update" command > is just for changing the password, and is not the primary way that passwords > get into the kernel to, say, unlock the device? That is correct. There is an unlock operation. But we do not expose that to the userspace via the sysfs knob. Unlock only happens during NVDIMM discovery/init. Mainly the security is to prevent someone taking the NVDIMM to another server or take it out of data center. The update command either enables security and add a passphrase to the NVDIMM or change the passphrase on a NVDIMM. > >>> You could instead require that the key be specified directly, ie. you >>> write "update <keyid>" to the attribute file. The driver can then call >>> key_lookup() to get the key - or, better still, we should make >>> lookup_user_key() available so that you can call that - which will do a >>> security check. >> >> I can attach the keyid to the nvdimm when I initially get a success key >> and the update can look it up easily enough. I'm not groking the >> lookup_user_key() comment. What determines a key to be a user key or a >> kernel key? > > lookup_user_key() converts a key serial number into a key applies all the > security checks to make sure that userspace is allowed to use that key for > some purpose. > > key_lookup(), on the other hand, has no security checks whatsoever. > > So if you're using a key ID given to you by userspace, you have to use > lookup_user_key(). Whereas if you want to find a key by description - and > potentially create it - you use request_key(). request_key() also does all > the appropriate security checks. > > So I would be tempted to make the update command take an explicit key ID > rather than calling request_key() - particularly as you want to control what > the password gets changed to. Possibly even make it take *two* explicit key > IDs: the key holding the new password and the key holding the old one. Then > you can have security checks on both and you can compare the descriptions to > make sure they're the same. Ok let me paraphrase what you are saying and see if I understand. The current implementation for change password: 1. trigger update via sysfs 2. kernel looks for key in keyring 3. kernel fetches key from userspace if no key in kernel keyring 4. kernel adds new key to keyring New: 1. userspace inject key into kernel keyring 2. userspace triggers update with new key id and old key id 3. kernel looks up keys with key-id in kernel 4. kernel tries to update with hardware 5. on success kernel accepts new key (or update old key payload) This pushes most of the complexity into userspace and is more secure. But I'm not sure if we need it. The admin with root access has access to everything already. We are just trying to protect against someone moving the DIMM w/o the right authority. Also, unlock right now basically is a request-key to userspace to retrieve the key before we unlock. If we go the other way it sounds like we need to inject all the keys from userspace first before unlock can happen? Doesn't this mean we'll end up having to call request-key anyhow in order to trigger an upcall app to do all this? Also we can't wait until userland comes up and execute an app to inject all the user keys before we initialize the DIMMs if we aren't going through request-key upcall. Also, this is step one. Eventually we would like to be able to just retrieve the passphrase from TPM instead of the fs or network. I do think associating with the current key id with the nvdimm object can make things a lot easier and I can just do key_lookup() instead of calling request_key(). What about: 1. user triggers update through sysfs 2. kernel fetches old key with key-id that's associated with the nvdimm object from keyring 3. kernel request_key from userspace for new key 4. kernel sends both payloads to hardware to attempt update 5. on success kernel invalidates old key and updates key-id associated with the nvdimm object In this case, all the other security ops should be able to lookup the key via key-id if that exists. Or they would request-key if the key does not exist (if the op is first to execute).
Dave Jiang <dave.jiang@intel.com> wrote: > New: > 1. userspace inject key into kernel keyring I presume this step is what you do to use the nvdimm? If so, this can use request_key() and then splice the key across to the kernel keyring. > 2. userspace triggers update with new key id and old key id > 3. kernel looks up keys with key-id in kernel > 4. kernel tries to update with hardware > 5. on success kernel accepts new key (or update old key payload) ... and splices the key into the kernel keyring. > Also, unlock right now basically is a request-key to userspace to retrieve > the key before we unlock. If we go the other way it sounds like we need to > inject all the keys from userspace first before unlock can happen? No, you'd still use request_key() for that. It's the change-password event that I'm suggesting you should use two explicitly nominated keys for. > I do think associating with the current key id with the nvdimm object > can make things a lot easier and I can just do key_lookup() instead of > calling request_key(). You shouldn't be using key_lookup(). > What about: > 1. user triggers update through sysfs > 2. kernel fetches old key with key-id that's associated with the nvdimm > object from keyring You can do that. You'd use key_search() for that. > 3. kernel request_key from userspace for new key This bit I quibble with. I think you should specify it to make sure that you get the password you intended and don't pick something else up by accident. > 4. kernel sends both payloads to hardware to attempt update > 5. on success kernel invalidates old key and updates key-id associated > with the nvdimm object I think you should just link the new key across. It should have the same description as the old key to the kernel keyring. This would displace the old key automatically. > ... lookup the key via key-id ... You shouldn't use key IDs within the kernel, except at the UAPI boundary. You should be using the key description as the handle - and, in this case, it should be an identifier you can match uniquely within the system with the particular nvdimm. David
diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c index 9155b8e63f0e..b0a62248467d 100644 --- a/drivers/acpi/nfit/intel.c +++ b/drivers/acpi/nfit/intel.c @@ -18,6 +18,62 @@ #include "intel.h" #include "nfit.h" +static int intel_dimm_security_update_passphrase( + struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, + struct nvdimm_key_data *old_data, + struct nvdimm_key_data *new_data) +{ + struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus); + int cmd_rc, rc = 0; + struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm); + struct { + struct nd_cmd_pkg pkg; + struct nd_intel_set_passphrase cmd; + } nd_cmd = { + .pkg = { + .nd_command = NVDIMM_INTEL_SET_PASSPHRASE, + .nd_family = NVDIMM_FAMILY_INTEL, + .nd_size_in = ND_INTEL_PASSPHRASE_SIZE * 2, + .nd_size_out = ND_INTEL_STATUS_SIZE, + .nd_fw_size = ND_INTEL_STATUS_SIZE, + }, + .cmd = { + .status = 0, + }, + }; + + if (!test_bit(NVDIMM_INTEL_SET_PASSPHRASE, &nfit_mem->dsm_mask)) + return -ENOTTY; + + if (old_data) + memcpy(nd_cmd.cmd.old_pass, old_data->data, + ND_INTEL_PASSPHRASE_SIZE); + memcpy(nd_cmd.cmd.new_pass, new_data->data, ND_INTEL_PASSPHRASE_SIZE); + rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd, + sizeof(nd_cmd), &cmd_rc); + if (rc < 0) + goto out; + if (cmd_rc < 0) { + rc = cmd_rc; + goto out; + } + + switch (nd_cmd.cmd.status) { + case 0: + break; + case ND_INTEL_STATUS_INVALID_PASS: + rc = -EINVAL; + goto out; + case ND_INTEL_STATUS_INVALID_STATE: + default: + rc = -ENXIO; + goto out; + } + + out: + return rc; +} + static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, struct nvdimm_key_data *nkey) { @@ -148,4 +204,5 @@ static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus, struct nvdimm_security_ops intel_security_ops = { .state = intel_dimm_security_state, .unlock = intel_dimm_security_unlock, + .change_key = intel_dimm_security_update_passphrase, }; diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c index 7600bbbb8a91..432bdd8d19e3 100644 --- a/drivers/nvdimm/dimm_devs.c +++ b/drivers/nvdimm/dimm_devs.c @@ -171,6 +171,73 @@ int nvdimm_security_unlock_dimm(struct device *dev) return rc; } +static int nvdimm_security_change_key(struct device *dev) +{ + struct nvdimm *nvdimm = to_nvdimm(dev); + struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); + struct key *key = NULL, *old_key = NULL; + int rc; + void *old_data, *new_data; + + if (!nvdimm->security_ops) + return 0; + + if (nvdimm->state == NVDIMM_SECURITY_FROZEN) + return -EBUSY; + + /* look for a key from keyring if exists and remove */ + old_key = nvdimm_search_key(dev); + if (old_key) { + dev_dbg(dev, "%s: killing old key: %#x\n", + __func__, old_key->serial); + key_invalidate(old_key); + key_put_sync(old_key); + } + + /* request new key from userspace */ + key = nvdimm_request_key(dev); + if (!key) { + dev_dbg(dev, "%s: failed to acquire new key\n", __func__); + return -ENXIO; + } + + dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial); + + if (key->datalen == NVDIMM_PASSPHRASE_LEN) { + old_data = NULL; + new_data = key->payload.data[0]; + } else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) { + new_data = key->payload.data[0]; + old_data = new_data + NVDIMM_PASSPHRASE_LEN; + } else { + key_invalidate(key); + key_put(key); + dev_warn(dev, "Incorrect key payload size for update: %u\n", + key->datalen); + return -EINVAL; + } + + down_read(&key->sem); + rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data, + new_data); + if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) + memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN); + up_read(&key->sem); + + /* + * If we successfully update, link the new key to our keyring, + * otherwise, nuke the key. + */ + if (rc == 0) + key_link(nvdimm_cred->thread_keyring, key); + else + key_invalidate(key); + + key_put(key); + nvdimm_security_get_state(dev); + return rc; +} + /* * Retrieve bus and dimm handle and return if this bus supports * get_config_data commands @@ -528,11 +595,58 @@ static ssize_t available_slots_show(struct device *dev, } static DEVICE_ATTR_RO(available_slots); +static ssize_t security_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct nvdimm *nvdimm = to_nvdimm(dev); + + switch (nvdimm->state) { + case NVDIMM_SECURITY_DISABLED: + return sprintf(buf, "disabled\n"); + case NVDIMM_SECURITY_UNLOCKED: + return sprintf(buf, "unlocked\n"); + case NVDIMM_SECURITY_LOCKED: + return sprintf(buf, "locked\n"); + case NVDIMM_SECURITY_FROZEN: + return sprintf(buf, "frozen\n"); + case NVDIMM_SECURITY_UNSUPPORTED: + default: + return sprintf(buf, "unsupported\n"); + } + + return -ENOTTY; +} + +static ssize_t security_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t len) + +{ + struct nvdimm *nvdimm = to_nvdimm(dev); + struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); + ssize_t rc = -EINVAL; + + wait_nvdimm_bus_probe_idle(&nvdimm_bus->dev); + if (atomic_read(&nvdimm->busy)) + return -EBUSY; + + if (sysfs_streq(buf, "update")) + rc = nvdimm_security_change_key(dev); + else + return -EINVAL; + + if (rc == 0) + rc = len; + + return rc; +} +static DEVICE_ATTR_RW(security); + static struct attribute *nvdimm_attributes[] = { &dev_attr_state.attr, &dev_attr_flags.attr, &dev_attr_commands.attr, &dev_attr_available_slots.attr, + &dev_attr_security.attr, NULL, }; diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h index 692217f82c6e..7889fea75281 100644 --- a/include/linux/libnvdimm.h +++ b/include/linux/libnvdimm.h @@ -162,6 +162,7 @@ enum nvdimm_security_state { NVDIMM_SECURITY_DISABLED, NVDIMM_SECURITY_UNLOCKED, NVDIMM_SECURITY_LOCKED, + NVDIMM_SECURITY_FROZEN, NVDIMM_SECURITY_UNSUPPORTED, }; @@ -178,6 +179,10 @@ struct nvdimm_security_ops { enum nvdimm_security_state *state); int (*unlock)(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, struct nvdimm_key_data *nkey); + int (*change_key)(struct nvdimm_bus *nvdimm_bus, + struct nvdimm *nvdimm, + struct nvdimm_key_data *old_data, + struct nvdimm_key_data *new_data); }; void badrange_init(struct badrange *badrange);
Add support for setting and/or updating passphrase on the Intel nvdimms. The passphrase is pulled from userspace through the kernel key management. We trigger the update via writing "update" to the sysfs attribute "security". The state of the security can also be read via the "security" attribute. libnvdimm will generically support the key_change API call. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/acpi/nfit/intel.c | 57 ++++++++++++++++++++++ drivers/nvdimm/dimm_devs.c | 114 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/libnvdimm.h | 5 ++ 3 files changed, 176 insertions(+)