[PATCH 0/3] nvme: Don't add namespaces for locked drives

[PATCH 0/3] nvme: Don't add namespaces for locked drives

[Jethro Beekman]

Hi all,

If an NVMe drive is locked with ATA Security, most commands sent to the drive 
will fail. This includes commands sent by the kernel upon discovery to probe 
for partitions. The failing happens in such a way that trying to do anything 
with the drive (e.g. sending an unlock command; unloading the nvme module) is 
basically impossible with the high default command timeout.

This patch adds a check to see if the drive is locked, and if it is, its 
namespaces are not initialized. It is expected that userspace will send the 
proper "security send/unlock" command and then reset the controller. Userspace 
tools are available at [1].

This is my first kernel patch so please let me know if you have any feedback.

I intend to also submit a future patch that tracks ATA Security commands sent 
from userspace and remembers the password so it can be submitted to a locked 
drive upon pm_resume. (still WIP)

Jethro Beekman

[1] https://github.com/jethrogb/nvme-ata-security

Jethro Beekman (3):
  nvme: When scanning namespaces, make sure the drive is not locked
  nvme: Add function for NVMe security receive command
  nvme: Check if drive is locked using ATA Security

 drivers/nvme/host/core.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

-- 
2.9.0

[PATCH 1/3] nvme: When scanning namespaces, make sure the drive is not locked

[Jethro Beekman]

Signed-off-by: Jethro Beekman <kernel@jbeekman.nl>
---
 drivers/nvme/host/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..3a0d48c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1377,6 +1377,12 @@ static void __nvme_scan_namespaces(struct nvme_ctrl *ctrl, unsigned nn)
 	}
 }
 
+static bool nvme_security_is_locked(struct nvme_ctrl *ctrl,
+		struct nvme_id_ctrl *id)
+{
+	return false;
+}
+
 void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
@@ -1385,6 +1391,11 @@ void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
 	if (nvme_identify_ctrl(ctrl, &id))
 		return;
 
+	if (nvme_security_is_locked(ctrl, id)) {
+		nvme_remove_namespaces(ctrl);
+		return;
+	}
+
 	mutex_lock(&ctrl->namespaces_mutex);
 	nn = le32_to_cpu(id->nn);
 	if (ctrl->vs >= NVME_VS(1, 1) &&
-- 
2.9.0

[PATCH 2/3] nvme: Add function for NVMe security receive command

[Jethro Beekman]

Signed-off-by: Jethro Beekman <kernel@jbeekman.nl>
---
 drivers/nvme/host/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3a0d48c..da027ed 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1377,6 +1377,18 @@ static void __nvme_scan_namespaces(struct nvme_ctrl *ctrl, unsigned nn)
 	}
 }
 
+int nvme_security_recv(struct nvme_ctrl *dev, u8 protocol, void *buf,
+		unsigned int len)
+{
+	struct nvme_command c = { };
+
+	c.common.opcode = nvme_admin_security_recv;
+	c.common.cdw10[0] = cpu_to_le32(((u32)protocol)<<24);
+	c.common.cdw10[1] = cpu_to_le32(len);
+
+	return nvme_submit_sync_cmd(dev->admin_q, &c, buf, len);
+}
+
 static bool nvme_security_is_locked(struct nvme_ctrl *ctrl,
 		struct nvme_id_ctrl *id)
 {
-- 
2.9.0

[PATCH 3/3] nvme: Check if drive is locked using ATA Security

[Jethro Beekman]

Signed-off-by: Jethro Beekman <kernel@jbeekman.nl>
---
 drivers/nvme/host/core.c | 49 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index da027ed..0164122 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1389,10 +1389,57 @@ int nvme_security_recv(struct nvme_ctrl *dev, u8 protocol, void *buf,
 	return nvme_submit_sync_cmd(dev->admin_q, &c, buf, len);
 }
 
+#define OACS_SECURITY (1<<0)
+#define SCSI_SECURITY_PROTOCOL_ATA_SECURITY 0xef
+#define ATA_SECURITY_LOCKED 0x4
+
 static bool nvme_security_is_locked(struct nvme_ctrl *ctrl,
 		struct nvme_id_ctrl *id)
 {
-	return false;
+	int err;
+	unsigned int i;
+	bool found;
+	u8 protocols[256+8]; /* 8 byte hdr + max number of possible protocols */
+	u8 ata_security[16];
+	u16 n;
+
+	/* security commands supported? */
+	if (!(le16_to_cpu(id->oacs) & OACS_SECURITY))
+		return false;
+
+	/* list security protocols */
+	err = nvme_security_recv(ctrl, 0, protocols, sizeof(protocols));
+	if (err) {
+		dev_warn(ctrl->device, "nvme_security_recv returned error %xh\n",
+					err);
+		return false;
+	}
+
+	/* find ata security protocol */
+	n = be16_to_cpup((__be16 *)(protocols+6));
+	if (n >= 256) {
+		dev_warn(ctrl->device, "security info protocol returned more than 256 protocols\n");
+		return false;
+	}
+	found = false;
+	for (i = 0; i <= n; i++) {
+		if (protocols[8+i] == SCSI_SECURITY_PROTOCOL_ATA_SECURITY) {
+			found = true;
+			break;
+		}
+	}
+	if (!found)
+		return false;
+
+	/* do ata security identify */
+	err = nvme_security_recv(ctrl, SCSI_SECURITY_PROTOCOL_ATA_SECURITY,
+			ata_security, sizeof(ata_security))
+	if (err) {
+		dev_warn(ctrl->device, "nvme_security_recv returned error %xh\n",
+					err);
+		return false;
+	}
+	return ata_security[1] == 0xe && (ata_security[9]&ATA_SECURITY_LOCKED);
 }
 
 void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
-- 
2.9.0

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Sagi Grimberg]

> Hi all,
>
> If an NVMe drive is locked with ATA Security, most commands sent to the drive
> will fail. This includes commands sent by the kernel upon discovery to probe
> for partitions. The failing happens in such a way that trying to do anything
> with the drive (e.g. sending an unlock command; unloading the nvme module) is
> basically impossible with the high default command timeout.
>
> This patch adds a check to see if the drive is locked, and if it is, its
> namespaces are not initialized. It is expected that userspace will send the
> proper "security send/unlock" command and then reset the controller. Userspace
> tools are available at [1].
>
> This is my first kernel patch so please let me know if you have any feedback.
>
> I intend to also submit a future patch that tracks ATA Security commands sent
> from userspace and remembers the password so it can be submitted to a locked
> drive upon pm_resume. (still WIP)
>
> Jethro Beekman
>
> [1] https://github.com/jethrogb/nvme-ata-security
>
> Jethro Beekman (3):
>    nvme: When scanning namespaces, make sure the drive is not locked
>    nvme: Add function for NVMe security receive command
>    nvme: Check if drive is locked using ATA Security

Hey Jethro,

I think it would make better sense to squash patches 1,3 together and
have patch 2 come before them:

patch 1: nvme: Add function for NVMe security receive command
patch 2: nvme: Check if drive is locked using ATA Security when scanning 
namespaces

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Keith Busch]

On Sun, Jun 19, 2016 at 04:06:31PM -0700, Jethro Beekman wrote:
> If an NVMe drive is locked with ATA Security, most commands sent to the drive 
> will fail. This includes commands sent by the kernel upon discovery to probe 
> for partitions. The failing happens in such a way that trying to do anything 
> with the drive (e.g. sending an unlock command; unloading the nvme module) is 
> basically impossible with the high default command timeout.

Why is the timeout a factor here? Is it because the error your drive
returns does not have DNR set and goes through 30 seconds of retries?

If so, I think we should probably have a limited retry count instead of
unlimited retries within the command timeout.
 
> This patch adds a check to see if the drive is locked, and if it is, its 
> namespaces are not initialized. It is expected that userspace will send the 
> proper "security send/unlock" command and then reset the controller. Userspace 
> tools are available at [1].

Aren't these security settings per-namespace rather than the entire device?
 
> I intend to also submit a future patch that tracks ATA Security commands sent 
> from userspace and remembers the password so it can be submitted to a locked 
> drive upon pm_resume. (still WIP)

This subjects the system to various attacks like cold boot or hotswap,
but that's what users want!

This is ATA security, though, so wouldn't ATA also benefit from this? The
payload setup/decoding should then go in a generic library for everyone.

Similar was said about the patch adding OPAL security to the NVMe driver:

  http://lists.infradead.org/pipermail/linux-nvme/2016-April/004428.html

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Jethro Beekman]

On 20-06-16 08:26, Keith Busch wrote:
> On Sun, Jun 19, 2016 at 04:06:31PM -0700, Jethro Beekman wrote:
>> If an NVMe drive is locked with ATA Security, most commands sent to the drive 
>> will fail. This includes commands sent by the kernel upon discovery to probe 
>> for partitions. The failing happens in such a way that trying to do anything 
>> with the drive (e.g. sending an unlock command; unloading the nvme module) is 
>> basically impossible with the high default command timeout.
> 
> Why is the timeout a factor here? Is it because the error your drive
> returns does not have DNR set and goes through 30 seconds of retries?

I looked into this but couldn't figure out where DNR's were being handled. Upon
closer inspection, I suppose I could add some debug code in nvme_complete_rq.

> If so, I think we should probably have a limited retry count instead of
> unlimited retries within the command timeout.

Would this just be a matter of setting req->retries and checking for it in
nvme_req_needs_retry? How does one keep track of the number of tries so far?

>> This patch adds a check to see if the drive is locked, and if it is, its 
>> namespaces are not initialized. It is expected that userspace will send the 
>> proper "security send/unlock" command and then reset the controller. Userspace 
>> tools are available at [1].
> 
> Aren't these security settings per-namespace rather than the entire device?

You're right, I assumed that admin commands can't have namespace ids, but
looking at the spec, that's not the case. Turns out there's a problem with the
driver then: nvme_ioctl never includes the ns for NVME_IOCTL_ADMIN_CMD. I'll fix
this and the above suggestion. Hopefully that also obviates the need for the
nvme_scan_namespaces code path, otherwise we'll have to rethink how to do this.

>> I intend to also submit a future patch that tracks ATA Security commands sent 
>> from userspace and remembers the password so it can be submitted to a locked 
>> drive upon pm_resume. (still WIP)
> 
> This subjects the system to various attacks like cold boot or hotswap,
> but that's what users want!

Yes. Unfortunately I couldn't think of a sane way to have userspace prompt for
the password on resume with a non-functional drive. Current BIOS implementations
generally also just unlock this way. I do intend to use the keyring API.

The effects of a hotswap can be partially mitigated by "salting" the password
with the drive serial number [1]. For maximum effect the kernel would check to
see if the drive SN has changed before resending the password, I'm not sure if
we want this extra complexity in the kernel though.

[1] https://jbeekman.nl/blog/2015/03/lenovo-thinkpad-hdd-password/

> This is ATA security, though, so wouldn't ATA also benefit from this? The
> payload setup/decoding should then go in a generic library for everyone.
> 
> Similar was said about the patch adding OPAL security to the NVMe driver:
> 
>   http://lists.infradead.org/pipermail/linux-nvme/2016-April/004428.html

Yes this would benefit from being generic. I actually looked whether this
already existed in the kernel and was surprised that it didn't. I suppose most
people using this functionality depend on their BIOS to handle everything. I
will contact Rafael to see we can come up with some generic drive security state
tracker.

Jethro

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Keith Busch]

On Mon, Jun 20, 2016 at 11:21:09AM -0700, Jethro Beekman wrote:
> On 20-06-16 08:26, Keith Busch wrote:
> 
> Would this just be a matter of setting req->retries and checking for it in
> nvme_req_needs_retry? How does one keep track of the number of tries so far?

I just sent a patch out earlier today to use req->retries to track the
retry count, and nvme module parameter to set the max retries. I think
that would fix the long delays you're seeing, assuming the patch is okay.
 
> You're right, I assumed that admin commands can't have namespace ids, but
> looking at the spec, that's not the case. Turns out there's a problem with the
> driver then: nvme_ioctl never includes the ns for NVME_IOCTL_ADMIN_CMD.

The NVME_IOCTL_ADMIN_CMD already takes any namespace identifier the user
put in that field.

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Jethro Beekman]

On 20-06-16 15:54, Keith Busch wrote:
> On Mon, Jun 20, 2016 at 11:21:09AM -0700, Jethro Beekman wrote:
>> On 20-06-16 08:26, Keith Busch wrote:
>>
>> Would this just be a matter of setting req->retries and checking for it in
>> nvme_req_needs_retry? How does one keep track of the number of tries so far?
> 
> I just sent a patch out earlier today to use req->retries to track the
> retry count, and nvme module parameter to set the max retries. I think
> that would fix the long delays you're seeing, assuming the patch is okay.

Your patch "nvme: Limit command retries" works for me and obviates the need for
this patch.

>> You're right, I assumed that admin commands can't have namespace ids, but
>> looking at the spec, that's not the case. Turns out there's a problem with the
>> driver then: nvme_ioctl never includes the ns for NVME_IOCTL_ADMIN_CMD.
> 
> The NVME_IOCTL_ADMIN_CMD already takes any namespace identifier the user
> put in that field.

I see, the ns argument is just to specify the queue. I assume userspace is
supposed to obtain the ns using NVME_IOCTL_ID? This seems broken, if I have an
open block device handle I can send commands to any nvme namespace as well as
the controller? I think on the block devices you should only be able to send
commands with your nsid. There was some discussion on the security implications
of this about a year ago [1], and it was decided to fix this, but it doesn't
look like this was actually merged?

[1] http://lists.infradead.org/pipermail/linux-nvme/2015-January/001446.html

Jethro

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Christoph Hellwig]

On Sun, Jun 19, 2016 at 04:06:31PM -0700, Jethro Beekman wrote:
> Hi all,
> 
> If an NVMe drive is locked with ATA Security, most commands sent to the drive 
> will fail. This includes commands sent by the kernel upon discovery to probe 
> for partitions. The failing happens in such a way that trying to do anything 
> with the drive (e.g. sending an unlock command; unloading the nvme module) is 
> basically impossible with the high default command timeout.

Do you have any spec that defines this ATA security protocol and how
it applies to NVMe?  The NVMe spec just referes to SPC4 for security
protocols, and I haven't been able to find a reference to an ATA
security protocol in it either, but I haven't tried hard yet.

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Christoph Hellwig]

On Mon, Jun 20, 2016 at 08:50:33PM -0700, Jethro Beekman wrote:
> >> You're right, I assumed that admin commands can't have namespace ids, but
> >> looking at the spec, that's not the case. Turns out there's a problem with the
> >> driver then: nvme_ioctl never includes the ns for NVME_IOCTL_ADMIN_CMD.
> > 
> > The NVME_IOCTL_ADMIN_CMD already takes any namespace identifier the user
> > put in that field.
> 
> I see, the ns argument is just to specify the queue. I assume userspace is
> supposed to obtain the ns using NVME_IOCTL_ID? This seems broken, if I have an
> open block device handle I can send commands to any nvme namespace as well as
> the controller? I think on the block devices you should only be able to send
> commands with your nsid. There was some discussion on the security implications
> of this about a year ago [1], and it was decided to fix this, but it doesn't
> look like this was actually merged?
> 
> [1] http://lists.infradead.org/pipermail/linux-nvme/2015-January/001446.html

I think the real problem here is to allow NVME_IOCTL_ADMIN_CMD on a
block device node - admin command in general do not apply to a
namespace, they apply to the whole controller.  Even if you look at the
nsid it's usually used for something global (e.g. the offset in the
namespace list or the namespace to be created / deleted).

Any admin command that applies to a namespace is a nightmare, and we
should not make it easier to issue it on a block device node but instead
build a proper abstraction.  Besides your usage which I can't even find
a spec for they only cases where admin command apply to actual existing
namespaces and could be somewhat safely issued by users having access
only to the namespace are the per-ns smart log and the per-ns features.

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Jethro Beekman]

On 24-06-16 00:37, Christoph Hellwig wrote:
> On Sun, Jun 19, 2016 at 04:06:31PM -0700, Jethro Beekman wrote:
>> Hi all,
>>
>> If an NVMe drive is locked with ATA Security, most commands sent to the drive 
>> will fail. This includes commands sent by the kernel upon discovery to probe 
>> for partitions. The failing happens in such a way that trying to do anything 
>> with the drive (e.g. sending an unlock command; unloading the nvme module) is 
>> basically impossible with the high default command timeout.
> 
> Do you have any spec that defines this ATA security protocol and how
> it applies to NVMe?  The NVMe spec just referes to SPC4 for security
> protocols, and I haven't been able to find a reference to an ATA
> security protocol in it either, but I haven't tried hard yet.

As you found NVMe points to SPC-4. SPC-4 lists protocol 0xEF "ATA Device Server
Password Security" as part of the SECURITY PROTOCOL IN command, pointing to
SAT-2. In one SAT-2 draft I could find there is are these sections

  12 SAT-specific SCSI extensions
  12.5 SAT-specific Security Protocols
  12.5.1 ATA Device Server Password Security Protocol

which provide a pretty straightforward translation of the ATA SECURITY feature
set (except that there is a new command to gather information that would
normally be part of ATA IDENTIFY). I have implemented all this and it seems to
work on my drive.

Jethro

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Christoph Hellwig]

On Fri, Jun 24, 2016 at 12:45:08AM -0700, Jethro Beekman wrote:
> As you found NVMe points to SPC-4. SPC-4 lists protocol 0xEF "ATA Device Server
> Password Security" as part of the SECURITY PROTOCOL IN command, pointing to
> SAT-2. In one SAT-2 draft I could find there is are these sections
> 
>   12 SAT-specific SCSI extensions
>   12.5 SAT-specific Security Protocols
>   12.5.1 ATA Device Server Password Security Protocol
> 
> which provide a pretty straightforward translation of the ATA SECURITY feature
> set (except that there is a new command to gather information that would
> normally be part of ATA IDENTIFY). I have implemented all this and it seems to
> work on my drive.

Oh, fun.  Can you add a little file in Documentation that explains this
chain and how we end up building the NVMe commands?

Re: [PATCH 3/3] nvme: Check if drive is locked using ATA Security

[Christoph Hellwig]

On Sun, Jun 19, 2016 at 04:06:34PM -0700, Jethro Beekman wrote:
> Signed-off-by: Jethro Beekman <kernel@jbeekman.nl>
> ---
>  drivers/nvme/host/core.c | 49 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index da027ed..0164122 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1389,10 +1389,57 @@ int nvme_security_recv(struct nvme_ctrl *dev, u8 protocol, void *buf,
>  	return nvme_submit_sync_cmd(dev->admin_q, &c, buf, len);
>  }
>  
> +#define OACS_SECURITY (1<<0)
> +#define SCSI_SECURITY_PROTOCOL_ATA_SECURITY 0xef
> +#define ATA_SECURITY_LOCKED 0x4

It would be great to have this out in a header or maybe rather
headers.  OACS_SECURITY should go into nvme.h, we probably should have a
new header for the SCSI security protocols (e.g.
include/scsi/scsi_security.h ?), and I'm not sure what to do with 
ATA_SECURITY_LOCKED - maybe just add it to scsi_security.h for now until
we get a more fully blown ata security implementation that even includes
ATA :))

>  static bool nvme_security_is_locked(struct nvme_ctrl *ctrl,
>  		struct nvme_id_ctrl *id)
>  {
> -	return false;
> +	int err;
> +	unsigned int i;
> +	bool found;
> +	u8 protocols[256+8]; /* 8 byte hdr + max number of possible protocols */

Please define a constant for the length in the new scsi_security.h
header.

> +	/* find ata security protocol */
> +	n = be16_to_cpup((__be16 *)(protocols+6));

Just use get_unaligned_be16 that operates directly on the char
array, similar to how we do in lots of places in the SCSI stack.

> +	for (i = 0; i <= n; i++) {
> +		if (protocols[8+i] == SCSI_SECURITY_PROTOCOL_ATA_SECURITY) {
> +			found = true;
> +			break;
> +		}
> +	}

I wonder if it might be a good idea to change the structure here a bit
to allow for future other protocols and have each protocol in a helper,
e.g. do something like

	for (i = 0; i <= n; i++) {
		switch (protocols[8 + i]) {
		case SCSI_SECURITY_PROTOCOL_ATA_SECURITY:
			locked |= nvme_security_ata_is_locked()
			break;
		default:
			break;
	}

> +	return ata_security[1] == 0xe && (ata_security[9]&ATA_SECURITY_LOCKED);

Can we have a sumbolic name for the 0xe?  Also please always add spaces
around your operators.

But after all this nitpicking the general idea looks fine, thanks a lot
for the patch!

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Christoph Hellwig]

On Mon, Jun 20, 2016 at 09:46:30AM +0300, Sagi Grimberg wrote:
> patch 1: nvme: Add function for NVMe security receive command
> patch 2: nvme: Check if drive is locked using ATA Security when scanning
> namespaces

Agreed. 

Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives

[Christoph Hellwig]

On Mon, Jun 20, 2016 at 11:26:39AM -0400, Keith Busch wrote:
> This is ATA security, though, so wouldn't ATA also benefit from this? The
> payload setup/decoding should then go in a generic library for everyone.

In principiple sharing this code would be fine, but right now the
actual ATA specific code is totally trivial.  Maybe we can have the
line of checking the actual data that we get from the ATA-specific
security send into a inline helper, but otherwise the to be shared bits
are mostly constants.

> Similar was said about the patch adding OPAL security to the NVMe driver:
> 
>   http://lists.infradead.org/pipermail/linux-nvme/2016-April/004428.html

OPAL is a lot more complex than checking this locked bit..

Re: [PATCH 1/3] nvme: When scanning namespaces, make sure the drive is not locked

[Christoph Hellwig]

On Sun, Jun 19, 2016 at 04:06:32PM -0700, Jethro Beekman wrote:
> +	if (nvme_security_is_locked(ctrl, id)) {
> +		nvme_remove_namespaces(ctrl);
> +		return;
> +	}

Should we print some information for the user that we are ignoring
a namespace because it is locked?