Re: [PATCH] PCI: endpoint: pci-epf-test: Free doorbell IRQ on unbind and deinit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] pci_epf_test_disable_doorbell() does not check the new doorbell_irq_registered flag, allowing a malicious host to trigger a NULL pointer dereference or double-free.
- [High] Module exit routine destroys workqueue before unregistering the driver, leading to a Use-After-Free.
--

commit dc35dc30c72a974fd9de8195ba6c7cac69c49ed9
Author: Fan Wu <fanwu01@zju.edu.cn>

PCI: endpoint: pci-epf-test: Free doorbell IRQ on unbind and deinit

This commit resolves a missing IRQ teardown in the endpoint teardown paths.
It introduces a new doorbell_irq_registered flag to ensure the doorbell IRQ 
is correctly freed on unbind and deinit, preventing EBUSY on subsequent 
doorbell allocations.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index d4905aa8e4c01..ab8df91891cec 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
[ ... ]
> @@ -772,6 +774,7 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
>  		goto err_doorbell_cleanup;
>  	}
>  
> +	epf_test->doorbell_irq_registered = true;
>  	reg->doorbell_data = cpu_to_le32(msg->data);
>  	reg->doorbell_bar = cpu_to_le32(bar);
>  

[Severity: Critical]
This is a pre-existing issue, but since this patch introduces the
doorbell_irq_registered flag, could it also be used in
pci_epf_test_disable_doorbell() to prevent a crash?

A PCIe host can write a valid BAR index directly to reg->doorbell_bar and
issue COMMAND_DISABLE_DOORBELL without a prior successful
COMMAND_ENABLE_DOORBELL.

When this happens, pci_epf_test_disable_doorbell() reads the bar value
directly from host-writable memory:

drivers/pci/endpoint/functions/pci-epf-test.c:pci_epf_test_disable_doorbell() {
	enum pci_barno bar = le32_to_cpu(reg->doorbell_bar);
	...
	if (bar < BAR_0)
		goto set_status_err;

	free_irq(epf->db_msg[0].virq, epf_test);
	...
}

Because the host provided a valid index, the sanity check is bypassed.

Since epf->db_msg is only allocated during enable, it remains unallocated.

Does unconditionally dereferencing epf->db_msg without checking
epf_test->doorbell_irq_registered allow a malicious host to trigger a NULL
pointer dereference or double-free?

Since a PCIe endpoint must robustly handle host inputs, could this lead to a
locally triggered remote DoS?

[ ... ]
> @@ -1374,6 +1381,10 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>  	struct pci_epc *epc = epf->epc;
>  
>  	cancel_delayed_work_sync(&epf_test->cmd_handler);
> +	if (epf_test->doorbell_irq_registered) {
> +		free_irq(epf->db_msg[0].virq, epf_test);
> +		pci_epf_test_doorbell_cleanup(epf_test);
> +	}
>  	if (epc->init_complete) {
>  		pci_epf_test_clean_dma_chan(epf_test);
>  		pci_epf_test_clear_bar(epf);

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a
use-after-free risk during module unload related to this teardown path.

In the module exit routine, the workqueue is destroyed before the driver
is unregistered:

drivers/pci/endpoint/functions/pci-epf-test.c:pci_epf_test_exit() {
	if (kpcitest_workqueue)
		destroy_workqueue(kpcitest_workqueue);
	pci_epf_unregister_driver(&test_driver);
}

If an interrupt fires or a delayed work reschedules itself before
pci_epf_unregister_driver() fully unbinds the endpoints, could the system
attempt to queue work to a freed workqueue?

Unbinding also issues cancel_delayed_work_sync() here.

Could this access invalid timer state since the workqueue was already
destroyed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905014525.420764-1-fanwu01@zju.edu.cn?part=1