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 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