diff mbox series

[v2,6/8] misc: fastrpc: add secure domain support

Message ID 20211209120626.26373-7-srinivas.kandagatla@linaro.org (mailing list archive)
State Superseded
Headers show
Series misc: fastrpc: Add missing DSP FastRPC features | expand

Commit Message

Srinivas Kandagatla Dec. 9, 2021, 12:06 p.m. UTC
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(-)

Comments

Bjorn Andersson Dec. 13, 2021, 6:37 p.m. UTC | #1
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?

>  	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
>
Srinivas Kandagatla Dec. 16, 2021, 11:27 a.m. UTC | #2
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
>>
diff mbox series

Patch

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" : "");
 	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;
 
 	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);
+	return err;
+
+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;