Re: [PATCH v4 1/2] mfd: loongson-se: Fix miscellaneous issues

[Lee Jones <lee@kernel.org>]

Oof, that's a lot of findings.  Please review and remedy each.

/* Sashiko Automation: Issues Found (10 Findings) */

On Mon, 29 Jun 2026, Qunqin Zhao wrote:

> Address multiple historical driver issues discovered by the Sashiko
> Automation system within the loongson_se_probe() initialization flow
> and the driver's interrupt service routines [1].
> 
> - Add an explicit bounds check for 'id' before accessing the fixed-size
>   'engines' array in se_irq_handler() to prevent potential out-of-bounds
>   memory writes.
> 
> - Switch the allocations from devm_kmalloc() to devm_kzalloc() to
>   guarantee the descriptor structures are properly zero-initialized.
> 
> - Return an error code from loongson_se_probe() if devm_request_irq()
>   fails, preventing an indefinite hang during completion waits.
> 
> - Add a reinit_completion() before waiting on the completions to ensure
>   proper hardware synchronization.
> 
> - Prevent writing uninitialized stack memory to device registers in
>   loongson_se_init() by properly initializing local command structures.
> 
> - Add a .remove callback to the platform driver to properly halt the
>   hardware.
> 
> Link: https://lore.kernel.org/all/20260618095949.GB1672911@google.com/ [1]
> Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
>  drivers/mfd/loongson-se.c       | 23 +++++++++++++++++++----
>  include/linux/mfd/loongson-se.h |  1 +
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> index 3902ba377d..aa34e663c4 100644
> --- a/drivers/mfd/loongson-se.c
> +++ b/drivers/mfd/loongson-se.c
> @@ -85,6 +85,8 @@ int loongson_se_send_engine_cmd(struct loongson_se_engine *engine)
>  	if (err)
>  		return err;
>  
> +	reinit_completion(&engine->completion);
> +

[Severity: High]
If a fast hardware completion occurs between loongson_se_poll() signaling the
hardware to start and this reinit_completion(), the IRQ handler runs and sets
the completion to done. Will this reinit_completion() then immediately clear
the done state, causing the subsequent wait_for_completion_interruptible()
to block indefinitely?


>  	return wait_for_completion_interruptible(&engine->completion);
>  }

[Severity: High]
This is a pre-existing issue, but controller commands also rely on
wait_for_completion_interruptible() without reinitializing the completion
structure. If loongson_se_send_controller_cmd() is interrupted by a signal,
it returns -ERESTARTSYS, and the hardware eventually sets the completion state
to done. Will the next controller command see the completion as already done
and return success without actually waiting?



[Severity: High]
This is a pre-existing issue, but does loongson_se_poll() hold an
irq-disabling spinlock for up to 10 milliseconds?

loongson_se_poll() acquires a spinlock with spin_lock_irq(), and then uses
readl_relaxed_poll_timeout_atomic() with a 10,000 us timeout. Could this
cause severe scheduling latency spikes or trigger hard lockup panics if the
hardware is slow?



>  EXPORT_SYMBOL_GPL(loongson_se_send_engine_cmd);
> @@ -150,7 +152,8 @@ static irqreturn_t se_irq_handler(int irq, void *dev_id)
>  	/* For engines */
>  	while (int_status) {
>  		id = __ffs(int_status);
> -		complete(&se->engines[id].completion);
> +		if (id < SE_ENGINE_MAX)
> +			complete(&se->engines[id].completion);
>  		int_status &= ~BIT(id);
>  		writel(BIT(id), se->base + SE_S2LINT_CL);
>  	}
> @@ -162,7 +165,7 @@ static irqreturn_t se_irq_handler(int irq, void *dev_id)
>  
>  static int loongson_se_init(struct loongson_se *se, dma_addr_t addr, int size)
>  {
> -	struct loongson_se_controller_cmd cmd;
> +	struct loongson_se_controller_cmd cmd = {0};
>  	int err;
>  
>  	cmd.command_id = SE_CMD_START;
> @@ -190,7 +193,7 @@ static int loongson_se_probe(struct platform_device *pdev)
>  	int nr_irq, irq, err, i;
>  	dma_addr_t paddr;
>  
> -	se = devm_kmalloc(dev, sizeof(*se), GFP_KERNEL);
> +	se = devm_kzalloc(dev, sizeof(*se), GFP_KERNEL);

[Severity: Critical]
This is a pre-existing issue, but switching to devm_kzalloc() without calling
init_completion() for the engines array leaves the completion structs
improperly initialized.

A zeroed struct list_head inside swait_queue_head evaluates as non-empty in
list_empty(). If an interrupt fires before a child driver calls
loongson_se_init_engine(), se_irq_handler() will pass the new bounds check
and call complete(), which dereferences a NULL list head. Can this cause a
deterministic kernel panic on spurious interrupts?



>  	if (!se)
>  		return -ENOMEM;
>  
> @@ -220,8 +223,10 @@ static int loongson_se_probe(struct platform_device *pdev)
>  	for (i = 0; i < nr_irq; i++) {
>  		irq = platform_get_irq(pdev, i);

[Severity: High]
This is a pre-existing issue, but the probe function returns -ENODEV if
platform_irq_count() fails earlier:

    nr_irq = platform_irq_count(pdev);
    if (nr_irq <= 0)
        return -ENODEV;

Does overwriting the return value of platform_irq_count() break probe deferral
if it returns -EPROBE_DEFER when the interrupt provider is not yet available?



[Severity: High]
This is a pre-existing issue, but earlier in probe, writel(SE_INT_ALL, ...)
enables all device interrupts before devm_request_irq() registers the handler.
If an interrupt fires in this window, no handler is present.

Additionally, if probe fails after this point, interrupts are never disabled.
Can this leave the hardware asserting unhandled interrupts and cause a system
lockup?


>  		err = devm_request_irq(dev, irq, se_irq_handler, 0, "loongson-se", se);
> -		if (err)
> +		if (err) {
>  			dev_err(dev, "failed to request IRQ: %d\n", irq);
> +			return err;
> +		}
>  	}
>  
>  	err = loongson_se_init(se, paddr, se->dmam_size);

[Severity: High]
This is a pre-existing issue, but is the driver blindly trusting the
firmware's dmam_size property without minimum bound checks?

In loongson_se_init_engine(), command buffers are placed at fixed offsets:

    engine->command = se->dmam_base + id * (2 * SE_ENGINE_CMD_SIZE);

If the provided dmam_size is smaller than the required space for all engines,
could these calculations yield pointers outside the allocated DMA buffer and
cause memory corruption?


> @@ -232,6 +237,15 @@ static int loongson_se_probe(struct platform_device *pdev)
>  				    ARRAY_SIZE(engines), NULL, 0, NULL);
>  }

[Severity: High]
This is a pre-existing issue, but if the devm_mfd_add_devices() call fails,
probe returns an error without sending SE_CMD_STOP to the already-started
controller.

Since devres will free the DMA buffers, can the still-running hardware
continue to access the freed memory and cause corruption?


>  
> +static void loongson_se_remove(struct platform_device *pdev)
> +{
> +	struct loongson_se *se = dev_get_drvdata(&pdev->dev);
> +	struct loongson_se_controller_cmd cmd = {0};
> +
> +	cmd.command_id = SE_CMD_STOP;
> +	loongson_se_send_controller_cmd(se, &cmd);

[Severity: High]
Because the child devices were registered via devm_mfd_add_devices(), their
unbind sequence happens later in the devres teardown phase, after this remove
callback returns.

If this halts the hardware controller before its child MFD devices are
unbound, will the child drivers still be fully active? Could any hardware
access by the children during their own remove callbacks fail or hang?



[Severity: High]
loongson_se_send_controller_cmd() waits using
wait_for_completion_interruptible(). If the thread executing the removal
receives a signal, the wait aborts immediately and returns -ERESTARTSYS.

Since this remove function ignores the return value, can devres free the
DMA buffers while the hardware is still processing the STOP command, leading
to DMA memory corruption?


> +}
> +
>  static const struct acpi_device_id loongson_se_acpi_match[] = {
>  	{ "LOON0011", 0 },
>  	{ }
> @@ -240,6 +254,7 @@ MODULE_DEVICE_TABLE(acpi, loongson_se_acpi_match);
>  
>  static struct platform_driver loongson_se_driver = {
>  	.probe   = loongson_se_probe,
> +	.remove  = loongson_se_remove,
>  	.driver  = {
>  		.name  = "loongson-se",
>  		.acpi_match_table = loongson_se_acpi_match,
> diff --git a/include/linux/mfd/loongson-se.h b/include/linux/mfd/loongson-se.h
> index 07afa0c252..8237ccab7b 100644
> --- a/include/linux/mfd/loongson-se.h
> +++ b/include/linux/mfd/loongson-se.h
> @@ -9,6 +9,7 @@
>  #define SE_SEND_CMD_REG_LEN		0x8
>  /* Controller command ID */
>  #define SE_CMD_START			0x0
> +#define SE_CMD_STOP			0x1
>  #define SE_CMD_SET_DMA			0x3
>  #define SE_CMD_SET_ENGINE_CMDBUF	0x4
>  
> -- 
> 2.47.2
> 

-- 
Lee Jones