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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Z8AjhHsVT9ZQTtZX@pluto>
Date: Thu, 27 Feb 2025 08:34:44 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Johan Hovold <johan@...nel.org>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
	Sibi Sankar <quic_sibis@...cinc.com>, 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 10:58:44AM +0100, Johan Hovold wrote:
> On Wed, Feb 26, 2025 at 12:31:27PM +0300, Dan Carpenter wrote:
> > On Wed, Feb 26, 2025 at 09:55:21AM +0100, Johan Hovold wrote:
> > > On Wed, Feb 26, 2025 at 09:12:23AM +0100, Johan Hovold wrote:
> > > > On Wed, Feb 26, 2025 at 08:13:38AM +0530, Sibi Sankar wrote:
> > > 
> > > > >  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.
> > > 

Hi,

> > > After taking a closer look, perhaps it needs to be done along these
> > > lines.
> > > 
> > > But calling the parameter 'force' or similar as Dan suggested should
> > > make it more readable.
> > > 
> > > > >  {
> > > > >  	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.
> > > 
> > > My idea here was that you could come up with some way of abstracting
> > > this so that you did not have to update every call site. Not sure how
> > > feasible that is.
> > 
> > I'm having a hard time finding your email.
> 
> 	https://lore.kernel.org/lkml/Z4Dt8E7C6upVtEGV@hovoldconsulting.com/
>  
> > Why does the scmi_proto_helpers_ops struct even exist?  We could just
> > call all these functions directly.  Do we have plans to actually create
> > different implementations?
> > 
> > If we got rid of the scmi_proto_helpers_ops struct then we could just
> > rename scmi_common_fastchannel_init() to __scmi_common_fastchannel_init()
> > and create a default wrapper around it and a _forced() wrapper.
> > 
> > Some other potentially stupid ideas in the spirit of brainstorming are
> > that we could add a quirks parameter which takes a flag instead of a
> > bool.  Or we could add a quirks flag to the scmi_protocol_handle struct.
> 
> Something like that, yes. :) I didn't try to implement it, but it seems
> like it should be possible implement this is a way that keeps the quirk
> handling isolated.
>

I hope next week to have a better look at this, in tne meantime just a
few considerations....

Sooner or later we should have introduced some sort of quirk framework
in SCMI to deal systematically with potentially out-of-spec FW, but as
in the name, it should be some sort of framework where you have a table of
quirks, related activation conditions and a few very well isolated points
where the quirks are placed and take action if enabled...this does not
seem the case here where instead an ad-hoc param is added to the function
that needs to be quirked...this does not scale and will make the codebase
a mess IMHO...

Last but not least, these quirks 'activations' in the SCMI world should
be driven mainly by the VENDOR/SUB-VENDOR/IMPLEMENTATION_VERS triple and
only as a last resort by a platform compatible match...because the
IMPLEMENTATION_VERSION, exposed by the FW and gathered via SCMI Base
protocol, is completely under the vendor control so it can, and should, be
used to identify broken FW-versions...indeed all the distinct SCMI protocols
are anyway versioned elsewhere according to the spec, so there is no need to
repeat SCMI protocol version here..I mean it is an IMPLEMENTATION version

As an example on a JUNO the SCP reference FW reports:

arm-scmi arm-scmi.1.auto: SCMI Protocol v2.0 'arm:arm' Firmware version 0x20f0000

where the FW version represent something like the FW release tag, I think...not
really sure about the menaing our FW gys give to it, BUT definitely it is not
a bare copy of the SCMI protocol version...

So...
...does the platfom-to-be-quirked at hand properly use the IMPLEMENTATION_VERSION
flag in a sensible way ?
IOW does that change between a bad and good (or less bad :D) version ?
Because shooting with the platform 'compatible-gun' should be the last resort
if all of the above does NOT happen in this beloved fw...

Anyway, after all of this babbling, I know, talk is cheap :D...so now I will shut
up and see if I can prototype something generic to deal with quirks, possibly
next week...

Thanks,
Cristian