Re: [PATCH v7 5/5] firmware: imx: adds miscdev

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 04, 2024 at 04:21:21PM +0530, Pankaj Gupta wrote:
> Adds the driver for communication interface to secure-enclave,
> for exchanging messages with NXP secure enclave HW IP(s) like
> EdgeLock Enclave from:
> - User-Space Applications via character driver.
> 
> ABI documentation for the NXP secure-enclave driver.
> 
> User-space library using this driver:
> - i.MX Secure Enclave library:
>   -- URL: https://github.com/nxp-imx/imx-secure-enclave.git,
> - i.MX Secure Middle-Ware:
>   -- URL: https://github.com/nxp-imx/imx-smw.git
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> ---
>  Documentation/ABI/testing/se-cdev   |  43 ++
>  drivers/firmware/imx/ele_base_msg.c |   8 +-
>  drivers/firmware/imx/ele_common.c   |  36 +-
>  drivers/firmware/imx/ele_common.h   |   6 +-
>  drivers/firmware/imx/se_ctrl.c      | 790 ++++++++++++++++++++++++++++++++++++
>  drivers/firmware/imx/se_ctrl.h      |  52 +++
>  include/uapi/linux/se_ioctl.h       |  94 +++++
>  7 files changed, 1010 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index e3e570a25e85..ae200ce64100 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -68,7 +68,7 @@ int ele_get_info(struct device *dev, struct ele_dev_info *s_info)
>  	tx_msg->data[0] = upper_32_bits(get_info_addr);
>  	tx_msg->data[1] = lower_32_bits(get_info_addr);
>  	tx_msg->data[2] = sizeof(*s_info);
> -	ret = ele_msg_send_rcv(priv,
> +	ret = ele_msg_send_rcv(priv->priv_dev_ctx,
>  			       tx_msg,
>  			       ELE_GET_INFO_REQ_MSG_SZ,
>  			       rx_msg,
> @@ -150,7 +150,7 @@ int ele_ping(struct device *dev)
>  		goto exit;
>  	}
>  
> -	ret = ele_msg_send_rcv(priv,
> +	ret = ele_msg_send_rcv(priv->priv_dev_ctx,
>  			       tx_msg,
>  			       ELE_PING_REQ_SZ,
>  			       rx_msg,
> @@ -206,7 +206,7 @@ int ele_service_swap(struct device *dev,
>  	tx_msg->data[3] = lower_32_bits(addr);
>  	tx_msg->data[4] = se_add_msg_crc((uint32_t *)&tx_msg[0],
>  						 ELE_SERVICE_SWAP_REQ_MSG_SZ);
> -	ret = ele_msg_send_rcv(priv,
> +	ret = ele_msg_send_rcv(priv->priv_dev_ctx,
>  			       tx_msg,
>  			       ELE_SERVICE_SWAP_REQ_MSG_SZ,
>  			       rx_msg,
> @@ -268,7 +268,7 @@ int ele_fw_authenticate(struct device *dev, phys_addr_t addr)
>  	tx_msg->data[0] = lower_32_bits(addr);
>  	tx_msg->data[2] = addr;
>  
> -	ret = ele_msg_send_rcv(priv,
> +	ret = ele_msg_send_rcv(priv->priv_dev_ctx,
>  			       tx_msg,
>  			       ELE_FW_AUTH_REQ_SZ,
>  			       rx_msg,
> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index a06d7015d3f1..9114c3594567 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -18,16 +18,18 @@ u32 se_add_msg_crc(u32 *msg, u32 msg_len)
>  	return crc;
>  }
>  
> -int ele_msg_rcv(struct se_if_priv *priv,
> +int ele_msg_rcv(struct se_if_device_ctx *dev_ctx,
>  		struct se_clbk_handle *se_clbk_hdl)
>  {
> +	struct se_if_priv *priv = dev_ctx->priv;
>  	int err = 0;
>  
>  	err = wait_event_interruptible(se_clbk_hdl->wq,
>  				       atomic_read(&se_clbk_hdl->pending_hdr) != 0);
>  	if (err < 0)
>  		dev_err(priv->dev,
> -			"Err[0x%x]:Interrupted by signal.\n",
> +			"%s: Err[0x%x]:Interrupted by signal.\n",
> +			se_clbk_hdl->dev_ctx->devname,
>  			err);
>  	else
>  		err = se_clbk_hdl->rx_msg_sz;
> @@ -37,10 +39,11 @@ int ele_msg_rcv(struct se_if_priv *priv,
>  	return err;
>  }
>  
> -int ele_msg_send(struct se_if_priv *priv,
> +int ele_msg_send(struct se_if_device_ctx *dev_ctx,
>  		 void *tx_msg,
>  		 int tx_msg_sz)
>  {
> +	struct se_if_priv *priv = dev_ctx->priv;
>  	struct se_msg_hdr *header;
>  	int err;
>  
> @@ -53,7 +56,8 @@ int ele_msg_send(struct se_if_priv *priv,
>  	if (header->size << 2 != tx_msg_sz) {
>  		err = -EINVAL;
>  		dev_err(priv->dev,
> -			"User buf hdr: 0x%x, sz mismatced with input-sz (%d != %d).",
> +			"%s: User buf hdr: 0x%x, sz mismatced with input-sz (%d != %d).",
> +			dev_ctx->devname,
>  			*(u32 *)header,
>  			header->size << 2, tx_msg_sz);
>  		goto exit;
> @@ -62,7 +66,9 @@ int ele_msg_send(struct se_if_priv *priv,
>  
>  	err = mbox_send_message(priv->tx_chan, tx_msg);
>  	if (err < 0) {
> -		dev_err(priv->dev, "Error: mbox_send_message failure.\n");
> +		dev_err(priv->dev,
> +			"%s: Error: mbox_send_message failure.",
> +			dev_ctx->devname);
>  		return err;
>  	}
>  	err = tx_msg_sz;
> @@ -72,24 +78,26 @@ int ele_msg_send(struct se_if_priv *priv,
>  }
>  
>  /* API used for send/receive blocking call. */
> -int ele_msg_send_rcv(struct se_if_priv *priv,
> +int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx,

You are heavily patching a file you introduced just in the last patch.
It seems you have messed up while rebasing. Please cleanup.

> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    u64 arg)
> +{
> +	struct se_if_priv *priv = dev_ctx->priv;
> +	struct se_ioctl_cmd_snd_rcv_rsp_info cmd_snd_rcv_rsp_info;
> +	struct se_api_msg *tx_msg __free(kfree) = NULL;
> +	struct se_api_msg *rx_msg __free(kfree) = NULL;
> +	int err = 0;
> +
> +	if (down_interruptible(&dev_ctx->fops_lock))
> +		return -EBUSY;
> +
> +	if (dev_ctx->status != SE_IF_CTX_OPENED) {
> +		err = -EINVAL;
> +		goto exit;
> +	}

You don't need this. Just trust Linux that it won't call your
ioctl/read/write ops when the device is not opened.

> +
> +	if (copy_from_user(&cmd_snd_rcv_rsp_info, (u8 *)arg,
> +			   sizeof(cmd_snd_rcv_rsp_info))) {
> +		dev_err(dev_ctx->priv->dev,

You have priv as a variable already, please use it throughout this
function.

> +static ssize_t se_if_fops_read(struct file *fp, char __user *buf,
> +			       size_t size, loff_t *ppos)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
> +	struct se_if_priv *priv = dev_ctx->priv;
> +	int err;
> +
> +	dev_dbg(priv->dev,
> +		"%s: read to buf %p(%zu), ppos=%lld\n",
> +			dev_ctx->miscdev.name,
> +			buf, size, ((ppos) ? *ppos : 0));
> +
> +	if (down_interruptible(&dev_ctx->fops_lock))
> +		return -EBUSY;
> +
> +	if (dev_ctx->status != SE_IF_CTX_OPENED) {
> +		err = -EINVAL;
> +		goto exit;
> +	}
> +
> +	if (dev_ctx != priv->cmd_receiver_clbk_hdl.dev_ctx) {
> +		err = -EINVAL;
> +		goto exit;
> +	}
> +
> +	err = ele_msg_rcv(dev_ctx, &priv->cmd_receiver_clbk_hdl);
> +	if (err < 0) {
> +		dev_err(dev_ctx->priv->dev,
> +				"%s: Err[0x%x]:Interrupted by signal.\n",
> +				dev_ctx->devname, err);
> +		dev_err(dev_ctx->priv->dev,
> +				"Current active dev-ctx count = %d.\n",
> +				dev_ctx->priv->active_devctx_count);

active_devctx_count is never used except for printing this message. I
don't think it provides meaningful information for userspace. Just dorp
it.

> +	dma_free_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
> +			  se_shared_mem_mgmt->non_secure_mem.ptr,
> +			  se_shared_mem_mgmt->non_secure_mem.dma_addr);
> +
> +	se_shared_mem_mgmt->non_secure_mem.ptr = NULL;
> +	se_shared_mem_mgmt->non_secure_mem.dma_addr = 0;
> +	se_shared_mem_mgmt->non_secure_mem.size = 0;
> +	se_shared_mem_mgmt->non_secure_mem.pos = 0;
> +}
> +
> +/* Need to copy the output data to user-device context.
> + */
> +int se_dev_ctx_cpy_out_data(struct se_shared_mem_mgmt_info *se_shared_mem_mgmt)

Only used in this file. Make it static.

> +{
> +	struct se_if_device_ctx *dev_ctx = container_of(se_shared_mem_mgmt,
> +							typeof(*dev_ctx),
> +							se_shared_mem_mgmt);

This function is only ever called like:

se_dev_ctx_cpy_out_data(&dev_ctx->se_shared_mem_mgmt);

Instead of this back and forth you could pass a struct se_if_device_ctx *
directly.

> +static int add_b_desc_to_pending_list(void *shared_ptr_with_pos,
> +			       struct se_ioctl_setup_iobuf *io,
> +			       struct se_shared_mem_mgmt_info *se_shared_mem_mgmt)
> +{
> +	struct se_buf_desc *b_desc = NULL;
> +
> +	b_desc = kzalloc(sizeof(*b_desc), GFP_KERNEL);
> +	if (!b_desc)
> +		return -ENOMEM;
> +
> +	if (b_desc) {

b_desc is not NULL here.

> +		b_desc->shared_buf_ptr = shared_ptr_with_pos;
> +		b_desc->usr_buf_ptr = io->user_buf;
> +		b_desc->size = io->length;
> +
> +		if (io->flags & SE_IO_BUF_FLAGS_IS_INPUT) {
> +			/*
> +			 * buffer is input:
> +			 * add an entry in the "pending input buffers" list so
> +			 * that copied data can be cleaned from shared memory
> +			 * later.
> +			 */
> +			list_add_tail(&b_desc->link, &se_shared_mem_mgmt->pending_in);
> +		} else {
> +			/*
> +			 * buffer is output:
> +			 * add an entry in the "pending out buffers" list so data
> +			 * can be copied to user space when receiving Secure-Enclave
> +			 * response.
> +			 */
> +			list_add_tail(&b_desc->link, &se_shared_mem_mgmt->pending_out);
> +		}
> +	}
> +
> +
> +	return 0;
> +}
> +
> +/* Open a character device. */
> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
> +	struct se_if_device_ctx *dev_ctx = container_of(fp->private_data,
> +							struct se_if_device_ctx,
> +							miscdev);
> +	struct se_if_priv *priv = dev_ctx->priv;
> +	int err = 0;
> +
> +	priv->dev_ctx_mono_count++;
> +	err = init_device_context(priv,
> +				  priv->dev_ctx_mono_count ?
> +					priv->dev_ctx_mono_count
> +					: priv->dev_ctx_mono_count++,

priv->dev_ctx_mono_count won't be 0 here as you just increased it.

> +				  &dev_ctx);
> +	if (err) {
> +		dev_err(priv->dev,
> +			"Failed[0x%x] to create device contexts.\n",
> +			err);
> +		goto exit;
> +	}
> +
> +	dev_ctx->status = SE_IF_CTX_OPENED;
> +
> +	fp->private_data = dev_ctx;
> +
> +exit:
> +	up(&dev_ctx->fops_lock);

You never acquired this semaphore which is likely not what you want.

> +	return err;
> +}
> +
> +/* Close a character device. */
> +static int se_if_fops_close(struct inode *nd, struct file *fp)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
> +	struct se_if_priv *priv = dev_ctx->priv;
> +
> +	/* Avoid race if closed at the same time */
> +	if (down_trylock(&dev_ctx->fops_lock))
> +		return -EBUSY;
> +
> +	/* The device context has not been opened */
> +	if (dev_ctx->status != SE_IF_CTX_OPENED)
> +		goto exit;
> +
> +	/* check if this device was registered as command receiver. */
> +	if (priv->cmd_receiver_clbk_hdl.dev_ctx == dev_ctx) {

This is racy. priv->cmd_receiver_clbk_hdl can be accessed by multiple
tasks without locking.

> +		priv->cmd_receiver_clbk_hdl.dev_ctx = NULL;
> +		kfree(priv->cmd_receiver_clbk_hdl.rx_msg);
> +		priv->cmd_receiver_clbk_hdl.rx_msg = NULL;
> +		atomic_set(&priv->cmd_receiver_clbk_hdl.pending_hdr, 0);
> +	}
> +
> +	/* check if this device was registered as waiting response. */
> +	if (priv->waiting_rsp_clbk_hdl.dev_ctx == dev_ctx) {

> +		priv->waiting_rsp_clbk_hdl.dev_ctx = NULL;
> +		mutex_unlock(&priv->se_if_cmd_lock);

This mutex will never be locked here.

> +	}
> +
> +	se_dev_ctx_shared_mem_cleanup(&dev_ctx->se_shared_mem_mgmt);
> +	cleanup_se_shared_mem(&dev_ctx->se_shared_mem_mgmt);
> +	dev_ctx->status = SE_IF_CTX_FREE;
> +
> +	priv->active_devctx_count--;
> +	list_del(&dev_ctx->link);
> +exit:
> +	up(&dev_ctx->fops_lock);
> +	kfree(dev_ctx->devname);
> +	kfree(dev_ctx);
> +
> +	return 0;
> +}
> +
>  static void se_if_probe_cleanup(struct platform_device *pdev)
>  {
> +	struct se_if_device_ctx *dev_ctx, *t_dev_ctx;
>  	struct device *dev = &pdev->dev;
>  	struct se_if_priv *priv;
>  
> @@ -247,6 +939,14 @@ static void se_if_probe_cleanup(struct platform_device *pdev)
>  		priv->imem.buf = NULL;
>  	}
>  
> +	list_for_each_entry_safe(dev_ctx, t_dev_ctx, &priv->dev_ctx_list, link) {
> +		list_del(&dev_ctx->link);
> +		priv->active_devctx_count--;
> +	}

The only place where this list is ever iterated over is here. active_devctx_count
is never used except for printing a message (see other comment).
Just drop this list.

> +
> +	devm_remove_action(dev, if_misc_deregister, &priv->priv_dev_ctx->miscdev);
> +	misc_deregister(&priv->priv_dev_ctx->miscdev);
> +
>  	/* No need to check, if reserved memory is allocated
>  	 * before calling for its release. Or clearing the
>  	 * un-set bit.
> @@ -254,6 +954,87 @@ static void se_if_probe_cleanup(struct platform_device *pdev)
>  	of_reserved_mem_device_release(dev);
>  }
>  
> +int init_device_context(struct se_if_priv *priv, int ch_id,
> +			struct se_if_device_ctx **new_dev_ctx)
> +{

static.

> +	const struct se_if_node_info *info = priv->info;
> +	struct se_if_device_ctx *dev_ctx;
> +	int ret = 0;
> +
> +	if (ch_id)
> +		dev_ctx = kzalloc(sizeof(*dev_ctx), GFP_KERNEL);
> +	else
> +		dev_ctx = devm_kzalloc(priv->dev, sizeof(*dev_ctx), GFP_KERNEL);
> +
> +	if (!dev_ctx) {
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	dev_ctx->status = SE_IF_CTX_FREE;
> +	dev_ctx->priv = priv;
> +
> +	if (ch_id)
> +		dev_ctx->devname = kasprintf(GFP_KERNEL, "%s_ch%d",
> +					     info->se_name, ch_id);
> +	else
> +		dev_ctx->devname = devm_kasprintf(priv->dev,
> +						  GFP_KERNEL, "%s_ch%d",
> +						  info->se_name, ch_id);
> +	if (!dev_ctx->devname) {
> +		ret = -ENOMEM;
> +		if (ch_id)
> +			kfree(dev_ctx);
> +
> +		return ret;
> +	}
> +
> +	sema_init(&dev_ctx->fops_lock, 1);

This semaphore only counts up to one in your code which basically makes
it a mutex. Just use a mutex instead.

> +/* Private struct for each char device instance. */
> +struct se_if_device_ctx {
> +	struct se_if_priv *priv;
> +	struct miscdevice miscdev;
> +	const char *devname;
> +
> +	enum se_if_dev_ctx_status_t status;
> +	struct semaphore fops_lock;
> +
> +	struct se_shared_mem_mgmt_info se_shared_mem_mgmt;
> +	struct notifier_block se_notify;

se_notify is never used.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux