* Re: [PATCH v3] mfd: loongson-se: Add multi-node support
2026-06-10 15:13 [PATCH v3] mfd: loongson-se: Add multi-node support Qunqin Zhao
@ 2026-06-18 9:59 ` Lee Jones
0 siblings, 0 replies; 2+ messages in thread
From: Lee Jones @ 2026-06-18 9:59 UTC (permalink / raw)
To: Qunqin Zhao; +Cc: chenhuacai, xry111, linux-kernel, loongarch, linux-crypto
/* Sashiko Automation: Issues Found (7 Findings) */
Worth chewing through.
> On the Loongson platform, each node is equipped with a security engine
> device. However, due to a hardware flaw, only the device on node 0 can
> trigger interrupts. Therefore, interrupts from other nodes are forwarded
> by node 0. We need to check in the interrupt handler of node 0 whether
> this interrupt is intended for other nodes, this can be accomplished via
> shared interrupt handling.
>
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
> V3:
> Using shared interrupts (IRQF_SHARED) instead of manually
> iterating through all devices to check for interrupts.
>
> Link to v2:
> https://lore.kernel.org/all/20260427165133.23350-1-zhaoqunqin@loongson.cn/
>
> drivers/mfd/loongson-se.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> index 3902ba377d6..e63ea40d5db 100644
> --- a/drivers/mfd/loongson-se.c
> +++ b/drivers/mfd/loongson-se.c
> @@ -219,7 +219,7 @@ static int loongson_se_probe(struct platform_device *pdev)
>
> for (i = 0; i < nr_irq; i++) {
> irq = platform_get_irq(pdev, i);
> - err = devm_request_irq(dev, irq, se_irq_handler, 0, "loongson-se", se);
> + err = devm_request_irq(dev, irq, se_irq_handler, IRQF_SHARED, "loongson-se", se);
[Severity: High]
With IRQF_SHARED now being used, does se_irq_handler() need to return
IRQ_NONE when it doesn't process any interrupts?
drivers/mfd/loongson-se.c:se_irq_handler() {
...
/* For engines */
while (int_status) {
id = __ffs(int_status);
complete(&se->engines[id].completion);
int_status &= ~BIT(id);
writel(BIT(id), se->base + SE_S2LINT_CL);
}
spin_unlock(&se->dev_lock);
return IRQ_HANDLED;
}
If another device sharing the IRQ line triggers an interrupt and
int_status is 0, the handler will skip processing but unconditionally
return IRQ_HANDLED.
This defeats the spurious interrupt detection mechanism in
note_interrupt(), which could lead to a system lockup if an interrupt
storm occurs on the shared line.
[Severity: Critical]
This is a pre-existing issue, but does se_irq_handler() lack bounds
checking when accessing the engines array?
drivers/mfd/loongson-se.c:se_irq_handler() {
...
/* For engines */
while (int_status) {
id = __ffs(int_status);
complete(&se->engines[id].completion);
...
The driver enables all 32 possible interrupts by writing SE_INT_ALL
(0xffffffff) to SE_S2LINT_EN in loongson_se_probe(). The engines array
has a fixed size of SE_ENGINE_MAX (16).
If the hardware sets any interrupt bit from 16 to 31, id will be >= 16,
resulting in an out-of-bounds memory write when calling complete().
Could this corrupt adjacent heap memory?
[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to use
devm_kmalloc() with GFP_KERNEL here instead of devm_kzalloc()?
drivers/mfd/loongson-se.c:loongson_se_probe() {
...
se = devm_kmalloc(dev, sizeof(*se), GFP_KERNEL);
...
writel(SE_INT_ALL, se->base + SE_S2LINT_EN);
...
err = devm_request_irq(dev, irq, se_irq_handler, IRQF_SHARED, "loongson-se", se);
...
The se->engines array contains uninitialized garbage because it is
allocated with devm_kmalloc(). Hardware interrupts are globally enabled
and the IRQ handler is registered before the engine completions are
initialized.
If a leftover interrupt from a prior hardware state is pending when the
handler registers, se_irq_handler() could execute
complete(&se->engines[id].completion) on random, uninitialized memory.
> if (err)
> dev_err(dev, "failed to request IRQ: %d\n", irq);
> }
[Severity: High]
This is a pre-existing issue, but should the probe function return an
error if devm_request_irq() fails?
If devm_request_irq() fails, the loop continues without returning an
error code. Immediately after, loongson_se_init() issues a hardware
command and calls wait_for_completion_interruptible(&se->cmd_completion).
Since no interrupt handler is registered to signal complete(), the
worker thread executing the probe will hang indefinitely.
> @@ -228,7 +228,7 @@ static int loongson_se_probe(struct platform_device *pdev)
> if (err)
> return err;
>
> - return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, engines,
> + return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, engines,
> ARRAY_SIZE(engines), NULL, 0, NULL);
> }
[Severity: High]
This isn't a bug introduced by this patch, but is a reinit_completion()
needed before waiting on the completions?
drivers/mfd/loongson-se.c:loongson_se_send_engine_cmd() {
...
return wait_for_completion_interruptible(&engine->completion);
}
If a user process executing this is interrupted by a signal, it returns
-ERESTARTSYS immediately while the hardware command continues executing.
When the hardware eventually finishes, the interrupt handler calls
complete(), incrementing the completion counter.
Because reinit_completion() is never called before initiating the next
command, the subsequent wait_for_completion_interruptible() will
immediately return success before the new hardware command has actually
finished, breaking hardware synchronization.
[Severity: High]
This is a pre-existing issue, but does loongson_se_init() write
uninitialized stack memory to the device registers?
drivers/mfd/loongson-se.c:loongson_se_init() {
struct loongson_se_controller_cmd cmd;
int err;
cmd.command_id = SE_CMD_START;
err = loongson_se_send_controller_cmd(se, &cmd);
...
For SE_CMD_START, only cmd.command_id is initialized, leaving the
cmd.info array uninitialized. loongson_se_send_controller_cmd() then
unconditionally writes SE_SEND_CMD_REG_LEN (8) u32 words to the device
via writel().
This writes 28 bytes of uninitialized kernel stack contents directly to
the device MMIO registers.
[Severity: High]
This isn't a bug introduced by this patch, but does the platform driver
declaration lack a .remove callback to properly halt the hardware?
drivers/mfd/loongson-se.c:
...
static struct platform_driver loongson_se_driver = {
.probe = loongson_se_probe,
.driver = {
.name = "loongson-se",
.acpi_match_table = loongson_se_acpi_match,
},
};
During driver unbind or module removal, devres will automatically free
the DMA buffer and the IRQ. Because there is no explicit teardown step
to send a stop command or disable hardware interrupts, the controller
remains fully active.
If the hardware continues to process and writes to the now-freed DMA
memory, could it cause memory corruption or trigger IOMMU faults?
>
>
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> --
> 2.47.2
>
--
Lee Jones
^ permalink raw reply [flat|nested] 2+ messages in thread