[PATCH 0/6] misc: fastrpc: fixes for 6.10

[PATCH 0/6] misc: fastrpc: fixes for 6.10

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Hi Greg,

Here are few fixes in nvmem for 6.10, Could you queue
these for next possible rc.

fixes include
- Fixing incorrect handing audio protection domain
- fix capablity query requests.

Ekansh Gupta (6):
  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      | 41 +++++++++++++++++++++++++++++--------
 include/uapi/misc/fastrpc.h |  3 +++
 2 files changed, 35 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 1/6] misc: fastrpc: Fix DSP capabilities request

[srinivas.kandagatla]

From: Ekansh Gupta <quic_ekangupt@quicinc.com>

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>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 4c67e2c5a82e..82ce4f58d70f 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.25.1

[PATCH 2/6] misc: fastrpc: Copy the complete capability structure to user

[srinivas.kandagatla]

From: Ekansh Gupta <quic_ekangupt@quicinc.com>

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>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 82ce4f58d70f..30b93012877f 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.25.1

[PATCH 3/6] misc: fastrpc: Avoid updating PD type for capability request

[srinivas.kandagatla]

From: Ekansh Gupta <quic_ekangupt@quicinc.com>

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>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 30b93012877f..3fef3eecb88c 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.25.1

[PATCH 4/6] misc: fastrpc: Fix memory leak in audio daemon attach operation

[srinivas.kandagatla]

From: Ekansh Gupta <quic_ekangupt@quicinc.com>

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>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 3fef3eecb88c..11d53b9322f1 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.25.1

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

[srinivas.kandagatla]

From: Ekansh Gupta <quic_ekangupt@quicinc.com>

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>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 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 11d53b9322f1..5680856c0fb8 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.25.1

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

[srinivas.kandagatla]

From: Ekansh Gupta <quic_ekangupt@quicinc.com>

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>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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.25.1

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[Selvaraj, Joel (MU-Student)]

Hi Srinivas Kandagatla and Ekansh Gupta,

On 6/28/24 06:45, srinivas.kandagatla@linaro.org wrote:
> From: Ekansh Gupta <quic_ekangupt@quicinc.com>
> 
> 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>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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;
> +}

This broke userspace for us. Sensors stopped working in SDM845 and other 
qcom SoC devices running postmarketOS. Trying to communicate with the 
fastrpc device just ends up with a permission denied error. This was 
previously working. I am not sure if this is intended. Here are my two 
observations:

1. if change the if condition to

`if (!fl->is_secure_dev && fl->cctx->secure)`

similar to how it's done in fastrpc's `is_session_rejected()` function, 
then it works. But I am not sure if this is an valid fix. But currently, 
fastrpc will simply deny access to all fastrpc device that contains the 
`qcom,non-secure-domain` dt property. Is that the intended change? 
Because I see a lot of adsp, cdsp and sdsp fastrpc nodes have that dt 
property.

2. In the `fastrpc_rpmsg_probe()` function, it is commented that,

"Unsigned PD offloading is only supported on CDSP"

Does this mean adsp and sdsp shouldn't have the `qcom,non-secure-domain` 
dt property? In fact, it was reported that removing this dt property and 
using the `/dev/fastrpc-sdsp-secure` node instead works fine too. Is 
this the correct way to fix it?

I don't know much about fastrpc, just reporting the issue and guessing 
here. It would be really if this can be fixed before the stable release.

Thank you,
Joel Selvaraj

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[gregkh]

On Thu, Aug 15, 2024 at 02:34:18AM +0000, Selvaraj, Joel (MU-Student) wrote:
> Hi Srinivas Kandagatla and Ekansh Gupta,
> 
> On 6/28/24 06:45, srinivas.kandagatla@linaro.org wrote:
> > From: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > 
> > 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>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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;
> > +}
> 
> This broke userspace for us. Sensors stopped working in SDM845 and other 
> qcom SoC devices running postmarketOS. Trying to communicate with the 
> fastrpc device just ends up with a permission denied error. This was 
> previously working. I am not sure if this is intended. Here are my two 
> observations:
> 
> 1. if change the if condition to
> 
> `if (!fl->is_secure_dev && fl->cctx->secure)`
> 
> similar to how it's done in fastrpc's `is_session_rejected()` function, 
> then it works. But I am not sure if this is an valid fix. But currently, 
> fastrpc will simply deny access to all fastrpc device that contains the 
> `qcom,non-secure-domain` dt property. Is that the intended change? 
> Because I see a lot of adsp, cdsp and sdsp fastrpc nodes have that dt 
> property.
> 
> 2. In the `fastrpc_rpmsg_probe()` function, it is commented that,
> 
> "Unsigned PD offloading is only supported on CDSP"
> 
> Does this mean adsp and sdsp shouldn't have the `qcom,non-secure-domain` 
> dt property? In fact, it was reported that removing this dt property and 
> using the `/dev/fastrpc-sdsp-secure` node instead works fine too. Is 
> this the correct way to fix it?
> 
> I don't know much about fastrpc, just reporting the issue and guessing 
> here. It would be really if this can be fixed before the stable release.

I will be glad to revert it, what was the git id for this in the tree
now?

thanks,

greg k-h

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[gregkh]

On Thu, Aug 15, 2024 at 07:15:50AM +0200, gregkh@linuxfoundation.org wrote:
> On Thu, Aug 15, 2024 at 02:34:18AM +0000, Selvaraj, Joel (MU-Student) wrote:
> > Hi Srinivas Kandagatla and Ekansh Gupta,
> > 
> > On 6/28/24 06:45, srinivas.kandagatla@linaro.org wrote:
> > > From: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > 
> > > 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>
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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;
> > > +}
> > 
> > This broke userspace for us. Sensors stopped working in SDM845 and other 
> > qcom SoC devices running postmarketOS. Trying to communicate with the 
> > fastrpc device just ends up with a permission denied error. This was 
> > previously working. I am not sure if this is intended. Here are my two 
> > observations:
> > 
> > 1. if change the if condition to
> > 
> > `if (!fl->is_secure_dev && fl->cctx->secure)`
> > 
> > similar to how it's done in fastrpc's `is_session_rejected()` function, 
> > then it works. But I am not sure if this is an valid fix. But currently, 
> > fastrpc will simply deny access to all fastrpc device that contains the 
> > `qcom,non-secure-domain` dt property. Is that the intended change? 
> > Because I see a lot of adsp, cdsp and sdsp fastrpc nodes have that dt 
> > property.
> > 
> > 2. In the `fastrpc_rpmsg_probe()` function, it is commented that,
> > 
> > "Unsigned PD offloading is only supported on CDSP"
> > 
> > Does this mean adsp and sdsp shouldn't have the `qcom,non-secure-domain` 
> > dt property? In fact, it was reported that removing this dt property and 
> > using the `/dev/fastrpc-sdsp-secure` node instead works fine too. Is 
> > this the correct way to fix it?
> > 
> > I don't know much about fastrpc, just reporting the issue and guessing 
> > here. It would be really if this can be fixed before the stable release.
> 
> I will be glad to revert it, what was the git id for this in the tree
> now?

Ah, nevermind, I found it, it's bab2f5e8fd5d ("misc: fastrpc: Restrict
untrusted app to attach to privileged PD") and is already in the stable
kernel trees.  Do you want to submit a revert or do you need/want me to
do it?

thanks,

greg k-h

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[Joel Selvaraj]

Hi greg k-h,

The git commit id is: bab2f5e8fd5d2f759db26b78d9db57412888f187

But I am bit hesitant if we should revert it because there is a CVE 
attached to it: https://ubuntu.com/security/CVE-2024-41024

Also, I am ok with changing userspace if it's necessary. It would be 
nice if the authors can clarify the ideal fix here.

Regards,
Joel Selvaraj

On 8/15/24 00:15, gregkh@linuxfoundation.org wrote:
> On Thu, Aug 15, 2024 at 02:34:18AM +0000, Selvaraj, Joel (MU-Student) wrote:
>> Hi Srinivas Kandagatla and Ekansh Gupta,
>>
>> On 6/28/24 06:45, srinivas.kandagatla@linaro.org wrote:
>>> From: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>
>>> 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>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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;
>>> +}
>>
>> This broke userspace for us. Sensors stopped working in SDM845 and other
>> qcom SoC devices running postmarketOS. Trying to communicate with the
>> fastrpc device just ends up with a permission denied error. This was
>> previously working. I am not sure if this is intended. Here are my two
>> observations:
>>
>> 1. if change the if condition to
>>
>> `if (!fl->is_secure_dev && fl->cctx->secure)`
>>
>> similar to how it's done in fastrpc's `is_session_rejected()` function,
>> then it works. But I am not sure if this is an valid fix. But currently,
>> fastrpc will simply deny access to all fastrpc device that contains the
>> `qcom,non-secure-domain` dt property. Is that the intended change?
>> Because I see a lot of adsp, cdsp and sdsp fastrpc nodes have that dt
>> property.
>>
>> 2. In the `fastrpc_rpmsg_probe()` function, it is commented that,
>>
>> "Unsigned PD offloading is only supported on CDSP"
>>
>> Does this mean adsp and sdsp shouldn't have the `qcom,non-secure-domain`
>> dt property? In fact, it was reported that removing this dt property and
>> using the `/dev/fastrpc-sdsp-secure` node instead works fine too. Is
>> this the correct way to fix it?
>>
>> I don't know much about fastrpc, just reporting the issue and guessing
>> here. It would be really if this can be fixed before the stable release.
> 
> I will be glad to revert it, what was the git id for this in the tree
> now?
> 
> thanks,
> 
> greg k-h

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[gregkh]

On Thu, Aug 15, 2024 at 03:35:13AM -0500, Joel Selvaraj wrote:
> Hi greg k-h,
> 
> The git commit id is: bab2f5e8fd5d2f759db26b78d9db57412888f187
> 
> But I am bit hesitant if we should revert it because there is a CVE attached
> to it: https://ubuntu.com/security/CVE-2024-41024

Not an issue if it is breaking things, let's get it right.  We can
trivially reject that CVE if needed.

> Also, I am ok with changing userspace if it's necessary. It would be nice if
> the authors can clarify the ideal fix here.

No, userspace should not break, that's not ok at all.  I'll get someone
to revert this later today, thanks!

greg k-h

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[gregkh]

On Thu, Aug 15, 2024 at 10:41:15AM +0200, gregkh@linuxfoundation.org wrote:
> On Thu, Aug 15, 2024 at 03:35:13AM -0500, Joel Selvaraj wrote:
> > Hi greg k-h,
> > 
> > The git commit id is: bab2f5e8fd5d2f759db26b78d9db57412888f187
> > 
> > But I am bit hesitant if we should revert it because there is a CVE attached
> > to it: https://ubuntu.com/security/CVE-2024-41024
> 
> Not an issue if it is breaking things, let's get it right.  We can
> trivially reject that CVE if needed.
> 
> > Also, I am ok with changing userspace if it's necessary. It would be nice if
> > the authors can clarify the ideal fix here.
> 
> No, userspace should not break, that's not ok at all.  I'll get someone
> to revert this later today, thanks!

Now sent:
	https://lore.kernel.org/r/20240815094920.8242-1-griffin@kroah.com

I'll queue this up later today.

thanks,

greg k-h

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[Joel Selvaraj]

On 8/15/24 05:02, gregkh@linuxfoundation.org wrote:
> On Thu, Aug 15, 2024 at 10:41:15AM +0200, gregkh@linuxfoundation.org wrote:
>> On Thu, Aug 15, 2024 at 03:35:13AM -0500, Joel Selvaraj wrote:
>>> Hi greg k-h,
>>>
>>> The git commit id is: bab2f5e8fd5d2f759db26b78d9db57412888f187
>>>
>>> But I am bit hesitant if we should revert it because there is a CVE attached
>>> to it: https://ubuntu.com/security/CVE-2024-41024
>>
>> Not an issue if it is breaking things, let's get it right.  We can
>> trivially reject that CVE if needed.
>>
>>> Also, I am ok with changing userspace if it's necessary. It would be nice if
>>> the authors can clarify the ideal fix here.
>>
>> No, userspace should not break, that's not ok at all.  I'll get someone
>> to revert this later today, thanks!
> 
> Now sent:
> 	https://lore.kernel.org/r/20240815094920.8242-1-griffin@kroah.com
> 
> I'll queue this up later today.

Thanks!
Joel Selvaraj

> thanks,
> 
> greg k-h

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[Srinivas Kandagatla]

On 15/08/2024 03:34, Selvaraj, Joel (MU-Student) wrote:
> Hi Srinivas Kandagatla and Ekansh Gupta,
> 
> On 6/28/24 06:45, srinivas.kandagatla@linaro.org wrote:
>> From: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>
>> 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>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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;
>> +}
> 
> This broke userspace for us. Sensors stopped working in SDM845 and other
> qcom SoC devices running postmarketOS. Trying to communicate with the
> fastrpc device just ends up with a permission denied error. This was
> previously working. I am not sure if this is intended. Here are my two
> observations:
> 
> 1. if change the if condition to
> 
> `if (!fl->is_secure_dev && fl->cctx->secure)`
> 
> similar to how it's done in fastrpc's `is_session_rejected()` function,
> then it works. But I am not sure if this is an valid fix. But currently,
> fastrpc will simply deny access to all fastrpc device that contains the
> `qcom,non-secure-domain` dt property. Is that the intended change?
> Because I see a lot of adsp, cdsp and sdsp fastrpc nodes have that dt
> property.
> 
> 2. In the `fastrpc_rpmsg_probe()` function, it is commented that,
> 
> "Unsigned PD offloading is only supported on CDSP"
> 
> Does this mean adsp and sdsp shouldn't have the `qcom,non-secure-domain`
> dt property? In fact, it was reported that removing this dt property and
> using the `/dev/fastrpc-sdsp-secure` node instead works fine too. Is
> this the correct way to fix it?

Yes, this is the ideal way to fix this, Audio DSP and Sensor DSPs are by 
default secure DSP's.

usage of "qcom,non-secure-domain" has been abused on all the platforms 
as the device tree bindings are not enforcing this checks to any new 
device tree entries. This needs fixing properly.

Ideally this patch has to fix the existing dts and update bindings to 
reflect that.

Sorry this has been over looked!

On the library side that you are using consider non-secure node as 
fallback only when secure node is missing.

given the mess with the current state of patch, reverting sounds good 
for me to start with.

--srini

> 
> I don't know much about fastrpc, just reporting the issue and guessing
> here. It would be really if this can be fixed before the stable release.
> 
> Thank you,
> Joel Selvaraj
> 
> 

Re: [PATCH 6/6] misc: fastrpc: Restrict untrusted app to attach to privileged PD

[gregkh]

On Thu, Aug 15, 2024 at 02:30:19PM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 15/08/2024 03:34, Selvaraj, Joel (MU-Student) wrote:
> > Hi Srinivas Kandagatla and Ekansh Gupta,
> > 
> > On 6/28/24 06:45, srinivas.kandagatla@linaro.org wrote:
> > > From: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > 
> > > 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>
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@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 5680856c0fb8..a7a2bcedb37e 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;
> > > +}
> > 
> > This broke userspace for us. Sensors stopped working in SDM845 and other
> > qcom SoC devices running postmarketOS. Trying to communicate with the
> > fastrpc device just ends up with a permission denied error. This was
> > previously working. I am not sure if this is intended. Here are my two
> > observations:
> > 
> > 1. if change the if condition to
> > 
> > `if (!fl->is_secure_dev && fl->cctx->secure)`
> > 
> > similar to how it's done in fastrpc's `is_session_rejected()` function,
> > then it works. But I am not sure if this is an valid fix. But currently,
> > fastrpc will simply deny access to all fastrpc device that contains the
> > `qcom,non-secure-domain` dt property. Is that the intended change?
> > Because I see a lot of adsp, cdsp and sdsp fastrpc nodes have that dt
> > property.
> > 
> > 2. In the `fastrpc_rpmsg_probe()` function, it is commented that,
> > 
> > "Unsigned PD offloading is only supported on CDSP"
> > 
> > Does this mean adsp and sdsp shouldn't have the `qcom,non-secure-domain`
> > dt property? In fact, it was reported that removing this dt property and
> > using the `/dev/fastrpc-sdsp-secure` node instead works fine too. Is
> > this the correct way to fix it?
> 
> Yes, this is the ideal way to fix this, Audio DSP and Sensor DSPs are by
> default secure DSP's.
> 
> usage of "qcom,non-secure-domain" has been abused on all the platforms as
> the device tree bindings are not enforcing this checks to any new device
> tree entries. This needs fixing properly.
> 
> Ideally this patch has to fix the existing dts and update bindings to
> reflect that.
> 
> Sorry this has been over looked!
> 
> On the library side that you are using consider non-secure node as fallback
> only when secure node is missing.
> 
> given the mess with the current state of patch, reverting sounds good for me
> to start with.

Great, can you ack the revert then and I'll queue it up to get to Linus
this week?

thanks,

greg k-h