On Thu 09 Dec 04:06 PST 2021, Srinivas Kandagatla wrote:
> ADSP/MDSP/SDSP are by default secured, which means it can only be loaded
> with a Signed process.
> Where as CDSP can be either be secured/unsecured. non-secured Compute DSP
> would allow users to load unsigned process and run hexagon instructions,
> but blocking access to secured hardware within the DSP. Where as signed
> process with secure CDSP would be allowed to access all the dsp resources.
>
> This patch adds basic code to create device nodes as per device tree property.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> drivers/misc/fastrpc.c | 61 +++++++++++++++++++++++++++++++++++-------
> 1 file changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 79fc59caacef..50f8e23b6b04 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -240,12 +240,15 @@ struct fastrpc_channel_ctx {
> /* Flag if dsp attributes are cached */
> bool valid_attributes;
> u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
> + struct fastrpc_device *secure_fdevice;
> struct fastrpc_device *fdevice;
> + bool secure;
> };
>
> struct fastrpc_device {
> struct fastrpc_channel_ctx *cctx;
> struct miscdevice miscdev;
> + bool secure;
> };
>
> struct fastrpc_user {
> @@ -1876,7 +1879,7 @@ static struct platform_driver fastrpc_cb_driver = {
> };
>
> static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
> - const char *domain)
> + bool is_secured, const char *domain)
> {
> struct fastrpc_device *fdev;
> int err;
> @@ -1885,15 +1888,21 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
> if (!fdev)
> return -ENOMEM;
>
> + fdev->secure = is_secured;
> fdev->cctx = cctx;
> fdev->miscdev.minor = MISC_DYNAMIC_MINOR;
> fdev->miscdev.fops = &fastrpc_fops;
> - fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s", domain);
> + fdev->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "fastrpc-%s%s",
> + domain, is_secured ? "-secure" : "");
Will this not result in existing userspace using the wrong misc device?
> err = misc_register(&fdev->miscdev);
> - if (err)
> + if (err) {
> kfree(fdev);
> - else
> - cctx->fdevice = fdev;
> + } else {
> + if (is_secured)
> + cctx->secure_fdevice = fdev;
> + else
> + cctx->fdevice = fdev;
> + }
>
> return err;
> }
> @@ -1904,6 +1913,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> struct fastrpc_channel_ctx *data;
> int i, err, domain_id = -1;
> const char *domain;
> + bool secure_dsp = false;
Afaict this is only every accessed after first being written. So no need
to initialize it.
>
> err = of_property_read_string(rdev->of_node, "label", &domain);
> if (err) {
> @@ -1927,10 +1937,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> if (!data)
> return -ENOMEM;
>
> - err = fastrpc_device_register(rdev, data, domains[domain_id]);
> - if (err) {
> - kfree(data);
> - return err;
> +
> + secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
> + data->secure = secure_dsp;
> +
> + switch (domain_id) {
> + case ADSP_DOMAIN_ID:
> + case MDSP_DOMAIN_ID:
> + case SDSP_DOMAIN_ID:
> + err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
> + if (err)
> + goto fdev_error;
> + break;
> + case CDSP_DOMAIN_ID:
> + /* Create both device nodes so that we can allow both Signed and Unsigned PD */
> + err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> + if (err)
> + goto fdev_error;
> +
> + err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
> + if (err)
> + goto fdev_error;
> + break;
> + default:
> + err = -EINVAL;
> + goto fdev_error;
> }
>
> kref_init(&data->refcount);
> @@ -1943,7 +1974,14 @@ 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 complete for %s with secure flag(%d) return: %d\n",
> + __func__, domains[domain_id], secure_dsp, err);
I would prefer that we don't spam the kernel log with such useful
information, in particular since it will happen every time we start or
restart a remoteproc with fastrpc. So dev_dbg perhaps?
> + return err;
I think that in the event that of_platform_populate() actually failed,
you will return an error here, fastrpc_rpmsg_remove() won't be called,
so you won't release the misc device or release &data->refcount. This
issue exists in the code today though...
Regards,
Bjorn
> +
> +fdev_error:
> + kfree(data);
> + return err;
> }
>
> static void fastrpc_notify_users(struct fastrpc_user *user)
> @@ -1970,6 +2008,9 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
> if (cctx->fdevice)
> misc_deregister(&cctx->fdevice->miscdev);
>
> + if (cctx->secure_fdevice)
> + misc_deregister(&cctx->secure_fdevice->miscdev);
> +
> of_platform_depopulate(&rpdev->dev);
>
> cctx->rpdev = NULL;
> --
> 2.21.0
>
