diff mbox series

[v2,7/8] misc: fastrpc: Restrict untrusted apk to spawn

Message ID 20240528112956.5979-8-quic_ekangupt@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add missing features to FastRPC driver | expand

Commit Message

Ekansh Gupta May 28, 2024, 11:29 a.m. UTC 
Untrusted application can attach to guestOS and staticPD if it can
make root PD, sensors PD or audio PD attach request. This is a
potential security issue as the untrusted application can crash
rootPD or staticPD. Restrict attach to guestOS or staticPD request
if request is being made using non-secure device node.

Also for untrusted dynamic processes, DSP HAL process opens the
device node on behalf of the application. Add a check to restrict
such untrusted applications from offloading to signed PD.

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

Comments

Dmitry Baryshkov May 28, 2024, 1:02 p.m. UTC | #1 
On Tue, May 28, 2024 at 04:59:53PM +0530, Ekansh Gupta wrote:
> Untrusted application can attach to guestOS and staticPD if it can
> make root PD, sensors PD or audio PD attach request. This is a
> potential security issue as the untrusted application can crash
> rootPD or staticPD. Restrict attach to guestOS or staticPD request
> if request is being made using non-secure device node.

This is obviously a fix. Please add proper Fixes tag and move it to the
top of the patchset.

> 
> Also for untrusted dynamic processes, DSP HAL process opens the
> device node on behalf of the application. Add a check to restrict
> such untrusted applications from offloading to signed PD.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 23dd20c22f6d..7f81a18b8aea 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -332,6 +332,7 @@ struct fastrpc_user {
>  	int pd;
>  	bool is_secure_dev;
>  	enum fastrpc_userpd_type userpd_type;
> +	bool untrusted_process;
>  	char *servloc_name;
>  	/* Lock for lists */
>  	spinlock_t lock;
> @@ -1274,20 +1275,24 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>  
>  static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_request)
>  {
> -	/* Check if the device node is non-secure and channel is secure*/
> +	/* Check if the device node is non-secure and channel is secure */

no unrelated cleanups, please.

>  	if (!fl->is_secure_dev && fl->cctx->secure) {
>  		/*
>  		 * Allow untrusted applications to offload only to Unsigned PD when
>  		 * channel is configured as secure and block untrusted apps on channel
>  		 * 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\n");
> -			return true;
> -		}
> +		if (!fl->cctx->unsigned_support || !unsigned_pd_request)
> +			goto reject_session;
>  	}
> +	/* Check if untrusted process is trying to offload to signed PD */
> +	if (fl->untrusted_process && !unsigned_pd_request)
> +		goto reject_session;
>  
>  	return false;
> +reject_session:
> +	dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");

Please drop this from dev_err. Use dev_dbg and return the error.
Otherwise the user can easily spam kernel logs.

> +	return true;
>  }
>  
>  static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
> @@ -1376,6 +1381,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  	} inbuf;
>  	u32 sc;
>  
> +	if (!fl->is_secure_dev) {
> +		dev_err(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");

Same thing here.

> +		return -EACCES;
> +	}
> +
>  	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
>  	if (!args)
>  		return -ENOMEM;
> @@ -1531,12 +1541,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  		goto err;
>  	}
>  
> +	/*
> +	 * Third-party apps don't have permission to open the fastrpc device, so
> +	 * it is opened on their behalf by DSP HAL. This is detected by

There is no DSP HAL on plain Linux. Also the question of permissions
depends on user setting up the system, so this probablye needs to be
rethought.

> +	 * comparing current PID with the one stored during device open.
> +	 */
> +	if (current->tgid != fl->tgid)
> +		fl->untrusted_process = true;
> +
>  	if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
>  		fl->userpd_type = UNSIGNED_PD;
>  
>  
>  	if (is_session_rejected(fl, !(fl->userpd_type == SIGNED_PD))) {
> -		err = -ECONNREFUSED;
> +		err = -EACCES;
>  		goto err;
>  	}
>  
> @@ -1818,6 +1836,11 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>  	int tgid = fl->tgid;
>  	u32 sc;
>  
> +	if (!fl->is_secure_dev) {
> +		dev_err(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");

And again dev_dbg please.

> +		return -EACCES;
> +	}
> +
>  	args[0].ptr = (u64)(uintptr_t) &tgid;
>  	args[0].length = sizeof(tgid);
>  	args[0].fd = -1;
> -- 
> 2.43.0
>
diff mbox series

Patch

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 23dd20c22f6d..7f81a18b8aea 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -332,6 +332,7 @@  struct fastrpc_user {
 	int pd;
 	bool is_secure_dev;
 	enum fastrpc_userpd_type userpd_type;
+	bool untrusted_process;
 	char *servloc_name;
 	/* Lock for lists */
 	spinlock_t lock;
@@ -1274,20 +1275,24 @@  static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 
 static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_request)
 {
-	/* Check if the device node is non-secure and channel is secure*/
+	/* Check if the device node is non-secure and channel is secure */
 	if (!fl->is_secure_dev && fl->cctx->secure) {
 		/*
 		 * Allow untrusted applications to offload only to Unsigned PD when
 		 * channel is configured as secure and block untrusted apps on channel
 		 * 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\n");
-			return true;
-		}
+		if (!fl->cctx->unsigned_support || !unsigned_pd_request)
+			goto reject_session;
 	}
+	/* Check if untrusted process is trying to offload to signed PD */
+	if (fl->untrusted_process && !unsigned_pd_request)
+		goto reject_session;
 
 	return false;
+reject_session:
+	dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n");
+	return true;
 }
 
 static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd)
@@ -1376,6 +1381,11 @@  static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	} inbuf;
 	u32 sc;
 
+	if (!fl->is_secure_dev) {
+		dev_err(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
+		return -EACCES;
+	}
+
 	args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL);
 	if (!args)
 		return -ENOMEM;
@@ -1531,12 +1541,20 @@  static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
+	/*
+	 * Third-party apps don't have permission to open the fastrpc device, so
+	 * it is opened on their behalf by DSP HAL. This is detected by
+	 * comparing current PID with the one stored during device open.
+	 */
+	if (current->tgid != fl->tgid)
+		fl->untrusted_process = true;
+
 	if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE)
 		fl->userpd_type = UNSIGNED_PD;
 
 
 	if (is_session_rejected(fl, !(fl->userpd_type == SIGNED_PD))) {
-		err = -ECONNREFUSED;
+		err = -EACCES;
 		goto err;
 	}
 
@@ -1818,6 +1836,11 @@  static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
 	int tgid = fl->tgid;
 	u32 sc;
 
+	if (!fl->is_secure_dev) {
+		dev_err(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n");
+		return -EACCES;
+	}
+
 	args[0].ptr = (u64)(uintptr_t) &tgid;
 	args[0].length = sizeof(tgid);
 	args[0].fd = -1;