Re: [PATCH net 4/6] net: enetc: avoid VF->PF mailbox timeout during SR-IOV teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing bounds checks on the number of VFs against the fixed-size rxmsg array can lead to out-of-bounds heap reads/writes and out-of-bounds MMIO accesses.
- [High] Missing cancel_work_sync() in the error path of enetc_msg_psi_init() leads to a potential NULL pointer dereference or unhandled interrupt.
--

commit 20bd14a48fb81c18f7cc12a0a4f0d93f01026fce
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: avoid VF->PF mailbox timeout during SR-IOV teardown

During SR-IOV teardown, this commit reorders the cleanup steps to call
pci_disable_sriov() before cleaning up the mailbox resources. This
eliminates a race window where VFs could send messages to an unresponsive
PF and time out waiting for a reply.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index 6939ab04dcdc0..052cab48df514 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -544,9 +544,9 @@ static int enetc_sriov_configure(struct pci_dev *pdev, int num_vfs)
>  	int err;
>  
>  	if (!num_vfs) {
> +		pci_disable_sriov(pdev);
>  		enetc_msg_psi_free(pf);
>  		pf->num_vfs = 0;
> -		pci_disable_sriov(pdev);
>  	} else {
>  		pf->num_vfs = num_vfs;

This problem wasn't introduced by this patch, but it exists in the codebase.
In enetc_msg_psi_init(), if enetc_msg_alloc_mbx() fails:

drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_psi_init() {
[ ... ]
	for (i = 0; i < pf->num_vfs; i++) {
		err = enetc_msg_alloc_mbx(si, i);
		if (err)
			goto err_init_mbx;
	}
[ ... ]
err_init_mbx:
	for (i--; i >= 0; i--)
		enetc_msg_free_mbx(si, i);

	free_irq(vector, si);

	return err;
}

Can this lead to a NULL pointer dereference or an unhandled interrupt?

If an interrupt fired and scheduled the work item before free_irq() was
called, would the work item remain queued and run asynchronously after the
function returns, since cancel_work_sync(&pf->msg_task) is missing?

A prior patch in the series fixed the cleanup in the normal teardown path,
but does this error path remain vulnerable?

If the executing enetc_msg_task() then runs, would it dereference the
mailbox memory (msg->vaddr) that was just cleared if a message was pending?
Would it also call enetc_msg_enable_mr_int(), enabling hardware interrupts
when no handler is registered anymore?


There is another problem that wasn't introduced by this patch, but exists
in the codebase. During enetc_pf_probe(), the driver initializes
pf->total_vfs:

drivers/net/ethernet/freescale/enetc/enetc_pf.c:enetc_pf_probe() {
[ ... ]
	pf->total_vfs = pci_sriov_get_totalvfs(pdev);
	if (pf->total_vfs) {
		pf->vf_state = kzalloc_objs(struct enetc_vf_state,
					    pf->total_vfs);
[ ... ]
}

Does this code lack bounds checking on the number of VFs against the fixed
size of the rxmsg array (ENETC_MAX_NUM_VFS)?

If a PCIe device or hypervisor presents a capability with an artificially
large TotalVFs, would loops iterating up to pf->num_vfs perform out-of-bounds
accesses to the pf->rxmsg array when SR-IOV is enabled?

Could this also result in out-of-bounds MMIO writes by indexing past
valid registers?

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