[PATCH 0/3] cxl/memdev: Make sanitize interfaces conditionally available

[PATCH 0/3] cxl/memdev: Make sanitize interfaces conditionally available

[Davidlohr Bueso]

Hi,

This is a requested[0] follow up (patch 3). Patches 1,2 are additional doc updates.

Thanks!

[0]: https://lore.kernel.org/linux-cxl/6498c0ec8cc41_8e1782946e@dwillia2-xfh.jf.intel.com.notmuch/

Davidlohr Bueso (3):
  cxl/memdev: Improve sanitize ABI descriptions
  cxl/memdev: Document security state in kern-doc
  cxl/memdev: Only show sanitize sysfs files when supported

 Documentation/ABI/testing/sysfs-bus-cxl | 15 +++++++--
 drivers/cxl/core/mbox.c                 | 45 ++++++++++++++++++++++++-
 drivers/cxl/core/memdev.c               | 19 +++++++++++
 drivers/cxl/cxlmem.h                    | 16 +++++++++
 4 files changed, 92 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH 1/3] cxl/memdev: Improve sanitize ABI descriptions

[Davidlohr Bueso]

Be more detailed about the CPU cache management situation. The same
goes for both sanitize and secure erase.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 6350dd82b9a9..c4c4acb1f3b3 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -82,7 +82,11 @@ Description:
 		whether it resides in persistent capacity, volatile capacity,
 		or the LSA, is made permanently unavailable by whatever means
 		is appropriate for the media type. This functionality requires
-		the device to be not be actively decoding any HPA ranges.
+		the device to be disabled, that is, not actively decoding any
+		HPA ranges. This permits avoiding explicit global CPU cache
+		management, relying instead for it to be done when a region
+		transitions between software programmed and hardware committed
+		states.
 
 
 What            /sys/bus/cxl/devices/memX/security/erase
@@ -92,7 +96,12 @@ Contact:        linux-cxl@vger.kernel.org
 Description:
 		(WO) Write a boolean 'true' string value to this attribute to
 		secure erase user data by changing the media encryption keys for
-		all user data areas of the device.
+		all user data areas of the device. This functionality requires
+		the device to be disabled, that is, not actively decoding any
+		HPA ranges. This permits avoiding explicit global CPU cache
+		management, relying instead for it to be done when a region
+		transitions between software programmed and hardware committed
+		states.
 
 
 What:		/sys/bus/cxl/devices/memX/firmware/
-- 
2.41.0

Re: [PATCH 1/3] cxl/memdev: Improve sanitize ABI descriptions

[Dave Jiang]

On 7/25/23 22:19, Davidlohr Bueso wrote:
> Be more detailed about the CPU cache management situation. The same
> goes for both sanitize and secure erase.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 6350dd82b9a9..c4c4acb1f3b3 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -82,7 +82,11 @@ Description:
>   		whether it resides in persistent capacity, volatile capacity,
>   		or the LSA, is made permanently unavailable by whatever means
>   		is appropriate for the media type. This functionality requires
> -		the device to be not be actively decoding any HPA ranges.
> +		the device to be disabled, that is, not actively decoding any
> +		HPA ranges. This permits avoiding explicit global CPU cache
> +		management, relying instead for it to be done when a region
> +		transitions between software programmed and hardware committed
> +		states.
>   
>   
>   What            /sys/bus/cxl/devices/memX/security/erase
> @@ -92,7 +96,12 @@ Contact:        linux-cxl@vger.kernel.org
>   Description:
>   		(WO) Write a boolean 'true' string value to this attribute to
>   		secure erase user data by changing the media encryption keys for
> -		all user data areas of the device.
> +		all user data areas of the device. This functionality requires
> +		the device to be disabled, that is, not actively decoding any
> +		HPA ranges. This permits avoiding explicit global CPU cache
> +		management, relying instead for it to be done when a region
> +		transitions between software programmed and hardware committed
> +		states.
>   
>   
>   What:		/sys/bus/cxl/devices/memX/firmware/

Re: [PATCH 1/3] cxl/memdev: Improve sanitize ABI descriptions

[Jonathan Cameron]

On Tue, 25 Jul 2023 22:19:38 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Be more detailed about the CPU cache management situation. The same
> goes for both sanitize and secure erase.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 6350dd82b9a9..c4c4acb1f3b3 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -82,7 +82,11 @@ Description:
>  		whether it resides in persistent capacity, volatile capacity,
>  		or the LSA, is made permanently unavailable by whatever means
>  		is appropriate for the media type. This functionality requires
> -		the device to be not be actively decoding any HPA ranges.
> +		the device to be disabled, that is, not actively decoding any
> +		HPA ranges. This permits avoiding explicit global CPU cache
> +		management, relying instead for it to be done when a region
> +		transitions between software programmed and hardware committed
> +		states.

That worries me a bit.  Sounds like we are leaving a possible attack vector
after a user will assume that all data is definitely gone and their secrets secure.

I'm not sure what the attack would be, but I'd be happier if we didn't forgo
the cache evictions.  This is not exactly a fast path at any time.
However I though region tear down (and resulting HDM decoder disables) were followed
by a flush anyway so there should be nothing there...

>  
>  
>  What            /sys/bus/cxl/devices/memX/security/erase
> @@ -92,7 +96,12 @@ Contact:        linux-cxl@vger.kernel.org
>  Description:
>  		(WO) Write a boolean 'true' string value to this attribute to
>  		secure erase user data by changing the media encryption keys for
> -		all user data areas of the device.
> +		all user data areas of the device. This functionality requires
> +		the device to be disabled, that is, not actively decoding any
> +		HPA ranges. This permits avoiding explicit global CPU cache
> +		management, relying instead for it to be done when a region
> +		transitions between software programmed and hardware committed
> +		states.
>  
>  
>  What:		/sys/bus/cxl/devices/memX/firmware/

Re: [PATCH 1/3] cxl/memdev: Improve sanitize ABI descriptions

[Davidlohr Bueso]

On Fri, 04 Aug 2023, Jonathan Cameron wrote:

>On Tue, 25 Jul 2023 22:19:38 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Be more detailed about the CPU cache management situation. The same
>> goes for both sanitize and secure erase.
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
>> index 6350dd82b9a9..c4c4acb1f3b3 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-cxl
>> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
>> @@ -82,7 +82,11 @@ Description:
>>		whether it resides in persistent capacity, volatile capacity,
>>		or the LSA, is made permanently unavailable by whatever means
>>		is appropriate for the media type. This functionality requires
>> -		the device to be not be actively decoding any HPA ranges.
>> +		the device to be disabled, that is, not actively decoding any
>> +		HPA ranges. This permits avoiding explicit global CPU cache
>> +		management, relying instead for it to be done when a region
>> +		transitions between software programmed and hardware committed
>> +		states.
>
>That worries me a bit.  Sounds like we are leaving a possible attack vector
>after a user will assume that all data is definitely gone and their secrets secure.
>
>I'm not sure what the attack would be, but I'd be happier if we didn't forgo
>the cache evictions.  This is not exactly a fast path at any time.
>However I though region tear down (and resulting HDM decoder disables) were followed
>by a flush anyway so there should be nothing there...

I had similar concerns, but ultimately could not come up with a potential
attack scenario by not doing the flushing explicitly, which is before+after the
operation, per the spec.

https://lore.kernel.org/linux-cxl/6422888b688fd_21a8294a3@dwillia2-xfh.jf.intel.com.notmuch/

The main motivation for avoiding these calls is not really that this is a fast
path, but it's that the global CPU flushing affects system-wide. That said, it's hard
to argue on defaulting on the safe side and just doing the flushes - exactly because
of reasons we don't know until too late. Dan?

Thanks,
Davidlohr

[PATCH 2/3] cxl/memdev: Document security state in kern-doc

[Davidlohr Bueso]

... as is the case with all members of struct cxl_memdev_state.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/cxl/cxlmem.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 79e99c873ca2..083c6e58bc49 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -434,6 +434,7 @@ struct cxl_dev_state {
  * @next_persistent_bytes: persistent capacity change pending device reset
  * @event: event log driver state
  * @poison: poison driver state info
+ * @security: security driver state info
  * @fw: firmware upload / activation state
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  *
-- 
2.41.0

Re: [PATCH 2/3] cxl/memdev: Document security state in kern-doc

[Dave Jiang]

On 7/25/23 22:19, Davidlohr Bueso wrote:
> ... as is the case with all members of struct cxl_memdev_state.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/cxlmem.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 79e99c873ca2..083c6e58bc49 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -434,6 +434,7 @@ struct cxl_dev_state {
>    * @next_persistent_bytes: persistent capacity change pending device reset
>    * @event: event log driver state
>    * @poison: poison driver state info
> + * @security: security driver state info
>    * @fw: firmware upload / activation state
>    * @mbox_send: @dev specific transport for transmitting mailbox commands
>    *

[PATCH 3/3] cxl/memdev: Only show sanitize sysfs files when supported

[Davidlohr Bueso]

If the device does not support Sanitize or Secure Erase commands,
hide the respective sysfs interfaces such that the operation can
never be attempted.

In order to be generic, keep track of the enabled security commands
found in the CEL - the driver does not support Security Passthrough.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 Documentation/ABI/testing/sysfs-bus-cxl |  6 ++--
 drivers/cxl/core/mbox.c                 | 45 ++++++++++++++++++++++++-
 drivers/cxl/core/memdev.c               | 19 +++++++++++
 drivers/cxl/cxlmem.h                    | 15 +++++++++
 4 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index c4c4acb1f3b3..087f762ebfd5 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -86,7 +86,8 @@ Description:
 		HPA ranges. This permits avoiding explicit global CPU cache
 		management, relying instead for it to be done when a region
 		transitions between software programmed and hardware committed
-		states.
+		states. If this file is not present, then there is no hardware
+		support for the operation.
 
 
 What            /sys/bus/cxl/devices/memX/security/erase
@@ -101,7 +102,8 @@ Description:
 		HPA ranges. This permits avoiding explicit global CPU cache
 		management, relying instead for it to be done when a region
 		transitions between software programmed and hardware committed
-		states.
+		states. If this file is not present, then there is no hardware
+		support for the operation.
 
 
 What:		/sys/bus/cxl/devices/memX/firmware/
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d6d067fbee97..ca60bb8114f2 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -121,6 +121,45 @@ static bool cxl_is_security_command(u16 opcode)
 	return false;
 }
 
+static void cxl_set_security_cmd_enabled(struct cxl_security_state *security,
+					 u16 opcode)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_SANITIZE:
+		set_bit(CXL_SEC_ENABLED_SANITIZE, security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_SECURE_ERASE:
+		set_bit(CXL_SEC_ENABLED_SECURE_ERASE,
+			security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_GET_SECURITY_STATE:
+		set_bit(CXL_SEC_ENABLED_GET_SECURITY_STATE,
+			security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_SET_PASSPHRASE:
+		set_bit(CXL_SEC_ENABLED_SET_PASSPHRASE,
+			security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_DISABLE_PASSPHRASE:
+		set_bit(CXL_SEC_ENABLED_DISABLE_PASSPHRASE,
+			security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_UNLOCK:
+		set_bit(CXL_SEC_ENABLED_UNLOCK, security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_FREEZE_SECURITY:
+		set_bit(CXL_SEC_ENABLED_FREEZE_SECURITY,
+			security->enabled_cmds);
+		break;
+	case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
+		set_bit(CXL_SEC_ENABLED_PASSPHRASE_SECURE_ERASE,
+			security->enabled_cmds);
+		break;
+	default:
+		break;
+	}
+}
+
 static bool cxl_is_poison_command(u16 opcode)
 {
 #define CXL_MBOX_OP_POISON_CMDS 0x43
@@ -677,7 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
 		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
 
-		if (!cmd && !cxl_is_poison_command(opcode)) {
+		if (!cmd && (!cxl_is_poison_command(opcode) ||
+			     !cxl_is_security_command(opcode))) {
 			dev_dbg(dev,
 				"Opcode 0x%04x unsupported by driver\n", opcode);
 			continue;
@@ -689,6 +729,9 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 		if (cxl_is_poison_command(opcode))
 			cxl_set_poison_cmd_enabled(&mds->poison, opcode);
 
+		if (cxl_is_security_command(opcode))
+			cxl_set_security_cmd_enabled(&mds->security, opcode);
+
 		dev_dbg(dev, "Opcode 0x%04x enabled\n", opcode);
 	}
 }
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index f99e7ec3cc40..14b547c07f54 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -477,9 +477,28 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
 	.attrs = cxl_memdev_pmem_attributes,
 };
 
+static umode_t cxl_memdev_security_visible(struct kobject *kobj,
+					   struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
+
+	if (a == &dev_attr_security_sanitize.attr &&
+	    !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds))
+		return 0;
+
+	if (a == &dev_attr_security_erase.attr &&
+	    !test_bit(CXL_SEC_ENABLED_SECURE_ERASE, mds->security.enabled_cmds))
+		return 0;
+
+	return a->mode;
+}
+
 static struct attribute_group cxl_memdev_security_attribute_group = {
 	.name = "security",
 	.attrs = cxl_memdev_security_attributes,
+	.is_visible = cxl_memdev_security_visible,
 };
 
 static const struct attribute_group *cxl_memdev_attribute_groups[] = {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 083c6e58bc49..f86afef90c91 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -244,6 +244,19 @@ enum poison_cmd_enabled_bits {
 	CXL_POISON_ENABLED_MAX
 };
 
+/* Device enabled security commands */
+enum security_cmd_enabled_bits {
+	CXL_SEC_ENABLED_SANITIZE,
+	CXL_SEC_ENABLED_SECURE_ERASE,
+	CXL_SEC_ENABLED_GET_SECURITY_STATE,
+	CXL_SEC_ENABLED_SET_PASSPHRASE,
+	CXL_SEC_ENABLED_DISABLE_PASSPHRASE,
+	CXL_SEC_ENABLED_UNLOCK,
+	CXL_SEC_ENABLED_FREEZE_SECURITY,
+	CXL_SEC_ENABLED_PASSPHRASE_SECURE_ERASE,
+	CXL_SEC_ENABLED_MAX
+};
+
 /**
  * struct cxl_poison_state - Driver poison state info
  *
@@ -346,6 +359,7 @@ struct cxl_fw_state {
  * struct cxl_security_state - Device security state
  *
  * @state: state of last security operation
+ * @enabled_cmds: All security commands enabled in the CEL
  * @poll: polling for sanitization is enabled, device has no mbox irq support
  * @poll_tmo_secs: polling timeout
  * @poll_dwork: polling work item
@@ -353,6 +367,7 @@ struct cxl_fw_state {
  */
 struct cxl_security_state {
 	unsigned long state;
+	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
 	bool poll;
 	int poll_tmo_secs;
 	struct delayed_work poll_dwork;
-- 
2.41.0

Re: [PATCH 3/3] cxl/memdev: Only show sanitize sysfs files when supported

[Dave Jiang]

On 7/25/23 22:19, Davidlohr Bueso wrote:
> If the device does not support Sanitize or Secure Erase commands,
> hide the respective sysfs interfaces such that the operation can
> never be attempted.
> 
> In order to be generic, keep track of the enabled security commands
> found in the CEL - the driver does not support Security Passthrough.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

LGTM

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   Documentation/ABI/testing/sysfs-bus-cxl |  6 ++--
>   drivers/cxl/core/mbox.c                 | 45 ++++++++++++++++++++++++-
>   drivers/cxl/core/memdev.c               | 19 +++++++++++
>   drivers/cxl/cxlmem.h                    | 15 +++++++++
>   4 files changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index c4c4acb1f3b3..087f762ebfd5 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -86,7 +86,8 @@ Description:
>   		HPA ranges. This permits avoiding explicit global CPU cache
>   		management, relying instead for it to be done when a region
>   		transitions between software programmed and hardware committed
> -		states.
> +		states. If this file is not present, then there is no hardware
> +		support for the operation.
>   
>   
>   What            /sys/bus/cxl/devices/memX/security/erase
> @@ -101,7 +102,8 @@ Description:
>   		HPA ranges. This permits avoiding explicit global CPU cache
>   		management, relying instead for it to be done when a region
>   		transitions between software programmed and hardware committed
> -		states.
> +		states. If this file is not present, then there is no hardware
> +		support for the operation.
>   
>   
>   What:		/sys/bus/cxl/devices/memX/firmware/
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d6d067fbee97..ca60bb8114f2 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -121,6 +121,45 @@ static bool cxl_is_security_command(u16 opcode)
>   	return false;
>   }
>   
> +static void cxl_set_security_cmd_enabled(struct cxl_security_state *security,
> +					 u16 opcode)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_SANITIZE:
> +		set_bit(CXL_SEC_ENABLED_SANITIZE, security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SECURE_ERASE:
> +		set_bit(CXL_SEC_ENABLED_SECURE_ERASE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_GET_SECURITY_STATE:
> +		set_bit(CXL_SEC_ENABLED_GET_SECURITY_STATE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SET_PASSPHRASE:
> +		set_bit(CXL_SEC_ENABLED_SET_PASSPHRASE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_DISABLE_PASSPHRASE:
> +		set_bit(CXL_SEC_ENABLED_DISABLE_PASSPHRASE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_UNLOCK:
> +		set_bit(CXL_SEC_ENABLED_UNLOCK, security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_FREEZE_SECURITY:
> +		set_bit(CXL_SEC_ENABLED_FREEZE_SECURITY,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
> +		set_bit(CXL_SEC_ENABLED_PASSPHRASE_SECURE_ERASE,
> +			security->enabled_cmds);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>   static bool cxl_is_poison_command(u16 opcode)
>   {
>   #define CXL_MBOX_OP_POISON_CMDS 0x43
> @@ -677,7 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>   		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
>   		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>   
> -		if (!cmd && !cxl_is_poison_command(opcode)) {
> +		if (!cmd && (!cxl_is_poison_command(opcode) ||
> +			     !cxl_is_security_command(opcode))) {
>   			dev_dbg(dev,
>   				"Opcode 0x%04x unsupported by driver\n", opcode);
>   			continue;
> @@ -689,6 +729,9 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>   		if (cxl_is_poison_command(opcode))
>   			cxl_set_poison_cmd_enabled(&mds->poison, opcode);
>   
> +		if (cxl_is_security_command(opcode))
> +			cxl_set_security_cmd_enabled(&mds->security, opcode);
> +
>   		dev_dbg(dev, "Opcode 0x%04x enabled\n", opcode);
>   	}
>   }
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f99e7ec3cc40..14b547c07f54 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -477,9 +477,28 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
>   	.attrs = cxl_memdev_pmem_attributes,
>   };
>   
> +static umode_t cxl_memdev_security_visible(struct kobject *kobj,
> +					   struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> +
> +	if (a == &dev_attr_security_sanitize.attr &&
> +	    !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds))
> +		return 0;
> +
> +	if (a == &dev_attr_security_erase.attr &&
> +	    !test_bit(CXL_SEC_ENABLED_SECURE_ERASE, mds->security.enabled_cmds))
> +		return 0;
> +
> +	return a->mode;
> +}
> +
>   static struct attribute_group cxl_memdev_security_attribute_group = {
>   	.name = "security",
>   	.attrs = cxl_memdev_security_attributes,
> +	.is_visible = cxl_memdev_security_visible,
>   };
>   
>   static const struct attribute_group *cxl_memdev_attribute_groups[] = {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 083c6e58bc49..f86afef90c91 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -244,6 +244,19 @@ enum poison_cmd_enabled_bits {
>   	CXL_POISON_ENABLED_MAX
>   };
>   
> +/* Device enabled security commands */
> +enum security_cmd_enabled_bits {
> +	CXL_SEC_ENABLED_SANITIZE,
> +	CXL_SEC_ENABLED_SECURE_ERASE,
> +	CXL_SEC_ENABLED_GET_SECURITY_STATE,
> +	CXL_SEC_ENABLED_SET_PASSPHRASE,
> +	CXL_SEC_ENABLED_DISABLE_PASSPHRASE,
> +	CXL_SEC_ENABLED_UNLOCK,
> +	CXL_SEC_ENABLED_FREEZE_SECURITY,
> +	CXL_SEC_ENABLED_PASSPHRASE_SECURE_ERASE,
> +	CXL_SEC_ENABLED_MAX
> +};
> +
>   /**
>    * struct cxl_poison_state - Driver poison state info
>    *
> @@ -346,6 +359,7 @@ struct cxl_fw_state {
>    * struct cxl_security_state - Device security state
>    *
>    * @state: state of last security operation
> + * @enabled_cmds: All security commands enabled in the CEL
>    * @poll: polling for sanitization is enabled, device has no mbox irq support
>    * @poll_tmo_secs: polling timeout
>    * @poll_dwork: polling work item
> @@ -353,6 +367,7 @@ struct cxl_fw_state {
>    */
>   struct cxl_security_state {
>   	unsigned long state;
> +	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
>   	bool poll;
>   	int poll_tmo_secs;
>   	struct delayed_work poll_dwork;

Re: [PATCH 3/3] cxl/memdev: Only show sanitize sysfs files when supported

[Jonathan Cameron]

On Tue, 25 Jul 2023 22:19:40 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> If the device does not support Sanitize or Secure Erase commands,
> hide the respective sysfs interfaces such that the operation can
> never be attempted.
> 
> In order to be generic, keep track of the enabled security commands
> found in the CEL - the driver does not support Security Passthrough.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Ah just realized I was too late on this one as it's upstream. Ah well I'll
comment anyway (I'd already reviewed most of it :)

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  6 ++--
>  drivers/cxl/core/mbox.c                 | 45 ++++++++++++++++++++++++-
>  drivers/cxl/core/memdev.c               | 19 +++++++++++
>  drivers/cxl/cxlmem.h                    | 15 +++++++++
>  4 files changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index c4c4acb1f3b3..087f762ebfd5 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -86,7 +86,8 @@ Description:
>  		HPA ranges. This permits avoiding explicit global CPU cache
>  		management, relying instead for it to be done when a region
>  		transitions between software programmed and hardware committed
> -		states.
> +		states. If this file is not present, then there is no hardware
> +		support for the operation.

Or older kernel.... 

I don't think we need to talk about when they aren't present as it just complicates
the ABI docs.  The similar case for the DOE is there to explain the weird corner
of it's there, but broken.
An example were it's not documented (I think) is the poison commands so I think
we should keep that absence as meaning the obvious (you can't use the interface).

>  
>  
>  What            /sys/bus/cxl/devices/memX/security/erase
> @@ -101,7 +102,8 @@ Description:
>  		HPA ranges. This permits avoiding explicit global CPU cache
>  		management, relying instead for it to be done when a region
>  		transitions between software programmed and hardware committed
> -		states.
> +		states. If this file is not present, then there is no hardware
> +		support for the operation.
>  
>  
>  What:		/sys/bus/cxl/devices/memX/firmware/
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d6d067fbee97..ca60bb8114f2 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -121,6 +121,45 @@ static bool cxl_is_security_command(u16 opcode)
>  	return false;
>  }
>  
> +static void cxl_set_security_cmd_enabled(struct cxl_security_state *security,
> +					 u16 opcode)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_SANITIZE:
> +		set_bit(CXL_SEC_ENABLED_SANITIZE, security->enabled_cmds);
> +		break;

I'm a fan of returning when nothing else to do.
(this is when I realized this was definitely upstream as I called this function
 in the switch cci code!)

> +	case CXL_MBOX_OP_SECURE_ERASE:
> +		set_bit(CXL_SEC_ENABLED_SECURE_ERASE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_GET_SECURITY_STATE:
> +		set_bit(CXL_SEC_ENABLED_GET_SECURITY_STATE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_SET_PASSPHRASE:
> +		set_bit(CXL_SEC_ENABLED_SET_PASSPHRASE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_DISABLE_PASSPHRASE:
> +		set_bit(CXL_SEC_ENABLED_DISABLE_PASSPHRASE,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_UNLOCK:
> +		set_bit(CXL_SEC_ENABLED_UNLOCK, security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_FREEZE_SECURITY:
> +		set_bit(CXL_SEC_ENABLED_FREEZE_SECURITY,
> +			security->enabled_cmds);
> +		break;
> +	case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
> +		set_bit(CXL_SEC_ENABLED_PASSPHRASE_SECURE_ERASE,
> +			security->enabled_cmds);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static bool cxl_is_poison_command(u16 opcode)
>  {
>  #define CXL_MBOX_OP_POISON_CMDS 0x43
> @@ -677,7 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
>  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>  
> -		if (!cmd && !cxl_is_poison_command(opcode)) {
> +		if (!cmd && (!cxl_is_poison_command(opcode) ||
> +			     !cxl_is_security_command(opcode))) {
>  			dev_dbg(dev,
>  				"Opcode 0x%04x unsupported by driver\n", opcode);
>  			continue;
> @@ -689,6 +729,9 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  		if (cxl_is_poison_command(opcode))
>  			cxl_set_poison_cmd_enabled(&mds->poison, opcode);
>  
> +		if (cxl_is_security_command(opcode))
> +			cxl_set_security_cmd_enabled(&mds->security, opcode);
> +
>  		dev_dbg(dev, "Opcode 0x%04x enabled\n", opcode);
>  	}
>  }
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f99e7ec3cc40..14b547c07f54 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -477,9 +477,28 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
>  	.attrs = cxl_memdev_pmem_attributes,
>  };
>  
> +static umode_t cxl_memdev_security_visible(struct kobject *kobj,
> +					   struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> +
> +	if (a == &dev_attr_security_sanitize.attr &&
> +	    !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds))
> +		return 0;
> +
> +	if (a == &dev_attr_security_erase.attr &&
> +	    !test_bit(CXL_SEC_ENABLED_SECURE_ERASE, mds->security.enabled_cmds))
> +		return 0;
> +
> +	return a->mode;
> +}
> +
>  static struct attribute_group cxl_memdev_security_attribute_group = {
>  	.name = "security",
>  	.attrs = cxl_memdev_security_attributes,
> +	.is_visible = cxl_memdev_security_visible,
>  };
>  
>  static const struct attribute_group *cxl_memdev_attribute_groups[] = {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 083c6e58bc49..f86afef90c91 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -244,6 +244,19 @@ enum poison_cmd_enabled_bits {
>  	CXL_POISON_ENABLED_MAX
>  };
>  
> +/* Device enabled security commands */
> +enum security_cmd_enabled_bits {
> +	CXL_SEC_ENABLED_SANITIZE,
> +	CXL_SEC_ENABLED_SECURE_ERASE,
> +	CXL_SEC_ENABLED_GET_SECURITY_STATE,
> +	CXL_SEC_ENABLED_SET_PASSPHRASE,
> +	CXL_SEC_ENABLED_DISABLE_PASSPHRASE,
> +	CXL_SEC_ENABLED_UNLOCK,
> +	CXL_SEC_ENABLED_FREEZE_SECURITY,
> +	CXL_SEC_ENABLED_PASSPHRASE_SECURE_ERASE,
> +	CXL_SEC_ENABLED_MAX
> +};
> +
>  /**
>   * struct cxl_poison_state - Driver poison state info
>   *
> @@ -346,6 +359,7 @@ struct cxl_fw_state {
>   * struct cxl_security_state - Device security state
>   *
>   * @state: state of last security operation
> + * @enabled_cmds: All security commands enabled in the CEL
>   * @poll: polling for sanitization is enabled, device has no mbox irq support
>   * @poll_tmo_secs: polling timeout
>   * @poll_dwork: polling work item
> @@ -353,6 +367,7 @@ struct cxl_fw_state {
>   */
>  struct cxl_security_state {
>  	unsigned long state;
> +	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
>  	bool poll;
>  	int poll_tmo_secs;
>  	struct delayed_work poll_dwork;

Re: [PATCH 3/3] cxl/memdev: Only show sanitize sysfs files when supported

[Davidlohr Bueso]

On Fri, 04 Aug 2023, Jonathan Cameron wrote:

>On Tue, 25 Jul 2023 22:19:40 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> If the device does not support Sanitize or Secure Erase commands,
>> hide the respective sysfs interfaces such that the operation can
>> never be attempted.
>>
>> In order to be generic, keep track of the enabled security commands
>> found in the CEL - the driver does not support Security Passthrough.
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>
>Ah just realized I was too late on this one as it's upstream. Ah well I'll
>comment anyway (I'd already reviewed most of it :)

Yes, this has already been picked up by Linus. Regardless, thanks for
having a look at the series.

>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-cxl |  6 ++--
>>  drivers/cxl/core/mbox.c                 | 45 ++++++++++++++++++++++++-
>>  drivers/cxl/core/memdev.c               | 19 +++++++++++
>>  drivers/cxl/cxlmem.h                    | 15 +++++++++
>>  4 files changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
>> index c4c4acb1f3b3..087f762ebfd5 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-cxl
>> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
>> @@ -86,7 +86,8 @@ Description:
>>		HPA ranges. This permits avoiding explicit global CPU cache
>>		management, relying instead for it to be done when a region
>>		transitions between software programmed and hardware committed
>> -		states.
>> +		states. If this file is not present, then there is no hardware
>> +		support for the operation.
>
>Or older kernel....
>
>I don't think we need to talk about when they aren't present as it just complicates
>the ABI docs.  The similar case for the DOE is there to explain the weird corner
>of it's there, but broken.
>An example were it's not documented (I think) is the poison commands so I think
>we should keep that absence as meaning the obvious (you can't use the interface).

*nod* - I'll have that present for the future.

Thanks,
Davidlohr