[PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[Li Jun]

Add a proper NULL check for the kmalloc() return value in syslog_show().
If memory allocation fails, syslog would be NULL and passing it to
misc_syslog() could lead to a NULL pointer dereference.

Signed-off-by: Li Jun <lijun01@kylinos.cn>
---
 drivers/firmware/imx/sm-misc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index ac9af824c2d4..fb8d7bdb5b08 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -79,6 +79,9 @@ static int syslog_show(struct seq_file *file, void *priv)
 	u16 size = SZ_4K / 4;
 	int ret;
 
+	if (!syslog)
+		return -ENOMEM;
+
 	if (!ph)
 		return -ENODEV;
 
-- 
2.25.1

Re: [PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Iterator state mismanagement causes out-of-bounds read, kernel memory leak, and output truncation in syslog_show.
- [High] Global state mismanagement leads to a Use-After-Free vulnerability and permanently broken probing.
- [High] Reusing a single `notifier_block` instance across multiple notification chains corrupts the linked lists.
- [High] Unvalidated SCMI firmware reset reason causes an array out-of-bounds read.
- [High] Lack of validation of `st->num_returned` against the SCMI RX buffer length allows an out-of-bounds read of the incoming SCMI message.
- [High] An integer underflow in `scmi_imx_misc_ctrl_get` allows a short firmware response to bypass bounds checking and leak stale RX buffer data.
--

commit 2dfb0ee488971a3d76d835d309316ae287807482
Author: Li Jun <lijun01@kylinos.cn>

firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

Add a proper NULL check for the kmalloc() return value in syslog_show().
If memory allocation fails, syslog would be NULL and passing it to
misc_syslog() could lead to a NULL pointer dereference.

> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index ac9af824c2d40..fb8d7bdb5b084 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -79,6 +79,9 @@ static int syslog_show(struct seq_file *file, void *priv)
>  	u16 size = SZ_4K / 4;
>  	int ret;
>  
> +	if (!syslog)
> +		return -ENOMEM;
> +
>  	if (!ph)
>  		return -ENODEV;
>  

[Severity: High]
This is a pre-existing issue, but does the size parameter get blindly
overwritten by the firmware response?

Looking at iter_misc_syslog_update_state():

iter_misc_syslog_update_state()
    *p->size = st->num_returned + st->num_remaining;

If the firmware returns an arbitrarily large num_remaining, it overwrites
the caller's size without bounds checking. Then, further down in
syslog_show(), this overwritten size is used for the hex dump:

syslog_show()
    seq_hex_dump(file, " ", DUMP_PREFIX_NONE, 16, sizeof(u32), syslog,
                 size * 4, false);

Since the buffer is strictly limited to 4KB via kmalloc(SZ_4K), could a
malicious or buggy firmware response set size up to 8190, causing an
out-of-bounds read of up to 12KB of uninitialized kernel heap memory and
leaking it to userspace? 

In the case of a multi-part response, will the output also be silently
truncated because size is replaced with the size of the last chunk
instead of being accumulated?

[Severity: High]
This is a pre-existing issue, but does storing the dynamically managed
protocol handle into global variables without a cleanup action lead to a
use-after-free?

In scmi_imx_misc_ctrl_probe():

scmi_imx_misc_ctrl_probe()
    imx_misc_ctrl_ops = handle->devm_protocol_get(sdev,
                                        SCMI_PROTOCOL_IMX_MISC, &ph);

If the probe subsequently fails or the device is later unbound, devres
automatically releases the protocol handle, but the global pointers
imx_misc_ctrl_ops and ph are never cleared.

Could exported APIs like scmi_imx_misc_ctrl_get() then blindly dereference
the dangling ph pointer after the handle has been released?

scmi_imx_misc_ctrl_get()
    if (!ph)
        return -EPROBE_DEFER;

    return imx_misc_ctrl_ops->misc_ctrl_get(ph, id, num, val);

Furthermore, will the driver permanently fail to probe on subsequent
attempts because of a lingering if (imx_misc_ctrl_ops) check?

[Severity: High]
This is a pre-existing issue, but does reusing a single static
notifier_block across multiple notification chains corrupt the linked lists?

In scmi_imx_misc_ctrl_probe():

scmi_imx_misc_ctrl_probe()
    ret = handle->notify_ops->devm_event_notifier_register(sdev,
                                     SCMI_PROTOCOL_IMX_MISC,
                                     SCMI_EVENT_IMX_MISC_CONTROL,
                                     &src_id,
                                     &scmi_imx_misc_ctrl_nb);

This registers the exact same static scmi_imx_misc_ctrl_nb instance inside
a loop for different src_id values. Since SCMI maintains separate notifier
chains for different sources, does registering the same notifier_block into
multiple linked lists overwrite its internal next pointer?

Could traversing the corrupted list when an event fires cause kernel
panics or infinite loops?

[Severity: High]
This is a pre-existing issue, but does an unvalidated firmware value cause
an array out-of-bounds read here?

In scmi_imx_misc_get_reason():

scmi_imx_misc_get_reason()
    dev_info(&sdev->dev, "%s Boot reason: %s, origin: %d, errid: %d\n",
             system ? "SYS" : "LM", rst[boot.reason], ...

The boot.reason value obtained from firmware is an 8-bit value (0-255),
but the rst array only contains 32 elements.

If a firmware response returns a reason code of 32 or higher, will this
trigger an out-of-bounds array read and dereference invalid memory as a
string pointer?

[Severity: High]
This is a pre-existing issue, but does a lack of validation of
st->num_returned allow an out-of-bounds read of the SCMI RX buffer?

In iter_misc_syslog_process_response():

iter_misc_syslog_process_response()
    p->array[st->desc_index + st->loop_idx] =
            le32_to_cpu(r->syslog[st->loop_idx]);

The SCMI core iterator framework does not validate that
num_returned * sizeof(u32) fits within the dynamically received transport
buffer size (t->rx.len).

Because st->num_returned is extracted from the firmware response and
trusted unconditionally, does this blindly read past the actual SCMI
response payload and leak adjacent transport memory?

[Severity: High]
This is a pre-existing issue, but is the payload bounds check vulnerable
to an integer underflow?

In scmi_imx_misc_ctrl_get():

scmi_imx_misc_ctrl_get()
    if (*num >= max_num ||
        *num * sizeof(__le32) > t->rx.len - sizeof(__le32)) {
            ph->xops->xfer_put(ph, t);
            return -EINVAL;
    }

If the received firmware response is shorter than the size of the header
(i.e., t->rx.len < 4), the subtraction on the unsigned size type will
underflow to a massive positive value.

Could this allow a short firmware response to bypass the bounds check
and leak stale transport memory to the caller from the out-of-bounds
out->val array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609004504.3402281-1-lijun01@kylinos.cn?part=1

Re: [PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[Frank Li]

On Tue, Jun 09, 2026 at 08:45:04AM +0800, Li Jun wrote:
>
> Add a proper NULL check for the kmalloc() return value in syslog_show().
> If memory allocation fails, syslog would be NULL and passing it to
> misc_syslog() could lead to a NULL pointer dereference.
>
> Signed-off-by: Li Jun <lijun01@kylinos.cn>
> ---

Add a fix tags

Frank

>  drivers/firmware/imx/sm-misc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index ac9af824c2d4..fb8d7bdb5b08 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -79,6 +79,9 @@ static int syslog_show(struct seq_file *file, void *priv)
>         u16 size = SZ_4K / 4;
>         int ret;
>
> +       if (!syslog)
> +               return -ENOMEM;
> +
>         if (!ph)
>                 return -ENODEV;
>
> --
> 2.25.1
>
>

[PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[Li Jun]

Add a proper NULL check for the kmalloc() return value in syslog_show().
If memory allocation fails, syslog would be NULL and passing it to
misc_syslog() could lead to a NULL pointer dereference.

Fixes: 80a4062e8821 ("firmware: imx: sm-misc: Dump syslog info")
Signed-off-by: Li Jun <lijun01@kylinos.cn>
---
 drivers/firmware/imx/sm-misc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index ac9af824c2d4..fb8d7bdb5b08 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -79,6 +79,9 @@ static int syslog_show(struct seq_file *file, void *priv)
 	u16 size = SZ_4K / 4;
 	int ret;
 
+	if (!syslog)
+		return -ENOMEM;
+
 	if (!ph)
 		return -ENODEV;
 
-- 
2.25.1

Re: [PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds read in `syslog_show()` due to unbounded firmware-controlled `size` update.
- [High] Missing bounds check on firmware-provided reset reason indices leads to out-of-bounds array access in `scmi_imx_misc_get_reason()`.
- [High] Sharing a single static `notifier_block` across multiple notifier chains in `scmi_imx_misc_ctrl_probe()` corrupts the chains.
--

commit eb4ad394e97f562afeae71b9d50d159b32ab9818
Author: Li Jun <lijun01@kylinos.cn>

firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

Add a proper NULL check for the kmalloc() return value in syslog_show().
If memory allocation fails, syslog would be NULL and passing it to
misc_syslog() could lead to a NULL pointer dereference.

> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index ac9af824c2d40..fb8d7bdb5b084 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -79,6 +79,9 @@ static int syslog_show(struct seq_file *file, void *priv)
>  	u16 size = SZ_4K / 4;
>  	int ret;
>  
> +	if (!syslog)
> +		return -ENOMEM;
> +
>  	if (!ph)
>  		return -ENODEV;
>  

[Severity: High]
This is a pre-existing issue, but could an out-of-bounds read occur if the
firmware returns a large size?

Looking at syslog_show(), the buffer is allocated with a fixed size of 4096
bytes:

drivers/firmware/imx/sm-misc.c:syslog_show() {
    ...
    void *syslog __free(kfree) = kmalloc(SZ_4K, GFP_KERNEL);
    u16 size = SZ_4K / 4;
    ...
    ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
    ...
    seq_hex_dump(file, " ", DUMP_PREFIX_NONE, 16, sizeof(u32), syslog,
                 size * 4, false);
}

If iter_misc_syslog_update_state() in the SCMI driver overwrites *p->size with
st->num_returned + st->num_remaining based on the firmware response, could a
buggy or malicious firmware return a large num_remaining (e.g., 2000),
causing size to be unbounded?

Would this cause seq_hex_dump() to read past the end of the kmalloc allocation
and leak adjacent kernel heap memory to userspace?

[Severity: High]
This is a pre-existing issue, but are we missing bounds checking on the
firmware-provided reset reason indices?

In scmi_imx_misc_get_reason(), the firmware provides boot.reason and
shutdown.reason as 8-bit masks:

drivers/firmware/imx/sm-misc.c:scmi_imx_misc_get_reason() {
    ...
    dev_info(&sdev->dev, "%s Boot reason: %s, origin: %d, errid: %d\n",
             system ? "SYS" : "LM", rst[boot.reason],
    ...
}

If the firmware returns a reason code >= 32, will the array lookup
rst[boot.reason] or rst[shutdown.reason] read out-of-bounds and cause a kernel
panic when dereferencing the pointer for the %s format specifier?

[Severity: High]
This is a pre-existing issue, but is it safe to share a single static
notifier_block across multiple notifier chains?

In scmi_imx_misc_ctrl_probe(), the driver calls devm_event_notifier_register()
in a loop, passing the same static scmi_imx_misc_ctrl_nb:

drivers/firmware/imx/sm-misc.c:scmi_imx_misc_ctrl_probe() {
    ...
    scmi_imx_misc_ctrl_nb.notifier_call = &scmi_imx_misc_ctrl_notifier;
    for (i = 0; i < num; i += 2) {
        ...
        ret = handle->notify_ops->devm_event_notifier_register(sdev,
                                                SCMI_PROTOCOL_IMX_MISC,
                                                SCMI_EVENT_IMX_MISC_CONTROL,
                                                &src_id,
                                                &scmi_imx_misc_ctrl_nb);
        ...
    }
}

Since a struct notifier_block contains an intrusive linked list pointer (next),
does registering the exact same instance on potentially multiple distinct
notifier chains silently overwrite the next pointer, corrupting the linked
lists and causing a kernel panic when the chains are traversed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610003814.75493-1-lijun01@kylinos.cn?part=1

[PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[Li Jun]

Add a proper NULL check for the kmalloc() return value in syslog_show().
If memory allocation fails, syslog would be NULL and passing it to
misc_syslog() could lead to a NULL pointer dereference.

Signed-off-by: Li Jun <lijun01@kylinos.cn>
---
 drivers/firmware/imx/sm-misc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index ac9af824c2d4..fb8d7bdb5b08 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -79,6 +79,9 @@ static int syslog_show(struct seq_file *file, void *priv)
 	u16 size = SZ_4K / 4;
 	int ret;
 
+	if (!syslog)
+		return -ENOMEM;
+
 	if (!ph)
 		return -ENODEV;
 
-- 
2.25.1

Re: [PATCH] firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The single global `notifier_block` instance `scmi_imx_misc_ctrl_nb` is registered multiple times into different notification chains, causing linked list corruption.
- [High] Missing `rx_len` validation allows an out-of-bounds read from the SCMI RX buffer.
- [High] Firmware-controlled `size` overwrite causes an out-of-bounds read in `syslog_show` and silent log truncation.
- [High] `scmi_imx_misc_ctrl_get()` overflows the caller's buffer by completely ignoring its original capacity limit.
- [High] `scmi_imx_misc_get_reason` accesses the `rst` string array without bounds checking `boot.reason` or `shutdown.reason`.
- [High] Global state (`ph` and `imx_misc_ctrl_ops`) is permanently corrupted on probe failure or device unbind, breaking deferred probing and causing Use-After-Free.
- [High] A TOCTOU race condition during global variable initialization can lead to a NULL pointer dereference in exported functions.
- [High] The `syslog` buffer is allocated via `kmalloc()` without `__GFP_ZERO`. Uninitialized kernel heap memory may be leaked to userspace.
--

commit 56e1cfdbed4d1640cd1f28d670f1d36502723eb1
Author: Li Jun <lijun01@kylinos.cn>
Subject: firmware: imx: sm-misc: Add NULL check for kmalloc in syslog_show

This commit adds a proper NULL check for the kmalloc() return value in 
syslog_show() to prevent a potential NULL pointer dereference.

> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index ac9af824c2d40..fb8d7bdb5b084 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -74,9 +74,12 @@ static int syslog_show(struct seq_file *file, void *priv)
>  {
>  	/* 4KB is large enough for syslog */
>  	void *syslog __free(kfree) = kmalloc(SZ_4K, GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but does this allocation need 
to use __GFP_ZERO?

If the firmware returns success but populates fewer words than expected, 
the uninitialized kernel heap memory in the syslog buffer could be dumped 
and leaked to userspace via seq_hex_dump(). Replacing kmalloc with kzalloc 
would prevent this.

>  	/* syslog API use num words, not num bytes */
>  	u16 size = SZ_4K / 4;
>  	int ret;
>  
> +	if (!syslog)
> +		return -ENOMEM;
> +
>  	if (!ph)
>  		return -ENODEV;
>  
>  	ret = imx_misc_ctrl_ops->misc_syslog(ph, &size, syslog);
>  	if (ret)
>  		return ret;
>  
>  	seq_hex_dump(file, " ", DUMP_PREFIX_NONE, 16, sizeof(u32), syslog, size * 4, false);

[Severity: High]
This is a pre-existing issue, but can this seq_hex_dump() read out of bounds?

In iter_misc_syslog_update_state(), *p->size is overwritten with 
st->num_returned + st->num_remaining. If the firmware returns 
num_returned = 0 and a large num_remaining (e.g., 4095), the size here 
would be inflated to 4095. This causes seq_hex_dump() to read past the 
4KB buffer bounds, potentially leaking kernel memory.

I also noticed a few other pre-existing issues in this driver:

In drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c:

> iter_misc_syslog_process_response() {
> ...
> 	p->array[st->desc_index + st->loop_idx] = le32_to_cpu(r->syslog[st->loop_idx]);

[Severity: High]
This isn't a bug introduced by this patch, but does this loop lack bounds 
checking against the actual received message length?

If st->num_returned is larger than the received SCMI payload length 
(st->rx_len), iterating up to st->num_returned could read past the end of 
the SCMI RX buffer.

> scmi_imx_misc_ctrl_get() {
> ...
> 	*num = le32_to_cpu(out->num);
> 
> 	if (*num >= max_num ||
> 	    *num * sizeof(__le32) > t->rx.len - sizeof(__le32)) {
> 		ph->xops->xfer_put(ph, t);
> 		return -EINVAL;
> 	}
> 
> 	for (i = 0; i < *num; i++)
> 		val[i] = le32_to_cpu(out->val[i]);

[Severity: High]
This is a pre-existing issue, but does this function overflow the caller's 
val array?

The function overwrites the caller's *num parameter with out->num without 
checking if it exceeds the caller's original capacity limit. If a caller 
passes a small stack buffer (like in fsl_mqs_sm_read) and out->num is 
larger than expected, this copy will write past the buffer bounds.

In drivers/firmware/imx/sm-misc.c:

> scmi_imx_misc_get_reason() {
> ...
> 	dev_info(&sdev->dev, "%s Boot reason: %s, origin: %d, errid: %d\n",
> 		 system ? "SYS" : "LM", rst[boot.reason],
> 		 boot.orig_valid ? boot.origin : -1,
> 		 boot.err_valid ? boot.errid : -1);

[Severity: High]
This isn't a bug introduced by this patch, but could this trigger an 
out-of-bounds read?

The rst string arrays contain 32 elements, but boot.reason is extracted 
using an 8-bit mask (0-255). If the firmware returns a reason code of 32 
or greater, accessing rst[boot.reason] will read out of bounds.

> scmi_imx_misc_ctrl_probe() {
> ...
> 	imx_misc_ctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_MISC, &ph);
> 	if (IS_ERR(imx_misc_ctrl_ops))
> 		return PTR_ERR(imx_misc_ctrl_ops);

[Severity: High]
This is a pre-existing issue, but does this leave dangling global pointers 
if the driver unbinds or probe fails?

If devm_protocol_get returns an error, imx_misc_ctrl_ops retains an error 
pointer, failing subsequent probes. If the driver unbinds, devres frees 
the protocol, but imx_misc_ctrl_ops and ph are never cleared. Exported 
functions will dereference these dangling pointers, causing a use-after-free.

> scmi_imx_misc_ctrl_probe() {
> ...
> 		ret = handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_MISC,
> 							       SCMI_EVENT_IMX_MISC_CONTROL,
> 							       &src_id,
> 							       &scmi_imx_misc_ctrl_nb);

[Severity: Critical]
This isn't a bug introduced by this patch, but doesn't this corrupt the 
blocking_notifier_head lists?

The loop registers the exact same global scmi_imx_misc_ctrl_nb instance 
for multiple different event sources. Registering a single notifier_block 
instance into multiple chains overwrites its next pointer, which corrupts 
the linked lists.

> scmi_imx_misc_ctrl_set() {
> 	if (!ph)
> 		return -EPROBE_DEFER;
> 
> 	return imx_misc_ctrl_ops->misc_ctrl_set(ph, id, 1, &val);

[Severity: High]
This is a pre-existing issue, but could this race with driver initialization?

During probe, handle->devm_protocol_get() assigns *ph before returning the 
operations assigned to imx_misc_ctrl_ops. If an external caller executes 
scmi_imx_misc_ctrl_set() after ph is assigned but before imx_misc_ctrl_ops 
is set, this will result in a NULL pointer dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608020345.247101-1-lijun01@kylinos.cn?part=1