[PATCH 0/4] Add secure domains support

[PATCH 0/4] Add secure domains support

[Jeya R]

This patch series adds secure domains support. All DSP domains other
than CDSP are set as secure by default and CDSP is set as secure domain
if fastrpc DT node carries secure domains property. If any process is
getting initialized using non-secure device and the dsp channel is
secure, then the session gets rejected. 

Jeya R (4):
  dt-bindings: devicetree documentation for secure domain
  misc: fastrpc: Add secure device node support
  misc: fastrpc: Set channel as secure
  misc: fastrpc: reject non-secure node for secure domain

 .../devicetree/bindings/misc/qcom,fastrpc.txt      |  6 ++
 drivers/misc/fastrpc.c                             | 64 +++++++++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/4] dt-bindings: devicetree documentation for secure domain

[Jeya R]

Add information about secure domain property.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt b/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt
index 2a1827a..276c1d1 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt
@@ -49,6 +49,12 @@ on the dsp.
 		    context bank. Defaults to 1 when this property
 		    is not specified.
 
+- secure-domains:
+	Usage: Optional
+	Value type: <empty>
+	Definition: Specify DSP domain is secure, must be
+		    "qcom,secure-domain"
+
 Example:
 
 adsp-pil {
-- 
2.7.4

Re: [PATCH 1/4] dt-bindings: devicetree documentation for secure domain

[Srinivas Kandagatla]

Thanks Jeya,


Subject line should look something like "misc: dt-bindings: fastrpc..."


On 24/09/2021 13:19, Jeya R wrote:
> Add information about secure domain property.
> 
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---
>   Documentation/devicetree/bindings/misc/qcom,fastrpc.txt | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt b/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt
> index 2a1827a..276c1d1 100644
> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt
> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.txt
> @@ -49,6 +49,12 @@ on the dsp.
>   		    context bank. Defaults to 1 when this property
>   		    is not specified.
>   
> +- secure-domains:

Should this not be qcom,secure-domain ??

You have added this binding for compute banks but the driver changes 
suggest that it should be for fastrpc.



> +	Usage: Optional
> +	Value type: <empty>

boolean?

> +	Definition: Specify DSP domain is secure, must be
> +		    "qcom,secure-domain"
May be reword this to "Only applicable for CDSP domains, presence of 
this property indicates that domain in secured"



> +
>   Example:
>   
>   adsp-pil {
> 

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

[Jeya R]

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 beda610..07c41a5 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;
 };
 
@@ -1214,10 +1216,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);
+
 	fl = kzalloc(sizeof(*fl), GFP_KERNEL);
 	if (!fl)
 		return -ENOMEM;
@@ -1640,6 +1655,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);
 
@@ -1651,7 +1675,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)
@@ -1676,6 +1704,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;
-- 
2.7.4

[PATCH 3/4] misc: fastrpc: Set channel as secure

[Jeya R]

Set channel as secure based on domain ID and secure domain DT property.
All DSP domains other than CDSP are set as secure by default and for
CDSP domain, secure flag is set if property is added to DT file.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
---
 drivers/misc/fastrpc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 07c41a5..631713d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -216,6 +216,7 @@ struct fastrpc_channel_ctx {
 	struct miscdevice miscdev;
 	struct miscdevice securedev;
 	struct kref refcount;
+	bool secure;
 };
 
 struct fastrpc_user {
@@ -1646,6 +1647,12 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (!data)
 		return -ENOMEM;
 
+	if (domain_id != CDSP_DOMAIN_ID)
+		data->secure = true;
+	else
+		data->secure = of_property_read_bool(rdev->of_node,
+				"qcom,secure-domain");
+
 	data->miscdev.minor = MISC_DYNAMIC_MINOR;
 	data->miscdev.name = devm_kasprintf(rdev, GFP_KERNEL, "fastrpc-%s",
 					    domains[domain_id]);
-- 
2.7.4

Re: [PATCH 3/4] misc: fastrpc: Set channel as secure

[Srinivas Kandagatla]

On 24/09/2021 13:19, Jeya R wrote:
> Set channel as secure based on domain ID and secure domain DT property.
> All DSP domains other than CDSP are set as secure by default and for
> CDSP domain, secure flag is set if property is added to DT file.
> 
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---
>   drivers/misc/fastrpc.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 07c41a5..631713d 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -216,6 +216,7 @@ struct fastrpc_channel_ctx {
>   	struct miscdevice miscdev;
>   	struct miscdevice securedev;
>   	struct kref refcount;
> +	bool secure;
>   };
>   
>   struct fastrpc_user {
> @@ -1646,6 +1647,12 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	if (domain_id != CDSP_DOMAIN_ID)
> +		data->secure = true;
> +	else
> +		data->secure = of_property_read_bool(rdev->of_node,
> +				"qcom,secure-domain");
> +

This whole logic of marking domains secured based on dsp type needs some 
explanation.

--srini


>   	data->miscdev.minor = MISC_DYNAMIC_MINOR;
>   	data->miscdev.name = devm_kasprintf(rdev, GFP_KERNEL, "fastrpc-%s",
>   					    domains[domain_id]);
> 

[PATCH 4/4] misc: fastrpc: reject non-secure node for secure domain

[Jeya R]

Reject session if domain is secure and device non-secure. Also check if
opened device node is proper.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
---
 drivers/misc/fastrpc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 631713d..adf2700 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -235,6 +235,7 @@ struct fastrpc_user {
 	spinlock_t lock;
 	/* lock for allocations */
 	struct mutex mutex;
+	int dev_minor;
 };
 
 static void fastrpc_free_map(struct kref *ref)
@@ -1013,6 +1014,17 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	return err;
 }
 
+static int is_session_rejected(struct fastrpc_user *fl)
+{
+	/* Check if the device node is non-secure and channel is secure*/
+	if ((fl->dev_minor == fl->cctx->miscdev.minor) && fl->cctx->secure) {
+		dev_err(&fl->cctx->rpdev->dev, "Cannot use non-secure device"
+				"node on secure channel\n");
+		return -EACCES;
+	}
+	return 0;
+}
+
 static int fastrpc_init_create_process(struct fastrpc_user *fl,
 					char __user *argp)
 {
@@ -1033,6 +1045,10 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 	} inbuf;
 	u32 sc;
 
+	err = is_session_rejected(fl);
+	if (err)
+		return err;
+
 	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
 	if (!args)
 		return -ENOMEM;
@@ -1221,6 +1237,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	struct fastrpc_user *fl = NULL;
 	struct miscdevice *currdev = NULL;
 	unsigned long flags;
+	int dev_minor = MINOR(inode->i_rdev);
 
 	if (!filp)
 		return -EFAULT;
@@ -1234,6 +1251,12 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	else
 		cctx = miscdev_to_cctx(filp->private_data);
 
+	if (!((dev_minor == cctx->miscdev.minor) ||
+	     (dev_minor == cctx->securedev.minor))) {
+		dev_err(&cctx->rpdev->dev, "Device node is not proper\n");
+		return -EFAULT;
+	}
+
 	fl = kzalloc(sizeof(*fl), GFP_KERNEL);
 	if (!fl)
 		return -ENOMEM;
@@ -1250,6 +1273,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	INIT_LIST_HEAD(&fl->user);
 	fl->tgid = current->tgid;
 	fl->cctx = cctx;
+	fl->dev_minor = dev_minor;
 
 	fl->sctx = fastrpc_session_alloc(cctx);
 	if (!fl->sctx) {
-- 
2.7.4

Re: [PATCH 4/4] misc: fastrpc: reject non-secure node for secure domain

[Srinivas Kandagatla]

On 24/09/2021 13:19, Jeya R wrote:
> Reject session if domain is secure and device non-secure. Also check if
> opened device node is proper.
> 
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---
>   drivers/misc/fastrpc.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 631713d..adf2700 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -235,6 +235,7 @@ struct fastrpc_user {
>   	spinlock_t lock;
>   	/* lock for allocations */
>   	struct mutex mutex;
> +	int dev_minor;
>   };
>   
>   static void fastrpc_free_map(struct kref *ref)
> @@ -1013,6 +1014,17 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>   	return err;
>   }
>   
> +static int is_session_rejected(struct fastrpc_user *fl)
> +{
> +	/* Check if the device node is non-secure and channel is secure*/
> +	if ((fl->dev_minor == fl->cctx->miscdev.minor) && fl->cctx->secure) {
> +		dev_err(&fl->cctx->rpdev->dev, "Cannot use non-secure device"
> +				"node on secure channel\n");

Am bit confused here,

so we create 2 device nodes /dev/fastrpc-adsp, /dev/fastrpc-adsp-secure
and mark any domain other than cdsp as secure
then here we check if access is via /dev/fastrpc-adsp then reject it.

But question is why did we even create /dev/fastrpc-adsp in first place?

Can you explain how is this secure and non secure nodes supposed to work?

> +		return -EACCES;
> +	}
> +	return 0;
> +}
> +
>   static int fastrpc_init_create_process(struct fastrpc_user *fl,
>   					char __user *argp)
>   {
> @@ -1033,6 +1045,10 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>   	} inbuf;
>   	u32 sc;
>   
> +	err = is_session_rejected(fl);
> +	if (err)
> +		return err;
> +
>   	args = kcalloc(FASTRPC_CREATE_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>   	if (!args)
>   		return -ENOMEM;
> @@ -1221,6 +1237,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
>   	struct fastrpc_user *fl = NULL;
>   	struct miscdevice *currdev = NULL;
>   	unsigned long flags;
> +	int dev_minor = MINOR(inode->i_rdev);
>   
>   	if (!filp)
>   		return -EFAULT;
> @@ -1234,6 +1251,12 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
>   	else
>   		cctx = miscdev_to_cctx(filp->private_data);
>   

> +	if (!((dev_minor == cctx->miscdev.minor) ||
> +	     (dev_minor == cctx->securedev.minor))) {
> +		dev_err(&cctx->rpdev->dev, "Device node is not proper\n");
> +		return -EFAULT;
> +	}

This is bit of over checking, how can we enter without accessing proper 
device node?

> +
>   	fl = kzalloc(sizeof(*fl), GFP_KERNEL);
>   	if (!fl)
>   		return -ENOMEM;
> @@ -1250,6 +1273,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
>   	INIT_LIST_HEAD(&fl->user);
>   	fl->tgid = current->tgid;
>   	fl->cctx = cctx;
> +	fl->dev_minor = dev_minor;
>   
>   	fl->sctx = fastrpc_session_alloc(cctx);
>   	if (!fl->sctx) {
> 

Re: [PATCH 0/4] Add secure domains support

[Srinivas Kandagatla]

On 24/09/2021 13:19, Jeya R wrote:
> This patch series adds secure domains support. All DSP domains other
> than CDSP are set as secure by default and CDSP is set as secure domain

This is going to break the existing devices that work with this driver? 
Alteast the non cdsp cases.
like msm8996, sdm845, sm8250 ....

> if fastrpc DT node carries secure domains property. If any process is
> getting initialized using non-secure device and the dsp channel is
> secure, then the session gets rejected.

Could you elaborate on what exactly you meant by secure here?
Is this SE linux policy we are talking about ?

Why can't we deal with this directly on /dev/[adsp|cdsp]-fastrpc nodes, 
why do we need this extra secured node?

--srini

> 
> Jeya R (4):
>    dt-bindings: devicetree documentation for secure domain
>    misc: fastrpc: Add secure device node support
>    misc: fastrpc: Set channel as secure
>    misc: fastrpc: reject non-secure node for secure domain
> 
>   .../devicetree/bindings/misc/qcom,fastrpc.txt      |  6 ++
>   drivers/misc/fastrpc.c                             | 64 +++++++++++++++++++++-
>   2 files changed, 68 insertions(+), 2 deletions(-)
>