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
