Re: [PATCH 05/11] firmware: arm_scmi: stop reading past PINCTRL_LIST_ASSOCIATIONS replies

[Clément Le Goffic]

Hi Cristian,

On 11/10/26 11:27 AM, Cristian Marussi wrote:
> On Fri, Oct 30, 2026 at 04:58:36PM +0100, Clément Le Goffic wrote:
>> The transport cuts a reply larger than the receive buffer, but the iterator
>> still reads every identifier the reply announces. Use only the identifiers
>> that were copied and ask again for the others.
> 
> Hi Clement,
> 
> thanks for these fixes...I'll start from this since it really intersects
> my current work....
> 
>>
>> Fixes: 2145af01b9cb ("firmware: arm_scmi: Add basic support for SCMI v3.2 pincontrol protocol")
>> Signed-off-by: Clément Le Goffic <clegoffic@xxxxxxxxxxxx>
>> ---
>>  drivers/firmware/arm_scmi/pinctrl.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
>> index 42cb1aef1fe1..69645ce73d9d 100644
>> --- a/drivers/firmware/arm_scmi/pinctrl.c
>> +++ b/drivers/firmware/arm_scmi/pinctrl.c
>> @@ -261,10 +261,22 @@ static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
>>  					   const void *response, void *priv)
>>  {
>>  	const struct scmi_resp_pinctrl_list_assoc *r = response;
>> +	unsigned int nr_fit = 0;
>>  
>>  	st->num_returned = RETURNED(r->flags);
>>  	st->num_remaining = REMAINING(r->flags);
>>  
>> +	if (st->rx_len > sizeof(*r))
>> +		nr_fit = (st->rx_len - sizeof(*r)) / sizeof(r->array[0]);
> 
> As Sashiko kindly pointed out to me a few other zillion times O_o in my
> Telemetry series, there were a couple of missing sanity checks in the
> iterators core so in my last series there is upfront a generic fix
> 
> https://lore.kernel.org/arm-scmi/avLTkAtQQSzpgu2A@pluto/T/#me258191dd55db99d53b570be7c51901c31bc70cf
> 
> ...so we could at least avoid to check here and everywhere else in SCMI
> iterators that you have at least a minimum size messages to work on.
> 
>> +
>> +	if (st->num_returned > nr_fit) {
>> +		if (!nr_fit)
>> +			return -EPROTO;
> 
> My core fix above will discard the iteration upfront in an equivalent way
> to this check togateher with the rx_len check above If I get this wrong...BUT..

Right, v2 keeps only your check below

> 
>> +
>> +		st->num_remaining += st->num_returned - nr_fit;
>> +		st->num_returned = nr_fit;
> 
> ...why are you fixing and carry on ?
> I dont feel that you can safely proceed at this point by just fixing the
> returned and the remaining...

Each request carries the index to restart from, so it stays in sync but it hides the DT misconfiguration

> 
> ...the check is of course needed, but it should be an hard check
> 
> 	if (st->rx_len < (sizeof(*r) + sizeof(r->array[0]) * st->num_returned))
>                 return -EINVAL;
> 
> ...because something is clearly wrong...indeed, learning from my current
> telemetry madness the above scenario could be due not only to a buggy FW
> BUT more easily to a misconfigured transport...if the transport size is
> misconfigured (as it is mostly NOT negotiated, simply configured) any
> message long enough will be silently truncated, so the fact especially
> that you guess remaining it can happen to be completely out of sync with
> the platform side...

I've seen your serie but didn't check such details. I'll use this hard check in the v2.

> 
> Are you fixing an existent known FW bug here ? in that case I would opt
> for a quirk...

No firmware bug, just the DT that had no arm,max-msg-size. Linux was using its default value that was not sync with my SCMI server value.
Setting it fixed it, no quirk needed.

Best regards,
Clément