linux-kernel - Re: [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem

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: <24d32753-c769-02f6-0ece-a3e69761f400@quicinc.com>
Date:   Fri, 26 May 2023 19:57:26 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Christian Marangi <ansuelsmth@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     Robert Marko <robimarko@...il.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH] soc: qcom: mdt_loader: Fix unconditional call to
 scm_pas_mem_setup



On 5/26/2023 5:25 PM, Christian Marangi wrote:
> Commit ebeb20a9cd3f ("soc: qcom: mdt_loader: Always invoke PAS
> mem_setup") dropped the relocate check and made pas_mem_setup run
> unconditionally. The code was later moved with commit f4e526ff7e38
> ("soc: qcom: mdt_loader: Extract PAS operations") to
> qcom_mdt_pas_init() effectively losing track of what was actually
> done.
> 
> The assumption that PAS mem_setup can be done anytime was effectively
> wrong, with no good reason and this caused regression on some SoC
> that use remoteproc to bringup ath11k. One example is IPQ8074 SoC that
> effectively broke resulting in remoteproc silently die and ath11k not
> working.
> 
> On this SoC FW relocate is not enabled and PAS mem_setup was correctly
> skipped in previous kernel version resulting in correct bringup and
> function of remoteproc and ath11k.
> 
> To fix the regression, reintroduce the relocate check in
> qcom_mdt_pas_init() and correctly skip PAS mem_setup where relocate is
> not enabled.
> 
> Fixes: ebeb20a9cd3f ("soc: qcom: mdt_loader: Always invoke PAS mem_setup")
> Tested-by: Robert Marko <robimarko@...il.com>
> Co-developed-by: Robert Marko <robimarko@...il.com>
> Signed-off-by: Robert Marko <robimarko@...il.com>
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> Cc: stable@...r.kernel.org

Thanks.

> ---
>   drivers/soc/qcom/mdt_loader.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 33dd8c315eb7..46820bcdae98 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -210,6 +210,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   	const struct elf32_hdr *ehdr;
>   	phys_addr_t min_addr = PHYS_ADDR_MAX;
>   	phys_addr_t max_addr = 0;
> +	bool relocate = false;
>   	size_t metadata_len;
>   	void *metadata;
>   	int ret;
> @@ -224,6 +225,9 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   		if (!mdt_phdr_valid(phdr))
>   			continue;
>   
> +		if (phdr->p_flags & QCOM_MDT_RELOCATABLE)
> +			relocate = true;
> +
>   		if (phdr->p_paddr < min_addr)
>   			min_addr = phdr->p_paddr;
>   
> @@ -246,11 +250,13 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   		goto out;
>   	}
>   
> -	ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);
> -	if (ret) {
> -		/* Unable to set up relocation */
> -		dev_err(dev, "error %d setting up firmware %s\n", ret, fw_name);
> -		goto out;
> +	if (relocate) {
> +		ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);
> +		if (ret) {
> +			/* Unable to set up relocation */
> +			dev_err(dev, "error %d setting up firmware %s\n", ret, fw_name);
> +			goto out;
> +		}
>   	}
>   
>   out:

LGTM, We still carry this in our downstream kernel for legacy reason.
Reviewed-by: Mukesh Ojha <quic_mojha@...cinc.com>

-- Mukesh