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]
Message-ID: <00a337d0-a82a-43cd-a106-dfe1ac5f9a11@quicinc.com>
Date: Tue, 9 Jan 2024 09:55:00 -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 9:38 PM, Ekansh Gupta wrote:
> 
> 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.
> 

Ah, got it. There can be multiple allocations/assignments per fastrpc_channel_ctx.

In that case:

Reviewed-by: Elliot Berman <quic_eberman@...cinc.com>

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