Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1426059 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] nvme: Don't add namespaces for locked drives Jethro Beekman <kernel@jbeekman.nl> - 2016-06-20 01:20 +0200
    Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Sagi Grimberg <sagigrim@gmail.com> - 2016-06-20 09:00 +0200
    Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Keith Busch <keith.busch@intel.com> - 2016-06-20 17:20 +0200
      Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Jethro Beekman <kernel@jbeekman.nl> - 2016-06-20 21:10 +0200
        Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Keith Busch <keith.busch@intel.com> - 2016-06-21 01:00 +0200
          Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Jethro Beekman <kernel@jbeekman.nl> - 2016-06-21 06:00 +0200

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

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

[toc] | [next] | [standalone]

#1426255

> 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

[toc] | [prev] | [next] | [standalone]

#1426692

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

[toc] | [prev] | [next] | [standalone]

#1426942

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

[toc] | [prev] | [next] | [standalone]

#1427117

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.

[toc] | [prev] | [next] | [standalone]

#1427255

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web