linux-kernel - Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

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: <161734672825.2260335.8472441215895199196@swboyd.mtv.corp.google.com>
Date:   Thu, 01 Apr 2021 23:58:48 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Elliot Berman <eberman@...eaurora.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Brian Masney <masneyb@...tation.org>,
        Stephan Gerhold <stephan@...hold.net>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>
Subject: Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM

Quoting Elliot Berman (2021-04-01 18:12:14)
> 
> It might be a good idea to wrap these lines from qcom_scm_call with #if 
> IS_ENABLED(CONFIG_ARM), and the corresponding ones in qcom_scm_call_atomic:
> 
>    case SMC_CONVENTION_LEGACY:
>        return scm_legacy_call(dev, desc, res);
> 
> If something is wrong with loaded firmware and LEGACY convention is 
> incorrectly selected, you would get a better hint about the problem: 
> "Unknown current SCM calling convention." You would still get the hint 
> earlier from __get_convention, but that may not be obvious to someone 
> unfamiliar with the SCM driver.
> 
> I'll defer to your/Bjorn's preference:
> 
> Acked-by: Elliot Berman <eberman@...eaurora.org>
> 
> with or without modifying qcom_scm_call{_atomic}.
> 

Maybe it would be better to catch that problem at the source and force
arm64 calling convention to be safe? I'm thinking of this patch, but it
could be even more fine tuned and probably the sc7180 check could be
removed in the process if the rest of this email makes sense.

If I understand correctly CONFIG_ARM64=y should never use legacy
convention (and never the 32-bit one either?), so we can figure that out
pretty easily and just force it to use 64-bit if the architecture is
arm64. If only the 64-bit convention is supported on arm64 then we
really don't even need to call into the firmware to figure it out on
arm64. We can do this convention detection stuff on arm32 (CONFIG_ARM)
and always assume 64-bit convention on CONFIG_ARM64. Is that right?

---8<---
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 747808a8ddf4..22187ebc1678 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -140,7 +140,13 @@ static enum qcom_scm_convention __get_convention(void)
 	if (!ret && res.result[0] == 1)
 		goto found;
 
-	probed_convention = SMC_CONVENTION_LEGACY;
+	if (IS_ENABLED(CONFIG_ARM)) {
+		probed_convention = SMC_CONVENTION_LEGACY;
+	} else {
+		probed_convention = SMC_CONVENTION_ARM_64;
+		forced = true;
+		WARN(1, "qcom_scm: Detected legacy convention on arm64; firmware is broken\n");
+	}
 found:
 	spin_lock_irqsave(&scm_query_lock, flags);
 	if (probed_convention != qcom_scm_convention) {