Re: [PATCH 04/26] dma-buf: Route SGT related actions through attach->map_type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dma-buf: dma_buf_get_sgt_ops() called before !attach NULL check
--

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

dma-buf: Route SGT related actions through attach->map_type

This commit routes sg_table map, unmap, and debugfs dump operations through
attach->map_type instead of calling attach->dmabuf->ops directly.

> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 16a7fbb8aaabe..348b47f266b48 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1253,12 +1253,14 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, "DMA_BUF");
>  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  					enum dma_data_direction direction)
>  {
> +	const struct dma_buf_mapping_sgt_exp_ops *sgt_exp_ops =
> +		dma_buf_get_sgt_ops(attach);
>  	struct sg_table *sg_table;
>  	signed long ret;
>  
>  	might_sleep();
>  
> -	if (WARN_ON(!attach || !attach->dmabuf))
> +	if (WARN_ON(!attach || !attach->dmabuf || !sgt_exp_ops))
>  		return ERR_PTR(-EINVAL);

[ ... ]

> @@ -1366,15 +1368,18 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  				struct sg_table *sg_table,
>  				enum dma_data_direction direction)
>  {
> +	const struct dma_buf_mapping_sgt_exp_ops *sgt_exp_ops =
> +		dma_buf_get_sgt_ops(attach);
> +
>  	might_sleep();
>  
> -	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> +	if (WARN_ON(!attach || !attach->dmabuf || !sg_table || !sgt_exp_ops))
>  		return;

[Severity: Medium]
Can calling dma_buf_get_sgt_ops(attach) in the variable initializers of
dma_buf_map_attachment() and dma_buf_unmap_attachment() cause a null
pointer dereference when attach is null?

In include/linux/dma-buf-mapping.h, dma_buf_get_sgt_ops() unconditionally
dereferences attach:

static inline const struct dma_buf_mapping_sgt_exp_ops *
dma_buf_get_sgt_ops(struct dma_buf_attachment *attach)
{
	if (attach->map_type.type != &dma_buf_mapping_sgt_type)
		return NULL;
	return container_of(attach->map_type.exp_ops,
			    struct dma_buf_mapping_sgt_exp_ops, ops);
}

Because dma_buf_get_sgt_ops(attach) runs before the WARN_ON(!attach || ...)
checks in both functions, passing a null attach will dereference a null
pointer instead of triggering the warning and returning early, and the
compiler may also optimize away the subsequent !attach checks.

Note that while the dma_buf_map_attachment() path is refactored in a later
commit in this series ("dma-buf: Let the exporter select mapping type at
pin/map") when __dma_buf_sgt_map_attachment() is split out after the
WARN_ON(!attach || !attach->dmabuf) check, dma_buf_unmap_attachment()
(later renamed to dma_buf_sgt_unmap_attachment() in "treewide: Rename
dma_buf_unmap_attachment(_unlocked) to dma_buf_sgt_*") keeps the
dma_buf_get_sgt_ops(attach) call before WARN_ON(!attach || ...).

Should sgt_exp_ops be initialized after the !attach check in both
functions?

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