Re: [PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

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

New issues:
- [High] The newly added `WARN_ON` can be trivially triggered by shared interrupts or untrusted hardware, leading to a Denial of Service (host panic) if `panic_on_warn` is enabled.
- [Medium] Direct, lockless read of the internal `done` field in `struct completion` causes a data race, violates API encapsulation, and relies on a flawed TOCTOU heuristic.

Pre-existing issues:
- [High] The shared IRQ handler unconditionally returns `IRQ_HANDLED` even when the interrupt was not generated by this device.
- [High] Use-after-free of `struct pci_endpoint_test` when the PCI device is unbound while a file descriptor is still open.
--

--- Patch [1]: [PATCH] misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers ---
commit e4f6d6484e68c6a478412d97f6734cf8590a6f97
Author: Niklas Cassel <[email protected]>

misc: pci_endpoint_test: Add WARN_ON() to detect broken EPC drivers

This commit adds a WARN_ON check to the IRQ handler in the PCI endpoint
test driver. It aims to detect if the driver receives multiple interrupts
sequentially without a successful wait in between.

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3635741c3e7a7..a212e53654c68 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -178,6 +178,19 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
>  	if (reg & STATUS_IRQ_RAISED) {
>  		test->last_irq = irq;
>  		complete(&test->irq_raised);
> +		/*
> +		 * The endpoint test driver performs all testing sequentially.
> +		 * This means that test->irq_raised.done should never exceed 1.
> +		 * If it does, then we received two IRQs in a row, without a
> +		 * successful wait_for_completion_timeout() call in between.
> +		 *
> +		 * While complete() increases test->irq_raised.done by one,
> +		 * wait_for_completion_timeout() reduces test->irq_raised.done
> +		 * by one on success.
> +		 *
> +		 * Please debug your EPC driver if you see this warning.
> +		 */
> +		WARN_ON(test->irq_raised.done > 1);

[Severity: Medium]
Does this direct, lockless read of the completion structure's internal state
introduce a data race?

If wait_for_completion_timeout() executes concurrently on another CPU between
the complete() call and this check, it could decrement done back to 0. Would
this window result in missing the duplicate IRQ this is attempting to detect?

[Severity: High]
Can this warning be trivially triggered by shared interrupts or untrusted
hardware?

If the driver requests its IRQ with IRQF_SHARED, an interrupt from another
device could cause this handler to execute. If it reads a stale
STATUS_IRQ_RAISED bit, could repeated shared interrupts trigger this WARN_ON
and potentially panic the host if panic_on_warn is enabled?

>  	}
>  
>  	return IRQ_HANDLED;

[Severity: High]
This isn't a bug introduced by this patch, but does this function
unconditionally return IRQ_HANDLED even when the interrupt wasn't generated
by this device?

If the status register does not have STATUS_IRQ_RAISED set, bypassing the
internal block, returning IRQ_HANDLED instead of IRQ_NONE could mask
unhandled interrupts from other devices sharing the same IRQ line.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does using devm_kzalloc() for the state
structure expose a use-after-free regression during device unbind?

If the PCI device is unbound, devm_kzalloc() frees the struct
pci_endpoint_test upon return from pci_endpoint_test_remove(). However, if
an application still holds the device node open, could a subsequent
ioctl() access the freed pointer when resolving the private data?

drivers/misc/pci_endpoint_test.c:pci_endpoint_test_ioctl() {
	...
	struct pci_endpoint_test *test = to_endpoint_test(file->private_data);
	struct pci_dev *pdev = test->pdev;
	
	mutex_lock(&test->mutex);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1