Re: [PATCH 2/5] misc: fastrpc: Add secure device node support

Srinivas Kandagatla
2021-11-25T14:59:12Z

Hi Jeya,

Thanks for the patch,

On 25/11/2021 14:15, Jeya R wrote:
> Register and deregister secure device node. Check for device name during
> device open get proper channel context.
>
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---
> drivers/misc/fastrpc.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 39aca77..0775554e 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -79,6 +79,7 @@
> #define SENSORS_PD (2)
>
> #define miscdev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, miscdev)
> +#define securedev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, securedev)
>
> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> "sdsp", "cdsp"};
> @@ -213,6 +214,7 @@ struct fastrpc_channel_ctx {
> struct idr ctx_idr;
> struct list_head users;
> struct miscdevice miscdev;
> + struct miscdevice securedev;
> struct kref refcount;
> };
>
> @@ -1218,10 +1220,23 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
>
> static int fastrpc_device_open(struct inode *inode, struct file *filp)
> {
> - struct fastrpc_channel_ctx *cctx = miscdev_to_cctx(filp->private_data);
> + struct fastrpc_channel_ctx *cctx = NULL;
> struct fastrpc_user *fl = NULL;
> + struct miscdevice *currdev = NULL;
> unsigned long flags;
>
> + if (!filp)
> + return -EFAULT;
> +
> + currdev = (struct miscdevice *)(filp->private_data);
> + if (!currdev)
> + return -EFAULT;
> +
> + if (strstr(currdev->name, "secure") != NULL)
> + cctx = securedev_to_cctx(filp->private_data);
> + else
> + cctx = miscdev_to_cctx(filp->private_data);
> +

Now we only have one of the two possibilities,
Either device node is "non secure" or a "secure"

If you create just one device node based on the device tree bindings
then you would not need to do any of these runtime checks.

something like this in fastrpc_rpmsg_probe() should do:

----------------------->cut<----------------------------------
data->secure = !(of_property_read_bool(rdev->of_node,
"qcom,non-secure-domain"));
data->miscdev.minor = MISC_DYNAMIC_MINOR;
data->miscdev.name = devm_kasprintf(rdev, GFP_KERNEL, "fastrpc-%s-%s",
domains[domain_id],
data->secure ? "secure" : "");
data->miscdev.fops = &fastrpc_fops;
err = misc_register(&data->miscdev);
if (err) {
kfree(data);
return err;
}
----------------------->cut<----------------------------------

--srini

> fl = kzalloc(sizeof(*fl), GFP_KERNEL);
> if (!fl)
> return -ENOMEM;
> @@ -1644,6 +1659,15 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> kfree(data);
> return err;
> }
> + data->securedev.minor = MISC_DYNAMIC_MINOR;
> + data->securedev.name = devm_kasprintf(rdev, GFP_KERNEL,
> + "fastrpc-%s-secure", domains[domain_id]);
> + data->securedev.fops = &fastrpc_fops;
> + err = misc_register(&data->securedev);
> + if (err) {
> + kfree(data);
> + return err;
> + }
>
> kref_init(&data->refcount);
>
> @@ -1655,7 +1679,11 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> data->domain_id = domain_id;
> data->rpdev = rpdev;
>
> - return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> + err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> + dev_info(rdev, "%s done for %s with nodes non-secure(%d), secure(%d) return: %d\n",
> + __func__, domains[domain_id],
> + data->miscdev.minor, data->securedev.minor, err);
> + return err;
> }
>
> static void fastrpc_notify_users(struct fastrpc_user *user)
> @@ -1680,6 +1708,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> spin_unlock_irqrestore(&cctx->lock, flags);
>
> misc_deregister(&cctx->miscdev);
> + misc_deregister(&cctx->securedev);
> of_platform_depopulate(&rpdev->dev);
>
> cctx->rpdev = NULL;
>