[PATCH 0/2] misc: pci_endpoint_test: Doorbell fix and improvement

[PATCH 0/2] misc: pci_endpoint_test: Doorbell fix and improvement

[Niklas Cassel]

Hello all,

There is one pci_endpoint_test fix and one improvement related to
the doorbell test case.

Kind regards,
Niklas


Niklas Cassel (2):
  misc: pci_endpoint_test: Fix doorbell success logic
  misc: pci_endpoint_test: Improve logic to check status for doorbell
    test case

 drivers/misc/pci_endpoint_test.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
2.55.0

[PATCH 1/2] misc: pci_endpoint_test: Fix doorbell success logic

[Niklas Cassel]

The following was observed:
pci-endpoint-test 0000:01:00.0: Failed to trigger doorbell in endpoint
ok 23 pcie_ep_doorbell.DOORBELL_TEST

We observed a bug where an EPC driver would send two IRQs during ENABLE
DOORBELL command.

Because we got two IRQs for a single command, test->irq_raised.done was
never decreased to zero by complete(), so the
wait_for_completion_timeout() after the writel() that rings the doorbell
would return immediately, before the endpoint had managed to set
STATUS_DOORBELL_SUCCESS and triggered an IRQ corresponding to that write.

Later, when re-reading the status after disabling the doorbell,
STATUS_DOORBELL_SUCCESS will have been set by the endpoint, thus we would
not fail the test case.

Improve the test case logic such that if we failed to trigger the doorbell
in the endpoint, we make sure that we also fail the test case.

Fixes: eefb83790a0d ("misc: pci_endpoint_test: Add doorbell test case")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3635741c3e7a..26ab9252251f 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -1071,6 +1071,7 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
 	struct pci_dev *pdev = test->pdev;
 	struct device *dev = &pdev->dev;
 	int irq_type = test->irq_type;
+	int ret = 0;
 	enum pci_barno bar;
 	u32 data, status;
 	u32 addr;
@@ -1119,8 +1120,11 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
 
 	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
 
-	if (!left || !(status & STATUS_DOORBELL_SUCCESS))
+	if (!left || !(status & STATUS_DOORBELL_SUCCESS)) {
 		dev_err(dev, "Failed to trigger doorbell in endpoint\n");
+		/* Store error code, but continue to disable doorbell. */
+		ret = -EINVAL;
+	}
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
 				 COMMAND_DISABLE_DOORBELL);
@@ -1134,10 +1138,7 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
 		return -EINVAL;
 	}
 
-	if (!(status & STATUS_DOORBELL_SUCCESS))
-		return -EINVAL;
-
-	return 0;
+	return ret;
 }
 
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
-- 
2.55.0

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

[Niklas Cassel]

The EPF driver drivers/pci/endpoint/functions/pci-epf-test.c
only sets STATUS_DOORBELL_ENABLE_SUCCESS as the final step in
pci_epf_test_enable_doorbell()

Likewise it only sets STATUS_DOORBELL_DISABLE_SUCCESS as the final step in
pci_epf_test_disable_doorbell().

The safest way is therefore to check if the SUCCESS bit was set,
not to check if the FAIL bit was set.

Because, during a wait_for_completion_timeout() timeout, if we did not
receive an IRQ, obviously the FAIL bit will not be set.

For the STATUS_DOORBELL_ENABLE case, the if-statement also fails if there
was a timeout ('left' == 0). However, we've seen an issue where a buggy EPC
driver was sending two IRQs in response to a single DOORBELL_ENABLE
command, and in this case the second wait_for_completion() will return
immediately (i.e. no timeout - 'left' != zero) and the SUCCESS bit will not
be set. Thus, the safest way is to simply check if the SUCCESS bit is set.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 26ab9252251f..1019bcd0628f 100644
--- 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)) {
 		dev_err(dev, "Failed to enable doorbell\n");
 		return -EINVAL;
 	}
@@ -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)) {
 		dev_err(dev, "Failed to disable doorbell\n");
 		return -EINVAL;
 	}
-- 
2.55.0

Re: [PATCH 1/2] misc: pci_endpoint_test: Fix doorbell success logic

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing validation of device-supplied BAR and offset allows arbitrary out-of-bounds memory write and NULL pointer dereference.
- [High] Use-after-free and race condition in device removal due to improper teardown ordering.
--

--- Patch [1]: [PATCH 1/2] misc: pci_endpoint_test: Fix doorbell success logic ---
commit fca8adde489026db6a363f41e32f957940be89f4
Author: Niklas Cassel <cassel@kernel.org>

misc: pci_endpoint_test: Fix doorbell success logic

This patch modifies the pci_endpoint_test_doorbell function to ensure
that if triggering the doorbell fails, the test stores the error code
but continues on to disable the doorbell rather than returning early.

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3635741c3e7a7..26ab9252251f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -1071,6 +1071,7 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
>  	struct pci_dev *pdev = test->pdev;
>  	struct device *dev = &pdev->dev;
>  	int irq_type = test->irq_type;
> +	int ret = 0;
>  	enum pci_barno bar;
>  	u32 data, status;
>  	u32 addr;
> @@ -1119,8 +1120,11 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)

[Severity: High]
This is a pre-existing issue, but since we are modifying the error handling
in pci_endpoint_test_doorbell(), is there an out-of-bounds write or NULL
pointer dereference possible just above this diff?

A malicious or malfunctioning PCIe endpoint can supply arbitrary values for
addr and bar. The code checks if bar is within the enum bounds, but does
not check if test->bar[bar] is actually mapped (i.e., non-NULL), nor does
it bounds-check addr against the mapped BAR's size before calling:

    writel(data, test->bar[bar] + addr);

Could this allow an arbitrary MMIO write to kernel space?

>  	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
>  
> -	if (!left || !(status & STATUS_DOORBELL_SUCCESS))
> +	if (!left || !(status & STATUS_DOORBELL_SUCCESS)) {
>  		dev_err(dev, "Failed to trigger doorbell in endpoint\n");
> +		/* Store error code, but continue to disable doorbell. */
> +		ret = -EINVAL;
> +	}
>  
>  	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>  				 COMMAND_DISABLE_DOORBELL);
> @@ -1134,10 +1138,7 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
>  		return -EINVAL;
>  	}
>  
> -	if (!(status & STATUS_DOORBELL_SUCCESS))
> -		return -EINVAL;
> -
> -	return 0;
> +	return ret;
>  }
>  
>  static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,

[Severity: High]
This isn't a bug introduced by this patch, but looking at how ioctls are
handled here, is there a use-after-free race condition during device
removal?

If a PCIe hot-unplug event or driver unbind occurs while a userspace
process is executing an ioctl, pci_endpoint_test_remove() will execute
concurrently:

    pci_endpoint_test_release_irq(test);
    pci_endpoint_test_free_irq_vectors(test);
    misc_deregister(&test->miscdev);

misc_deregister() does not wait for existing active file operations to
complete. Because the test struct is managed by devm_kzalloc(), it is freed
immediately after remove() returns.

Could the concurrent ioctl handler:

    mutex_lock(&test->mutex);

access the freed test memory or interact with torn-down IRQs or MMIO
regions? Does the driver need to synchronize device teardown with active
ioctls?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730122045.1382749-4-cassel@kernel.org?part=1

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

[sashiko-bot]

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 <cassel@kernel.org>

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/20260730122045.1382749-4-cassel@kernel.org?part=2

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

[Niklas Cassel]

On Thu, Jul 30, 2026 at 12:33:57PM +0000, sashiko-bot@kernel.org wrote:
> 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 <cassel@kernel.org>
> 
> 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?

With this logic, every readl() in drivers/misc/pci_endpoint_test.c should check
if the return value is 0xFFFFFFFF.


Which they do not:

reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);          
        if (!(reg & STATUS_READ_SUCCESS))                                       
                ret = -EIO;                                   


This brings pci_endpoint_test_doorbell() in line with pci_endpoint_test_read().

I still think this patch makes sense.


Kind regards,
Niklas