[PATCH 1/3] firmware: arm_scmi: imx: Track copied syslog words

[PATCH 1/3] firmware: arm_scmi: imx: Track copied syslog words

[Sudeep Holla]

The MISC syslog iterator records the number of returned words plus
firmware-reported remaining words in the caller's size output.

The debugfs consumer uses that value to dump its fixed-size buffer. A
malformed or inconsistent response can therefore make it read beyond
the copied data and allocation.

Return the accumulated number of words copied into the caller's buffer
instead. The generic iterator bounds this count to the initial buffer
capacity before processing each response.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-c3c9437c-ba1a-4db6-837f-cb0950d89018
Fixes: 4fa62e80c7db ("firmware: arm_scmi: imx: Support getting syslog of MISC protocol")
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index 637973fb45e6..0c708b2dd496 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -438,7 +438,7 @@ static int iter_misc_syslog_update_state(struct scmi_iterator_state *st,
 
 	st->num_returned = RETURNED(r->numlogflags);
 	st->num_remaining = REMAINING(r->numlogflags);
-	*p->size = st->num_returned + st->num_remaining;
+	*p->size = st->desc_index + st->num_returned;
 
 	return 0;
 }
-- 
2.43.0

[PATCH 2/3] firmware: arm_scmi: imx: Validate MISC control response size

[Sudeep Holla]

scmi_imx_misc_ctrl_get() uses @num to report the number of
values returned by firmware, but it did not retain the caller's value
as the capacity of @val.

A response with more values than the caller allocated was copied into
@val unchecked. For example, the MQS regmap callback supplies storage
for one value. Retain the input capacity and reject responses exceeding
it before copying, then update @num only after a successful copy.

Document the in/out @num contract of the exported helper.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-6897b44a-57fb-4964-95f8-447d0ce526e4
Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c  | 16 ++++++++++++----
 include/linux/firmware/imx/sm.h                  |  6 ++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index 0c708b2dd496..f00f7acba845 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -273,10 +273,16 @@ static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
 {
 	struct scmi_imx_misc_ctrl_get_out *out;
 	struct scmi_xfer *t;
+	u32 num_values, max_values;
 	int ret, i;
 	int max_msg_size = ph->hops->get_max_msg_size(ph);
 	int max_num = (max_msg_size - sizeof(*out)) / sizeof(__le32);
 
+	if (!num || !val || !*num)
+		return -EINVAL;
+
+	max_values = *num;
+
 	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
 	if (ret)
 		return ret;
@@ -290,16 +296,18 @@ static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
 	ret = ph->xops->do_xfer(ph, t);
 	if (!ret) {
 		out = t->rx.buf;
-		*num = le32_to_cpu(out->num);
+		num_values = le32_to_cpu(out->num);
 
-		if (*num >= max_num ||
-		    *num * sizeof(__le32) > t->rx.len - sizeof(__le32)) {
+		if (num_values > max_values || num_values >= max_num ||
+		    num_values * sizeof(__le32) > t->rx.len - sizeof(__le32)) {
 			ph->xops->xfer_put(ph, t);
 			return -EINVAL;
 		}
 
-		for (i = 0; i < *num; i++)
+		for (i = 0; i < num_values; i++)
 			val[i] = le32_to_cpu(out->val[i]);
+
+		*num = num_values;
 	}
 
 	ph->xops->xfer_put(ph, t);
diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
index ba5d93bd6158..0aec7da0ae78 100644
--- a/include/linux/firmware/imx/sm.h
+++ b/include/linux/firmware/imx/sm.h
@@ -29,6 +29,12 @@
 #define SCMI_IMX952_CTRL_BYPASS_AUDMIX	8U      /* WAKE AUDMIX */
 
 #if IS_ENABLED(CONFIG_IMX_SCMI_MISC_DRV)
+/**
+ * scmi_imx_misc_ctrl_get() - Read an i.MX MISC control
+ * @id: Control identifier
+ * @num: On entry, capacity of @val in words; on return, words read
+ * @val: Buffer for the control value
+ */
 int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
 int scmi_imx_misc_ctrl_set(u32 id, u32 val);
 #else
-- 
2.43.0

[PATCH 3/3] firmware: arm_scmi: Unregister notifier on enable failure

[Sudeep Holla]

scmi_notifier_register() adds the caller's notifier block before
enabling the corresponding event at the platform.

If enablement fails, the caller receives an error and can free its
notifier block. A concurrent registration can retain and later enable
the handler, leaving the failed caller's block in its notification
chain. Event delivery can then dereference the freed block.

Remove the notifier block before dropping the handler reference when
event enablement fails.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-c3c9437c-ba1a-4db6-837f-cb0950d89018
Fixes: e7c215f358a3 ("firmware: arm_scmi: Add notification callbacks-registration")
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/firmware/arm_scmi/notify.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index fb60fb07c0ed..e5de0d29c346 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -1395,8 +1395,10 @@ static int scmi_notifier_register(const struct scmi_handle *handle,
 	/* Enable events for not pending handlers */
 	if (!IS_HNDL_PENDING(hndl)) {
 		ret = scmi_event_handler_enable_events(hndl);
-		if (ret)
+		if (ret) {
+			blocking_notifier_chain_unregister(&hndl->chain, nb);
 			scmi_put_handler(ni, hndl);
+		}
 	}
 
 	return ret;
-- 
2.43.0

Re: [PATCH 1/3] firmware: arm_scmi: imx: Track copied syslog words

[Peng Fan]

On Wed, Oct 28, 2026 at 01:05:00PM +0000, Sudeep Holla wrote:
>The MISC syslog iterator records the number of returned words plus
>firmware-reported remaining words in the caller's size output.
>
>The debugfs consumer uses that value to dump its fixed-size buffer. A
>malformed or inconsistent response can therefore make it read beyond
>the copied data and allocation.
>
>Return the accumulated number of words copied into the caller's buffer
>instead. The generic iterator bounds this count to the initial buffer
>capacity before processing each response.
>
>Reported-by: Sashiko <sashiko-bot@kernel.org>
>Closes: https://sashiko.dev/#/bug/linux-c3c9437c-ba1a-4db6-837f-cb0950d89018

This should be: https://sashiko.dev/#/bug/linux-67e325a9-473e-450a-a2a0-bf4c0c720624

>Fixes: 4fa62e80c7db ("firmware: arm_scmi: imx: Support getting syslog of MISC protocol")
>Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 2/3] firmware: arm_scmi: imx: Validate MISC control response size

[Peng Fan]

On Wed, Oct 28, 2026 at 01:05:01PM +0000, Sudeep Holla wrote:
>scmi_imx_misc_ctrl_get() uses @num to report the number of
>values returned by firmware, but it did not retain the caller's value
>as the capacity of @val.
>
>A response with more values than the caller allocated was copied into
>@val unchecked. For example, the MQS regmap callback supplies storage
>for one value. Retain the input capacity and reject responses exceeding
>it before copying, then update @num only after a successful copy.
>
>Document the in/out @num contract of the exported helper.
>
>Reported-by: Sashiko <sashiko-bot@kernel.org>
>Closes: https://sashiko.dev/#/bug/linux-6897b44a-57fb-4964-95f8-447d0ce526e4
>Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
>Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 3/3] firmware: arm_scmi: Unregister notifier on enable failure

[Peng Fan]

On Wed, Oct 28, 2026 at 01:05:02PM +0000, Sudeep Holla wrote:
>scmi_notifier_register() adds the caller's notifier block before
>enabling the corresponding event at the platform.
>
>If enablement fails, the caller receives an error and can free its
>notifier block. A concurrent registration can retain and later enable
>the handler, leaving the failed caller's block in its notification
>chain. Event delivery can then dereference the freed block.
>
>Remove the notifier block before dropping the handler reference when
>event enablement fails.
>
>Reported-by: Sashiko <sashiko-bot@kernel.org>
>Closes: https://sashiko.dev/#/bug/linux-c3c9437c-ba1a-4db6-837f-cb0950d89018
>Fixes: e7c215f358a3 ("firmware: arm_scmi: Add notification callbacks-registration")
>Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 1/3] firmware: arm_scmi: imx: Track copied syslog words

[Sudeep Holla]

On Wed, Oct 28, 2026 at 09:49:09PM +0800, Peng Fan wrote:
> On Wed, Oct 28, 2026 at 01:05:00PM +0000, Sudeep Holla wrote:
> >The MISC syslog iterator records the number of returned words plus
> >firmware-reported remaining words in the caller's size output.
> >
> >The debugfs consumer uses that value to dump its fixed-size buffer. A
> >malformed or inconsistent response can therefore make it read beyond
> >the copied data and allocation.
> >
> >Return the accumulated number of words copied into the caller's buffer
> >instead. The generic iterator bounds this count to the initial buffer
> >capacity before processing each response.
> >
> >Reported-by: Sashiko <sashiko-bot@kernel.org>
> >Closes: https://sashiko.dev/#/bug/linux-c3c9437c-ba1a-4db6-837f-cb0950d89018
> 
> This should be: https://sashiko.dev/#/bug/linux-67e325a9-473e-450a-a2a0-bf4c0c720624
>

Thanks, my bad copy-paste error. Thought it was copied but no 😢.

> >Fixes: 4fa62e80c7db ("firmware: arm_scmi: imx: Support getting syslog of MISC protocol")
> >Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> 
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> 

Thanks!

-- 
Regards,
Sudeep

Re: [PATCH 2/3] firmware: arm_scmi: imx: Validate MISC control response size

[Philip Radford]

On Wed, Oct 28, 2026 at 01:05:01PM +0000, Sudeep Holla wrote:
> scmi_imx_misc_ctrl_get() uses @num to report the number of
> values returned by firmware, but it did not retain the caller's value
> as the capacity of @val.
> 

Hi,

> A response with more values than the caller allocated was copied into
> @val unchecked. For example, the MQS regmap callback supplies storage
> for one value. Retain the input capacity and reject responses exceeding
> it before copying, then update @num only after a successful copy.
> 
> Document the in/out @num contract of the exported helper.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/bug/linux-6897b44a-57fb-4964-95f8-447d0ce526e4
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c  | 16 ++++++++++++----
>  include/linux/firmware/imx/sm.h                  |  6 ++++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> index 0c708b2dd496..f00f7acba845 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
> @@ -273,10 +273,16 @@ static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
>  {
>  	struct scmi_imx_misc_ctrl_get_out *out;
>  	struct scmi_xfer *t;
> +	u32 num_values, max_values;
>  	int ret, i;
>  	int max_msg_size = ph->hops->get_max_msg_size(ph);
>  	int max_num = (max_msg_size - sizeof(*out)) / sizeof(__le32);
>  
> +	if (!num || !val || !*num)
> +		return -EINVAL;
> +
> +	max_values = *num;
> +
>  	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
>  	if (ret)
>  		return ret;
> @@ -290,16 +296,18 @@ static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
>  	ret = ph->xops->do_xfer(ph, t);
>  	if (!ret) {
>  		out = t->rx.buf;
> -		*num = le32_to_cpu(out->num);
> +		num_values = le32_to_cpu(out->num);
>  
> -		if (*num >= max_num ||
> -		    *num * sizeof(__le32) > t->rx.len - sizeof(__le32)) {
> +		if (num_values > max_values || num_values >= max_num ||
> +		    num_values * sizeof(__le32) > t->rx.len - sizeof(__le32)) {
>  			ph->xops->xfer_put(ph, t);
>  			return -EINVAL;
>  		}

Why does num_values have to be less than max_num, can it not be equal to? Isn't max_num
already smaller than max_msg_size due to the initialisation above?

Regards,
Phil