[PATCH v5 0/7] Add missing fixes to FastRPC driver

[PATCH v5 0/7] Add missing fixes to FastRPC driver

[Ekansh Gupta]

This patch series adds the listed bug fixes that have been missing
in upstream fastRPC driver.
- Fix DSP capabilities request.
- Fix issues in audio daemon attach operation.
- Restrict untrusted app to attach to privilegeded PD.

Changes in v2:
- Added separate patch to add newlines in dev_err.
- Added a bug fix in fastrpc capability function.
- Added a new patch to save and restore interrupted context.
- Fixed config dependency for PDR support.

Changes in v3:
- Dropped interrupted context patch.
- Splitted few of the bug fix patches.
- Added Fixes tag wherever applicable.
- Updated proper commit message for few of the patches.

Changes in v4:
- Dropped untrusted process and system unsigned PD patches.
- Updated proper commit message for few of the patches.
- Splitted patches in more meaningful way.
- Added helped functions for fastrpc_req_mmap.

Changes in v5:
- Dropped PDR patch. It will be shared in a separate patch series.
- Dropped fastrpc_req_mmap and remote_heap specific changes from this
  series. These patches will be shared separately as a new patch series.
- Changed patch series subject as this series is no longer carrying any
  new feature changes.

Ekansh Gupta (7):
  misc: fastrpc: Add missing dev_err newlines
  misc: fastrpc: Fix DSP capabilities request
  misc: fastrpc: Copy the complete capability structure to user
  misc: fastrpc: Avoid updating PD type for capability request
  misc: fastrpc: Fix memory leak in audio daemon attach operation
  misc: fastrpc: Fix ownership reassignment of remote heap
  misc: fastrpc: Restrict untrusted app to attach to privileged PD

 drivers/misc/fastrpc.c      | 51 +++++++++++++++++++++++++++----------
 include/uapi/misc/fastrpc.h |  3 +++
 2 files changed, 40 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH v5 1/7] misc: fastrpc: Add missing dev_err newlines

[Ekansh Gupta]

Few dev_err calls are missing newlines. This can result in unrelated
lines getting appended which might make logs difficult to understand.
Add trailing newlines to avoid this.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/misc/fastrpc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4c67e2c5a82e..4028cb96bcf2 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -325,7 +325,7 @@ static void fastrpc_free_map(struct kref *ref)
 			err = qcom_scm_assign_mem(map->phys, map->size,
 				&src_perms, &perm, 1);
 			if (err) {
-				dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
+				dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
 						map->phys, map->size, err);
 				return;
 			}
@@ -816,7 +816,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
 		map->attr = attr;
 		err = qcom_scm_assign_mem(map->phys, (u64)map->size, &src_perms, dst_perms, 2);
 		if (err) {
-			dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
+			dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
 					map->phys, map->size, err);
 			goto map_err;
 		}
@@ -1222,7 +1222,7 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques
 		 * that does not support unsigned PD offload
 		 */
 		if (!fl->cctx->unsigned_support || !unsigned_pd_request) {
-			dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD");
+			dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
 			return true;
 		}
 	}
@@ -1285,7 +1285,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 							&src_perms,
 							fl->cctx->vmperms, fl->cctx->vmcount);
 			if (err) {
-				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
+				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n",
 					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
 				goto err_map;
 			}
@@ -1337,7 +1337,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 						(u64)fl->cctx->remote_heap->size,
 						&src_perms, &dst_perms, 1);
 		if (err)
-			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
+			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n",
 				fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
 	}
 err_map:
-- 
2.43.0

[PATCH v5 2/7] misc: fastrpc: Fix DSP capabilities request

[Ekansh Gupta]

The DSP capability request call expects 2 arguments. First is the
information about the total number of attributes to be copied from
DSP and second is the information about the buffer where the DSP
needs to copy the information. The current design is passing the
information about the size to be copied from DSP which would be
considered as a bad argument to the call by DSP causing a failure
suggesting the same. The second argument carries the information
about the buffer where the DSP needs to copy the capability
information and the size to be copied. As the first entry of
capability attribute is getting skipped, same should also be
considered while sending the information to DSP. Add changes to
pass proper arguments to DSP.

Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/misc/fastrpc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4028cb96bcf2..0c5bba1d355e 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1693,14 +1693,19 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
 {
 	struct fastrpc_invoke_args args[2] = { 0 };
 
-	/* Capability filled in userspace */
+	/*
+	 * Capability filled in userspace. This carries the information
+	 * about the remoteproc support which is fetched from the remoteproc
+	 * sysfs node by userspace.
+	 */
 	dsp_attr_buf[0] = 0;
+	dsp_attr_buf_len -= 1;
 
 	args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len;
 	args[0].length = sizeof(dsp_attr_buf_len);
 	args[0].fd = -1;
 	args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
-	args[1].length = dsp_attr_buf_len;
+	args[1].length = dsp_attr_buf_len * sizeof(u32);
 	args[1].fd = -1;
 	fl->pd = USER_PD;
 
@@ -1730,7 +1735,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
 	if (!dsp_attributes)
 		return -ENOMEM;
 
-	err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
+	err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
 	if (err == DSP_UNSUPPORTED_API) {
 		dev_info(&cctx->rpdev->dev,
 			 "Warning: DSP capabilities not supported on domain: %d\n", domain);
-- 
2.43.0

[PATCH v5 3/7] misc: fastrpc: Copy the complete capability structure to user

[Ekansh Gupta]

User is passing capability ioctl structure(argp) to get DSP
capabilities. This argp is copied to a local structure to get domain
and attribute_id information. After getting the capability, only
capability value is getting copied to user argp which will not be
useful if the use is trying to get the capability by checking the
capability member of fastrpc_ioctl_capability structure. Copy the
complete capability structure so that user can get the capability
value from the expected member of the structure.

Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/misc/fastrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 0c5bba1d355e..c033865d8059 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1788,7 +1788,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
 	if (err)
 		return err;
 
-	if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
+	if (copy_to_user(argp, &cap, sizeof(cap)))
 		return -EFAULT;
 
 	return 0;
-- 
2.43.0

[PATCH v5 4/7] misc: fastrpc: Avoid updating PD type for capability request

[Ekansh Gupta]

When user is requesting for DSP capability, the process pd type is
getting updated to USER_PD which is incorrect as DSP will assume the
process which is making the request is a user PD and this will never
get updated back to the original value. The actual PD type should not
be updated for capability request and it should be serviced by the
respective PD on DSP side. Don't change process's PD type for DSP
capability request.

Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/misc/fastrpc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index c033865d8059..96c19f4919fe 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1707,7 +1707,6 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
 	args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
 	args[1].length = dsp_attr_buf_len * sizeof(u32);
 	args[1].fd = -1;
-	fl->pd = USER_PD;
 
 	return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
 				       FASTRPC_SCALARS(0, 1, 1), args);
-- 
2.43.0

Re: [PATCH v5 4/7] misc: fastrpc: Avoid updating PD type for capability request

[Dmitry Baryshkov]

On Tue, Jun 11, 2024 at 04:04:37PM +0530, Ekansh Gupta wrote:
> When user is requesting for DSP capability, the process pd type is
> getting updated to USER_PD which is incorrect as DSP will assume the
> process which is making the request is a user PD and this will never
> get updated back to the original value. The actual PD type should not
> be updated for capability request and it should be serviced by the
> respective PD on DSP side. Don't change process's PD type for DSP
> capability request.
> 
> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/misc/fastrpc.c | 1 -
>  1 file changed, 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH v5 5/7] misc: fastrpc: Fix memory leak in audio daemon attach operation

[Ekansh Gupta]

Audio PD daemon send the name as part of the init IOCTL call. This
name needs to be copied to kernel for which memory is allocated.
This memory is never freed which might result in memory leak. Free
the memory when it is not needed.

Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/misc/fastrpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 96c19f4919fe..1ba85c70e3ff 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1320,6 +1320,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		goto err_invoke;
 
 	kfree(args);
+	kfree(name);
 
 	return 0;
 err_invoke:
-- 
2.43.0

[PATCH v5 6/7] misc: fastrpc: Fix ownership reassignment of remote heap

[Ekansh Gupta]

Audio PD daemon will allocate memory for audio PD dynamic loading
usage when it is attaching for the first time to audio PD. As
part of this, the memory ownership is moved to the VM where
audio PD can use it. In case daemon process is killed without any
impact to DSP audio PD, the daemon process will retry to attach to
audio PD and in this case memory won't be reallocated. If the invoke
fails due to any reason, as part of err_invoke, the memory ownership
is getting reassigned to HLOS even when the memory was not allocated.
At this time the audio PD might still be using the memory and an
attemp of ownership reassignment would result in memory issue.

Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 1ba85c70e3ff..24dc1cba40e9 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1238,6 +1238,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	struct fastrpc_phy_page pages[1];
 	char *name;
 	int err;
+	bool scm_done = false;
 	struct {
 		int pgid;
 		u32 namelen;
@@ -1289,6 +1290,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
 				goto err_map;
 			}
+			scm_done = true;
 		}
 	}
 
@@ -1324,7 +1326,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 	return 0;
 err_invoke:
-	if (fl->cctx->vmcount) {
+	if (fl->cctx->vmcount && scm_done) {
 		u64 src_perms = 0;
 		struct qcom_scm_vmperm dst_perms;
 		u32 i;
-- 
2.43.0

Re: [PATCH v5 6/7] misc: fastrpc: Fix ownership reassignment of remote heap

[Dmitry Baryshkov]

On Tue, Jun 11, 2024 at 04:04:39PM +0530, Ekansh Gupta wrote:
> Audio PD daemon will allocate memory for audio PD dynamic loading

What is Audio PD daemon? Is it something running on the CPU or on the
DSP? Is it adsprpcd or some other daemon?

> usage when it is attaching for the first time to audio PD. As
> part of this, the memory ownership is moved to the VM where

Which VM?

> audio PD can use it. In case daemon process is killed without any
> impact to DSP audio PD, the daemon process will retry to attach to
> audio PD and in this case memory won't be reallocated. If the invoke
> fails due to any reason, as part of err_invoke, the memory ownership
> is getting reassigned to HLOS even when the memory was not allocated.
> At this time the audio PD might still be using the memory and an
> attemp of ownership reassignment would result in memory issue.

What kind of 'memory issues'? Is it even possible to reclaim the memory
back?

> 
> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 1ba85c70e3ff..24dc1cba40e9 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1238,6 +1238,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  	struct fastrpc_phy_page pages[1];
>  	char *name;
>  	int err;
> +	bool scm_done = false;
>  	struct {
>  		int pgid;
>  		u32 namelen;
> @@ -1289,6 +1290,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
>  				goto err_map;
>  			}
> +			scm_done = true;
>  		}
>  	}
>  
> @@ -1324,7 +1326,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  
>  	return 0;
>  err_invoke:
> -	if (fl->cctx->vmcount) {
> +	if (fl->cctx->vmcount && scm_done) {
>  		u64 src_perms = 0;
>  		struct qcom_scm_vmperm dst_perms;
>  		u32 i;
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v5 6/7] misc: fastrpc: Fix ownership reassignment of remote heap

[Ekansh Gupta]

On 6/11/2024 5:29 PM, Dmitry Baryshkov wrote:
> On Tue, Jun 11, 2024 at 04:04:39PM +0530, Ekansh Gupta wrote:
>> Audio PD daemon will allocate memory for audio PD dynamic loading
> What is Audio PD daemon? Is it something running on the CPU or on the
> DSP? Is it adsprpcd or some other daemon?
It's adsprpcd which is going to attach to Audio PD.
>
>> usage when it is attaching for the first time to audio PD. As
>> part of this, the memory ownership is moved to the VM where
> Which VM?
In audio PD case, it's the following VMIDs:
QCOM_SCM_VMID_LPASS
QCOM_SCM_VMID_ADSP_HEAP

Defined here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/firmware/qcom,scm.h?h=v6.10-rc3

These are expected to be added to fastrpc DT node as "qcom,vmids"

>
>> audio PD can use it. In case daemon process is killed without any
>> impact to DSP audio PD, the daemon process will retry to attach to
>> audio PD and in this case memory won't be reallocated. If the invoke
>> fails due to any reason, as part of err_invoke, the memory ownership
>> is getting reassigned to HLOS even when the memory was not allocated.
>> At this time the audio PD might still be using the memory and an
>> attemp of ownership reassignment would result in memory issue.
> What kind of 'memory issues'? Is it even possible to reclaim the memory
> back?
In case when audio PD on DSP is still using the memory, the ownership should not be
moved to HLOS. This might happen in daemon kill scenario where remote_heap is not
allocated again, but if due to any reason if the fastrpc_internal_invoke fails, that might
result in the ownership change of remote_heap memory.
>
>> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 1ba85c70e3ff..24dc1cba40e9 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1238,6 +1238,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  	struct fastrpc_phy_page pages[1];
>>  	char *name;
>>  	int err;
>> +	bool scm_done = false;
>>  	struct {
>>  		int pgid;
>>  		u32 namelen;
>> @@ -1289,6 +1290,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
>>  				goto err_map;
>>  			}
>> +			scm_done = true;
>>  		}
>>  	}
>>  
>> @@ -1324,7 +1326,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>  
>>  	return 0;
>>  err_invoke:
>> -	if (fl->cctx->vmcount) {
>> +	if (fl->cctx->vmcount && scm_done) {
>>  		u64 src_perms = 0;
>>  		struct qcom_scm_vmperm dst_perms;
>>  		u32 i;
>> -- 
>> 2.43.0
>>

Re: [PATCH v5 6/7] misc: fastrpc: Fix ownership reassignment of remote heap

[Dmitry Baryshkov]

On Wed, Jun 12, 2024 at 09:54:47AM +0530, Ekansh Gupta wrote:
> 
> 
> On 6/11/2024 5:29 PM, Dmitry Baryshkov wrote:
> > On Tue, Jun 11, 2024 at 04:04:39PM +0530, Ekansh Gupta wrote:
> >> Audio PD daemon will allocate memory for audio PD dynamic loading
> > What is Audio PD daemon? Is it something running on the CPU or on the
> > DSP? Is it adsprpcd or some other daemon?
> It's adsprpcd which is going to attach to Audio PD.

Ack

> >
> >> usage when it is attaching for the first time to audio PD. As
> >> part of this, the memory ownership is moved to the VM where
> > Which VM?
> In audio PD case, it's the following VMIDs:
> QCOM_SCM_VMID_LPASS
> QCOM_SCM_VMID_ADSP_HEAP
> 
> Defined here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/firmware/qcom,scm.h?h=v6.10-rc3
> 
> These are expected to be added to fastrpc DT node as "qcom,vmids"

Ok, good.

> 
> >
> >> audio PD can use it. In case daemon process is killed without any
> >> impact to DSP audio PD, the daemon process will retry to attach to
> >> audio PD and in this case memory won't be reallocated. If the invoke
> >> fails due to any reason, as part of err_invoke, the memory ownership
> >> is getting reassigned to HLOS even when the memory was not allocated.
> >> At this time the audio PD might still be using the memory and an
> >> attemp of ownership reassignment would result in memory issue.
> > What kind of 'memory issues'? Is it even possible to reclaim the memory
> > back?
> In case when audio PD on DSP is still using the memory, the ownership should not be
> moved to HLOS. This might happen in daemon kill scenario where remote_heap is not
> allocated again, but if due to any reason if the fastrpc_internal_invoke fails, that might
> result in the ownership change of remote_heap memory.

You are describing the expected behaviour. not the observed issue.

Also, the second quesiton didn't get the answer. Is it possible to free
/reclaim the memory?

> >
> >> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> >> Cc: stable <stable@kernel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index 1ba85c70e3ff..24dc1cba40e9 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -1238,6 +1238,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  	struct fastrpc_phy_page pages[1];
> >>  	char *name;
> >>  	int err;
> >> +	bool scm_done = false;
> >>  	struct {
> >>  		int pgid;
> >>  		u32 namelen;
> >> @@ -1289,6 +1290,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> >>  				goto err_map;
> >>  			}
> >> +			scm_done = true;
> >>  		}
> >>  	}
> >>  
> >> @@ -1324,7 +1326,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  
> >>  	return 0;
> >>  err_invoke:
> >> -	if (fl->cctx->vmcount) {
> >> +	if (fl->cctx->vmcount && scm_done) {
> >>  		u64 src_perms = 0;
> >>  		struct qcom_scm_vmperm dst_perms;
> >>  		u32 i;
> >> -- 
> >> 2.43.0
> >>
> 

-- 
With best wishes
Dmitry

[PATCH v5 7/7] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[Ekansh Gupta]

Untrusted application with access to only non-secure fastrpc device
node can attach to root_pd or static PDs if it can make the respective
init request. This can cause problems as the untrusted application
can send bad requests to root_pd or static PDs. Add changes to reject
attach to privileged PDs if the request is being made using non-secure
fastrpc device node.

Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/misc/fastrpc.c      | 22 +++++++++++++++++++---
 include/uapi/misc/fastrpc.h |  3 +++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 24dc1cba40e9..9b4dfe152303 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2087,6 +2087,16 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
 	return err;
 }
 
+static int is_attach_rejected(struct fastrpc_user *fl)
+{
+	/* Check if the device node is non-secure */
+	if (!fl->is_secure_dev) {
+		dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
+		return -EACCES;
+	}
+	return 0;
+}
+
 static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 				 unsigned long arg)
 {
@@ -2099,13 +2109,19 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 		err = fastrpc_invoke(fl, argp);
 		break;
 	case FASTRPC_IOCTL_INIT_ATTACH:
-		err = fastrpc_init_attach(fl, ROOT_PD);
+		err = is_attach_rejected(fl);
+		if (!err)
+			err = fastrpc_init_attach(fl, ROOT_PD);
 		break;
 	case FASTRPC_IOCTL_INIT_ATTACH_SNS:
-		err = fastrpc_init_attach(fl, SENSORS_PD);
+		err = is_attach_rejected(fl);
+		if (!err)
+			err = fastrpc_init_attach(fl, SENSORS_PD);
 		break;
 	case FASTRPC_IOCTL_INIT_CREATE_STATIC:
-		err = fastrpc_init_create_static_process(fl, argp);
+		err = is_attach_rejected(fl);
+		if (!err)
+			err = fastrpc_init_create_static_process(fl, argp);
 		break;
 	case FASTRPC_IOCTL_INIT_CREATE:
 		err = fastrpc_init_create_process(fl, argp);
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index f33d914d8f46..91583690bddc 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -8,11 +8,14 @@
 #define FASTRPC_IOCTL_ALLOC_DMA_BUFF	_IOWR('R', 1, struct fastrpc_alloc_dma_buf)
 #define FASTRPC_IOCTL_FREE_DMA_BUFF	_IOWR('R', 2, __u32)
 #define FASTRPC_IOCTL_INVOKE		_IOWR('R', 3, struct fastrpc_invoke)
+/* This ioctl is only supported with secure device nodes */
 #define FASTRPC_IOCTL_INIT_ATTACH	_IO('R', 4)
 #define FASTRPC_IOCTL_INIT_CREATE	_IOWR('R', 5, struct fastrpc_init_create)
 #define FASTRPC_IOCTL_MMAP		_IOWR('R', 6, struct fastrpc_req_mmap)
 #define FASTRPC_IOCTL_MUNMAP		_IOWR('R', 7, struct fastrpc_req_munmap)
+/* This ioctl is only supported with secure device nodes */
 #define FASTRPC_IOCTL_INIT_ATTACH_SNS	_IO('R', 8)
+/* This ioctl is only supported with secure device nodes */
 #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static)
 #define FASTRPC_IOCTL_MEM_MAP		_IOWR('R', 10, struct fastrpc_mem_map)
 #define FASTRPC_IOCTL_MEM_UNMAP		_IOWR('R', 11, struct fastrpc_mem_unmap)
-- 
2.43.0

Re: (subset) [PATCH v5 0/7] Add missing fixes to FastRPC driver

[Srinivas Kandagatla]

On Tue, 11 Jun 2024 16:04:33 +0530, Ekansh Gupta wrote:
> This patch series adds the listed bug fixes that have been missing
> in upstream fastRPC driver.
> - Fix DSP capabilities request.
> - Fix issues in audio daemon attach operation.
> - Restrict untrusted app to attach to privilegeded PD.
> 
> Changes in v2:
> - Added separate patch to add newlines in dev_err.
> - Added a bug fix in fastrpc capability function.
> - Added a new patch to save and restore interrupted context.
> - Fixed config dependency for PDR support.
> 
> [...]

Applied, thanks!

[2/7] misc: fastrpc: Fix DSP capabilities request
      commit: 8bac43bb507f1fe6e56762ca350c8b6f41096959
[3/7] misc: fastrpc: Copy the complete capability structure to user
      commit: 552244bb57914612f4db79f0f52c6130af45c50b
[4/7] misc: fastrpc: Avoid updating PD type for capability request
      commit: 7718647366694bf1821a87e08a2ee4ef62012270
[5/7] misc: fastrpc: Fix memory leak in audio daemon attach operation
      commit: f3080b096933b6633d71e5345f72a79ec25faaa9
[6/7] misc: fastrpc: Fix ownership reassignment of remote heap
      commit: 2a732868df39b717046a4f03c40f84db8be9c687
[7/7] misc: fastrpc: Restrict untrusted app to attach to privileged PD
      commit: 435f39b8991cd719fbbceb6872602629417c9272

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Re: [PATCH v5 0/7] Add missing fixes to FastRPC driver

[Srinivas Kandagatla]

Thanks for Patches,

Please send the patches in correct order, fixes will follow enhancements.



--srini

On 11/06/2024 11:34, Ekansh Gupta wrote:
> This patch series adds the listed bug fixes that have been missing
> in upstream fastRPC driver.
> - Fix DSP capabilities request.
> - Fix issues in audio daemon attach operation.
> - Restrict untrusted app to attach to privilegeded PD.
> 
> Changes in v2:
> - Added separate patch to add newlines in dev_err.
> - Added a bug fix in fastrpc capability function.
> - Added a new patch to save and restore interrupted context.
> - Fixed config dependency for PDR support.
> 
> Changes in v3:
> - Dropped interrupted context patch.
> - Splitted few of the bug fix patches.
> - Added Fixes tag wherever applicable.
> - Updated proper commit message for few of the patches.
> 
> Changes in v4:
> - Dropped untrusted process and system unsigned PD patches.
> - Updated proper commit message for few of the patches.
> - Splitted patches in more meaningful way.
> - Added helped functions for fastrpc_req_mmap.
> 
> Changes in v5:
> - Dropped PDR patch. It will be shared in a separate patch series.
> - Dropped fastrpc_req_mmap and remote_heap specific changes from this
>    series. These patches will be shared separately as a new patch series.
> - Changed patch series subject as this series is no longer carrying any
>    new feature changes.
> 
> Ekansh Gupta (7):
>    misc: fastrpc: Add missing dev_err newlines
>    misc: fastrpc: Fix DSP capabilities request
>    misc: fastrpc: Copy the complete capability structure to user
>    misc: fastrpc: Avoid updating PD type for capability request
>    misc: fastrpc: Fix memory leak in audio daemon attach operation
>    misc: fastrpc: Fix ownership reassignment of remote heap
>    misc: fastrpc: Restrict untrusted app to attach to privileged PD
> 
>   drivers/misc/fastrpc.c      | 51 +++++++++++++++++++++++++++----------
>   include/uapi/misc/fastrpc.h |  3 +++
>   2 files changed, 40 insertions(+), 14 deletions(-)
> 

Re: (subset) [PATCH v5 0/7] Add missing fixes to FastRPC driver

[Srinivas Kandagatla]

On Tue, 11 Jun 2024 16:04:33 +0530, Ekansh Gupta wrote:
> This patch series adds the listed bug fixes that have been missing
> in upstream fastRPC driver.
> - Fix DSP capabilities request.
> - Fix issues in audio daemon attach operation.
> - Restrict untrusted app to attach to privilegeded PD.
> 
> Changes in v2:
> - Added separate patch to add newlines in dev_err.
> - Added a bug fix in fastrpc capability function.
> - Added a new patch to save and restore interrupted context.
> - Fixed config dependency for PDR support.
> 
> [...]

Applied, thanks!

[1/7] misc: fastrpc: Add missing dev_err newlines
      commit: 372eb825c2040b81b6c20b8ff662a6a551f236f9

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>