linux-kernel - Re: [PATCH v1] misc: fastrpc: Pass proper arguments to scm call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 8 Jan 2024 17:12:30 -0800
From: Elliot Berman <quic_eberman@...cinc.com>
To: Ekansh Gupta <quic_ekangupt@...cinc.com>, <srinivas.kandagatla@...aro.org>,
        <linux-arm-msm@...r.kernel.org>
CC: <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
        stable
	<stable@...nel.org>
Subject: Re: [PATCH v1] misc: fastrpc: Pass proper arguments to scm call



On 1/8/2024 2:05 AM, Ekansh Gupta wrote:
> For CMA memory allocation, ownership is assigned to DSP to make it
> accessible by the PD running on the DSP. With current implementation
> HLOS VM is stored in the channel structure during rpmsg_probe and
> this VM is passed to qcom_scm call as the source VM.
> 
> The qcom_scm call will overwrite the passed source VM with the next
> VM which would cause a problem in case the scm call is again needed.
> Adding a local copy of source VM whereever scm call is made to avoid
> this problem.
> 

The perms in fastrpc_channel_ctx should always reflect the current
permission bits, so I'm surprised you see problem.

What is the scenario where that's not the case?

> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")> Cc: stable <stable@...nel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
> ---
>  drivers/misc/fastrpc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 1c6c62a7f7f5..c13efa7727e0 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -263,7 +263,6 @@ struct fastrpc_channel_ctx {
>  	int domain_id;
>  	int sesscount;
>  	int vmcount;
> -	u64 perms;
>  	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
>  	struct rpmsg_device *rpdev;
>  	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
> @@ -1279,9 +1278,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  
>  		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
>  		if (fl->cctx->vmcount) {
> +			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
>  			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>  							(u64)fl->cctx->remote_heap->size,
> -							&fl->cctx->perms,
> +							&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",
> @@ -1915,8 +1916,10 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  
>  	/* Add memory to static PD pool, protection thru hypervisor */
>  	if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
> +		u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> +
>  		err = qcom_scm_assign_mem(buf->phys, (u64)buf->size,
> -			&fl->cctx->perms, fl->cctx->vmperms, fl->cctx->vmcount);
> +			&src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>  		if (err) {
>  			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>  					buf->phys, buf->size, err);
> @@ -2290,7 +2293,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  
>  	if (vmcount) {
>  		data->vmcount = vmcount;
> -		data->perms = BIT(QCOM_SCM_VMID_HLOS);
>  		for (i = 0; i < data->vmcount; i++) {
>  			data->vmperms[i].vmid = vmids[i];
>  			data->vmperms[i].perm = QCOM_SCM_PERM_RWX;