Re: [PATCH v4 07/10] dma-buf: heaps: Add support for Tegra VPR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] `tegra_vpr_map_dma_buf()` unconditionally calls `dma_map_sgtable()` on an SG table initialized with a NULL page for static VPRs.
- [High] Ad-hoc invocation of system PM callbacks (`pm_generic_freeze` / `pm_generic_thaw`) without `device_lock()` and freezing userspace risks severe data races and hardware state corruption.
- [High] Memory allocated with `kvmalloc_array()` is incorrectly freed using `kfree()`, which will cause a kernel panic if the allocation fell back to vmalloc.
- [High] `tegra_vpr_setup_chunks()` can calculate a chunk size of 0 for the final chunk, causing initialization to fail.
- [High] Calling `dma_heap_get_drvdata(heap)` before checking if `rmem->priv` is NULL causes a NULL pointer dereference.
- [High] If `tegra_vpr_chunk_activate` fails during the `tegra_vpr_recycle` error recovery path, it causes a CMA memory leak and invalid chunk state.
- [High] Potential NULL pointer dereference in `tegra_vpr_device_init()` if the device has no bound driver.
- [Medium] Unprotected iteration over the `vpr->buffers` list in `tegra_vpr_debugfs_show_chunks()` causes data races.
- [Medium] An off-by-one error in `tegra_vpr_find_free_region()` prevents allocations from using the last page of the VPR.
- [Medium] The device node reference acquired via `of_node_get()` is leaked on the error paths of `tegra_vpr_add_heap()`.
- [Low] The `dirty` bitmap is declared using a runtime variable, creating a Variable Length Array (VLA).
--

--- Patch [7]: [PATCH v4 07/10] dma-buf: heaps: Add support for Tegra VPR ---
commit c6dee57f094afce846e231b53f10aa5c8c4898a7
Author: Thierry Reding <treding@nvidia.com>

dma-buf: heaps: Add support for Tegra VPR

NVIDIA Tegra SoCs commonly define a Video-Protection-Region, which is a
region of memory dedicated to content-protected video decode and
playback. Expose the VPR as a DMA heap so that applications and drivers
can allocate buffers from this region.

> diff --git a/drivers/dma-buf/heaps/tegra-vpr.c b/drivers/dma-buf/heaps/tegra-vpr.c
> new file mode 100644
> index 0000000000000..b56dfa5c7a0a0
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/tegra-vpr.c
[ ... ]
> @@ -171,6 +171,11 @@ static int tegra_vpr_resize(struct tegra_vpr *vpr)
>  	}
>  
>  	list_for_each_entry(node, &vpr->devices, node) {
>  		err = pm_generic_freeze(node->dev);

[Severity: High]
Does this code introduce a data race or risk hardware corruption by invoking
pm_generic_freeze() directly from an ioctl context? These callbacks are
normally executed during system-wide suspend and expect device_lock() to be
held and userspace to be frozen.

>  		if (err < 0) {
>  			pr_err("failed to freeze %s: %d\n",
>  			       dev_name(node->dev), err);
[ ... ]
> @@ -368,6 +368,7 @@ static int tegra_vpr_activate_chunks(struct tegra_vpr *vpr,
>  				     struct tegra_vpr_buffer *buffer)
>  {
>  	DECLARE_BITMAP(dirty, vpr->num_chunks);

[Severity: Low]
Does this introduce a variable length array (VLA)? vpr->num_chunks is not a
compile-time constant, so this expands to a dynamic allocation on the stack,
which might trigger build errors with -Wvla.

>  	unsigned int i, bottom, top;
>  	int err = 0, ret;
[ ... ]
> @@ -503,8 +503,8 @@ static int tegra_vpr_find_free_region(struct tegra_vpr *vpr,
>  				      unsigned int num_pages,
>  				      unsigned long align)
>  {
>  	return tegra_vpr_find_free_region_in_range(vpr, 0, vpr->num_pages - 1,
>  						   num_pages, align);

[Severity: Medium]
Does this code leak the last page of the VPR due to an off-by-one boundary
check? tegra_vpr_find_free_region_in_range() loops while
pos + num_pages <= end_page, meaning passing vpr->num_pages - 1 as end_page
prevents allocations from using the final page.

>  }
[ ... ]
> @@ -668,9 +668,9 @@ tegra_vpr_buffer_allocate(struct tegra_vpr *vpr, size_t size)
>  		/*
>  		 * Memory is backed by struct page, so track which ones we
>  		 * use.
>  		 */
>  		buffer->pages = kvmalloc_array(buffer->num_pages,
>  					       sizeof(*buffer->pages),
>  					       GFP_KERNEL);

[Severity: High]
Does this memory leak or cause a panic if the allocation falls back to
vmalloc? buffer->pages is later freed using kfree(), which will fail for
vmalloc addresses.

>  		if (!buffer->pages) {
>  			err = -ENOMEM;
[ ... ]
> @@ -746,8 +746,8 @@ static void tegra_vpr_buffer_release(struct tegra_vpr_buffer *buffer)
>  
>  	bitmap_clear(vpr->bitmap, buffer->pageno, buffer->num_pages);
>  	list_del(&buffer->list);
>  	kfree(buffer->pages);

[Severity: High]
Is it possible for this code to panic by calling kfree() on a vmalloc
pointer? buffer->pages was allocated using kvmalloc_array().

>  	kfree(buffer);
[ ... ]
> @@ -776,9 +776,9 @@ static int tegra_vpr_attach(struct dma_buf *buf,
>  		if (sg_alloc_table(&attach->sgt, 1, GFP_KERNEL)) {
>  			err = -ENOMEM;
>  			goto free;
>  		}
>  
>  		sg_set_page(attach->sgt.sgl, NULL, buffer->size, 0);

[Severity: Critical]
Can this code cause a panic when an importer maps the buffer? For static
VPRs, the SG table is initialized with a NULL page here, but later passed to
dma_map_sgtable() which attempts to dereference it.

>  		sg_dma_address(attach->sgt.sgl) = buffer->start;
>  		sg_dma_len(attach->sgt.sgl) = buffer->size;
>  	}
[ ... ]
> @@ -806,8 +806,8 @@ tegra_vpr_map_dma_buf(struct dma_buf_attachment *attachment,
>  	struct tegra_vpr_attachment *attach = attachment->priv;
>  	struct sg_table *sgt = &attach->sgt;
>  	int err;
>  
>  	err = dma_map_sgtable(attachment->dev, sgt, direction,
>  			      DMA_ATTR_SKIP_CPU_SYNC);

[Severity: Critical]
Does this code trigger a panic for static VPR buffers? The sgt can be
populated with a NULL page by tegra_vpr_attach(), causing dma_map_sgtable()
to dereference it.

>  	if (err < 0)
>  		return ERR_PTR(err);
[ ... ]
> @@ -826,6 +826,7 @@ static void tegra_vpr_unmap_dma_buf(struct dma_buf_attachment *attachment,
>  static void tegra_vpr_recycle(struct tegra_vpr *vpr)
>  {
>  	DECLARE_BITMAP(dirty, vpr->num_chunks);

[Severity: Low]
Does this introduce a variable length array (VLA)? vpr->num_chunks is not a
compile-time constant, so this expands to a dynamic stack allocation which
might trigger build errors with -Wvla.

>  	unsigned int i;
>  	int err;
[ ... ]
> @@ -890,7 +890,7 @@ static void tegra_vpr_recycle(struct tegra_vpr *vpr)
>  		/*
>  		 * This may not be fully activated at this point, but we need
>  		 * to keep track of it anyway to make sure the CMA region can
>  		 * eventually be released. The WARN_ON above tells us when it
>  		 * happens: here be dragons.
>  		 */
>  		set_bit(i, vpr->active);

[Severity: High]
Does this code create an inconsistent chunk state and leak CMA memory? If
tegra_vpr_chunk_activate() fails on the error path above, the chunk's active
flag remains false, but this blindly sets it as active in the VPR bitmap.
Subsequent allocations may see the chunk as inactive, allocating new memory
and leaking the old CMA allocation.

>  	}
>  }
[ ... ]
> @@ -1023,8 +1023,8 @@ static void tegra_vpr_debugfs_show_chunks(struct tegra_vpr *vpr,
>  			   chunk->active ? "active" : "inactive",
>  			   chunk->num_buffers);
>  	}
>  
>  	list_for_each_entry(buffer, &vpr->buffers, list) {

[Severity: Medium]
Does this unprotected traversal of vpr->buffers cause data races? This loop
iterates over the list without holding mutex_lock(&vpr->lock), which can race
against concurrent buffer allocation or release operations.

>  		string_get_size(buffer->size, 1, STRING_UNITS_2, buf,
>  				sizeof(buf));
[ ... ]
> @@ -1079,9 +1079,9 @@ static int tegra_vpr_setup_chunks(struct tegra_vpr *vpr, const char *name)
>  	for (i = 0; i < vpr->num_chunks; i++) {
>  		size_t size = limit - start;
>  		phys_addr_t end;
>  
>  		size = min_t(size_t, size, max_size);
>  		end = start + size - 1;
>  
>  		err = tegra_vpr_chunk_init(vpr, &vpr->chunks[i], start, size,
>  					   order, name);

[Severity: High]
Can this code fail during initialization if the calculated size is 0? If
vpr->size is not a power of 2, earlier chunks could consume all available
space. For the final chunk, size would evaluate to 0, which gets passed to
tegra_vpr_chunk_init() and cma_create(), failing the allocation.

>  		if (err < 0) {
>  			pr_err("failed to create VPR chunk: %d\n", err);
[ ... ]
> @@ -1134,6 +1134,7 @@ static int tegra_vpr_add_heap(struct reserved_mem *rmem,
>  	INIT_LIST_HEAD(&vpr->devices);
>  	mutex_init(&vpr->lock);
>  
>  	vpr->resizable = !of_property_read_bool(np, "no-map");
>  	vpr->dev_node = of_node_get(np);

[Severity: Medium]
Does this code leak the device node reference on error paths? If subsequent
operations like bitmap_zalloc() or dma_heap_add() fail, the function jumps to
the free label but does not call of_node_put(vpr->dev_node).

>  	vpr->align = PAGE_SIZE;
>  	vpr->base = rmem->base;
[ ... ]
> @@ -1215,9 +1215,9 @@ static int tegra_vpr_node_init(unsigned long offset, struct reserved_mem *rmem)
>  
>  static int tegra_vpr_device_init(struct reserved_mem *rmem, struct device *dev)
>  {
>  	struct dma_heap *heap = rmem->priv;
>  	struct tegra_vpr *vpr = dma_heap_get_drvdata(heap);
>  	const struct dev_pm_ops *pm = dev->driver->pm;

[Severity: High]
Can these lines cause a kernel panic due to NULL pointer dereferences?
dma_heap_get_drvdata() dereferences the heap pointer before checking if
rmem->priv (the heap) is NULL. Furthermore, dev->driver is dereferenced
without checking if a driver is actually bound to the device.

>  	struct tegra_vpr_device *node;
>  
>  	if (!rmem->priv)
>  		return -EPROBE_DEFER;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-tegra-vpr-v4-0-5510d16af89e@nvidia.com?part=7