Re: [PATCH 03/26] dma-buf: Add dma_buf_mapping_attach()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dma-buf: NULL pointer dereference of importer_ops in dma_buf_dynamic_attach()
--

commit 4415ca9de8f7d7c189e76efc4aa022c3564f88f1
Author: Jason Gunthorpe <jgg@ziepe.ca>

dma-buf: Add dma_buf_mapping_attach()

This commit introduces dma_buf_mapping_attach() to match importer-supported
mapping types against exporter-supported mapping types during attachment. It
also converts dma_buf_attach() and dma_buf_dynamic_attach() into wrappers
around dma_buf_mapping_attach().

> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 4c9add51f9ef5..16a7fbb8aaabe 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c

[ ... ]

> @@ -1025,31 +1035,55 @@ dma_buf_pin_on_map(struct dma_buf_attachment *attach)

[ ... ]

> -	attach->dev = dev;
>  	attach->dmabuf = dmabuf;
> -	if (importer_ops)
> -		attach->peer2peer = importer_ops->allow_peer2peer;
>  	attach->importer_ops = importer_ops;
>  	attach->importer_priv = importer_priv;

[ ... ]

> @@ -1068,23 +1103,53 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,

[ ... ]

> +/**
> + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list
> + * @dmabuf:		[in]	buffer to attach device to.
> + * @dev:		[in]	device to be attached.
> + * @importer_ops:	[in]	importer operations for the attachment
> + * @importer_priv:	[in]	importer private pointer for the attachment
> + *
> + * Wrapper to call dma_buf_mapping_attach() for drivers which only support SGT.
> + */
> +struct dma_buf_attachment *
> +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
> +		       const struct dma_buf_attach_ops *importer_ops,
> +		       void *importer_priv)
> +{
> +	struct dma_buf_mapping_match sgt_match[] = {
> +		DMA_BUF_IMAPPING_SGT(dev, importer_ops->allow_peer2peer ?
> +						  DMA_SGT_IMPORTER_ACCEPTS_P2P :
> +						  DMA_SGT_NO_P2P),
> +	};

[Severity: High]
Can importer_ops be NULL when passed to dma_buf_dynamic_attach(), causing a
NULL pointer dereference when evaluating importer_ops->allow_peer2peer?

Previously, dma_buf_dynamic_attach() guarded this dereference with
if (importer_ops). In-tree callers still rely on passing a NULL importer_ops
pointer here, such as xe_gem_prime_import() when running the non-dynamic KUnit
test cases in drivers/gpu/drm/xe/tests/xe_dma_buf.c:

drivers/gpu/drm/xe/xe_dma_buf.c:xe_gem_prime_import() {
	...
	attach_ops = &xe_dma_buf_attach_ops;
#if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
	if (test)
		attach_ops = test->attach_ops;
#endif

	attach = dma_buf_dynamic_attach(dma_buf, dev->dev, attach_ops, obj);
	...
}

Note that a later commit in the series, 77ea3fee521c ("dma-buf: Rename
dma_buf_dynamic_attach() to dma_buf_sgt_dynamic_attach()"), renames this
function while keeping the unconditional importer_ops->allow_peer2peer
dereference.

Should dma_buf_dynamic_attach() check importer_ops &&
importer_ops->allow_peer2peer before dereferencing importer_ops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026053623.202642-1-apopple@nvidia.com?part=3