linux-kernel - Re: [PATCH v3 2/4] firmware: qcom

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ff9ec6f5-9c7c-546b-5814-159d7e2843a8@quicinc.com>
Date:   Tue, 22 Aug 2023 21:07:59 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Robert Marko <robimarko@...il.com>, <agross@...nel.org>,
        <andersson@...nel.org>, <konrad.dybcio@...aro.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <conor+dt@...nel.org>, <quic_gurus@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC:     <computersforpeace@...il.com>
Subject: Re: [PATCH v3 2/4] firmware: qcom_scm: disable SDI if required



On 8/16/2023 10:15 PM, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
> 
> Luckily, SDI can be disabled via an SCM call.
> 
> So, lets use the boolean DT property to identify boards that have SDI
> enabled by default and use the SCM call to disable SDI during SCM probe.
> It is important to disable it as soon as possible as we might have a WDT
> assertion at any time which would then leave the board in debug mode,
> thus disabling it during SCM removal is not enough.
> 
> Signed-off-by: Robert Marko <robimarko@...il.com>
> ---
> Changes in v3:
> * Squashed ("firmware: qcom: scm: Add SDI disable support") and
> ("firmware: qcom_scm: disable SDI if required")
> ---
>   drivers/firmware/qcom_scm.c | 29 +++++++++++++++++++++++++++++
>   drivers/firmware/qcom_scm.h |  1 +
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 06fe8aca870d..de9d1a11d097 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>   }
>   EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
>   
> +static int qcom_scm_disable_sdi(void)
> +{
> +	int ret;
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_BOOT,
> +		.cmd = QCOM_SCM_BOOT_SDI_CONFIG,
> +		.args[0] = 1, /* Disable watchdog debug */
> +		.args[1] = 0, /* Disable SDI */
> +		.arginfo = QCOM_SCM_ARGS(2),
> +		.owner = ARM_SMCCC_OWNER_SIP,
> +	};
> +	struct qcom_scm_res res;
> +
> +	ret = qcom_scm_clk_enable();
> +	if (ret)
> +		return ret;
> +	ret = qcom_scm_call(__scm->dev, &desc, &res);

Would you not be wanting this call to be atomic ?

> +
> +	qcom_scm_clk_disable();
> +
> +	return ret ? : res.result[0];
> +}
> +
>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   {
>   	struct qcom_scm_desc desc = {
> @@ -1468,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	if (download_mode)
>   		qcom_scm_set_download_mode(true);
>   
> +	/*
> +	 * Disable SDI if indicated by DT that it is enabled by default.
> +	 */
> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
> +		qcom_scm_disable_sdi();

Why don't we do this call in qcom_scm_shutdown()
also does it not conflict with above download_mode
we have enabled download mode but disabling SDI
means (hard reset) and will not be collecting
crash dump?

-Mukesh

> +
>   	return 0;
>   }
>   
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index e6e512bd57d1..7b68fa820495 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>   #define QCOM_SCM_SVC_BOOT		0x01
>   #define QCOM_SCM_BOOT_SET_ADDR		0x01
>   #define QCOM_SCM_BOOT_TERMINATE_PC	0x02
> +#define QCOM_SCM_BOOT_SDI_CONFIG	0x09
>   #define QCOM_SCM_BOOT_SET_DLOAD_MODE	0x10
>   #define QCOM_SCM_BOOT_SET_ADDR_MC	0x11
>   #define QCOM_SCM_BOOT_SET_REMOTE_STATE	0x0a