Re: [PATCH v6] misc: Add Nitro Secure Module driver

["Arnd Bergmann" <arnd@arndb.de>]

On Tue, Oct 10, 2023, at 23:34, Alexander Graf wrote:
> This patch adds a driver for NSM that exposes a /dev/nsm device node which
> user space can issue an ioctl on this device with raw NSM CBOR formatted
> commands to request attestation documents, influence PCR states, read
> entropy and enumerate status of the device. In addition, the driver
> implements a hwrng backend.

I haven't had a chance to actually read the v3 submission in enough
detail, but assuming we're going to go with the simple pass-through
interface here, I've looked for some mostly cosmetic improvements
that you may want to integrate.

> +/* Timeout for NSM virtqueue respose in milliseconds. */
> +#define NSM_DEFAULT_TIMEOUT_MSECS (120000) /* 2 minutes */
> +
> +struct nsm {
> +	struct virtio_device *vdev;
> +	struct virtqueue     *vq;
> +	struct mutex          lock;
> +	wait_queue_head_t     wq;
> +	bool                  device_notified;

Instead of a manual wait queue plus a bool, using a
'struct completion' simplifies this a little bit.

> +
> +/* Maximum length input data */
> +struct nsm_data_req {
> +	__u32 len;
> +	__u8  data[NSM_REQUEST_MAX_SIZE];
> +};

> +/* Maximum length output data */
> +struct nsm_data_resp {
> +	__u32 len;
> +	__u8  data[NSM_RESPONSE_MAX_SIZE];
> +};

You have endian-conversion for some of the data fields
but not the 'len field here, I guess these should be
__le32 instead of __u32, with the appropriate le32_to_cpu()
and cpu_to_le32() conversion when passing the native
u32 word from userspace.

It does seem odd that you have a little-endian length field
here, but big-endian length fields inside of the cbor
data. 

> +#define CBOR_HEADER_SIZE_U8  (CBOR_HEADER_SIZE_SHORT + sizeof(u8))
> +#define CBOR_HEADER_SIZE_U16 (CBOR_HEADER_SIZE_SHORT + sizeof(u16))
> +#define CBOR_HEADER_SIZE_U32 (CBOR_HEADER_SIZE_SHORT + sizeof(u32))
> +#define CBOR_HEADER_SIZE_U64 (CBOR_HEADER_SIZE_SHORT + sizeof(u64))

Similarly, I guess these should be __be16/__be32/__be64?

> +	} else if (cbor_short_size == CBOR_LONG_SIZE_U32) {
> +		if (cbor_object_size < CBOR_HEADER_SIZE_U32)
> +			return -EFAULT;
> +		/* 4 bytes */
> +		array_len = cbor_object[1] << 24 |
> +			cbor_object[2] << 16 |
> +			cbor_object[3] << 8  |
> +			cbor_object[4];
> +		array_offset = CBOR_HEADER_SIZE_U32;
> +	} else if (cbor_short_size == CBOR_LONG_SIZE_U64) {
> +		if (cbor_object_size < CBOR_HEADER_SIZE_U64)
> +			return -EFAULT;
> +		/* 8 bytes */
> +		array_len = (u64) cbor_object[1] << 56 |
> +			  (u64) cbor_object[2] << 48 |
> +			  (u64) cbor_object[3] << 40 |
> +			  (u64) cbor_object[4] << 32 |
> +			  (u64) cbor_object[5] << 24 |
> +			  (u64) cbor_object[6] << 16 |
> +			  (u64) cbor_object[7] << 8  |
> +			  (u64) cbor_object[8];

These could use be{!6,32,64}_to_cpup() for clarity.

> +static int nsm_rng_read(struct hwrng *rng, void *data, size_t max, 
> bool wait)
> +{
> +	struct nsm *nsm = hwrng_to_nsm(rng);
> +	struct device *dev = &nsm->vdev->dev;
> +	struct nsm_msg *msg;
> +	int rc = 0;
> +
> +	/* NSM always needs to wait for a response */
> +	if (!wait)
> +		return 0;
> +
> +	msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	rc = fill_req_get_random(nsm, &msg->req);
> +	if (rc != 0)
> +		goto out;
> +
> +	rc = nsm_sendrecv_msg(nsm, msg);
> +	if (rc != 0)
> +		goto out;
> +
> +	rc = parse_resp_get_random(nsm, &msg->resp, data, max);
> +	if (rc < 0)
> +		goto out;

It looks like the bulk of this function happens inside of
nsm_sendrecv_msg(), which uses a mutex for serialization.

In this case, I think you can replace the dynamic allocation
during read() with a preallocated buffer in the device that
always gets used here, after you extend the mutex out to the
entire fill_req_get_random()/nsm_sendrecv_msg()/parse_resp_get_random()
block.

> +static long nsm_dev_ioctl(struct file *file, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	void __user *argp = u64_to_user_ptr((u64)arg);
> +	struct nsm *nsm = file_to_nsm(file);
> +	struct nsm_msg *msg;
> +	struct nsm_raw raw;
> +	int r = 0;
> +
> +	if (cmd != NSM_IOCTL_RAW)
> +		return -EINVAL;
> +
> +	if (_IOC_SIZE(cmd) != sizeof(raw))
> +		return -EINVAL;
> +
> +	/* Allocate message buffers to device */
> +	r = -ENOMEM;
> +	msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> +	if (!msg)
> +		goto out;
> +
> +	/* Copy user argument struct to kernel argument struct */
> +	r = -EFAULT;
> +	if (copy_from_user(&raw, argp, _IOC_SIZE(cmd)))
> +		goto out;
> +
> +	/* Convert kernel argument struct to device request */
> +	r = fill_req_raw(nsm, &msg->req, &raw);
> +	if (r)
> +		goto out;
> +
> +	/* Send message to NSM and read reply */
> +	r = nsm_sendrecv_msg(nsm, msg);
> +	if (r)
> +		goto out;
> +
> +	/* Parse device response into kernel argument struct */
> +	r = parse_resp_raw(nsm, &msg->resp, &raw);
> +	if (r)
> +		goto out;

And the same is probably true here.

> +static int nsm_dev_file_open(struct inode *node, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static int nsm_dev_file_close(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}

These are not needed if they don't do anything.

     Arnd