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 Christoph Hellwig <hch@infradead.org> - 2016-06-24 10:10 +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
            Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Christoph Hellwig <hch@infradead.org> - 2016-06-24 09:50 +0200
      Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Christoph Hellwig <hch@infradead.org> - 2016-06-24 10:20 +0200
    Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Christoph Hellwig <hch@infradead.org> - 2016-06-24 09:40 +0200
      Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Jethro Beekman <kernel@jbeekman.nl> - 2016-06-24 09:50 +0200
        Re: [PATCH 0/3] nvme: Don't add namespaces for locked drives Christoph Hellwig <hch@infradead.org> - 2016-06-24 10:10 +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]

#1430441

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. 

[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] | [next] | [standalone]

#1430431

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.

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

#1430449

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..

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

#1430427

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.

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

#1430430

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

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

#1430445

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?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web