| 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
| ||
|
Message-ID: <ea7b7065-6520-eb1f-b6f3-3b2e7138cc31@codeaurora.org>
Date: Wed, 7 Jun 2017 21:57:25 +0530
From: "Dwivedi, Avaneesh Kumar (avani)" <akdwived@...eaurora.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: sboyd@...eaurora.org, agross@...eaurora.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v5 3/4] remoteproc: qcom: Make secure world call for mem
ownership switch
On 6/2/2017 11:25 PM, Bjorn Andersson wrote:
> On Thu 01 Jun 14:42 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
>
>> Hi Bjorn,
>>
>> Thanks lot many for such a blazing fast response :)
>>
>> regarding your points.
>>
>> a- Do you mean caller's of q6v5_xfer_mem_ownership() should pass two
>> additional inputs i.e. *next_perm and *next_vmid
>>
> You have two cases; assign to HLOS and assign to MSS, so I imagine that
> you pass a single indicator of which you want to assign. I.e. rather
> than looking at what the current state is and flipping you pass the
> conditional of that if statement as a parameter.
OK
>
>> and that based on successful return of qcom_scm_assign () they should be
>> treated as *current_perm and *current_vmid
>>
> Instead of your index, you take a "int *curr_perms", which you use as
> the current vmid list and you assign at the end of the function (like
> you do today).
>
> So to transfer the ownership to the MSS you would make a function call
> like:
>
> ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_owner, ..., true);
>
> mpss_owner would have to be initialize to HLOS before calling this, but
> will always be holding the current value.
i am not finding compelling enough region to carry an input pointer to
hold current ownership
specially when i am carrying a boolean flag to check whether next->vmid
will be MSS or HLOS
I mean where am i going to use this current owner info in mss rproc
driver, i am yet not getting enough reason.
while the local array did job of maintaining and flipping the ownership
based on info if which image ownership transfer is being called.
>
>> by caller? if so caller will have to do flipping between MSS and HLOS,
>> isn't it?
>>
> As far as I can see each call to this driver is either a "transfer to
> MSS" or "transfer to HLOS", so that shouldn't be a problem.
Yes this job will be done by bool flag, but again what is then use of
carry pointers mpss_owner, mba_owner.
>
>> Also code churning will increase this way, and also logging the way is
>> being handled now may require to change again.
>>
>> or am i misunderstanding your proposal?
>>
> Could be that I'm missing something, if above doesn't make sense please
> do let me know.
I can not say it does not make sense, probably something subtle i am
missing to see.
>
>> Sorry for inconvenience, but if you could through little more light, that
>> will help.
>>
> There's no inconvenience, thanks for reaching out for clarifications on
> my comments.
Thanks for such a nice gesture.
i feel that maintaining the local array for ownership switching looks,
too raw way of doing something.
may be i can improve that, but first i need to get understanding of
your vision in suggesting above changes.
>
> Regards,
> Bjorn
>
>> -Avaneesh
>>
>>
>> On 6/2/2017 2:12 AM, Bjorn Andersson wrote:
>>> On Thu 01 Jun 12:17 PDT 2017, Avaneesh Kumar Dwivedi wrote:
>>>
>>>> MSS proc on msm8996 can not access fw loaded region without stage
>>>> second translation of memory pages where mpss image are loaded.
>>>> This patch in order to enable mss boot on msm8996 invoke scm call
>>>> to switch or share ownership between apps and modem.
>>>>
>>> Overall this looks good now, I have two minor notes that I want you to
>>> fix up though.
>>>
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>>>> @@ -288,6 +290,40 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
>>>> return &table;
>>>> }
>>>> +static int q6v5_xfer_mem_ownership(struct q6v5 *qproc,
>>>> + int image, phys_addr_t addr,
>>> Rather than relying on a static int to keep track of current permissions
>>> pass a "int *current_perms", that you update on success.
>>>
>>> Add int mba_perm and int mpss_perm to the struct q6v5 and initialize
>>> them in probe and just keep the metadata_perm on the stack in
>>> q6v5_mpss_init_image.
>>>
>>>> + size_t size)
>>>> +{
>>>> + static int current_owner[3][1] = {{BIT(QCOM_SCM_VMID_HLOS)},
>>>> + {BIT(QCOM_SCM_VMID_HLOS)},
>>>> + {BIT(QCOM_SCM_VMID_HLOS)} };
>>>> + struct qcom_scm_vmperm next[] = {{0} };
>>> You don't need to initialize this, and if you just keep it "struct
>>> qcom_scm_vmperm next" you can pass it as &next in the
>>> qcom_scm_assign_mem() call.
>>>
>>>> + int ret;
>>>> +
>>>> + if (!qproc->need_mem_protection)
>>>> + return 0;
>>>> +
>>>> + if (current_owner[image][0] == BIT(QCOM_SCM_VMID_HLOS)) {
>>> And rather than making this flip back and forth with every call, I think
>>> it's more robust if you pass the new expected owner as a parameter to
>>> the function. Simplest way I can think of it to add a "bool
>>> remote_owner" as a parameter; it makes the changes minimal and works
>>> with the naming of the function.
>>>
>>>> + next->vmid = QCOM_SCM_VMID_MSS_MSA;
>>>> + next->perm = QCOM_SCM_PERM_RW;
>>>> + } else {
>>>> + next->vmid = QCOM_SCM_VMID_HLOS;
>>>> + next->perm = QCOM_SCM_PERM_RWX;
>>>> + }
>>>> +
>>>> + ret = qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K),
>>>> + current_owner[image][0], next, 1);
>>>> + if (ret < 0) {
>>>> + pr_err("Failed to assign %s memory access in range %p to %p ret = %d\n",
>>>> + (image == 0 ? "MDT" : image == 1 ? "MBA" : "MPSS"),
>>>> + (void *)addr, (void *)(addr + size), ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + current_owner[image][0] = ret;
>>>> + return 0;
>>>> +}
>>>> +
>>> Regards,
>>> Bjorn
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Powered by blists - more mailing lists