linux-kernel - Re: [PATCH V2] firmware: qcom_scm: use the SCM

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] [day] [month] [year] [list]
Date:   Mon, 25 Sep 2023 09:32:31 +0530
From:   Kathiravan Thirumoorthy <quic_kathirav@...cinc.com>
To:     Elliot Berman <quic_eberman@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC:     Trilok Soni <quic_tsoni@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <stable@...r.kernel.org>
Subject: Re: [PATCH V2] firmware: qcom_scm: use the SCM_CONVENTION based on
 ARM / ARM64


On 9/25/2023 8:23 AM, Elliot Berman wrote:
>
> On 9/19/2023 8:29 AM, Bjorn Andersson wrote:
>> On Fri, Sep 15, 2023 at 10:10:32PM +0300, Dmitry Baryshkov wrote:
>>> On Fri, 15 Sept 2023 at 18:17, Bjorn Andersson <andersson@...nel.org> wrote:
>>>> On Wed, Jun 07, 2023 at 10:23:45AM +0530, Kathiravan T wrote:
>>>>> During SCM probe, to identify the SCM convention, scm call is made with
>>>>> SMC_CONVENTION_ARM_64 followed by SMC_CONVENTION_ARM_32. Based on the
>>>>> result what convention to be used is decided.
>>>>>
>>>>> IPQ chipsets starting from IPQ807x, supports both 32bit and 64bit kernel
>>>>> variants, however TZ firmware runs in 64bit mode. When running on 32bit
>>>>> kernel, scm call is made with SMC_CONVENTION_ARM_64 is causing the
>>>>> system crash, due to the difference in the register sets between ARM and
>>>>> AARCH64, which is accessed by the TZ.
>>>>>
>>>>> To avoid this, use SMC_CONVENTION_ARM_64 only on ARM64 builds.
>>>>>
>>>> My memory of this is cloudy, but I feel the logic is complicated because
>>>> early 64-bit boards all used 32-bit TZ. So, I really would like Elliot's
>>>> input before picking this change.
>>> But this codepath is not changed by this patch. Only the 32-bit
>>> codepath is altered.
>>>
>> Ohh, you're right, sorry about that. Would still be nice to see some
>> feedback from the team here...
>>
>>
>> The commit message is talking about the convention check crashing the
>> system, the only part of the convention checker that seems to matter to
>> me is the "calling convention" bit in the smc call.
>>
>> Per the "SMC calling convention specification", the 64-bit calling
>> convention bit can only be used when the client is 64-bit. So perhaps
>> this is the actual problem?
>>
>> Beyond that, another practical problem I can see is if we pass more than
>> 4 arguments to a call the layout of the extra arguments will not match
>> between the two worlds (as Linux will pass an array of unsigned long).
>>
>>
>> With this in mind, I'd like the commit message to be more specific.
>>
>> Afaict, this is not an issue with the convention detection, but rather
>> the invalid to call __scm_smc_call() with 64-bit convention on a 32-bit
>> system. Working around this by having an undocumented #if ARM64 in
>> another part of the driver isn't clear enough, IMHO.
>>
>> Moving the check to __scm_smc_call(), or at least documenting the
>> behavior there (and next to the #if) seems reasonable.
>>
> In terms of disallowing 64-bit convention to be probed on a 32-bit kernel:
>
> Reviewed-By: Elliot Berman <quic_eberman@...cinc.com>
>
> I first thought moving the check to __scm_smc_call() would be better but
> then I realized we would be adding an extra runtime check for each SCM call
> that either always passes or always fails. I think the current #if is best
> as-is, although it would be good to add some comments explaining why as
> Bjorn mentioned.


Thanks everyone for taking time to review the patch! Will add a comment 
above the #if block and post the next version.


>
>> Regards,
>> Bjorn
>>
>>
>>>> Regards,
>>>> Bjorn
>>>>
>>>>> Cc: stable@...r.kernel.org
>>>>> Fixes: 9a434cee773a ("firmware: qcom_scm: Dynamically support SMCCC and legacy conventions")
>>>>> Signed-off-by: Kathiravan T <quic_kathirav@...cinc.com>
>>>>> ---
>>>>> Changes in V2:
>>>>>        - Added the Fixes tag and cc'd stable mailing list
>>>>>
>>>>>   drivers/firmware/qcom_scm.c | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>>>> index fde33acd46b7..db6754db48a0 100644
>>>>> --- a/drivers/firmware/qcom_scm.c
>>>>> +++ b/drivers/firmware/qcom_scm.c
>>>>> @@ -171,6 +171,7 @@ static enum qcom_scm_convention __get_convention(void)
>>>>>        if (likely(qcom_scm_convention != SMC_CONVENTION_UNKNOWN))
>>>>>                return qcom_scm_convention;
>>>>>
>>>>> +#if IS_ENABLED(CONFIG_ARM64)
>>>>>        /*
>>>>>         * Device isn't required as there is only one argument - no device
>>>>>         * needed to dma_map_single to secure world
>>>>> @@ -191,6 +192,7 @@ static enum qcom_scm_convention __get_convention(void)
>>>>>                forced = true;
>>>>>                goto found;
>>>>>        }
>>>>> +#endif
>>>>>
>>>>>        probed_convention = SMC_CONVENTION_ARM_32;
>>>>>        ret = __scm_smc_call(NULL, &desc, probed_convention, &res, true);
>>>>> --
>>>>> 2.17.1
>>>>>
>>>
>>>
>>> -- 
>>> With best wishes
>>> Dmitry