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

[sashiko-bot@kernel.org]

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