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

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

[Jay Buddhabhatti]

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;
+
 	clkd->info.min_rate = clkd->r.rates[RATE_MIN];
 	if (!clkd->r.rate_discrete) {
 		clkd->info.max_rate = clkd->r.rates[RATE_MAX];
-- 
2.34.1

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Jay Buddhabhatti]

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
> 

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

[Sudeep Holla]

On Mon, Oct 05, 2026 at 06:06:09PM +0530, Jay Buddhabhatti wrote:
> 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.
> 

You did mention this is issue in the firmware. If the generic solution(once
agreed upon) doesn't work on your platform, then you need to fix it with
a quirk I am afraid. I will let you propose the patch and take it from there.

-- 
Regards,
Sudeep