* OPAL fixups @ 2017-02-17 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme Hi all, this contains a few more OPAL-related fixups. It tones down warnings a bit, allocates the OPAL-ѕpecific data structure in a separate dynamic allocation, checks for support of Security Send/Receive in NVMe before using them, and makes sure we re-discovery the security capabilities after each reset. ^ permalink raw reply [flat|nested] 24+ messages in thread
* OPAL fixups @ 2017-02-17 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) Hi all, this contains a few more OPAL-related fixups. It tones down warnings a bit, allocates the OPAL-?pecific data structure in a separate dynamic allocation, checks for support of Security Send/Receive in NVMe before using them, and makes sure we re-discovery the security capabilities after each reset. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/4] block/sed-opal: tone down not supported warnings 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 12:59 ` Christoph Hellwig -1 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme Not having OPAL or a sub-feature supported is an entirely normal condition for many drives, so don't warn about it. Keep the messages, but tone them down to debug only. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Scott Bauer <scott.bauer@intel.com> --- block/sed-opal.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/sed-opal.c b/block/sed-opal.c index e95b8a57053d..bcdd5b6d02e8 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -387,16 +387,16 @@ static int opal_discovery0_end(struct opal_dev *dev) } if (!supported) { - pr_err("This device is not Opal enabled. Not Supported!\n"); + pr_debug("This device is not Opal enabled. Not Supported!\n"); return -EOPNOTSUPP; } if (!single_user) - pr_warn("Device doesn't support single user mode\n"); + pr_debug("Device doesn't support single user mode\n"); if (!found_com_id) { - pr_warn("Could not find OPAL comid for device. Returning early\n"); + pr_debug("Could not find OPAL comid for device. Returning early\n"); return -EOPNOTSUPP;; } @@ -1951,7 +1951,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv) mutex_init(&opal_dev->dev_lock); opal_dev->send_recv = send_recv; if (check_opal_support(opal_dev) < 0) - pr_warn("Opal is not supported on this device\n"); + pr_debug("Opal is not supported on this device\n"); opal_dev->initialized = true; } EXPORT_SYMBOL(init_opal_dev); -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/4] block/sed-opal: tone down not supported warnings @ 2017-02-17 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) Not having OPAL or a sub-feature supported is an entirely normal condition for many drives, so don't warn about it. Keep the messages, but tone them down to debug only. Signed-off-by: Christoph Hellwig <hch at lst.de> Reviewed-by: Scott Bauer <scott.bauer at intel.com> --- block/sed-opal.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/sed-opal.c b/block/sed-opal.c index e95b8a57053d..bcdd5b6d02e8 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -387,16 +387,16 @@ static int opal_discovery0_end(struct opal_dev *dev) } if (!supported) { - pr_err("This device is not Opal enabled. Not Supported!\n"); + pr_debug("This device is not Opal enabled. Not Supported!\n"); return -EOPNOTSUPP; } if (!single_user) - pr_warn("Device doesn't support single user mode\n"); + pr_debug("Device doesn't support single user mode\n"); if (!found_com_id) { - pr_warn("Could not find OPAL comid for device. Returning early\n"); + pr_debug("Could not find OPAL comid for device. Returning early\n"); return -EOPNOTSUPP;; } @@ -1951,7 +1951,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv) mutex_init(&opal_dev->dev_lock); opal_dev->send_recv = send_recv; if (check_opal_support(opal_dev) < 0) - pr_warn("Opal is not supported on this device\n"); + pr_debug("Opal is not supported on this device\n"); opal_dev->initialized = true; } EXPORT_SYMBOL(init_opal_dev); -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 12:59 ` Christoph Hellwig -1 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme Insted of bloating the containing structure with it all the time this allocates struct opal_dev dynamically. Additionally this allows moving the definition of struct opal_dev into sed-opal.c. For this a new private data field is added to it that is passed to the send/receive callback. After that a lot of internals can be made private as well. Signed-off-by: Christoph Hellwig <hch@lst.de> Tested-by: Scott Bauer <scott.bauer@intel.com> Reviewed-by: Scott Bauer <scott.bauer@intel.com> --- block/opal_proto.h | 23 ++++++++++ block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++---- drivers/nvme/host/core.c | 9 ++-- drivers/nvme/host/nvme.h | 14 ++---- drivers/nvme/host/pci.c | 8 +++- include/linux/sed-opal.h | 116 ++--------------------------------------------- 6 files changed, 131 insertions(+), 140 deletions(-) diff --git a/block/opal_proto.h b/block/opal_proto.h index af9abc56c157..f40c9acf8895 100644 --- a/block/opal_proto.h +++ b/block/opal_proto.h @@ -19,6 +19,29 @@ #ifndef _OPAL_PROTO_H #define _OPAL_PROTO_H +/* + * These constant values come from: + * SPC-4 section + * 6.30 SECURITY PROTOCOL IN command / table 265. + */ +enum { + TCG_SECP_00 = 0, + TCG_SECP_01, +}; + +/* + * Token defs derived from: + * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00 + * 3.2.2 Data Stream Encoding + */ +enum opal_response_token { + OPAL_DTA_TOKENID_BYTESTRING = 0xe0, + OPAL_DTA_TOKENID_SINT = 0xe1, + OPAL_DTA_TOKENID_UINT = 0xe2, + OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */ + OPAL_DTA_TOKENID_INVALID = 0X0 +}; + #define DTAERROR_NO_METHOD_STATUS 0x89 #define GENERIC_HOST_SESSION_NUM 0x41 diff --git a/block/sed-opal.c b/block/sed-opal.c index bcdd5b6d02e8..d1c52ba4d62d 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -31,6 +31,77 @@ #include "opal_proto.h" +#define IO_BUFFER_LENGTH 2048 +#define MAX_TOKS 64 + +typedef int (*opal_step)(struct opal_dev *dev); + +enum opal_atom_width { + OPAL_WIDTH_TINY, + OPAL_WIDTH_SHORT, + OPAL_WIDTH_MEDIUM, + OPAL_WIDTH_LONG, + OPAL_WIDTH_TOKEN +}; + +/* + * On the parsed response, we don't store again the toks that are already + * stored in the response buffer. Instead, for each token, we just store a + * pointer to the position in the buffer where the token starts, and the size + * of the token in bytes. + */ +struct opal_resp_tok { + const u8 *pos; + size_t len; + enum opal_response_token type; + enum opal_atom_width width; + union { + u64 u; + s64 s; + } stored; +}; + +/* + * From the response header it's not possible to know how many tokens there are + * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later + * if we start dealing with messages that have more than that, we can increase + * this number. This is done to avoid having to make two passes through the + * response, the first one counting how many tokens we have and the second one + * actually storing the positions. + */ +struct parsed_resp { + int num; + struct opal_resp_tok toks[MAX_TOKS]; +}; + +struct opal_dev { + bool supported; + + void *data; + sec_send_recv *send_recv; + + const opal_step *funcs; + void **func_data; + int state; + struct mutex dev_lock; + u16 comid; + u32 hsn; + u32 tsn; + u64 align; + u64 lowest_lba; + + size_t pos; + u8 cmd[IO_BUFFER_LENGTH]; + u8 resp[IO_BUFFER_LENGTH]; + + struct parsed_resp parsed; + size_t prev_d_len; + void *prev_data; + + struct list_head unlk_lst; +}; + + static const u8 opaluid[][OPAL_UID_LENGTH] = { /* users */ [OPAL_SMUID_UID] = @@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data) static int opal_send_cmd(struct opal_dev *dev) { - return dev->send_recv(dev, dev->comid, TCG_SECP_01, + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01, dev->cmd, IO_BUFFER_LENGTH, true); } static int opal_recv_cmd(struct opal_dev *dev) { - return dev->send_recv(dev, dev->comid, TCG_SECP_01, + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01, dev->resp, IO_BUFFER_LENGTH, false); } @@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev) return ret; } -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv) +struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv) { - if (opal_dev->initialized) - return; - INIT_LIST_HEAD(&opal_dev->unlk_lst); - mutex_init(&opal_dev->dev_lock); - opal_dev->send_recv = send_recv; - if (check_opal_support(opal_dev) < 0) + struct opal_dev *dev; + + dev = kmalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return NULL; + + INIT_LIST_HEAD(&dev->unlk_lst); + mutex_init(&dev->dev_lock); + dev->data = data; + dev->send_recv = send_recv; + if (check_opal_support(dev) != 0) { pr_debug("Opal is not supported on this device\n"); - opal_dev->initialized = true; + kfree(dev); + return NULL; + } + return dev; } EXPORT_SYMBOL(init_opal_dev); @@ -2351,6 +2430,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) if (!capable(CAP_SYS_ADMIN)) return -EACCES; + if (!dev) + return -ENOTSUPP; if (!dev->supported) { pr_err("Not supported\n"); return -ENOTSUPP; diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 04c48e75ff16..8aeb4a623b65 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode, return nvme_nvm_ioctl(ns, cmd, arg); #endif if (is_sed_ioctl(cmd)) - return sed_ioctl(&ns->ctrl->opal_dev, cmd, + return sed_ioctl(ns->ctrl->opal_dev, cmd, (void __user *) arg); return -ENOTTY; } @@ -1085,18 +1085,17 @@ static const struct pr_ops nvme_pr_ops = { }; #ifdef CONFIG_BLK_SED_OPAL -int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp, - void *buffer, size_t len, bool send) +int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len, + bool send) { + struct nvme_ctrl *ctrl = data; struct nvme_command cmd; - struct nvme_ctrl *ctrl = NULL; memset(&cmd, 0, sizeof(cmd)); if (send) cmd.common.opcode = nvme_admin_security_send; else cmd.common.opcode = nvme_admin_security_recv; - ctrl = container_of(dev, struct nvme_ctrl, opal_dev); cmd.common.nsid = 0; cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8); cmd.common.cdw10[1] = cpu_to_le32(len); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 569cba14cede..5126c4bbee1a 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -126,7 +126,7 @@ struct nvme_ctrl { struct list_head node; struct ida ns_ida; - struct opal_dev opal_dev; + struct opal_dev *opal_dev; char name[12]; char serial[20]; @@ -270,16 +270,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl); void nvme_queue_scan(struct nvme_ctrl *ctrl); void nvme_remove_namespaces(struct nvme_ctrl *ctrl); -#ifdef CONFIG_BLK_SED_OPAL -int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp, - void *buffer, size_t len, bool send); -#else -static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp, - void *buffer, size_t len, bool send) -{ - return 0; -} -#endif /* CONFIG_BLK_DEV_SED_OPAL */ +int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len, + bool send); #define NVME_NR_AERS 1 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e25d632bd9f2..5db8a38a8b43 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl) if (dev->ctrl.admin_q) blk_put_queue(dev->ctrl.admin_q); kfree(dev->queues); + kfree(dev->ctrl.opal_dev); kfree(dev); } @@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit); + if (!dev->ctrl.opal_dev) { + dev->ctrl.opal_dev = + init_opal_dev(&dev->ctrl, &nvme_sec_submit); + } if (was_suspend) - opal_unlock_from_suspend(&dev->ctrl.opal_dev); + opal_unlock_from_suspend(dev->ctrl.opal_dev); result = nvme_setup_io_queues(dev); if (result) diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h index 205d520ea688..deee23d012e7 100644 --- a/include/linux/sed-opal.h +++ b/include/linux/sed-opal.h @@ -21,117 +21,14 @@ #include <uapi/linux/sed-opal.h> #include <linux/kernel.h> -/* - * These constant values come from: - * SPC-4 section - * 6.30 SECURITY PROTOCOL IN command / table 265. - */ -enum { - TCG_SECP_00 = 0, - TCG_SECP_01, -}; struct opal_dev; -#define IO_BUFFER_LENGTH 2048 -#define MAX_TOKS 64 - -typedef int (*opal_step)(struct opal_dev *dev); -typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp, - void *buffer, size_t len, bool send); - - -enum opal_atom_width { - OPAL_WIDTH_TINY, - OPAL_WIDTH_SHORT, - OPAL_WIDTH_MEDIUM, - OPAL_WIDTH_LONG, - OPAL_WIDTH_TOKEN -}; - -/* - * Token defs derived from: - * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00 - * 3.2.2 Data Stream Encoding - */ -enum opal_response_token { - OPAL_DTA_TOKENID_BYTESTRING = 0xe0, - OPAL_DTA_TOKENID_SINT = 0xe1, - OPAL_DTA_TOKENID_UINT = 0xe2, - OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */ - OPAL_DTA_TOKENID_INVALID = 0X0 -}; - -/* - * On the parsed response, we don't store again the toks that are already - * stored in the response buffer. Instead, for each token, we just store a - * pointer to the position in the buffer where the token starts, and the size - * of the token in bytes. - */ -struct opal_resp_tok { - const u8 *pos; - size_t len; - enum opal_response_token type; - enum opal_atom_width width; - union { - u64 u; - s64 s; - } stored; -}; - -/* - * From the response header it's not possible to know how many tokens there are - * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later - * if we start dealing with messages that have more than that, we can increase - * this number. This is done to avoid having to make two passes through the - * response, the first one counting how many tokens we have and the second one - * actually storing the positions. - */ -struct parsed_resp { - int num; - struct opal_resp_tok toks[MAX_TOKS]; -}; - -/** - * struct opal_dev - The structure representing a OPAL enabled SED. - * @supported: Whether or not OPAL is supported on this controller. - * @send_recv: The combined sec_send/sec_recv function pointer. - * @opal_step: A series of opal methods that are necessary to complete a command. - * @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. - * @unlk_lst: A list of Locking ranges to unlock on this device during a resume. - */ -struct opal_dev { - bool initialized; - bool supported; - sec_send_recv *send_recv; - - const opal_step *funcs; - void **func_data; - int state; - struct mutex dev_lock; - u16 comid; - u32 hsn; - u32 tsn; - u64 align; - u64 lowest_lba; - - size_t pos; - u8 cmd[IO_BUFFER_LENGTH]; - u8 resp[IO_BUFFER_LENGTH]; - - struct parsed_resp parsed; - size_t prev_d_len; - void *prev_data; - - struct list_head unlk_lst; -}; +typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer, + size_t len, bool send); #ifdef CONFIG_BLK_SED_OPAL bool opal_unlock_from_suspend(struct opal_dev *dev); -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv); +struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv); int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr); static inline bool is_sed_ioctl(unsigned int cmd) @@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev) { return false; } -static inline void init_opal_dev(struct opal_dev *opal_dev, - sec_send_recv *send_recv) -{ - opal_dev->supported = false; - opal_dev->initialized = true; -} +#define init_opal_dev(data, send_recv) NULL #endif /* CONFIG_BLK_SED_OPAL */ #endif /* LINUX_OPAL_H */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically @ 2017-02-17 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) Insted of bloating the containing structure with it all the time this allocates struct opal_dev dynamically. Additionally this allows moving the definition of struct opal_dev into sed-opal.c. For this a new private data field is added to it that is passed to the send/receive callback. After that a lot of internals can be made private as well. Signed-off-by: Christoph Hellwig <hch at lst.de> Tested-by: Scott Bauer <scott.bauer at intel.com> Reviewed-by: Scott Bauer <scott.bauer at intel.com> --- block/opal_proto.h | 23 ++++++++++ block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++---- drivers/nvme/host/core.c | 9 ++-- drivers/nvme/host/nvme.h | 14 ++---- drivers/nvme/host/pci.c | 8 +++- include/linux/sed-opal.h | 116 ++--------------------------------------------- 6 files changed, 131 insertions(+), 140 deletions(-) diff --git a/block/opal_proto.h b/block/opal_proto.h index af9abc56c157..f40c9acf8895 100644 --- a/block/opal_proto.h +++ b/block/opal_proto.h @@ -19,6 +19,29 @@ #ifndef _OPAL_PROTO_H #define _OPAL_PROTO_H +/* + * These constant values come from: + * SPC-4 section + * 6.30 SECURITY PROTOCOL IN command / table 265. + */ +enum { + TCG_SECP_00 = 0, + TCG_SECP_01, +}; + +/* + * Token defs derived from: + * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00 + * 3.2.2 Data Stream Encoding + */ +enum opal_response_token { + OPAL_DTA_TOKENID_BYTESTRING = 0xe0, + OPAL_DTA_TOKENID_SINT = 0xe1, + OPAL_DTA_TOKENID_UINT = 0xe2, + OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */ + OPAL_DTA_TOKENID_INVALID = 0X0 +}; + #define DTAERROR_NO_METHOD_STATUS 0x89 #define GENERIC_HOST_SESSION_NUM 0x41 diff --git a/block/sed-opal.c b/block/sed-opal.c index bcdd5b6d02e8..d1c52ba4d62d 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -31,6 +31,77 @@ #include "opal_proto.h" +#define IO_BUFFER_LENGTH 2048 +#define MAX_TOKS 64 + +typedef int (*opal_step)(struct opal_dev *dev); + +enum opal_atom_width { + OPAL_WIDTH_TINY, + OPAL_WIDTH_SHORT, + OPAL_WIDTH_MEDIUM, + OPAL_WIDTH_LONG, + OPAL_WIDTH_TOKEN +}; + +/* + * On the parsed response, we don't store again the toks that are already + * stored in the response buffer. Instead, for each token, we just store a + * pointer to the position in the buffer where the token starts, and the size + * of the token in bytes. + */ +struct opal_resp_tok { + const u8 *pos; + size_t len; + enum opal_response_token type; + enum opal_atom_width width; + union { + u64 u; + s64 s; + } stored; +}; + +/* + * From the response header it's not possible to know how many tokens there are + * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later + * if we start dealing with messages that have more than that, we can increase + * this number. This is done to avoid having to make two passes through the + * response, the first one counting how many tokens we have and the second one + * actually storing the positions. + */ +struct parsed_resp { + int num; + struct opal_resp_tok toks[MAX_TOKS]; +}; + +struct opal_dev { + bool supported; + + void *data; + sec_send_recv *send_recv; + + const opal_step *funcs; + void **func_data; + int state; + struct mutex dev_lock; + u16 comid; + u32 hsn; + u32 tsn; + u64 align; + u64 lowest_lba; + + size_t pos; + u8 cmd[IO_BUFFER_LENGTH]; + u8 resp[IO_BUFFER_LENGTH]; + + struct parsed_resp parsed; + size_t prev_d_len; + void *prev_data; + + struct list_head unlk_lst; +}; + + static const u8 opaluid[][OPAL_UID_LENGTH] = { /* users */ [OPAL_SMUID_UID] = @@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data) static int opal_send_cmd(struct opal_dev *dev) { - return dev->send_recv(dev, dev->comid, TCG_SECP_01, + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01, dev->cmd, IO_BUFFER_LENGTH, true); } static int opal_recv_cmd(struct opal_dev *dev) { - return dev->send_recv(dev, dev->comid, TCG_SECP_01, + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01, dev->resp, IO_BUFFER_LENGTH, false); } @@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev) return ret; } -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv) +struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv) { - if (opal_dev->initialized) - return; - INIT_LIST_HEAD(&opal_dev->unlk_lst); - mutex_init(&opal_dev->dev_lock); - opal_dev->send_recv = send_recv; - if (check_opal_support(opal_dev) < 0) + struct opal_dev *dev; + + dev = kmalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return NULL; + + INIT_LIST_HEAD(&dev->unlk_lst); + mutex_init(&dev->dev_lock); + dev->data = data; + dev->send_recv = send_recv; + if (check_opal_support(dev) != 0) { pr_debug("Opal is not supported on this device\n"); - opal_dev->initialized = true; + kfree(dev); + return NULL; + } + return dev; } EXPORT_SYMBOL(init_opal_dev); @@ -2351,6 +2430,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) if (!capable(CAP_SYS_ADMIN)) return -EACCES; + if (!dev) + return -ENOTSUPP; if (!dev->supported) { pr_err("Not supported\n"); return -ENOTSUPP; diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 04c48e75ff16..8aeb4a623b65 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode, return nvme_nvm_ioctl(ns, cmd, arg); #endif if (is_sed_ioctl(cmd)) - return sed_ioctl(&ns->ctrl->opal_dev, cmd, + return sed_ioctl(ns->ctrl->opal_dev, cmd, (void __user *) arg); return -ENOTTY; } @@ -1085,18 +1085,17 @@ static const struct pr_ops nvme_pr_ops = { }; #ifdef CONFIG_BLK_SED_OPAL -int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp, - void *buffer, size_t len, bool send) +int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len, + bool send) { + struct nvme_ctrl *ctrl = data; struct nvme_command cmd; - struct nvme_ctrl *ctrl = NULL; memset(&cmd, 0, sizeof(cmd)); if (send) cmd.common.opcode = nvme_admin_security_send; else cmd.common.opcode = nvme_admin_security_recv; - ctrl = container_of(dev, struct nvme_ctrl, opal_dev); cmd.common.nsid = 0; cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8); cmd.common.cdw10[1] = cpu_to_le32(len); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 569cba14cede..5126c4bbee1a 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -126,7 +126,7 @@ struct nvme_ctrl { struct list_head node; struct ida ns_ida; - struct opal_dev opal_dev; + struct opal_dev *opal_dev; char name[12]; char serial[20]; @@ -270,16 +270,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl); void nvme_queue_scan(struct nvme_ctrl *ctrl); void nvme_remove_namespaces(struct nvme_ctrl *ctrl); -#ifdef CONFIG_BLK_SED_OPAL -int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp, - void *buffer, size_t len, bool send); -#else -static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp, - void *buffer, size_t len, bool send) -{ - return 0; -} -#endif /* CONFIG_BLK_DEV_SED_OPAL */ +int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len, + bool send); #define NVME_NR_AERS 1 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e25d632bd9f2..5db8a38a8b43 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl) if (dev->ctrl.admin_q) blk_put_queue(dev->ctrl.admin_q); kfree(dev->queues); + kfree(dev->ctrl.opal_dev); kfree(dev); } @@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit); + if (!dev->ctrl.opal_dev) { + dev->ctrl.opal_dev = + init_opal_dev(&dev->ctrl, &nvme_sec_submit); + } if (was_suspend) - opal_unlock_from_suspend(&dev->ctrl.opal_dev); + opal_unlock_from_suspend(dev->ctrl.opal_dev); result = nvme_setup_io_queues(dev); if (result) diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h index 205d520ea688..deee23d012e7 100644 --- a/include/linux/sed-opal.h +++ b/include/linux/sed-opal.h @@ -21,117 +21,14 @@ #include <uapi/linux/sed-opal.h> #include <linux/kernel.h> -/* - * These constant values come from: - * SPC-4 section - * 6.30 SECURITY PROTOCOL IN command / table 265. - */ -enum { - TCG_SECP_00 = 0, - TCG_SECP_01, -}; struct opal_dev; -#define IO_BUFFER_LENGTH 2048 -#define MAX_TOKS 64 - -typedef int (*opal_step)(struct opal_dev *dev); -typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp, - void *buffer, size_t len, bool send); - - -enum opal_atom_width { - OPAL_WIDTH_TINY, - OPAL_WIDTH_SHORT, - OPAL_WIDTH_MEDIUM, - OPAL_WIDTH_LONG, - OPAL_WIDTH_TOKEN -}; - -/* - * Token defs derived from: - * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00 - * 3.2.2 Data Stream Encoding - */ -enum opal_response_token { - OPAL_DTA_TOKENID_BYTESTRING = 0xe0, - OPAL_DTA_TOKENID_SINT = 0xe1, - OPAL_DTA_TOKENID_UINT = 0xe2, - OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */ - OPAL_DTA_TOKENID_INVALID = 0X0 -}; - -/* - * On the parsed response, we don't store again the toks that are already - * stored in the response buffer. Instead, for each token, we just store a - * pointer to the position in the buffer where the token starts, and the size - * of the token in bytes. - */ -struct opal_resp_tok { - const u8 *pos; - size_t len; - enum opal_response_token type; - enum opal_atom_width width; - union { - u64 u; - s64 s; - } stored; -}; - -/* - * From the response header it's not possible to know how many tokens there are - * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later - * if we start dealing with messages that have more than that, we can increase - * this number. This is done to avoid having to make two passes through the - * response, the first one counting how many tokens we have and the second one - * actually storing the positions. - */ -struct parsed_resp { - int num; - struct opal_resp_tok toks[MAX_TOKS]; -}; - -/** - * struct opal_dev - The structure representing a OPAL enabled SED. - * @supported: Whether or not OPAL is supported on this controller. - * @send_recv: The combined sec_send/sec_recv function pointer. - * @opal_step: A series of opal methods that are necessary to complete a command. - * @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. - * @unlk_lst: A list of Locking ranges to unlock on this device during a resume. - */ -struct opal_dev { - bool initialized; - bool supported; - sec_send_recv *send_recv; - - const opal_step *funcs; - void **func_data; - int state; - struct mutex dev_lock; - u16 comid; - u32 hsn; - u32 tsn; - u64 align; - u64 lowest_lba; - - size_t pos; - u8 cmd[IO_BUFFER_LENGTH]; - u8 resp[IO_BUFFER_LENGTH]; - - struct parsed_resp parsed; - size_t prev_d_len; - void *prev_data; - - struct list_head unlk_lst; -}; +typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer, + size_t len, bool send); #ifdef CONFIG_BLK_SED_OPAL bool opal_unlock_from_suspend(struct opal_dev *dev); -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv); +struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv); int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr); static inline bool is_sed_ioctl(unsigned int cmd) @@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev) { return false; } -static inline void init_opal_dev(struct opal_dev *opal_dev, - sec_send_recv *send_recv) -{ - opal_dev->supported = false; - opal_dev->initialized = true; -} +#define init_opal_dev(data, send_recv) NULL #endif /* CONFIG_BLK_SED_OPAL */ #endif /* LINUX_OPAL_H */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands. 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 12:59 ` Christoph Hellwig -1 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme From: Scott Bauer <scott.bauer@intel.com> We need to verify that the controller supports the security commands before actually trying to issue them. Signed-off-by: Scott Bauer <scott.bauer@intel.com> [hch: moved the check so that we don't call into the OPAL code if not supported] Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/core.c | 1 + drivers/nvme/host/nvme.h | 1 + drivers/nvme/host/pci.c | 2 +- include/linux/nvme.h | 1 + 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 8aeb4a623b65..44a1a257e0b5 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1285,6 +1285,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) return -EIO; } + ctrl->oacs = le16_to_cpu(id->oacs); ctrl->vid = le16_to_cpu(id->vid); ctrl->oncs = le16_to_cpup(&id->oncs); atomic_set(&ctrl->abort_limit, id->acl + 1); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 5126c4bbee1a..14cfc6f7facb 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -140,6 +140,7 @@ struct nvme_ctrl { u32 max_hw_sectors; u16 oncs; u16 vid; + u16 oacs; atomic_t abort_limit; u8 event_limit; u8 vwc; diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 5db8a38a8b43..ddc51adb594d 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,7 +1789,7 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if (!dev->ctrl.opal_dev) { + if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { dev->ctrl.opal_dev = init_opal_dev(&dev->ctrl, &nvme_sec_submit); } diff --git a/include/linux/nvme.h b/include/linux/nvme.h index 3e2ed49c3ad8..0b676a02cf3e 100644 --- a/include/linux/nvme.h +++ b/include/linux/nvme.h @@ -244,6 +244,7 @@ enum { NVME_CTRL_ONCS_DSM = 1 << 2, NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3, NVME_CTRL_VWC_PRESENT = 1 << 0, + NVME_CTRL_OACS_SEC_SUPP = 1 << 0, }; struct nvme_lbaf { -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands. @ 2017-02-17 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) From: Scott Bauer <scott.bauer@intel.com> We need to verify that the controller supports the security commands before actually trying to issue them. Signed-off-by: Scott Bauer <scott.bauer at intel.com> [hch: moved the check so that we don't call into the OPAL code if not supported] Signed-off-by: Christoph Hellwig <hch at lst.de> --- drivers/nvme/host/core.c | 1 + drivers/nvme/host/nvme.h | 1 + drivers/nvme/host/pci.c | 2 +- include/linux/nvme.h | 1 + 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 8aeb4a623b65..44a1a257e0b5 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1285,6 +1285,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) return -EIO; } + ctrl->oacs = le16_to_cpu(id->oacs); ctrl->vid = le16_to_cpu(id->vid); ctrl->oncs = le16_to_cpup(&id->oncs); atomic_set(&ctrl->abort_limit, id->acl + 1); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 5126c4bbee1a..14cfc6f7facb 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -140,6 +140,7 @@ struct nvme_ctrl { u32 max_hw_sectors; u16 oncs; u16 vid; + u16 oacs; atomic_t abort_limit; u8 event_limit; u8 vwc; diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 5db8a38a8b43..ddc51adb594d 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,7 +1789,7 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if (!dev->ctrl.opal_dev) { + if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { dev->ctrl.opal_dev = init_opal_dev(&dev->ctrl, &nvme_sec_submit); } diff --git a/include/linux/nvme.h b/include/linux/nvme.h index 3e2ed49c3ad8..0b676a02cf3e 100644 --- a/include/linux/nvme.h +++ b/include/linux/nvme.h @@ -244,6 +244,7 @@ enum { NVME_CTRL_ONCS_DSM = 1 << 2, NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3, NVME_CTRL_VWC_PRESENT = 1 << 0, + NVME_CTRL_OACS_SEC_SUPP = 1 << 0, }; struct nvme_lbaf { -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 12:59 ` Christoph Hellwig -1 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme A device may change capabilities after each reset, e.g. due to a firmware upgrade. We should thus check for Security Send/Receive and OPAL support after each reset. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index ddc51adb594d..c5986850f88b 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { + kfree(dev->ctrl.opal_dev); + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { dev->ctrl.opal_dev = init_opal_dev(&dev->ctrl, &nvme_sec_submit); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset @ 2017-02-17 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw) A device may change capabilities after each reset, e.g. due to a firmware upgrade. We should thus check for Security Send/Receive and OPAL support after each reset. Signed-off-by: Christoph Hellwig <hch at lst.de> --- drivers/nvme/host/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index ddc51adb594d..c5986850f88b 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { + kfree(dev->ctrl.opal_dev); + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { dev->ctrl.opal_dev = init_opal_dev(&dev->ctrl, &nvme_sec_submit); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 15:26 ` Keith Busch -1 siblings, 0 replies; 24+ messages in thread From: Keith Busch @ 2017-02-17 15:26 UTC (permalink / raw) To: Christoph Hellwig Cc: scott.bauer, jonathan.derrick, axboe, linux-block, linux-nvme On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > if (result) > goto out; > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > + kfree(dev->ctrl.opal_dev); > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > dev->ctrl.opal_dev = > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > } A couple things. This has a use-after-free in opal_unlock_from_suspend if the nvme device had an opal_dev before, but no longer support the capability after resume. So you'd want to set ctrl.opal_dev to NULL after the free. But we don't want to unconditionally free it anyway during resume since opal_unlock_from_suspend requires the exisiting opal_dev state information saved in the 'unlk_list'. Something like this instead: --- diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index ddc51ad..8fa6be9 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { - dev->ctrl.opal_dev = - init_opal_dev(&dev->ctrl, &nvme_sec_submit); + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) + if (was_suspend && dev->ctrl.opal_dev) + opal_unlock_from_suspend(dev->ctrl.opal_dev); + else if (!dev->ctrl.opal_dev) + dev->ctrl.opal_dev = + init_opal_dev(&dev->ctrl, &nvme_sec_submit); + } else { + kfree(dev->ctrl.opal_dev); + dev->ctrl.opal_dev = NULL; } - if (was_suspend) - opal_unlock_from_suspend(dev->ctrl.opal_dev); result = nvme_setup_io_queues(dev); if (result) -- ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset @ 2017-02-17 15:26 ` Keith Busch 0 siblings, 0 replies; 24+ messages in thread From: Keith Busch @ 2017-02-17 15:26 UTC (permalink / raw) On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote: > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > if (result) > goto out; > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > + kfree(dev->ctrl.opal_dev); > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > dev->ctrl.opal_dev = > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > } A couple things. This has a use-after-free in opal_unlock_from_suspend if the nvme device had an opal_dev before, but no longer support the capability after resume. So you'd want to set ctrl.opal_dev to NULL after the free. But we don't want to unconditionally free it anyway during resume since opal_unlock_from_suspend requires the exisiting opal_dev state information saved in the 'unlk_list'. Something like this instead: --- diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index ddc51ad..8fa6be9 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { - dev->ctrl.opal_dev = - init_opal_dev(&dev->ctrl, &nvme_sec_submit); + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) + if (was_suspend && dev->ctrl.opal_dev) + opal_unlock_from_suspend(dev->ctrl.opal_dev); + else if (!dev->ctrl.opal_dev) + dev->ctrl.opal_dev = + init_opal_dev(&dev->ctrl, &nvme_sec_submit); + } else { + kfree(dev->ctrl.opal_dev); + dev->ctrl.opal_dev = NULL; } - if (was_suspend) - opal_unlock_from_suspend(dev->ctrl.opal_dev); result = nvme_setup_io_queues(dev); if (result) -- ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset 2017-02-17 15:26 ` Keith Busch @ 2017-02-17 15:41 ` Scott Bauer -1 siblings, 0 replies; 24+ messages in thread From: Scott Bauer @ 2017-02-17 15:41 UTC (permalink / raw) To: Keith Busch Cc: Christoph Hellwig, jonathan.derrick, axboe, linux-block, linux-nvme On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote: > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > + kfree(dev->ctrl.opal_dev); > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > dev->ctrl.opal_dev = > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > } > > A couple things. > > This has a use-after-free in opal_unlock_from_suspend if the nvme > device had an opal_dev before, but no longer support the capability > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > But we don't want to unconditionally free it anyway during resume > since opal_unlock_from_suspend requires the exisiting opal_dev state > information saved in the 'unlk_list'. > > Something like this instead: > > --- > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index ddc51ad..8fa6be9 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work) > if (result) > goto out; > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > - dev->ctrl.opal_dev = > - init_opal_dev(&dev->ctrl, &nvme_sec_submit); > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) > + if (was_suspend && dev->ctrl.opal_dev) > + opal_unlock_from_suspend(dev->ctrl.opal_dev); > + else if (!dev->ctrl.opal_dev) > + dev->ctrl.opal_dev = > + init_opal_dev(&dev->ctrl, &nvme_sec_submit); > + } else { > + kfree(dev->ctrl.opal_dev); > + dev->ctrl.opal_dev = NULL; Keith's comments made me realize something even deeper as well. Assuming the firmware changed and we no longer support security commands we need to free the opal_dev structure like we're doing but there is a possiblity that there were saved ranges in the structure that we need to free as well. If the user had previously told the kernel to unlock 5 ranges coming out of a suspend there are 5 structures we need to free inside the opal_dev before we free the opal dev. We'll need to re-introduce free_opal_dev() in the opal code like we had a while back. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset @ 2017-02-17 15:41 ` Scott Bauer 0 siblings, 0 replies; 24+ messages in thread From: Scott Bauer @ 2017-02-17 15:41 UTC (permalink / raw) On Fri, Feb 17, 2017@10:26:51AM -0500, Keith Busch wrote: > On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote: > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > + kfree(dev->ctrl.opal_dev); > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > dev->ctrl.opal_dev = > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > } > > A couple things. > > This has a use-after-free in opal_unlock_from_suspend if the nvme > device had an opal_dev before, but no longer support the capability > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > But we don't want to unconditionally free it anyway during resume > since opal_unlock_from_suspend requires the exisiting opal_dev state > information saved in the 'unlk_list'. > > Something like this instead: > > --- > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index ddc51ad..8fa6be9 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work) > if (result) > goto out; > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > - dev->ctrl.opal_dev = > - init_opal_dev(&dev->ctrl, &nvme_sec_submit); > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) > + if (was_suspend && dev->ctrl.opal_dev) > + opal_unlock_from_suspend(dev->ctrl.opal_dev); > + else if (!dev->ctrl.opal_dev) > + dev->ctrl.opal_dev = > + init_opal_dev(&dev->ctrl, &nvme_sec_submit); > + } else { > + kfree(dev->ctrl.opal_dev); > + dev->ctrl.opal_dev = NULL; Keith's comments made me realize something even deeper as well. Assuming the firmware changed and we no longer support security commands we need to free the opal_dev structure like we're doing but there is a possiblity that there were saved ranges in the structure that we need to free as well. If the user had previously told the kernel to unlock 5 ranges coming out of a suspend there are 5 structures we need to free inside the opal_dev before we free the opal dev. We'll need to re-introduce free_opal_dev() in the opal code like we had a while back. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset 2017-02-17 15:26 ` Keith Busch @ 2017-02-17 17:01 ` Christoph Hellwig -1 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 17:01 UTC (permalink / raw) To: Keith Busch Cc: Christoph Hellwig, scott.bauer, jonathan.derrick, axboe, linux-block, linux-nvme On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote: > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > + kfree(dev->ctrl.opal_dev); > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > dev->ctrl.opal_dev = > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > } > > A couple things. > > This has a use-after-free in opal_unlock_from_suspend if the nvme > device had an opal_dev before, but no longer support the capability > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > But we don't want to unconditionally free it anyway during resume > since opal_unlock_from_suspend requires the exisiting opal_dev state > information saved in the 'unlk_list'. > > Something like this instead: Yes, that looks fine to me. We'll probably also need the additional fixup Scott pointed out. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset @ 2017-02-17 17:01 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 17:01 UTC (permalink / raw) On Fri, Feb 17, 2017@10:26:51AM -0500, Keith Busch wrote: > On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote: > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > + kfree(dev->ctrl.opal_dev); > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > dev->ctrl.opal_dev = > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > } > > A couple things. > > This has a use-after-free in opal_unlock_from_suspend if the nvme > device had an opal_dev before, but no longer support the capability > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > But we don't want to unconditionally free it anyway during resume > since opal_unlock_from_suspend requires the exisiting opal_dev state > information saved in the 'unlk_list'. > > Something like this instead: Yes, that looks fine to me. We'll probably also need the additional fixup Scott pointed out. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset 2017-02-17 17:01 ` Christoph Hellwig @ 2017-02-17 16:55 ` Scott Bauer -1 siblings, 0 replies; 24+ messages in thread From: Scott Bauer @ 2017-02-17 16:55 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, jonathan.derrick, axboe, linux-block, linux-nvme On Fri, Feb 17, 2017 at 06:01:28PM +0100, Christoph Hellwig wrote: > On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote: > > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > > if (result) > > > goto out; > > > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > > + kfree(dev->ctrl.opal_dev); > > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > > dev->ctrl.opal_dev = > > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > > } > > > > A couple things. > > > > This has a use-after-free in opal_unlock_from_suspend if the nvme > > device had an opal_dev before, but no longer support the capability > > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > > > But we don't want to unconditionally free it anyway during resume > > since opal_unlock_from_suspend requires the exisiting opal_dev state > > information saved in the 'unlk_list'. > > > > Something like this instead: > > Yes, that looks fine to me. We'll probably also need the additional > fixup Scott pointed out. I'm working on it now. Do you want a diff like Keith did or a separate patch? ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset @ 2017-02-17 16:55 ` Scott Bauer 0 siblings, 0 replies; 24+ messages in thread From: Scott Bauer @ 2017-02-17 16:55 UTC (permalink / raw) On Fri, Feb 17, 2017@06:01:28PM +0100, Christoph Hellwig wrote: > On Fri, Feb 17, 2017@10:26:51AM -0500, Keith Busch wrote: > > On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote: > > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > > if (result) > > > goto out; > > > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > > + kfree(dev->ctrl.opal_dev); > > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > > dev->ctrl.opal_dev = > > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > > } > > > > A couple things. > > > > This has a use-after-free in opal_unlock_from_suspend if the nvme > > device had an opal_dev before, but no longer support the capability > > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > > > But we don't want to unconditionally free it anyway during resume > > since opal_unlock_from_suspend requires the exisiting opal_dev state > > information saved in the 'unlk_list'. > > > > Something like this instead: > > Yes, that looks fine to me. We'll probably also need the additional > fixup Scott pointed out. I'm working on it now. Do you want a diff like Keith did or a separate patch? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset 2017-02-17 16:55 ` Scott Bauer @ 2017-02-17 17:16 ` Christoph Hellwig -1 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 17:16 UTC (permalink / raw) To: Scott Bauer Cc: Christoph Hellwig, Keith Busch, jonathan.derrick, axboe, linux-block, linux-nvme On Fri, Feb 17, 2017 at 09:55:51AM -0700, Scott Bauer wrote: > I'm working on it now. Do you want a diff like Keith did or a separate patch? Please make it a proper patch that applies on top of my patches 1-3 (where 3 really is yours anyway). I'll respin 4 with the updates from Keith on top of 1-3 + your patch then. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset @ 2017-02-17 17:16 ` Christoph Hellwig 0 siblings, 0 replies; 24+ messages in thread From: Christoph Hellwig @ 2017-02-17 17:16 UTC (permalink / raw) On Fri, Feb 17, 2017@09:55:51AM -0700, Scott Bauer wrote: > I'm working on it now. Do you want a diff like Keith did or a separate patch? Please make it a proper patch that applies on top of my patches 1-3 (where 3 really is yours anyway). I'll respin 4 with the updates from Keith on top of 1-3 + your patch then. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: OPAL fixups 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 15:27 ` Keith Busch -1 siblings, 0 replies; 24+ messages in thread From: Keith Busch @ 2017-02-17 15:27 UTC (permalink / raw) To: Christoph Hellwig Cc: scott.bauer, jonathan.derrick, axboe, linux-block, linux-nvme On Fri, Feb 17, 2017 at 01:59:37PM +0100, Christoph Hellwig wrote: > Hi all, > > this contains a few more OPAL-related fixups. It tones down warnings a bit, > allocates the OPAL-ѕpecific data structure in a separate dynamic allocation, > checks for support of Security Send/Receive in NVMe before using them, > and makes sure we re-discovery the security capabilities after each reset. Patches 1-3 look good to me. ^ permalink raw reply [flat|nested] 24+ messages in thread
* OPAL fixups @ 2017-02-17 15:27 ` Keith Busch 0 siblings, 0 replies; 24+ messages in thread From: Keith Busch @ 2017-02-17 15:27 UTC (permalink / raw) On Fri, Feb 17, 2017@01:59:37PM +0100, Christoph Hellwig wrote: > Hi all, > > this contains a few more OPAL-related fixups. It tones down warnings a bit, > allocates the OPAL-?pecific data structure in a separate dynamic allocation, > checks for support of Security Send/Receive in NVMe before using them, > and makes sure we re-discovery the security capabilities after each reset. Patches 1-3 look good to me. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: OPAL fixups 2017-02-17 12:59 ` Christoph Hellwig @ 2017-02-17 19:42 ` Jens Axboe -1 siblings, 0 replies; 24+ messages in thread From: Jens Axboe @ 2017-02-17 19:42 UTC (permalink / raw) To: Christoph Hellwig, scott.bauer, keith.busch, jonathan.derrick Cc: linux-block, linux-nvme On 02/17/2017 05:59 AM, Christoph Hellwig wrote: > Hi all, > > this contains a few more OPAL-related fixups. It tones down warnings a bit, > allocates the OPAL-ѕpecific data structure in a separate dynamic allocation, > checks for support of Security Send/Receive in NVMe before using them, > and makes sure we re-discovery the security capabilities after each reset. Applied 1-3, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 24+ messages in thread
* OPAL fixups @ 2017-02-17 19:42 ` Jens Axboe 0 siblings, 0 replies; 24+ messages in thread From: Jens Axboe @ 2017-02-17 19:42 UTC (permalink / raw) On 02/17/2017 05:59 AM, Christoph Hellwig wrote: > Hi all, > > this contains a few more OPAL-related fixups. It tones down warnings a bit, > allocates the OPAL-?pecific data structure in a separate dynamic allocation, > checks for support of Security Send/Receive in NVMe before using them, > and makes sure we re-discovery the security capabilities after each reset. Applied 1-3, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-02-17 19:42 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-17 12:59 OPAL fixups Christoph Hellwig 2017-02-17 12:59 ` Christoph Hellwig 2017-02-17 12:59 ` [PATCH 1/4] block/sed-opal: tone down not supported warnings Christoph Hellwig 2017-02-17 12:59 ` Christoph Hellwig 2017-02-17 12:59 ` [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically Christoph Hellwig 2017-02-17 12:59 ` Christoph Hellwig 2017-02-17 12:59 ` [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands Christoph Hellwig 2017-02-17 12:59 ` Christoph Hellwig 2017-02-17 12:59 ` [PATCH 4/4] nvme: re-check security protocol support after reset Christoph Hellwig 2017-02-17 12:59 ` Christoph Hellwig 2017-02-17 15:26 ` Keith Busch 2017-02-17 15:26 ` Keith Busch 2017-02-17 15:41 ` Scott Bauer 2017-02-17 15:41 ` Scott Bauer 2017-02-17 17:01 ` Christoph Hellwig 2017-02-17 17:01 ` Christoph Hellwig 2017-02-17 16:55 ` Scott Bauer 2017-02-17 16:55 ` Scott Bauer 2017-02-17 17:16 ` Christoph Hellwig 2017-02-17 17:16 ` Christoph Hellwig 2017-02-17 15:27 ` OPAL fixups Keith Busch 2017-02-17 15:27 ` Keith Busch 2017-02-17 19:42 ` Jens Axboe 2017-02-17 19:42 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.