Re: [PATCH 2/2] misc: pci_endpoint_test: Improve logic to check status for doorbell test case

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

New issues:
- [Medium] PCIe surprise removals masked by incorrectly evaluating an all-1s read as success.

Pre-existing issues:
- [High] Unvalidated endpoint input leads to NULL pointer dereference and out-of-bounds MMIO write.
- [High] Race condition between module teardown and active ioctls leading to double-free or UAF of IRQ vectors.
- [High] Unbounded, uninterruptible waits on endpoint interrupts create unkillable tasks and a global ioctl deadlock.
--

--- Patch [2]: [PATCH 2/2] misc: pci_endpoint_test: Improve logic to check status for doorbell test case ---
commit e34bb71884c618c46be8a57cc0405805a9d2c585
Author: Niklas Cassel <[email protected]>

misc: pci_endpoint_test: Improve logic to check status for doorbell test case

This patch modifies the doorbell test case to verify if the SUCCESS bit was
set rather than checking if the FAIL bit was set. This handles cases where
an IRQ timeout occurs or buggy EPC drivers send multiple IRQs.

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -1094,7 +1094,7 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
>  	left = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
>  
>  	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> -	if (!left || (status & STATUS_DOORBELL_ENABLE_FAIL)) {
> +	if (!left || !(status & STATUS_DOORBELL_ENABLE_SUCCESS)) {

[Severity: Medium]
If the PCIe endpoint surprise-disconnects or the link goes down, MMIO reads
like pci_endpoint_test_readl() will return all-1s (0xFFFFFFFF). By checking
!(status & STATUS_DOORBELL_ENABLE_SUCCESS), an all-1s read evaluates as true
for the bitwise AND, making the logical NOT false.

Does this inadvertently bypass the error handler during a hardware failure?

[ ... ]

> 	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> 	if (bar < BAR_0 || bar >= PCI_STD_NUM_BARS) {
> 		dev_err(dev, "BAR %d reported by endpoint out of range [0, %u]\n",
> 			bar, PCI_STD_NUM_BARS - 1);
> 		return -ERANGE;
> 	}
> 
> 	writel(data, test->bar[bar] + addr);

[Severity: High]
This is a pre-existing issue, but is it possible for a buggy or malicious
PCIe endpoint to provide a BAR index that was never memory-mapped by the
host (where test->bar[bar] is NULL), or provide an excessively large addr
offset via PCI_ENDPOINT_TEST_DB_OFFSET?

Could this lead to a NULL pointer dereference or an out-of-bounds MMIO write
if test->bar[bar] or addr are not validated before the writel() call?

[ ... ]

> @@ -1133,7 +1133,7 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
>  
>  	status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
>  
> -	if (status & STATUS_DOORBELL_DISABLE_FAIL) {
> +	if (!(status & STATUS_DOORBELL_DISABLE_SUCCESS)) {

[Severity: Medium]
Similarly here, if the read returns all-1s due to a disconnect, will this
falsely indicate success and bypass the error handler?

[ ... ]

> static void pci_endpoint_test_remove(struct pci_dev *pdev)
> {
[ ... ]
> 	pci_endpoint_test_release_irq(test);
> 	pci_endpoint_test_free_irq_vectors(test);
> 
> 	misc_deregister(&test->miscdev);

[Severity: High]
This is a pre-existing issue, but is there a race condition here between
module teardown and active ioctls?

If IRQ resources are released before misc_deregister() and without acquiring
test->mutex, could an existing open file descriptor execute an ioctl
concurrently, leading to a use-after-free or double-free of IRQ vectors?

[ ... ]

> static bool pci_endpoint_test_copy(struct pci_endpoint_test *test)
> {
[ ... ]
> 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> 				 COMMAND_COPY);
> 
> 	wait_for_completion(&test->irq_raised);

[Severity: High]
This is also a pre-existing issue, but does this unbounded wait create a
potential local denial of service?

If a user issues an ioctl (like PCITEST_COPY), the driver holds test->mutex,
sends a command to the endpoint, and waits here. If the endpoint
malfunctions and never fires the IRQ, will the calling task become an
unkillable zombie in TASK_UNINTERRUPTIBLE while holding the device-wide
mutex indefinitely?

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