Re: [PATCH v2] misc: pci_endpoint_test: fix use-after-free after device unbind

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Adding a timeout to DMA transfer ioctls without aborting the ongoing hardware operation allows the PCI endpoint to perform DMA into freed physical memory, causing a use-after-free and memory corruption.

Pre-existing issues:
- [Critical] Spurious delayed completions from timed-out commands desynchronize the ioctl state machine, causing subsequent commands to prematurely return and free in-use DMA buffers.
--

commit 55971777f70d30f6a31f34520e097bb21bd66951
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

misc: pci_endpoint_test: fix use-after-free after device unbind

This commit changes the lifetime management of the pci_endpoint_test object
by using a kref and taking a reference during .open(). It also replaces
wait_for_completion() with wait_for_completion_timeout() to prevent
unbounded waits on missing endpoint interrupts.

Link: https://lore.kernel.org/all/178144969601.60470.7358419009914000395@gmail.com/

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index dbd017cabbb92..65d768dab4504 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
[ ... ]
> @@ -804,10 +815,17 @@ static int 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);
> +	if (!wait_for_completion_timeout(&test->irq_raised,
> +					 msecs_to_jiffies(1000))) {
> +		ret = -ETIMEDOUT;
> +		goto err_dst_unmap;
> +	}
>  
> +err_dst_unmap:
>  	dma_unmap_single(dev, orig_dst_phys_addr, size + alignment,
>  			 DMA_FROM_DEVICE);
> +	if (ret)
> +		goto err_dst_phys_addr;

[Severity: Critical]
If this times out, does the endpoint hardware continue its DMA transfer?

On timeout, this jumps to err_dst_unmap, where dma_unmap_single() and
subsequently kfree() will be called to free the memory. If the PCI endpoint
hardware is still actively processing the transfer and wasn't aborted, won't
it perform DMA into freed physical memory, causing memory corruption?

[ ... ]
> @@ -909,12 +927,17 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
>  	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>  				 COMMAND_READ);
>  
> -	wait_for_completion(&test->irq_raised);
> +	if (!wait_for_completion_timeout(&test->irq_raised,
> +					 msecs_to_jiffies(1000))) {
> +		ret = -ETIMEDOUT;
> +		goto err_unmap_addr;
> +	}
>  
>  	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
>  	if (!(reg & STATUS_READ_SUCCESS))
>  		ret = -EIO;
>  
> +err_unmap_addr:
>  	dma_unmap_single(dev, orig_phys_addr, size + alignment,
>  			 DMA_TO_DEVICE);

[Severity: Critical]
Similar to pci_endpoint_test_copy() above, if this times out, won't the
source buffer be unmapped and freed while the endpoint hardware might still be
reading from it?

[ ... ]
> @@ -1000,10 +1023,17 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
>  	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>  				 COMMAND_WRITE);
>  
> -	wait_for_completion(&test->irq_raised);
> +	if (!wait_for_completion_timeout(&test->irq_raised,
> +					 msecs_to_jiffies(1000))) {
> +		ret = -ETIMEDOUT;
> +		goto err_unmap_addr;
> +	}
>  
> +err_unmap_addr:
>  	dma_unmap_single(dev, orig_phys_addr, size + alignment,
>  			 DMA_FROM_DEVICE);
> +	if (ret)
> +		goto err_phys_addr;

[Severity: Critical]
As with the other ioctls, if this times out, won't the destination buffer
be unmapped and freed while the endpoint hardware might still be writing to it?

[ ... ]
> @@ -1141,10 +1171,15 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  {
>  	int ret = -EINVAL;
>  	enum pci_barno bar;
> -	struct pci_endpoint_test *test = to_endpoint_test(file->private_data);
> -	struct pci_dev *pdev = test->pdev;
> +	struct pci_endpoint_test *test = file->private_data;
> +	struct pci_dev *pdev;
>  
>  	mutex_lock(&test->mutex);
> +	pdev = test->pdev;
> +	if (!pdev) {
> +		ret = -ENODEV;
> +		goto ret;
> +	}
>  
>  	reinit_completion(&test->irq_raised);
>  	test->last_irq = -ENODATA;

[Severity: Critical]
This is a pre-existing issue, but won't reinit_completion() here be racy
after a previous command timed out?

Since a timed-out command leaves the endpoint hardware running, a delayed
interrupt from that prior command could arrive later. Because
reinit_completion() is called without first ensuring the hardware is idle or
discarding pending interrupts, could a spurious completion from a timed-out
command falsely wake up a subsequent ioctl's wait_for_completion_timeout()?

If this happens for the DMA-based READ, WRITE, or COPY ioctls, it would cause
them to prematurely wake up and free their in-use DMA buffers before the
hardware actually finishes the new transfer, leading to a use-after-free
and memory corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622165249.374155-1-shuangpeng.kernel@gmail.com?part=1