RE: [PATCH 2/7] cxl/security: Add security state sysfs ABI

[Dan Williams <dan.j.williams@intel.com>]

Davidlohr Bueso wrote:
> This adds the sysfs memdev's security/ directory with
> a single 'state' file, which is always visible. In the
> case of unsupported security features, this will show
> disabled.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  8 ++++
>  drivers/cxl/core/memdev.c               | 49 +++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 3acf2f17a73f..e9c432a5a841 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -57,6 +57,14 @@ Description:
>  		host PCI device for this memory device, emit the CPU node
>  		affinity for this device.
>  
> +What:		/sys/bus/cxl/devices/memX/security/state
> +Date:		February, 2023
> +KernelVersion:	v6.4
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The security state for that device. The following states
> +		are available: frozen, locked, unlocked and disabled (which
> +		is also the case for any unsupported security features).
>  
>  What:		/sys/bus/cxl/devices/*/devtype
>  Date:		June, 2021
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 0af8856936dc..47cc625bb1b0 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. */
>  
> +#include <linux/memregion.h>
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> @@ -89,6 +90,43 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>  static struct device_attribute dev_attr_pmem_size =
>  	__ATTR(size, 0444, pmem_size_show, NULL);
>  
> +static ssize_t security_state_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	u32 sec_out;
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_get_security_output {
> +		__le32 flags;
> +	} out;
> +	struct cxl_mbox_cmd mbox_cmd = {
> +		.opcode = CXL_MBOX_OP_GET_SECURITY_STATE,
> +		.payload_out = &out,
> +		.size_out = sizeof(out),
> +	};
> +
> +	if (!cpu_cache_has_invalidate_memregion())
> +		goto disabled;

I think this can go as security state can still be read even if
unlocking is not safely possible.

> +
> +	if (cxl_internal_send_cmd(cxlds, &mbox_cmd) < 0)
> +		goto disabled;

I would prefer to not have an any-user triggerable way to spam mailbox
commands. Security state should be read from a cached value that gets
updated when security operations are run.

> +
> +	sec_out = le32_to_cpu(out.flags);
> +	if (!(sec_out & CXL_PMEM_SEC_STATE_USER_PASS_SET))
> +		goto disabled;
> +	if (sec_out & CXL_PMEM_SEC_STATE_FROZEN)
> +		return sysfs_emit(buf, "frozen\n");
> +	if (sec_out & CXL_PMEM_SEC_STATE_LOCKED)
> +		return sysfs_emit(buf, "locked\n");
> +	else
> +		return sysfs_emit(buf, "unlocked\n");
> +disabled:
> +	return sysfs_emit(buf, "disabled\n");
> +}
> +
> +static struct device_attribute dev_attr_security_state =
> +	__ATTR(state, 0444, security_state_show, NULL);

This looks copied from pmem_size above, however that one is using
open-coded __ATTR() because the attribute name, "size", does not match
the prefix of the show() handler, "pmem_size_show()". In this case the
shorter DEVICE_ATTR_RO() helper can be used.