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

Groups > linux.kernel > #1544749 > unrolled thread

[PATCH v3 0/5] SED OPAL Library

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] SED OPAL Library Scott Bauer <scott.bauer@intel.com> - 2016-12-19 20:50 +0100
    Re: [PATCH v3 2/5] lib: Add Sed-opal library Keith Busch <keith.busch@intel.com> - 2016-12-19 22:30 +0100
      Re: [PATCH v3 2/5] lib: Add Sed-opal library Christoph Hellwig <hch@infradead.org> - 2016-12-20 07:10 +0100
    Re: [PATCH v3 2/5] lib: Add Sed-opal library Al Viro <viro@ZenIV.linux.org.uk> - 2016-12-20 08:00 +0100
    Re: [PATCH v3 2/5] lib: Add Sed-opal library Christoph Hellwig <hch@infradead.org> - 2016-12-20 08:30 +0100
      Re: [PATCH v3 2/5] lib: Add Sed-opal library Jon Derrick <jonathan.derrick@intel.com> - 2016-12-20 23:10 +0100
        Re: [PATCH v3 2/5] lib: Add Sed-opal library Christoph Hellwig <hch@infradead.org> - 2016-12-21 10:50 +0100
      Re: [PATCH v3 2/5] lib: Add Sed-opal library Scott Bauer <scott.bauer@intel.com> - 2016-12-20 23:10 +0100
        Re: [PATCH v3 2/5] lib: Add Sed-opal library Christoph Hellwig <hch@infradead.org> - 2016-12-21 10:50 +0100

#1544749 — [PATCH v3 0/5] SED OPAL Library

Changes from v2->v3:

1) Removed the necessity of passing around block devices into the opal
   code. We now pass around a sed_context structure which contains a
   previously allocated opal_dev structure, sec_ops fn pointers, and
   opaque data for the send/recv functions to use.
2) Removed the allocation of the opal_dev structure from the opal_code
   to the driver wishing to use opal. The driver will use a sed_context,
   structure and allocate an opal_dev structure for each device. In the
   case of NVMe we store the sed_context structure in the control struct.
   When someone wishes to issue opal commands down to the controller they
   open the char dev. In the NVMe open implementation we assign our
   sed_context into the file structure.

   Pushing the burden of allocating and storing the opal_dev into the
   driver alleviates a bunch of look up code we had in v1/v2. Now by the
   time we get into the sed-opal code the driver has assigned us a
   sed_context and we operate directly on that.

   This should help with in-kernel use cases as well.
3) Since Opal will operate on the entire device, not per-namespace,
   we moved from block/ to fs/ and will operate on the nvme character
   driver. Because of that  the sed "guts" have moved from block/ back
   to lib/.
4) Numerous code clean ups in sed-opal.c to shorten the file. ~700
   lines reduced
5) Removed the variadic test_and_add_va for a hopefully easier to understand
   and maintain ADD_TOKEN macro that assigns values into the flat buffer
   and does error checking.

--------------------------------------------------------------------

This Patch series implements a large portion of the Opal protocol for
self encrypting devices. The driver has the capability of storing a
locking range's password. The password can then be replayed
during a resume from previous suspend-to-RAM.

The driver also supports logic to bring the device out of a factory
default-inactive state into a functional Opal state.

The following logic is supported in order to bring the tper into a
working state:

1) Taking Ownership of the drive (Setting the Admin CPIN).
2) Activating the Locking SP (In Single User Mode or Normal Mode).
3) Setting up Locking Ranges (Single User or Normal Mode).
4) Adding users to Locking Ranges (Normal Mode Only).
5) Locking or Unlocking Locking Ranges (Single User Mode or Normal Mode).
6) Reverting the TPer (Restore to factory default).
7) Setting LR/User passwords (Single User Mode or Normal Mode).
8) Enabling/disabling Shadow MBR.
9) Enabling Users in the LockingSP (Normal Mode Only).
10) Saving Password for resume from suspend.
11) Erase and Secure erasing locking ranges.

All commands are exported through the Fs ioctl.

Scott Bauer (5):
  include: Add definitions for sed
  lib: Add Sed-opal library
  fs: Wire up SED/Opal to ioctl
  nvme: Implement resume_from_suspend and SED Allocation code.
  Maintainers: Add Information for SED Opal library

 MAINTAINERS                   |   10 +
 drivers/nvme/host/core.c      |   67 ++
 drivers/nvme/host/nvme.h      |    8 +-
 drivers/nvme/host/pci.c       |   10 +-
 fs/ioctl.c                    |    3 +
 include/linux/fs.h            |    2 +
 include/linux/sed-opal.h      |   38 +
 include/linux/sed.h           |   76 ++
 include/uapi/linux/sed-opal.h |   94 ++
 include/uapi/linux/sed.h      |   64 ++
 lib/Makefile                  |    2 +-
 lib/sed-opal.c                | 2376 +++++++++++++++++++++++++++++++++++++++++
 lib/sed-opal_internal.h       |  601 +++++++++++
 lib/sed.c                     |  197 ++++
 14 files changed, 3545 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/sed-opal.h
 create mode 100644 include/linux/sed.h
 create mode 100644 include/uapi/linux/sed-opal.h
 create mode 100644 include/uapi/linux/sed.h
 create mode 100644 lib/sed-opal.c
 create mode 100644 lib/sed-opal_internal.h
 create mode 100644 lib/sed.c

[toc] | [next] | [standalone]

#1544778 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

On Mon, Dec 19, 2016 at 12:35:46PM -0700, Scott Bauer wrote:
> This patch implements the necessary logic to bring an Opal
> enabled drive out of a factory-enabled into a working
> Opal state.
> 
> This patch set also enables logic to save a password to
> be replayed during a resume from suspend.
> 
> Signed-off-by: Scott Bauer <scott.bauer@intel.com>
> Signed-off-by: Rafael Antognolli <Rafael.Antognolli@intel.com>
> ---
>  lib/Makefile            |    2 +-
>  lib/sed-opal.c          | 2376 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/sed-opal_internal.h |  601 ++++++++++++
>  lib/sed.c               |  197 ++++
>  4 files changed, 3175 insertions(+), 1 deletion(-)
>  create mode 100644 lib/sed-opal.c
>  create mode 100644 lib/sed-opal_internal.h
>  create mode 100644 lib/sed.c
> 
> diff --git a/lib/Makefile b/lib/Makefile
> index 50144a3..acb5d82 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -36,7 +36,7 @@ obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
>  	 gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
>  	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
>  	 percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
> -	 once.o
> +	 once.o sed.o sed-opal.o

This seems like an optional library that some environments may wish to
opt-out of building into the kernel. Any reason not to add an entry into
the Kconfig to turn this on/off?

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

#1544929 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

On Mon, Dec 19, 2016 at 04:34:15PM -0500, Keith Busch wrote:
> This seems like an optional library that some environments may wish to
> opt-out of building into the kernel. Any reason not to add an entry into
> the Kconfig to turn this on/off?

This needs to be a CONFIG_BLOCK_SED / CONFIG_BLOCK_SED_OPAL and
should move to block/.

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

#1544950 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

On Mon, Dec 19, 2016 at 12:35:46PM -0700, Scott Bauer wrote:

> +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> +		   unsigned long arg)
> +{
> +	struct sed_key key;
> +	struct sed_context *sed_ctx;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EACCES;
> +
> +	if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> +		return -ENODEV;
> +
> +	sed_ctx = filep->f_sedctx;

First of all, that's a bisect hazard.  What's more, looking through the
rest of patchset, WTF does it
	* need to be called that early in ioctl(2) handling, instead of
having ->ioctl() instance for that sucker calling it?
	* _not_ get your ->f_sedctx as an explicit argument, passed by
the caller in ->ioctl(), seeing that it's possible to calculate by
file->private_data?
	* store that thing in struct file itself, bloating it for everything
all for the sake of few drivers that might want to use that helper?

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

#1544963 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

> +	u8 lr;
> +	size_t key_name_len;
> +	char key_name[36];

Who is going to use the key_name?  I can't find another reference to
it anywhere else in the code.   The reason why this tripped me off was
the hardcoded length so I was going to check on how access to it is
bounds checked.

> +/**
> + * struct opal_dev - The structure representing a OPAL enabled SED.
> + * @sed_ctx:The SED context, contains fn pointers to sec_send/recv.
> + * @opal_step:A series of opal methods that are necessary to complete a comannd.
> + * @func_data:An array of parameters for the opal methods above.
> + * @state:Describes the current opal_step we're working on.
> + * @dev_lock:Locks the entire opal_dev structure.
> + * @parsed:Parsed response from controller.
> + * @prev_data:Data returned from a method to the controller
> + * @error_cb:Error function that handles closing sessions after a failed method.
> + * @unlk_lst:A list of Locking ranges to unlock on this device during a resume.
> + */

Needs spaces after the colons.

> +	u16 comID;
> +	u32 HSN;
> +	u32 TSN;

Please use lower case variable names in Linux code, and separate
words by underscores if needed.

> +DEFINE_SPINLOCK(list_spinlock);

Should be marked static.

> +#define TPER_SYNC_SUPPORTED BIT(0)

This sounds like a protocol constant and should go into the header
with the rest of the protocol.

> +static bool check_tper(const void *data)
> +{
> +	const struct d0_tper_features *tper = data;
> +	u8 flags = tper->supported_features;
> +
> +	if (!(flags & TPER_SYNC_SUPPORTED)) {
> +		pr_err("TPer sync not supported. flags = %d\n",
> +		       tper->supported_features);

It would be great to use dev_err / dev_warn / etc in the opal code,
although for that you'll need to pass a struct device from the driver
into the code.

> +static bool check_SUM(const void *data)

The comment on member naming above also applies to variables and function
names.

> +	bool foundComID = false, supported = true, single_user = false;
> +	const struct d0_header *hdr;
> +	const u8 *epos, *cpos;
> +	u16 comID = 0;
> +	int error = 0;
> +
> +	epos = dev->cmd.resp;
> +	cpos = dev->cmd.resp;
> +	hdr = (struct d0_header *)dev->cmd.resp;

You probably want to structure your command buffers to use void pointers
and avoid ths kind of casting.

> +#define TINY_ATOM_DATA_MASK GENMASK(5, 0)
> +#define TINY_ATOM_SIGNED BIT(6)
> +
> +#define SHORT_ATOM_ID BIT(7)
> +#define SHORT_ATOM_BYTESTRING BIT(5)
> +#define SHORT_ATOM_SIGNED BIT(4)
> +#define SHORT_ATOM_LEN_MASK GENMASK(3, 0)

Protocol constants for the header again?

> +#define ADD_TOKEN_STRING(cmd, key, keylen)		        \
> +	if (!err)					        \
> +		err = test_and_add_string(cmd, key, keylen);
> +
> +#define ADD_TOKEN(type, cmd, tok)				\
> +	if (!err)						\
> +		err = test_and_add_token_##type(cmd, tok);

Please remove these macros and just open code the calls.  If you want
to avoid writing the if err lines again and again just pass err
by reference to the functions and move the err check into the add_token*
helpers.

> +	if ((hdr->cp.length == 0)
> +	    || (hdr->pkt.length == 0)
> +	    || (hdr->subpkt.length == 0)) {

No need for the inner braces, also please place the operators at the
end of the previous line instead of the beginning of the new line.

> +	while (cpos < total) {
> +		if (!(pos[0] & 0x80)) /* tiny atom */
> +			token_length = response_parse_tiny(iter, pos);
> +		else if (!(pos[0] & 0x40)) /* short atom */
> +			token_length = response_parse_short(iter, pos);
> +		else if (!(pos[0] & 0x20)) /* medium atom */
> +			token_length = response_parse_medium(iter, pos);
> +		else if (!(pos[0] & 0x10)) /* long atom */
> +			token_length = response_parse_long(iter, pos);

Please add symbolic names for these constants to the protocol header.

> +	if (num_entries == 0) {
> +		pr_err("Couldn't parse response.\n");
> +		return -EINVAL;
> +	resp->num = num_entries;

Where is the closing brace for that if?

> +	if (!((resp->toks[n].width == OPAL_WIDTH_TINY) ||
> +	      (resp->toks[n].width == OPAL_WIDTH_SHORT))) {

No need for the inner braces.

> +		pr_err("Atom is not short or tiny: %d\n",
> +		       resp->toks[n].width);
> +		return 0;
> +	}
> +
> +	return resp->toks[n].stored.u;
> +}
> +
> +static u8 response_status(const struct parsed_resp *resp)
> +{
> +	if ((token_type(resp, 0) == OPAL_DTA_TOKENID_TOKEN)
> +	    && (response_get_token(resp, 0) == OPAL_ENDOFSESSION)) {

Same here, also please fix the operator placement.

> +	if ((token_type(resp, resp->num - 1) != OPAL_DTA_TOKENID_TOKEN) ||
> +	    (token_type(resp, resp->num - 5) != OPAL_DTA_TOKENID_TOKEN) ||
> +	    (response_get_token(resp, resp->num - 1) != OPAL_ENDLIST) ||
> +	    (response_get_token(resp, resp->num - 5) != OPAL_STARTLIST))

More brace removal here please (and probably a lot more later on, I'm
going to skip them from here)

> +static inline void opal_dev_get(struct opal_dev *dev)
> +{
> +	mutex_lock(&dev->dev_lock);
> +}
> +
> +static inline void opal_dev_put(struct opal_dev *dev)
> +{
> +	mutex_unlock(&dev->dev_lock);
> +}

No trivial wrappers around locking primitives, please.

> +static int add_suspend_info(struct opal_dev *dev, struct opal_suspend_data *sus)
> +{
> +	struct opal_suspend_data *iter;
> +	bool found = false;
> +
> +	if (list_empty(&dev->unlk_lst))
> +		goto add_out;
> +
> +	list_for_each_entry(iter, &dev->unlk_lst, node) {

list_for_each_entry will do the right thing for an empty
list, you can remove the above check,

> +		if (iter->lr == sus->lr) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		/* Replace the old with the new */
> +		list_del(&iter->node);
> +		kfree(iter);
> +	}

Just move the list_del and kfree inside the if above and you
can remove the found variable.

> +
> +int activate_lsp(struct opal_dev *dev)

static?

> +static int get_msid_cpin_pin_cont(struct opal_dev *dev)
> +{
> +	const char *msid_pin;
> +	size_t strlen;
> +	int error = 0;
> +
> +	error = parse_and_check_status(dev);
> +	if (error)
> +		return error;
> +
> +	strlen = response_get_string(&dev->parsed, 4, &msid_pin);
> +	if (!msid_pin) {
> +		pr_err("%s: Couldn't extract PIN from response\n", __func__);
> +		return 11;

please add constant for your magic return values.

> +	}
> +
> +	dev->prev_data = kmemdup(msid_pin, strlen, GFP_KERNEL);
> +	if (!dev->prev_data)
> +		return -ENOMEM;
> +
> +	dev->prev_d_len = strlen;
> +
> + err_return:
> +	return 0;

I can't find anything actually jumping to this label.  And if it did
it could just return directly.

> +
> +static struct opal_dev *get_opal_dev(struct sed_context *sedc,
> +				     const opal_step *funcs)
> +{
> +	struct opal_dev *dev = sedc->dev;
> +	if (dev) {
> +		dev->state = 0;
> +		dev->funcs = funcs;
> +		dev->TSN = 0;
> +		dev->HSN = 0;
> +		dev->error_cb = end_opal_session_error;
> +		dev->error_cb_data = dev;
> +		dev->func_data = NULL;
> +		dev->sed_ctx = sedc;
> +		opal_dev_get(dev);
> +	}
> +	return dev;

Who serialized access to sedv->dev?

> +	struct opal_dev *dev;
> +	void *data[3] = { NULL };
> +	const opal_step erase_funcs[] = {
> +		opal_discovery0,
> +		start_auth_opal_session,
> +		get_active_key,
> +		gen_key,
> +		end_opal_session,
> +		NULL,
> +	};

static?

> +EXPORT_SYMBOL(opal_enable_disable_shadow_mbr);

As far as I can tell nothing but alloc_opal_dev and
opal_unlock_from_suspend is every called from another module,
so all these exports could be dropped.


> +	struct opal_dev *dev;
> +		const opal_step funcs[] = {

wrong indentation.  also all these arrays of functions should be
marked static.

> +	if (!list_empty(&dev->unlk_lst)) {
> +		list_for_each_entry(suspend, &dev->unlk_lst, node) {

No need for the list_empty check.

> --- /dev/null
> +++ b/lib/sed-opal_internal.h

This pretty much seem to contain the OPAL protocol defintions, so why
not opal_proto.h?

> +#ifndef _NVME_OPAL_INTERNAL_H
> +#define _NVME_OPAL_INTERNAL_H

And this doesn't seem to match the file name.

> +#include <linux/key-type.h>
> +#include <keys/user-type.h>

These don't seem to be needed.

> +/*
> + * Derived from:
> + * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
> + * Section: 5.1.5 Method Status Codes
> + */
> +static const char *opal_errors[] = {

static consta char * const opal_errors[] = {

also please move this into a .c file so that it's not duplicated
in every file that includes the header.

> +static const char *opal_error_to_human(int error)
> +{
> +	if (error == 0x3f)
> +		return "Failed";
> +
> +	if (error >= ARRAY_SIZE(opal_errors) || error < 0)
> +		return "Unknown Error";
> +
> +	return opal_errors[error];
> +}

Same for this one.

> +/*
> + * User IDs used in the TCG storage SSCs
> + * Derived from: TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
> + * Section: 6.3 Assigned UIDs
> + */
> +static const u8 OPALUID[][8] = {

And all the other following variable delcarations.

Also please use lower case names with underscores as separators
for your variable and type names.  Constants can remain in all caps.

> +static const size_t OPAL_UID_LENGTH = 8;
> +static const size_t OPAL_MSID_KEYLEN = 15;
> +static const size_t OPAL_UID_LENGTH_HALF = 4;

Use a #define or enum fo these constants.


> +struct key *request_user_key(const char *master_desc, const u8 **master_key,
> +			     size_t *master_keylen);

This one doesn't actually seem to be used.

> +int sed_save(struct sed_context *sed_ctx, struct sed_key *key)
> +{
> +	switch (key->sed_type) {
> +	case OPAL_LOCK_UNLOCK:
> +		return opal_save(sed_ctx, key);
> +	}
> +
> +	return -EOPNOTSUPP;

It seems to me that we should skip this whole sed_type indirections
and just specify the ioctls directly for OPAL (which would include
opalite and pyrite as subsets).  The only other protocols of interest
for Linux would be the ATA "security" plain text passwords, which can
be handled differently, or enterprise SSC which we can hopefully avoid
to implement entirely.

> +
> +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> +		   unsigned long arg)
> +{
> +	struct sed_key key;
> +	struct sed_context *sed_ctx;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EACCES;
> +
> +	if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> +		return -ENODEV;

In the previous version this was called from the block driver which
could pass in the context (and ops).  Why was this changed?

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

#1545493 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

On Mon, Dec 19, 2016 at 11:28:47PM -0800, Christoph Hellwig wrote:
[snip]
> > +	while (cpos < total) {
> > +		if (!(pos[0] & 0x80)) /* tiny atom */
> > +			token_length = response_parse_tiny(iter, pos);
> > +		else if (!(pos[0] & 0x40)) /* short atom */
> > +			token_length = response_parse_short(iter, pos);
> > +		else if (!(pos[0] & 0x20)) /* medium atom */
> > +			token_length = response_parse_medium(iter, pos);
> > +		else if (!(pos[0] & 0x10)) /* long atom */
> > +			token_length = response_parse_long(iter, pos);
> 
> Please add symbolic names for these constants to the protocol header.
> 
I get tripped up by this logic every time I look at it. I'd almost
rather see something like the following, which more closely follows the
TCG core spec and the optimizing compiler should turn it into something
similar to above anyways:

	if (pos[0] <= 0x7F)
		token_length = response_parse_tiny..
	else if (pos[0] <= 0xBF)
		token_length = response_parse_short..
	else if (pos[0] <= 0xDF)
		token_length = response_parse_medium..
	else if (pos[0] <= 0xE3)
		token_length = response_parse_long..

Then just add those 0x7F, 0xBF, 0xDF, and 0xE3 constants to proto
header. We could even add in a TCG reserved error for 0xE4-0xEF

Also instead of tracking cpos, we could subtract from total until
negative (if you make it signed). It's similar to how we do length in
nvme_setup_prps.

[snip]
> > --- /dev/null
> > +++ b/lib/sed-opal_internal.h
> 
> This pretty much seem to contain the OPAL protocol defintions, so why
> not opal_proto.h?
Since there might eventually be a whole class of opal-like sed
protocols, why does it make more sense to have opal_proto.h instead of
sed-opal.h or some variation? This is similar to how leds-*.h look to
me. Although I agree that sed-ATA.h would be dishonest since ATA
security doesn't imply a self-encrypting-disk.

[snip]
> > +int sed_save(struct sed_context *sed_ctx, struct sed_key *key)
> > +{
> > +	switch (key->sed_type) {
> > +	case OPAL_LOCK_UNLOCK:
> > +		return opal_save(sed_ctx, key);
> > +	}
> > +
> > +	return -EOPNOTSUPP;
> 
> It seems to me that we should skip this whole sed_type indirections
> and just specify the ioctls directly for OPAL (which would include
> opalite and pyrite as subsets).  The only other protocols of interest
> for Linux would be the ATA "security" plain text passwords, which can
> be handled differently, or enterprise SSC which we can hopefully avoid
> to implement entirely.
I'm on board with this if you think we won't have enough different, but
similar, SED protocols to justify the indirection. In that case you can
ignore the above comment as well.

> 
> > +
> > +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> > +		   unsigned long arg)
> > +{
> > +	struct sed_key key;
> > +	struct sed_context *sed_ctx;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EACCES;
> > +
> > +	if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> > +		return -ENODEV;
> 
> In the previous version this was called from the block driver which
> could pass in the context (and ops).  Why was this changed?
> 

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

#1545686 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

On Tue, Dec 20, 2016 at 03:07:46PM -0700, Jon Derrick wrote:
> > This pretty much seem to contain the OPAL protocol defintions, so why
> > not opal_proto.h?
> Since there might eventually be a whole class of opal-like sed
> protocols, why does it make more sense to have opal_proto.h instead of
> sed-opal.h or some variation? This is similar to how leds-*.h look to
> me. Although I agree that sed-ATA.h would be dishonest since ATA
> security doesn't imply a self-encrypting-disk.

As far as I can tell the NVMe / SCSI / ATA security landscape looks
like:

 - ATA security - specified in ATA, kinda implicitly referenced by SPC
   and then even more implitly in NVMe.  Actually implemented in various
   NVMe consumer devices because OEMs insist on it.  Basically just
   a trivial plain text password lock/unlock.

 - TCG OPAL including the subsets OPALite and Pyrite, whereas the latter
   is just the above lock/unlock in a TCG way.

 - TCG Enterprise SSC

I think it's pretty obvious that ATA security should be something on
it's own.  OPALite and Pyrite are strict subsets of OPAL, so having them
in something named opal shouldn't be surprising.  The big question
is what to do about Enterprise SSC.  Which has some overlaps with OPAL
but also major differences, so keeping it separate if we ever have to
implement it (I hope we don't) would be best.

> I'm on board with this if you think we won't have enough different, but
> similar, SED protocols to justify the indirection. In that case you can
> ignore the above comment as well.

This goes back to the above.  The only thing that is not a strict subset
of OPAL but kinda sorta similar is TCG Enterprise SSC, but my preference
would be to ignore it, and the second best preference would be to keep
it separate.

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

#1545495 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

> > +
> > +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> > +		   unsigned long arg)
> > +{
> > +	struct sed_key key;
> > +	struct sed_context *sed_ctx;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EACCES;
> > +
> > +	if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> > +		return -ENODEV;
> 
> In the previous version this was called from the block driver which
> could pass in the context (and ops).  Why was this changed?
> 

Do you mean from nvme_ioctl? or from blk_ioctl? It was removed from  blk_ioctl
because I must have misinterpreted your comments here:
http://lists.infradead.org/pipermail/linux-nvme/2016-December/007364.html

Copied from the link: 
>
> > and directly use
> > block_device. Then if we add the security send/receive operations to the
> > block_device_operations, that will simplify chaining the security request
> > to the driver without needing to thread the driver's requested callback
> > and data the way you have to here since all the necessary information
> > is encapsulated in the block_device.

> Maybe.  I need to look at the TCG spec again (oh my good, what a fucking
> mess), but if I remember the context if it is the whole nvme controller
> and not just a namespace, so a block_device might be the wrong context.
> Then again we can always go from the block_device to the controller
> fairly easily.  So instead of adding the security operation to the
> block_device_operations which we don't really need for now maybe we
> should add a security_conext to the block device so that we can avoid
> all the lookup code?

I took your hesitation about the block_device to mean try something new,
that combined with my concern about having namespaces have the ability to
lock the global range which can span ourside their LBA ranges lead me to
remove it from block and put it in the char dev world.

On the same note there is a public review of
TCG Storage Opal SSC Feature Set: Configurable Namespace Locking:
Which Jon Derrick found for us:
https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Feature_Set_Namespaces_phase_1b_v1_00_r1_19_public-review.pdf

Where they are thinking about doing a complete 180 from:
https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf

And now Namespaces can have their own global locking range as well as locking
objects within them. 

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

#1545685 — Re: [PATCH v3 2/5] lib: Add Sed-opal library

On Tue, Dec 20, 2016 at 02:55:10PM -0700, Scott Bauer wrote:
> Do you mean from nvme_ioctl? or from blk_ioctl? It was removed from  blk_ioctl
> because I must have misinterpreted your comments here:
> http://lists.infradead.org/pipermail/linux-nvme/2016-December/007364.html

nvme_ioctl, aka the ioctl method of the block driver.  In the future
I expect another caller in scsi_ioctl or sd_ioctl.

> I took your hesitation about the block_device to mean try something new,
> that combined with my concern about having namespaces have the ability to
> lock the global range which can span ourside their LBA ranges lead me to
> remove it from block and put it in the char dev world.

In the link I literally meant the struct block_device, not nessecarily
the block device node.

> 
> On the same note there is a public review of
> TCG Storage Opal SSC Feature Set: Configurable Namespace Locking:
> Which Jon Derrick found for us:
> https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Feature_Set_Namespaces_phase_1b_v1_00_r1_19_public-review.pdf
> 
> Where they are thinking about doing a complete 180 from:
> https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf
> 
> And now Namespaces can have their own global locking range as well as locking
> objects within them. 

Interesting.  From a quick look this is how it should have been done
from the start.  Now the big question is when devices are going to
implement it, and how we can support both from the same driver, as this
means we'll have to deal with two different ways to allocate the
security context based on the device.  And do the horrible mess that the
TCG SIISs are there is absolute no way to discover this at the NVMe
level, but we actually need to start doing TCG method calls to even
figure out what's going on.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web