| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0bdc9250-670d-453a-4474-d240da3894f2@linaro.org>
Date: Thu, 16 Dec 2021 11:27:16 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: robh+dt@...nel.org, gregkh@...uxfoundation.org,
devicetree@...r.kernel.org, ekangupt@....qualcomm.com,
jeyr@...eaurora.org, bkumar@....qualcomm.com,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 6/8] misc: fastrpc: add secure domain support
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@...aro.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
>>
Powered by blists - more mailing lists