Re: [RFC PATCH] firmware: arm_scmi: skip empty CLOCK_DESCRIBE_RATES replies

[Jay Buddhabhatti <jbuddhab@amd.com>]

Hi Sudeep,

Thanks for the review. Please find my response inline.

On 10/1/2026 8:14 PM, Sudeep Holla wrote:
> On Thu, Oct 01, 2026 at 04:45:38AM -0700, Jay Buddhabhatti wrote:
>> Some platforms advertise reserved or uninstantiated clock IDs that still
>> succeed CLOCK_DESCRIBE_RATES with zero rates. After dynamic rate
>> allocation, kcalloc(0) returns ZERO_SIZE_PTR and protocol init then
>> dereferences rates[0], which panics.
>>
>> Do not allocate or index the rate array when the firmware reports an
>> empty list, so unused IDs are skipped instead of taking down the SCMI
>> clock provider.
>>
>> Fixes: 62ba967595e0 ("firmware: arm_scmi: Make clock rates allocation dynamic")
>> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
>> ---
>> The SCMI server is the source of this zero rate and successful response
>> and it should be fixed in SCMI server. This defensive check in Linux is
>> still useful because firmware responses must be validated before
>> de-referencing dynamically allocated data, The panic is a Linux
>> regression introduced by dynamic rate allocation; previous fixed array
>> tolerated the same response and other SCMI implementations could return
>> the same unexpected response.
>> ---
>>   drivers/firmware/arm_scmi/clock.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
>> index 0278705d809e..8934a95527e2 100644
>> --- a/drivers/firmware/arm_scmi/clock.c
>> +++ b/drivers/firmware/arm_scmi/clock.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/math64.h>
>>   #include <linux/module.h>
>>   #include <linux/limits.h>
>> +#include <linux/slab.h>
>>   #include <linux/sort.h>
>>   
>>   #include "protocols.h"
>> @@ -484,6 +485,13 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
>>   	if (!st->max_resources) {
>>   		unsigned int tot_rates = st->num_returned + st->num_remaining;
>>   
>> +		/*
>> +		 * Unused/reserved clock IDs return 0 rates. kmalloc(0)
>> +		 * returns ZERO_SIZE_PTR and must not be dereferenced.
>> +		 */
>> +		if (!tot_rates)
>> +			return 0;
>> +
>>   		p->clkd->r.rates = devm_kcalloc(p->dev, tot_rates,
>>   						sizeof(*p->clkd->r.rates), GFP_KERNEL);
>>   		if (!p->clkd->r.rates)
>> @@ -505,6 +513,9 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
>>   	struct scmi_clk_ipriv *p = priv;
>>   	const struct scmi_msg_resp_clock_describe_rates *r = response;
>>   
>> +	if (ZERO_OR_NULL_PTR(p->clkd->r.rates))
>> +		return -EPROTO;
>> +
>>   	p->clkd->r.rates[p->clkd->r.num_rates] = RATE_TO_U64(r->rate[st->loop_idx]);
>>   
>>   	/* Count only effectively discovered rates */
>> @@ -622,6 +633,13 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph,
>>   	if (ret)
>>   		return ret;
>>   
>> +	/*
>> +	 * Some platforms expose reserved clock IDs with an empty
>> +	 * CLOCK_DESCRIBE_RATES reply. Do not dereference rates[].
>> +	 */
>> +	if (!clkd->r.num_rates || ZERO_OR_NULL_PTR(clkd->r.rates))
>> +		return 0;
>> +
> 
> I expect the Clock rate control bit to be unset in the permissions for
> these clock, else it may be dangerous to do this. Please add that check.

I will add that check in new version. If the Clock rate control bit is 
set, Linux clears its local copy by setting rate_ctrl_forbidden. The 
clock remains registered, but clk-scmi does not install set_rate and 
scmi_clock_rate_set() returns -EACCES. This avoids exposing rate changes 
when no valid rates were described.

Regards,
Jay
>