Re: [PATCH v5 4/6] PCI: dwc: Expose endpoint DMA resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] TOCTOU race condition and lockless access of `edma` state leads to kernel heap buffer overflow.
- [High] Missing implementation of `delegate_dma_chan` and `reclaim_dma_chan` in `pci_epc_ops` makes exposed DMA channels unusable.
--

--- Patch [4]: [PATCH v5 4/6] PCI: dwc: Expose endpoint DMA resources ---
commit 0cf8b17e421eb425ac8f9d3f97c02af0eb190199
Author: Koichiro Den <den@valinux.co.jp>

PCI: dwc: Expose endpoint DMA resources

Expose the DesignWare endpoint-integrated eDMA register window, logical
DMA channels, and linked-list descriptor memories through the EPC
auxiliary resource API. Expose DMA controller and channel resources only
after the local DW eDMA provider has been registered.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 7d2794945704e..ef1e2b6d85cf3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c

[ ... ]

> @@ -865,14 +895,23 @@ dw_pcie_ep_get_aux_resources_count(struct pci_epc *epc, u8 func_no,
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct dw_edma_chip *edma = &pci->edma;
> +	int ret;
> +	int count = 0;
>  
>  	if (!pci->edma_reg_size)
>  		return 0;
>  
> -	if (edma->db_offset == ~0)
> -		return 0;
> +	ret = dw_pcie_ep_check_edma_vfunc(vfunc_no);
> +	if (ret)
> +		return ret;
>  
> -	return 1;
> +	if (dw_pcie_ep_has_edma_ll_resources(edma))
> +		count += 1 + 2 * (edma->ll_wr_cnt + edma->ll_rd_cnt);
> +
> +	if (edma->dw && edma->db_offset != ~0)
> +		count++;

[Severity: Critical]
Is it possible for edma->dw to change concurrently here (e.g. if the link
state changes and dw_edma_probe() completes), leading to an inconsistent
count and a buffer overflow in dw_pcie_ep_get_aux_resources() below?

If edma->dw is NULL during dw_pcie_ep_has_edma_ll_resources(), but becomes
non-NULL before the second check, count will be evaluated as 1.

> +
> +	return count;
>  }
>  
>  static int
> @@ -888,6 +927,8 @@ dw_pcie_ep_get_aux_resources(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	resource_size_t db_offset = edma->db_offset;
>  	resource_size_t dma_ctrl_bar_offset = 0;
>  	resource_size_t dma_reg_size;
> +	bool has_edma_ll_resources;
> +	unsigned int i;
>  	int count;
>  
>  	count = dw_pcie_ep_get_aux_resources_count(epc, func_no, vfunc_no);
> @@ -901,6 +942,7 @@ dw_pcie_ep_get_aux_resources(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		return 0;
>  
>  	dma_reg_size = pci->edma_reg_size;
> +	has_edma_ll_resources = dw_pcie_ep_has_edma_ll_resources(edma);

[Severity: Critical]
Since count could be calculated as 1 earlier, the bounds check
(num_resources < count) passes. However, has_edma_ll_resources is
re-evaluated here. 

If edma->dw is now non-NULL, this evaluates to true, and the function
will write 1 + 2 * (wr_cnt + rd_cnt) elements into the resources array which
may only be sized for 1 element, causing an out-of-bounds write.

[ ... ]

> @@ -917,7 +1029,7 @@ dw_pcie_ep_get_aux_resources(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  				  sizeof(u32), dma_reg_size))
>  		return -EINVAL;
>  
> -	resources[0] = (struct pci_epc_aux_resource) {
> +	resources[count] = (struct pci_epc_aux_resource) {
>  		.type = PCI_EPC_AUX_DOORBELL_MMIO,
>  		.phys_addr = pci->edma_reg_phys + db_offset,
>  		.size = sizeof(u32),

[Severity: High]
This patch exposes DMA channels via dw_pcie_ep_get_aux_resources(), but does
epc_ops also need to implement the delegate_dma_chan and reclaim_dma_chan
callbacks?

Without these callbacks in epc_ops, if an EPF driver attempts to reserve one
of these newly exposed DMA channels by calling pci_epc_delegate_dma_chan(),
will it immediately fail with -EOPNOTSUPP?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717050635.2145014-1-den@valinux.co.jp?part=4