[RESEND PATCH v1 0/2] Privileged process support on remote subsystem

[RESEND PATCH v1 0/2] Privileged process support on remote subsystem

[Ekansh Gupta]

Add support to run remote user process as privileged on remote
subsystem. The privileged user process can be given prioritized
access to remote processor resources. This is achieved in kernel
based on the group ID of the process. The kernel will have a
pre-defined fastrpc group ID and if the process's group ID matches
with it, then the process is treated as a privileged process. This
information is sent to the remote processor during PD initialization
and the PD is treated as a privileged PD.

Ekansh Gupta (2):
  dt-bindings: misc: fastrpc: add fastrpc group IDs property
  misc: fastrpc: detect privileged processes based on group ID

 .../devicetree/bindings/misc/qcom,fastrpc.yaml     |   6 +
 drivers/misc/fastrpc.c                             | 124 +++++++++++++++++++++
 2 files changed, 130 insertions(+)

-- 
2.7.4

[RESEND PATCH v1 1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

[Ekansh Gupta]

Add "qcom,fastrpc-gids" property to the list of optional properties.
This property contains the list of privileged group IDs which is
used to offload process to remote subsystem with increased privileges.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
index 1ab9588..2a2124f 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
@@ -57,6 +57,11 @@ properties:
       Virtual machine IDs for remote processor.
     $ref: "/schemas/types.yaml#/definitions/uint32-array"
 
+  qcom,fastrpc-gids:
+    description:
+      Group IDs for fastrpc.
+    $ref: "/schemas/types.yaml#/definitions/uint32-array"
+
   "#address-cells":
     const: 1
 
@@ -120,6 +125,7 @@ examples:
             qcom,glink-channels = "fastrpcglink-apps-dsp";
             label = "sdsp";
             qcom,non-secure-domain;
+            qcom,fastrpc-gids = <2908>
             #address-cells = <1>;
             #size-cells = <0>;
 
-- 
2.7.4

[RESEND PATCH v1 2/2] misc: fastrpc: detect privileged processes based on group ID

[Ekansh Gupta]

Get the information on privileged group IDs during rpmsg probing based
on DT property. Check if the process requesting an offload to remote
subsystem is privileged by comparing it's group ID with privileged
group ID. Initialization process attributes are updated for a
privileged process which is sent to remote process for resource
management.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 30d4d04..6c7db0b 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -21,6 +21,7 @@
 #include <linux/firmware/qcom/qcom_scm.h>
 #include <uapi/misc/fastrpc.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/cred.h>
 
 #define ADSP_DOMAIN_ID (0)
 #define MDSP_DOMAIN_ID (1)
@@ -166,6 +167,11 @@ struct fastrpc_mem_unmap_req_msg {
 	u64 len;
 };
 
+struct gid_list {
+	u32 *gids;
+	u32 gidcount;
+};
+
 struct fastrpc_msg {
 	int pid;		/* process group id */
 	int tid;		/* thread id */
@@ -277,6 +283,7 @@ struct fastrpc_channel_ctx {
 	struct fastrpc_device *fdevice;
 	struct fastrpc_buf *remote_heap;
 	struct list_head invoke_interrupted_mmaps;
+	struct gid_list gidlist;
 	bool secure;
 	bool unsigned_support;
 	u64 dma_mask;
@@ -305,6 +312,7 @@ struct fastrpc_user {
 	spinlock_t lock;
 	/* lock for allocations */
 	struct mutex mutex;
+	struct gid_list gidlist;
 };
 
 static void fastrpc_free_map(struct kref *ref)
@@ -522,6 +530,31 @@ static void fastrpc_context_put_wq(struct work_struct *work)
 }
 
 #define CMP(aa, bb) ((aa) == (bb) ? 0 : (aa) < (bb) ? -1 : 1)
+
+static u32 sorted_lists_intersection(u32 *listA,
+		u32 lenA, u32 *listB, u32 lenB)
+{
+	u32 i = 0, j = 0;
+
+	while (i < lenA && j < lenB) {
+		if (listA[i] < listB[j])
+			i++;
+		else if (listA[i] > listB[j])
+			j++;
+		else
+			return listA[i];
+	}
+	return 0;
+}
+
+static int uint_cmp_func(const void *p1, const void *p2)
+{
+	u32 a1 = *((u32 *)p1);
+	u32 a2 = *((u32 *)p2);
+
+	return CMP(a1, a2);
+}
+
 static int olaps_cmp(const void *a, const void *b)
 {
 	struct fastrpc_buf_overlap *pa = (struct fastrpc_buf_overlap *)a;
@@ -1230,6 +1263,50 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
 	return false;
 }
 
+static int fastrpc_get_process_gids(struct gid_list *gidlist)
+{
+	struct group_info *group_info = get_current_groups();
+	int i, num_gids;
+	u32 *gids = NULL;
+
+	if (!group_info)
+		return -EFAULT;
+
+	num_gids = group_info->ngroups + 1;
+	gids = kcalloc(num_gids, sizeof(u32), GFP_KERNEL);
+	if (!gids)
+		return -ENOMEM;
+
+	/* Get the real GID */
+	gids[0] = __kgid_val(current_gid());
+
+	/* Get the supplemental GIDs */
+	for (i = 1; i < num_gids; i++)
+		gids[i] = __kgid_val(group_info->gid[i - 1]);
+
+	sort(gids, num_gids, sizeof(*gids), uint_cmp_func, NULL);
+	gidlist->gids = gids;
+	gidlist->gidcount = num_gids;
+
+	return 0;
+}
+
+static void fastrpc_check_privileged_process(struct fastrpc_user *fl,
+				struct fastrpc_init_create *init)
+{
+	u32 gid = sorted_lists_intersection(fl->gidlist.gids,
+			fl->gidlist.gidcount, fl->cctx->gidlist.gids,
+			fl->cctx->gidlist.gidcount);
+
+	/* disregard any privilege bits from userspace */
+	init->attrs &= (~FASTRPC_MODE_PRIVILEGED);
+	if (gid) {
+		dev_info(&fl->cctx->rpdev->dev, "%s: %s (PID %d, GID %u) is a privileged process\n",
+				__func__, current->comm, fl->tgid, gid);
+		init->attrs |= FASTRPC_MODE_PRIVILEGED;
+	}
+}
+
 static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 					      char __user *argp)
 {
@@ -1386,6 +1463,8 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
+	fastrpc_get_process_gids(&fl->gidlist);
+
 	inbuf.pgid = fl->tgid;
 	inbuf.namelen = strlen(current->comm) + 1;
 	inbuf.filelen = init.filelen;
@@ -1400,6 +1479,8 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 			goto err;
 	}
 
+	fastrpc_check_privileged_process(fl, &init);
+
 	memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
 		       1024 * 1024);
 	err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
@@ -1519,6 +1600,7 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	spin_lock_irqsave(&cctx->lock, flags);
 	list_del(&fl->user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
+	kfree(fl->gidlist.gids);
 
 	if (fl->init_mem)
 		fastrpc_buf_free(fl->init_mem);
@@ -2118,6 +2200,43 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 	return err;
 }
 
+static int fastrpc_init_privileged_gids(struct device *dev, char *prop_name,
+						struct gid_list *gidlist)
+{
+	int err = 0;
+	u32 len = 0, i;
+	u32 *gids = NULL;
+
+	if (!of_find_property(dev->of_node, prop_name, &len))
+		return 0;
+	if (len == 0)
+		return 0;
+
+	len /= sizeof(u32);
+	gids = kcalloc(len, sizeof(u32), GFP_KERNEL);
+	if (!gids)
+		return -ENOMEM;
+
+	for (i = 0; i < len; i++) {
+		err = of_property_read_u32_index(dev->of_node, prop_name,
+								i, &gids[i]);
+		if (err) {
+			dev_err(dev, "%s: failed to read GID %u\n",
+					__func__, i);
+			goto read_error;
+		}
+		dev_info(dev, "adsprpc: %s: privileged GID: %u\n", __func__, gids[i]);
+	}
+	sort(gids, len, sizeof(*gids), uint_cmp_func, NULL);
+	gidlist->gids = gids;
+	gidlist->gidcount = len;
+
+	return 0;
+read_error:
+	kfree(gids);
+	return err;
+}
+
 static const struct file_operations fastrpc_fops = {
 	.open = fastrpc_device_open,
 	.release = fastrpc_device_release,
@@ -2277,6 +2396,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (!data)
 		return -ENOMEM;
 
+	err = fastrpc_init_privileged_gids(rdev, "qcom,fastrpc-gids", &data->gidlist);
+	if (err)
+		dev_err(rdev, "Privileged gids init failed.\n");
+
 	if (vmcount) {
 		data->vmcount = vmcount;
 		data->perms = BIT(QCOM_SCM_VMID_HLOS);
@@ -2382,6 +2505,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	if (cctx->remote_heap)
 		fastrpc_buf_free(cctx->remote_heap);
 
+	kfree(cctx->gidlist.gids);
 	of_platform_depopulate(&rpdev->dev);
 
 	fastrpc_channel_ctx_put(cctx);
-- 
2.7.4

Re: [RESEND PATCH v1 1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

[Rob Herring]

On Wed, 07 Jun 2023 22:00:06 +0530, Ekansh Gupta wrote:
> Add "qcom,fastrpc-gids" property to the list of optional properties.
> This property contains the list of privileged group IDs which is
> used to offload process to remote subsystem with increased privileges.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dts:36.17-18 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1686155407-20054-2-git-send-email-quic_ekangupt@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RESEND PATCH v1 1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

[Krzysztof Kozlowski]

On 07/06/2023 18:30, Ekansh Gupta wrote:
> Add "qcom,fastrpc-gids" property to the list of optional properties.
> This property contains the list of privileged group IDs which is
> used to offload process to remote subsystem with increased privileges.

Why or when anyone would use this property?

> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> index 1ab9588..2a2124f 100644
> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> @@ -57,6 +57,11 @@ properties:
>        Virtual machine IDs for remote processor.
>      $ref: "/schemas/types.yaml#/definitions/uint32-array"
>  
> +  qcom,fastrpc-gids:
> +    description:
> +      Group IDs for fastrpc.

You just pasted here property name. It does not help me to understand
what's this. Explain in description.

> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"

Drop quotes.

missing min/maxItems.

> +
>    "#address-cells":
>      const: 1
>  
> @@ -120,6 +125,7 @@ examples:
>              qcom,glink-channels = "fastrpcglink-apps-dsp";
>              label = "sdsp";
>              qcom,non-secure-domain;
> +            qcom,fastrpc-gids = <2908>

Eh, what does 2908 stands for? Why not 3012 or 1842? How anyone can
figure this out?

Best regards,
Krzysztof

Re: [RESEND PATCH v1 2/2] misc: fastrpc: detect privileged processes based on group ID

[Krzysztof Kozlowski]

On 07/06/2023 18:30, Ekansh Gupta wrote:
> Get the information on privileged group IDs during rpmsg probing based
> on DT property. Check if the process requesting an offload to remote
> subsystem is privileged by comparing it's group ID with privileged
> group ID. Initialization process attributes are updated for a
> privileged process which is sent to remote process for resource
> management.
> 



> +
>  static const struct file_operations fastrpc_fops = {
>  	.open = fastrpc_device_open,
>  	.release = fastrpc_device_release,
> @@ -2277,6 +2396,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	err = fastrpc_init_privileged_gids(rdev, "qcom,fastrpc-gids", &data->gidlist);
> +	if (err)
> +		dev_err(rdev, "Privileged gids init failed.\n");
> +

What about error paths? No need for cleanup?

Best regards,
Krzysztof

Re: [RESEND PATCH v1 0/2] Privileged process support on remote subsystem

[Greg KH]

On Wed, Jun 07, 2023 at 10:00:05PM +0530, Ekansh Gupta wrote:
> Add support to run remote user process as privileged on remote
> subsystem. The privileged user process can be given prioritized
> access to remote processor resources. This is achieved in kernel
> based on the group ID of the process. The kernel will have a
> pre-defined fastrpc group ID and if the process's group ID matches
> with it, then the process is treated as a privileged process. This
> information is sent to the remote processor during PD initialization
> and the PD is treated as a privileged PD.
> 
> Ekansh Gupta (2):
>   dt-bindings: misc: fastrpc: add fastrpc group IDs property
>   misc: fastrpc: detect privileged processes based on group ID
> 
>  .../devicetree/bindings/misc/qcom,fastrpc.yaml     |   6 +
>  drivers/misc/fastrpc.c                             | 124 +++++++++++++++++++++
>  2 files changed, 130 insertions(+)
> 
> -- 
> 2.7.4
> 

Why is this a RESEND?

thanks,

greg k-h

Re: [RESEND PATCH v1 1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

[Ekansh Gupta]

On 6/7/2023 10:51 PM, Rob Herring wrote:
> 
> On Wed, 07 Jun 2023 22:00:06 +0530, Ekansh Gupta wrote:
>> Add "qcom,fastrpc-gids" property to the list of optional properties.
>> This property contains the list of privileged group IDs which is
>> used to offload process to remote subsystem with increased privileges.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dts:36.17-18 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1512: dt_binding_check] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1686155407-20054-2-git-send-email-quic_ekangupt@quicinc.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 
Thanks for helping with this information. I'll check and re-submit the 
patch.

Re: [RESEND PATCH v1 0/2] Privileged process support on remote subsystem

[Ekansh Gupta]

On 6/8/2023 12:17 AM, Greg KH wrote:
> On Wed, Jun 07, 2023 at 10:00:05PM +0530, Ekansh Gupta wrote:
>> Add support to run remote user process as privileged on remote
>> subsystem. The privileged user process can be given prioritized
>> access to remote processor resources. This is achieved in kernel
>> based on the group ID of the process. The kernel will have a
>> pre-defined fastrpc group ID and if the process's group ID matches
>> with it, then the process is treated as a privileged process. This
>> information is sent to the remote processor during PD initialization
>> and the PD is treated as a privileged PD.
>>
>> Ekansh Gupta (2):
>>    dt-bindings: misc: fastrpc: add fastrpc group IDs property
>>    misc: fastrpc: detect privileged processes based on group ID
>>
>>   .../devicetree/bindings/misc/qcom,fastrpc.yaml     |   6 +
>>   drivers/misc/fastrpc.c                             | 124 +++++++++++++++++++++
>>   2 files changed, 130 insertions(+)
>>
>> -- 
>> 2.7.4
>>
> 
> Why is this a RESEND?
> 
I missed sending the patches to some necessary people and lists. Hence, 
resent the patch series.

Thanks,
Ekansh
> thanks,
> 
> greg k-h

Re: [RESEND PATCH v1 0/2] Privileged process support on remote subsystem

[Greg KH]

On Thu, Jun 08, 2023 at 03:23:06PM +0530, Ekansh Gupta wrote:
> 
> 
> On 6/8/2023 12:17 AM, Greg KH wrote:
> > On Wed, Jun 07, 2023 at 10:00:05PM +0530, Ekansh Gupta wrote:
> > > Add support to run remote user process as privileged on remote
> > > subsystem. The privileged user process can be given prioritized
> > > access to remote processor resources. This is achieved in kernel
> > > based on the group ID of the process. The kernel will have a
> > > pre-defined fastrpc group ID and if the process's group ID matches
> > > with it, then the process is treated as a privileged process. This
> > > information is sent to the remote processor during PD initialization
> > > and the PD is treated as a privileged PD.
> > > 
> > > Ekansh Gupta (2):
> > >    dt-bindings: misc: fastrpc: add fastrpc group IDs property
> > >    misc: fastrpc: detect privileged processes based on group ID
> > > 
> > >   .../devicetree/bindings/misc/qcom,fastrpc.yaml     |   6 +
> > >   drivers/misc/fastrpc.c                             | 124 +++++++++++++++++++++
> > >   2 files changed, 130 insertions(+)
> > > 
> > > -- 
> > > 2.7.4
> > > 
> > 
> > Why is this a RESEND?
> > 
> I missed sending the patches to some necessary people and lists. Hence,
> resent the patch series.

Please always say so when doing a resend as I think the documentation
asks you to, right?

thanks,

greg k-h

Re: [RESEND PATCH v1 1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

[Ekansh Gupta]

On 6/8/2023 12:14 AM, Krzysztof Kozlowski wrote:
> On 07/06/2023 18:30, Ekansh Gupta wrote:
>> Add "qcom,fastrpc-gids" property to the list of optional properties.
>> This property contains the list of privileged group IDs which is
>> used to offload process to remote subsystem with increased privileges.
> 
> Why or when anyone would use this property?
> 
This property can be used for the use cases which requires prioritized 
access to remote subsystem resources.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> index 1ab9588..2a2124f 100644
>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> @@ -57,6 +57,11 @@ properties:
>>         Virtual machine IDs for remote processor.
>>       $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>   
>> +  qcom,fastrpc-gids:
>> +    description:
>> +      Group IDs for fastrpc.
> 
> You just pasted here property name. It does not help me to understand
> what's this. Explain in description.
> 
I'll add more details here when I re-submit the patch.
>> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> 
> Drop quotes.
> 
> missing min/maxItems.
> 
I'll update this in v2. Thanks for reviewing.
>> +
>>     "#address-cells":
>>       const: 1
>>   
>> @@ -120,6 +125,7 @@ examples:
>>               qcom,glink-channels = "fastrpcglink-apps-dsp";
>>               label = "sdsp";
>>               qcom,non-secure-domain;
>> +            qcom,fastrpc-gids = <2908>
> 
> Eh, what does 2908 stands for? Why not 3012 or 1842? How anyone can
> figure this out?
> 
There is no hard restriction for this value to be 2908, it can be 
anything. Just that the process which needs a privileged offload to any 
remote subsystem should have the same gid which is defined here, whether 
it is 2908 or anything else.

The intention of having 2908 in example is because this is used in 
Android world where any process running with 2908 gid can offload as 
privileged process to remote subsystem.
Ref: 
https://review.lineageos.org/c/LineageOS/android_device_motorola_sm7250-common/+/305612/4/config.fs

But, it can be set to anything just that the process also needs to adapt 
to this gid.

Thanks,
Ekansh
> Best regards,
> Krzysztof
> 

Re: [RESEND PATCH v1 2/2] misc: fastrpc: detect privileged processes based on group ID

[Ekansh Gupta]

On 6/8/2023 12:17 AM, Krzysztof Kozlowski wrote:
> On 07/06/2023 18:30, Ekansh Gupta wrote:
>> Get the information on privileged group IDs during rpmsg probing based
>> on DT property. Check if the process requesting an offload to remote
>> subsystem is privileged by comparing it's group ID with privileged
>> group ID. Initialization process attributes are updated for a
>> privileged process which is sent to remote process for resource
>> management.
>>
> 
> 
> 
>> +
>>   static const struct file_operations fastrpc_fops = {
>>   	.open = fastrpc_device_open,
>>   	.release = fastrpc_device_release,
>> @@ -2277,6 +2396,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   	if (!data)
>>   		return -ENOMEM;
>>   
>> +	err = fastrpc_init_privileged_gids(rdev, "qcom,fastrpc-gids", &data->gidlist);
>> +	if (err)
>> +		dev_err(rdev, "Privileged gids init failed.\n");
>> +
> 
> What about error paths? No need for cleanup?
> 
All the necessary clean-up is added as part of 
fastrpc_init_privileged_gids error path. There is no requirement to have 
any additional handling in error path other that error log. Also there 
is no intention to fail the probe in case gid information is not 
properly read.

Thanks,
Ekansh
> Best regards,
> Krzysztof
> 

Re: [RESEND PATCH v1 0/2] Privileged process support on remote subsystem

[Ekansh Gupta]

On 6/8/2023 3:46 PM, Greg KH wrote:
> On Thu, Jun 08, 2023 at 03:23:06PM +0530, Ekansh Gupta wrote:
>>
>>
>> On 6/8/2023 12:17 AM, Greg KH wrote:
>>> On Wed, Jun 07, 2023 at 10:00:05PM +0530, Ekansh Gupta wrote:
>>>> Add support to run remote user process as privileged on remote
>>>> subsystem. The privileged user process can be given prioritized
>>>> access to remote processor resources. This is achieved in kernel
>>>> based on the group ID of the process. The kernel will have a
>>>> pre-defined fastrpc group ID and if the process's group ID matches
>>>> with it, then the process is treated as a privileged process. This
>>>> information is sent to the remote processor during PD initialization
>>>> and the PD is treated as a privileged PD.
>>>>
>>>> Ekansh Gupta (2):
>>>>     dt-bindings: misc: fastrpc: add fastrpc group IDs property
>>>>     misc: fastrpc: detect privileged processes based on group ID
>>>>
>>>>    .../devicetree/bindings/misc/qcom,fastrpc.yaml     |   6 +
>>>>    drivers/misc/fastrpc.c                             | 124 +++++++++++++++++++++
>>>>    2 files changed, 130 insertions(+)
>>>>
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>> Why is this a RESEND?
>>>
>> I missed sending the patches to some necessary people and lists. Hence,
>> resent the patch series.
> 
> Please always say so when doing a resend as I think the documentation
> asks you to, right?
> 
This was the first time for me to resend a patch and in documentation I 
just saw "RESEND" needs to be added to the subject. Apologies for 
missing this. I'm still learning and going forwards, for re-sending also 
I'll follow the etiquette followed when resubmitting a patch with 
changes. Thanks for the suggestion! :)
> thanks,
> 
> greg k-h

Re: [RESEND PATCH v1 1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

[Krzysztof Kozlowski]

On 08/06/2023 12:36, Ekansh Gupta wrote:
> 
> 
> On 6/8/2023 12:14 AM, Krzysztof Kozlowski wrote:
>> On 07/06/2023 18:30, Ekansh Gupta wrote:
>>> Add "qcom,fastrpc-gids" property to the list of optional properties.
>>> This property contains the list of privileged group IDs which is
>>> used to offload process to remote subsystem with increased privileges.
>>
>> Why or when anyone would use this property?
>>
> This property can be used for the use cases which requires prioritized 
> access to remote subsystem resources.

This does not help me much yet.... Which systems or boards need
prioritized access?

>>>
>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>> ---
>>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> index 1ab9588..2a2124f 100644
>>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> @@ -57,6 +57,11 @@ properties:
>>>         Virtual machine IDs for remote processor.
>>>       $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>>   
>>> +  qcom,fastrpc-gids:
>>> +    description:
>>> +      Group IDs for fastrpc.
>>
>> You just pasted here property name. It does not help me to understand
>> what's this. Explain in description.
>>
> I'll add more details here when I re-submit the patch.
>>> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>
>> Drop quotes.
>>
>> missing min/maxItems.
>>
> I'll update this in v2. Thanks for reviewing.
>>> +
>>>     "#address-cells":
>>>       const: 1
>>>   
>>> @@ -120,6 +125,7 @@ examples:
>>>               qcom,glink-channels = "fastrpcglink-apps-dsp";
>>>               label = "sdsp";
>>>               qcom,non-secure-domain;
>>> +            qcom,fastrpc-gids = <2908>
>>
>> Eh, what does 2908 stands for? Why not 3012 or 1842? How anyone can
>> figure this out?
>>
> There is no hard restriction for this value to be 2908, it can be 
> anything. Just that the process which needs a privileged offload to any 
> remote subsystem should have the same gid which is defined here, whether 
> it is 2908 or anything else.
> 
> The intention of having 2908 in example is because this is used in 
> Android world where any process running with 2908 gid can offload as 
> privileged process to remote subsystem.
> Ref: 
> https://review.lineageos.org/c/LineageOS/android_device_motorola_sm7250-common/+/305612/4/config.fs
> 
> But, it can be set to anything just that the process also needs to adapt 
> to this gid.

So this is process ID? Group ID? GID? Why would you ever encode group
IDs, which change in every installation, or process IDs, which change
every boot, in DTS common for all devices?

This is not a DT property.

Best regards,
Krzysztof

Re: [RESEND PATCH v1 2/2] misc: fastrpc: detect privileged processes based on group ID

[Krzysztof Kozlowski]

On 08/06/2023 12:44, Ekansh Gupta wrote:
> 
> 
> On 6/8/2023 12:17 AM, Krzysztof Kozlowski wrote:
>> On 07/06/2023 18:30, Ekansh Gupta wrote:
>>> Get the information on privileged group IDs during rpmsg probing based
>>> on DT property. Check if the process requesting an offload to remote
>>> subsystem is privileged by comparing it's group ID with privileged
>>> group ID. Initialization process attributes are updated for a
>>> privileged process which is sent to remote process for resource
>>> management.
>>>
>>
>>
>>
>>> +
>>>   static const struct file_operations fastrpc_fops = {
>>>   	.open = fastrpc_device_open,
>>>   	.release = fastrpc_device_release,
>>> @@ -2277,6 +2396,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>   	if (!data)
>>>   		return -ENOMEM;
>>>   
>>> +	err = fastrpc_init_privileged_gids(rdev, "qcom,fastrpc-gids", &data->gidlist);
>>> +	if (err)
>>> +		dev_err(rdev, "Privileged gids init failed.\n");
>>> +
>>
>> What about error paths? No need for cleanup?
>>
> All the necessary clean-up is added as part of 
> fastrpc_init_privileged_gids error path. There is no requirement to have 

Where? How that code is called after fastrpc_device_register() failure?
Or after of_platform_populate() failure?

Please show me the code flow.

> any additional handling in error path other that error log. Also there 
> is no intention to fail the probe in case gid information is not 
> properly read.

This is not related. I don't talk about fastrpc_init_privileged_gids()
failures. Look where did I leave my comment.

Review comments are placed in proper places, not in unrelated parts of
code. The placement of review comment is important as this is the
context of bug in your patch.

Best regards,
Krzysztof