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

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

[Fan Wu]

Neither pci_epf_test_unbind() nor pci_epf_test_epc_deinit() frees the
doorbell IRQ. epc_deinit() clears the BARs, while unbind() can free the
BAR backing store; if the doorbell is enabled, its IRQ action remains
registered with epf_test as dev_id and epf->db_msg stays allocated. A
doorbell interrupt may already have awakened the threaded handler when
teardown starts, and since cancel_delayed_work_sync() drains only the
command worker and clear_bar() does not wait for the IRQ thread,
unbind() can free that backing while the handler still dereferences
epf_test->reg[]. The leftover IRQ also makes the next doorbell
allocation on the same function fail with -EBUSY.

Free the doorbell IRQ in both paths, guarded by a
doorbell_irq_registered flag set once request_threaded_irq() has
succeeded and cleared in pci_epf_test_doorbell_cleanup(), the
chokepoint shared by the enable error path, disable_doorbell() and the
teardown sites, so no disarm path can double-free. In unbind(), the
drain depends on the driver's own flag, not on epc->init_complete.

This issue was found by an in-house static analysis tool.

Fixes: eff0c286aa91 ("PCI: endpoint: pci-epf-test: Add doorbell test support")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index d4905aa..ab8df91 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -95,6 +95,7 @@ struct pci_epf_test {
 	const struct pci_epc_features *epc_features;
 	struct pci_epf_bar	db_bar;
 	bool			db_bar_programmed;
+	bool			doorbell_irq_registered;
 	size_t			bar_size[PCI_STD_NUM_BARS];
 };
 
@@ -721,6 +722,7 @@ static void pci_epf_test_doorbell_cleanup(struct pci_epf_test *epf_test)
 	struct pci_epf *epf = epf_test->epf;
 
 	reg->doorbell_bar = cpu_to_le32(NO_BAR);
+	epf_test->doorbell_irq_registered = false;
 
 	pci_epf_free_doorbell(epf);
 }
@@ -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);
 
@@ -1238,6 +1241,10 @@ static void pci_epf_test_epc_deinit(struct pci_epf *epf)
 
 	cancel_delayed_work_sync(&epf_test->cmd_handler);
 	pci_epf_test_clean_dma_chan(epf_test);
+	if (epf_test->doorbell_irq_registered) {
+		free_irq(epf->db_msg[0].virq, epf_test);
+		pci_epf_test_doorbell_cleanup(epf_test);
+	}
 	pci_epf_test_clear_bar(epf);
 }
 
@@ -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);

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

[sashiko-bot]

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