linux-kernel - Re: [RFC V6 2/2] firmware: arm_scmi: Add quirk to bypass SCP fw bug

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: <Z77M5iXHQsdMptWm@hovoldconsulting.com>
Date: Wed, 26 Feb 2025 09:12:22 +0100
From: Johan Hovold <johan@...nel.org>
To: Sibi Sankar <quic_sibis@...cinc.com>
Cc: sudeep.holla@....com, cristian.marussi@....com,
	dmitry.baryshkov@...aro.org, maz@...nel.org,
	linux-kernel@...r.kernel.org, arm-scmi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
	konradybcio@...nel.org
Subject: Re: [RFC V6 2/2] firmware: arm_scmi: Add quirk to bypass SCP fw bug

On Wed, Feb 26, 2025 at 08:13:38AM +0530, Sibi Sankar wrote:
> The addition of per message-id fastchannel support check exposed
> a SCP firmware bug which leads to a device crash on X1E platforms.
> The X1E firmware supports fastchannel on LEVEL_GET but fails to
> have this set in the protocol message attributes and the fallback
> to regular messaging leads to a device crash since that implementation
> has a bug for all the X1E devices in the wild. Fix this by introducing
> a quirk that selectively skips the per message-id fastchannel check only
> for the LEVEL_GET message on X1E platforms.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
> ---
>  drivers/firmware/arm_scmi/driver.c    |  5 +++--
>  drivers/firmware/arm_scmi/perf.c      | 30 +++++++++++++++++++++------
>  drivers/firmware/arm_scmi/powercap.c  |  8 +++----
>  drivers/firmware/arm_scmi/protocols.h |  2 +-
>  4 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 9313b9020fc1..b182fa8e8ccb 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1903,7 +1903,8 @@ static void
>  scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
>  			     u8 describe_id, u32 message_id, u32 valid_size,
>  			     u32 domain, void __iomem **p_addr,
> -			     struct scmi_fc_db_info **p_db, u32 *rate_limit)
> +			     struct scmi_fc_db_info **p_db, u32 *rate_limit,
> +			     bool skip_check)

This does not look like it will scale.

>  {
>  	int ret;
>  	u32 flags;
> @@ -1919,7 +1920,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
>  
>  	/* Check if the MSG_ID supports fastchannel */
>  	ret = scmi_protocol_msg_check(ph, message_id, &attributes);
> -	if (!ret && !MSG_SUPPORTS_FASTCHANNEL(attributes))
> +	if (!ret && !MSG_SUPPORTS_FASTCHANNEL(attributes) && !skip_check)

Why can't you just make sure that the bit is set in attributes as I
suggested? That seems like it should allow for a minimal implementation
of this.

>  		return;
>  
>  	if (!p_addr) {

> @@ -1282,6 +1288,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
>  {
>  	int domain, ret;
>  	u32 version;
> +	struct device_node *np;
>  	struct scmi_perf_info *pinfo;
>  
>  	ret = ph->xops->version_get(ph, &version);
> @@ -1297,6 +1304,17 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
>  
>  	pinfo->version = version;
>  
> +	/*
> +	 * Some X1E devices support fastchannel for LEVEL_GET but erroneously
> +	 * says otherwise in the protocol message attributes. Add a quirk to
> +	 * force fastchannel on LEVEL_GET to prevent crashes on such devices.
> +	 */
> +	np = of_find_compatible_node(NULL, NULL, "qcom,x1e80100");

Here you should use of_machine_is_compatible().

> +	if (np) {
> +		pinfo->quirks = BIT(PERF_QUIRK_SKIP_FASTCHANNEL_LEVEL_GET);
> +		of_node_put(np);
> +	}
> +
>  	ret = scmi_perf_attributes_get(ph, pinfo);
>  	if (ret)
>  		return ret;

Johan