Re: [PATCH v7 09/10] PCI: endpoint: Add DMA endpoint function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing `.remove` callback in the EPF driver allows Use-After-Free upon manual driver unbind.
- [High] TOCTOU race in ConfigFS attributes allows bypassing bound state checks, leading to heap buffer overflows.
- [Medium] Mixing goto-based error handling with scope-based `__free()` cleanup in the same function.
--

--- Patch [9]: [PATCH v7 09/10] PCI: endpoint: Add DMA endpoint function ---
commit 59168807cdf2645897d81a698bfcd8a7744d66f2
Author: Koichiro Den <den@valinux.co.jp>
Subject: PCI: endpoint: Add DMA endpoint function

Add pci-epf-dma, an endpoint function that exposes selected
endpoint-integrated DMA channels as a separate PCI DMA controller
function.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-dma.c b/drivers/pci/endpoint/functions/pci-epf-dma.c
> new file mode 100644
> index 0000000000000..7e1c3e184a06b
> --- /dev/null
> +++ b/drivers/pci/endpoint/functions/pci-epf-dma.c
[ ... ]
> +static int pci_epf_dma_collect_resources(struct pci_epf_dma *epf_dma)
> +{
> +	const struct pci_epc_aux_resource *ep_to_rc_aux_chan[EDMA_MAX_WR_CH] = {};
> +	const struct pci_epc_aux_resource *rc_to_ep_aux_chan[EDMA_MAX_RD_CH] = {};
> +	const struct pci_epc_aux_resource *ep_to_rc_desc[EDMA_MAX_WR_CH] = {};
> +	const struct pci_epc_aux_resource *rc_to_ep_desc[EDMA_MAX_RD_CH] = {};
> +	const struct pci_epc_aux_resource *ctrl = NULL;
> +	struct pci_epf *epf = epf_dma->epf;
> +	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
> +	int count, i, ret;
> +
> +	count = pci_epc_get_aux_resources_count(epc, epf->func_no,
> +						epf->vfunc_no);
> +	if (count <= 0)
> +		return count ?: -ENODEV;
> +
> +	struct pci_epc_aux_resource *res __free(kfree) =
> +						kzalloc_objs(*res, count);

[Severity: Medium]
Does this code mix goto-based error handling and scope-based cleanup in the
same function?

The res variable uses __free(kfree) for cleanup, but later failures jump to
a goto label instead of returning directly:

	for (i = 0; i < epf_dma->wr_chans; i++) {
		ret = pci_epf_dma_claim_channel(epf_dma, ep_to_rc_aux_chan[i],
						&epf_dma->ep_to_rc_chan[i]);
		if (ret)
			goto err_release_channels;
	}

[ ... ]
> +static int pci_epf_dma_build_metadata(struct pci_epf_dma *epf_dma)
> +{
> +	const struct pci_epf_dma_bar_map *ctrl_map;
> +	u16 entry_size = PCI_EP_DMA_METADATA_CH_ENTRY_SIZE;
> +	u16 wr_table, rd_table, total_len;
> +	__le32 *metadata = epf_dma->metadata_addr;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!metadata)
> +		return -EINVAL;
> +
> +	ctrl_map = pci_epf_dma_find_map(epf_dma, epf_dma->ctrl);
> +	if (!ctrl_map)
> +		return -EINVAL;
> +	if (epf_dma->wr_chans > FIELD_MAX(PCI_EP_DMA_METADATA_CTRL_WR_CH_COUNT) ||
> +	    epf_dma->rd_chans > FIELD_MAX(PCI_EP_DMA_METADATA_CTRL_RD_CH_COUNT) ||
> +	    entry_size > FIELD_MAX(PCI_EP_DMA_METADATA_CTRL_CH_ENTRY_SIZE) ||
> +	    ctrl_map->res->size > U32_MAX)
> +		return -EOVERFLOW;
> +
> +	wr_table = epf_dma->wr_chans ? PCI_EP_DMA_METADATA_HDR_LEN : 0;
> +	rd_table = epf_dma->rd_chans ?
> +		   PCI_EP_DMA_METADATA_HDR_LEN + epf_dma->wr_chans * entry_size : 0;
> +	total_len = PCI_EP_DMA_METADATA_HDR_LEN +
> +		    (epf_dma->wr_chans + epf_dma->rd_chans) * entry_size;
> +
> +	memset(metadata, 0, total_len);

[Severity: High]
Can a concurrent write to the configfs attributes cause an out-of-bounds
memset() here?

During the binding process (pci_epf_dma_bind), pci_epf_dma_build_layout()
allocates space based on the current wr_chans. However, the store
function for the attribute checks epf_dma->epf->epc without holding a lock:

static ssize_t pci_epf_dma_wr_chans_store(...)
{
	...
	if (epf_dma->epf->epc)
		return -EOPNOTSUPP;
	...
	epf_dma->wr_chans = val;
	...
}

Could this allow wr_chans to increase after the space is allocated but
before total_len is calculated for this memset?

[ ... ]
> +static struct pci_epf_driver pci_epf_dma_driver = {
> +	.driver.name	= "pci_epf_dma",
> +	.probe		= pci_epf_dma_probe,
> +	.id_table	= pci_epf_dma_ids,
> +	.ops		= &pci_epf_dma_ops,
> +	.owner		= THIS_MODULE,
> +};

[Severity: Critical]
If a user manually unbinds the driver via sysfs, does the lack of a .remove
callback lead to a use-after-free?

Because there is no .remove callback, the unbind operation appears to free
the epf_dma structure via devres cleanup, but leaves the epf->event_ops
pointer populated and the map_work delayed work still pending.

Could an in-flight map_work execution or a subsequent EPC link event try to
dereference the freed epf_dma memory?

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