Received-SPF: pass (google.com: domain of linux-kernel+bounces-192359-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 28 May 2024 16:02:04 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Ekansh Gupta <quic_ekangupt@quicinc.com>
Cc: srinivas.kandagatla@linaro.org, linux-arm-msm@vger.kernel.org, 
	gregkh@linuxfoundation.org, quic_bkumar@quicinc.com, linux-kernel@vger.kernel.org, 
	quic_chennak@quicinc.com
Subject: Re: [PATCH v2 7/8] misc: fastrpc: Restrict untrusted apk to spawn
Message-ID: <w26h3ufedlbv7rxwqkjin47snx37ljpduq442lzbthrdvdhowt@o4wujbbjlssv>
References: <20240528112956.5979-1-quic_ekangupt@quicinc.com>
 <20240528112956.5979-8-quic_ekangupt@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240528112956.5979-8-quic_ekangupt@quicinc.com>

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
> 

-- 
With best wishes
Dmitry