Re: [PATCH net 5/6] net: enetc: fix initialization order to prevent use of uninitialized resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] This problem wasn't introduced by this patch, but lack of bounds checking on `pf->num_vfs` leads to an out-of-bounds array access in `enetc_msg_alloc_mbx()`.
- [High] This problem wasn't introduced by this patch, but in `enetc_msg_free_mbx()`, `dma_free_coherent()` is called to free the mailbox DMA buffer before writing 0 to the device's DMA address registers (`ENETC_PSIVMSGRCVAR0/1`).
- [High] This problem wasn't introduced by this patch, but stale hardware state in the Message Receive Register (`ENETC_PSIMSGRR`) can cause an infinite loop in the `enetc_msg_task()` workqueue handler.
- [High] This problem wasn't introduced by this patch, but lockless read-modify-write operations on the `ENETC_PSIIER` register can clobber interrupt states.
--

commit fa9d7e3799c5420c70515e10bd1626738a874ff5
Author: Wei Fang <wei.fang@nxp.com>
net: enetc: fix initialization order to prevent use of uninitialized resources

This commit reorders the initialization sequence in enetc_msg_psi_init() to
ensure all resources, including the msg_task work struct and mailbox DMA
buffers, are fully initialized before the interrupt handler is registered.
This prevents spurious interrupts from causing kernel panics or memory
corruption by scheduling an uninitialized work struct.

Link: https://sashiko.dev/#/patchset/20260511080805.2052495-1-wei.fang%40nxp.com #1

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index fe9680a97b0e7..c156a566a751e 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> @@ -109,6 +109,15 @@ int enetc_msg_psi_init(struct enetc_pf *pf)
>  	struct enetc_si *si = pf->si;
>  	int vector, i, err;
>  
> +	/* initialize PSI mailbox */
> +	INIT_WORK(&pf->msg_task, enetc_msg_task);

This problem wasn't introduced by this patch, but can stale hardware state in
the Message Receive Register (ENETC_PSIMSGRR) cause an infinite loop in the
enetc_msg_task() workqueue handler?

If the device was previously configured with more VFs, and a bit for a higher
VF remains set in ENETC_PSIMSGRR, re-initializing the driver with fewer VFs
might trigger a deadlock.

The inner loop in enetc_msg_task() only iterates up to the current pf->num_vfs
and therefore ignores the stale bit. Because the bit is never cleared
(it is a w1c register), the outer for (;;) loop continuously reads a non-zero
mr_mask and spins indefinitely, locking up the kernel worker thread.

> +
> +	for (i = 0; i < pf->num_vfs; i++) {
> +		err = enetc_msg_alloc_mbx(si, i);

This problem wasn't introduced by this patch, but does the lack of bounds
checking on pf->num_vfs lead to an out-of-bounds array access in
enetc_msg_alloc_mbx()?

The array pf->rxmsg has a fixed size of ENETC_MAX_NUM_VFS (2). However,
pf->num_vfs is populated from the user-requested SR-IOV VFs, bounded only by
the hardware's total_VFs capability.

If a device reports total_VFs > 2, a user could request more than 2 VFs,
causing enetc_msg_alloc_mbx() to iterate past the end of the rxmsg array.

This out-of-bounds write would corrupt subsequent fields in struct enetc_pf
(including the msg_task workqueue struct initialized by this patch), leading
to kernel crashes and potential privilege escalation when the corrupted work
struct is scheduled.

> +		if (err)
> +			goto free_mbx;
> +	}
> +

[ ... ]

> @@ -117,32 +126,21 @@ int enetc_msg_psi_init(struct enetc_pf *pf)

[ ... ]

>  	/* enable MR interrupts */
>  	enetc_msg_enable_mr_int(&si->hw);

This problem wasn't introduced by this patch, but can lockless
read-modify-write operations on the ENETC_PSIIER register clobber interrupt
states?

Looking at enetc_msg_enable_mr_int():

drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_enable_mr_int() {
	u32 psiier = enetc_rd(hw, ENETC_PSIIER);

	enetc_wr(hw, ENETC_PSIIER, psiier | ENETC_PSIIER_MR_MASK);
}

enetc_msg_enable_mr_int() and enetc_msg_disable_mr_int() perform unprotected
read-modify-write operations on the ENETC_PSIIER register.

Because enetc_msg_enable_mr_int() can run in process context (via the
workqueue or during initialization) and enetc_msg_disable_mr_int() runs in
hard interrupt context (enetc_msg_psi_msix()), a race exists.

If an interrupt fires while the workqueue is executing the RMW operation, the
workqueue could write back a stale value of ENETC_PSIIER, obliterating the
state changes made by the interrupt handler. Should this be protected by a
spinlock?

>  
>  	return 0;
>  
> -err_init_mbx:
> +free_mbx:
>  	for (i--; i >= 0; i--)
>  		enetc_msg_free_mbx(si, i);

This problem wasn't introduced by this patch, but in enetc_msg_free_mbx(), is
the mailbox DMA buffer freed before clearing the device's DMA address
registers?

Looking at enetc_msg_free_mbx():

drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_free_mbx() {
	...
	dma_free_coherent(&si->pdev->dev, msg->size, msg->vaddr, msg->dma);
	memset(msg, 0, sizeof(*msg));

	enetc_wr(hw, ENETC_PSIVMSGRCVAR0(idx), 0);
	enetc_wr(hw, ENETC_PSIVMSGRCVAR1(idx), 0);
}

Freeing the DMA memory before clearing the hardware registers allows the
hardware to perform a DMA write into freed kernel memory if a message arrives
or a pending DMA completes in that race window, leading to memory corruption.

Should the hardware DMA configuration be cleared before the backing memory is
freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513103021.2190593-1-wei.fang@nxp.com?part=5