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: Tue, 9 Jan 2024 11:08:45 +0530
From: Ekansh Gupta <quic_ekangupt@...cinc.com>
To: Elliot Berman <quic_eberman@...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/9/2024 6:42 AM, Elliot Berman wrote:
>
> 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?

Thanks for reviewing the changes, Elliot. FastRPC driver is storing
the bitfield of HLOS VMID in fastrpc_channel_ctx in perms(cctx->perms)
and remoteproc specific VMID information from device tree in vmperms(cctx->vmperms).
This information is intended to be passed to qcom_scm call when there is
a requirement to move the ownership of memory to any remoteproc VM. As
the srcvm is overwritten with the new VM, cctx->perms cannot be reused if
the same request comes for any other memory allocation.

The problem is seen with audioPD daemon. When the daemon is stated, it
allocates some memory for audioPD and moves the ownership from HLOS to
ADSP VM using qcom_scm call. After this, audioPD makes a request for some
more memory which is again allocated in kernel and as per current
implementation, qcom_scm call is again made with cctx->perms as srcVm
which is no longer storing HLOS vmid. Hence using a local variable to
make qcom_scm call where there is a need to move ownership from HLOS
to remoteproc VM.

Please let me know if you have any more queries.

--ekansh

>
>> 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;