Messages in this thread | | | Subject | Re: [PATCH v2 6/8] misc: fastrpc: add secure domain support | From | Srinivas Kandagatla <> | Date | Thu, 16 Dec 2021 11:27:16 +0000 |
| |
On 13/12/2021 18:37, Bjorn Andersson wrote: > 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@linaro.org> >> --- >> 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?
No, we will end up with
fastrpc-cdsp fastrpc-cdsp-secure
based on the qcom,non-secure-domain DT property
so we still have the same old name, as long as the dt-property is correctly set.
> >> 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.
that's true, I can remove this in next spin.
> >> >> 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?
agree, will change. > >> + 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...
Thanks, that is a good point I will see if I can fix that in next spin.
--srini
> > 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 >>
| |