Re: [PATCH v6 09/12] dma-buf: heaps: Add support for Tegra VPR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [High] Calling `cma_init_reserved_mem` on a `no-map` region causes a boot-time kernel panic.
- [High] Memory allocated with `kvmalloc_array()` is incorrectly freed with `kfree()`.
- [High] Calling `dma_map_sgtable()` with an SG list containing a NULL page maps garbage physical addresses.
- [High] Direct invocation of system PM callbacks (`pm_generic_freeze`/`thaw`) bypasses the PM core.
- [High] 0-sized chunks generate an integer underflow that permanently breaks VPR buffer allocation.
- [High] Pointer arithmetic on `struct page` arrays across MAX_ORDER / memory sections causes panics on SPARSEMEM.
- [High] Direct map corruption when returning partially invalidated pages to the CMA allocator.
- [High] Integer overflow in allocation size allows a zero-sized allocation that permanently bricks the heap.
- [High] Shrinking VPR restores linear mapping before the hardware protection is removed, risking speculative SErrors.
- [Medium] Memory and reference leaks on the error path of `tegra_vpr_add_heap()`.
- [Medium] Missing lock acquisition when traversing `vpr->buffers` list in `tegra_vpr_debugfs_show_chunks`.
- [Medium] Dereferencing `dev->driver->pm` without checking if `dev->driver` is valid.
- [Low] Off-by-one error in VPR end_page calculation permanently wastes the last page.
- [Low] Use of dynamically sized Variable Length Arrays (VLAs) via `DECLARE_BITMAP`.
- [Low] The commit message claims userspace processes are frozen during resize, but the code does not implement this.
--

commit 08e43ed119b9e546aa530f96844c1805cc1597a2
Author: Thierry Reding <treding@nvidia.com>
Subject: dma-buf: heaps: Add support for Tegra VPR

This commit introduces DMA-BUF heap support for the NVIDIA Tegra Video
Protection Region (VPR). It adds initialization for CMA memory during boot
and a heap driver for dynamically allocating and resizing chunks of this
protected memory.

>     Unfortunately, resizing the VPR is slightly tricky because the GPU found
>     on Tegra SoCs must be in reset during the VPR resize operation. This is
>     currently implemented by freezing all userspace processes and calling
>     invoking the GPU's freeze() implementation, resizing and the thawing the
>     GPU and userspace processes. This is quite heavy-handed, so eventually

[Severity: Low]
The commit message states that userspace processes are frozen during resize, 
but this doesn't seem to be implemented in the code (perhaps removed in v4?). 
Could the commit message be updated to reflect the current behavior?

> diff --git a/drivers/dma-buf/heaps/tegra-vpr-init.c b/drivers/dma-buf/heaps/tegra-vpr-init.c
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/tegra-vpr-init.c
> @@ -0,0 +1,133 @@

[ ... ]

> +static int __init tegra_vpr_node_init(unsigned long offset,
> +				      struct reserved_mem *rmem)
> +{
> +	struct cma *cma;
> +	int err;
> +
> +	if (!IS_ALIGNED(rmem->base, SZ_1M)) {
> +		pr_err("%s: base is not aligned to 1 MiB\n", rmem->name);
> +		return -EINVAL;
> +	}
> +
> +	if (!IS_ALIGNED(rmem->size, SZ_1M)) {
> +		pr_err("%s: size is not aligned to 1 MiB\n", rmem->name);
> +		return -EINVAL;
> +	}
> +
> +	err = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name,
> +				    &cma);

[Severity: High]
Can this cause a panic during early boot if the firmware configures a fixed 
VPR using the no-map property? A no-map region is removed from the linear 
memory map (no struct page allocation), but cma_init_reserved_mem() is called 
unconditionally. When CMA later tries to return pages to the buddy allocator 
via pfn_to_page(), won't it access invalid pointers?

[ ... ]

> +static int tegra_vpr_device_init(struct reserved_mem *rmem, struct device *dev)
> +{
> +	const struct dev_pm_ops *pm = dev->driver->pm;

[Severity: Medium]
Is it safe to blindly dereference dev->driver here? If this is called on a 
device node without a bound driver, dev->driver would be NULL, causing a panic. 
Should we verify dev->driver is valid first?

> diff --git a/drivers/dma-buf/heaps/tegra-vpr.c b/drivers/dma-buf/heaps/tegra-vpr.c
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/tegra-vpr.c
> @@ -0,0 +1,1210 @@

[ ... ]

> +static int tegra_vpr_resize(struct tegra_vpr *vpr)
> +{
> +	struct tegra_vpr_device *node;
> +	phys_addr_t base, size;
> +	int err, status = 0;
> +
> +	err = tegra_vpr_get_extents(vpr, &base, &size);
> +	if (err < 0) {
> +		pr_err("%s(): failed to get VPR extents: %d\n", __func__, err);
> +		return err;
> +	}
> +
> +	list_for_each_entry(node, &vpr->devices, node) {
> +		err = pm_generic_freeze(node->dev);

[ ... ]

> +	list_for_each_entry_continue_reverse(node, &vpr->devices, node) {
> +		err = pm_generic_thaw(node->dev);

[Severity: High]
Does calling pm_generic_freeze() and pm_generic_thaw() directly bypass the 
PM core's synchronization? Normally these are coordinated under device_lock. 
If a device's driver is concurrently unbinding, dev->driver might become NULL 
during the callback, which could lead to a kernel panic. Should this be 
coordinated with the PM core instead?

[ ... ]

> +static bool tegra_vpr_chunk_overlaps(struct tegra_vpr_chunk *chunk,
> +				     unsigned int start, unsigned int limit)
> +{
> +	unsigned int first = chunk->offset;
> +	unsigned int last = chunk->offset + chunk->num_pages - 1;

[Severity: High]
Could chunk->num_pages be 0 for trailing chunks if the total VPR size doesn't 
evenly divide into num_chunks? 

Looking at tegra_vpr_setup_chunks():
    size = min_t(size_t, size, max_size);
    end = start + size - 1;

If the remaining size is 0, this would cause chunk->num_pages to be 0, making 
the calculation underflow to UINT_MAX. This would cause the 0-sized chunk to 
falsely overlap all future allocations and break the VPR heap.

[ ... ]

> +static int tegra_vpr_activate_chunks(struct tegra_vpr *vpr,
> +				     struct tegra_vpr_buffer *buffer)
> +{
> +	DECLARE_BITMAP(dirty, vpr->num_chunks);

[Severity: Low]
Since vpr->num_chunks is evaluated at runtime, doesn't DECLARE_BITMAP create 
a Variable Length Array (VLA) on the stack? The kernel globally forbids VLAs 
and this will likely cause a build failure with -Werror=vla.

> +	unsigned int i, bottom, top;
> +	int err = 0, ret;
> +
> +	bitmap_zero(dirty, vpr->num_chunks);
> +
> +	/* activate any inactive chunks that overlap this buffer */
> +	for_each_set_bit(i, buffer->chunks, vpr->num_chunks) {
> +		struct tegra_vpr_chunk *chunk = &vpr->chunks[i];
> +
> +		if (chunk->active)
> +			continue;
> +
> +		err = tegra_vpr_chunk_alloc(chunk);
> +		if (err < 0)
> +			goto deactivate;
> +
> +		err = tegra_vpr_chunk_activate(chunk);
> +		if (err < 0) {
> +			tegra_vpr_chunk_release(chunk);
> +			goto deactivate;
> +		}

[Severity: High]
If set_direct_map_invalid_noflush() fails partially, tegra_vpr_chunk_activate() 
returns an error, and the caller immediately invokes tegra_vpr_chunk_release(). 
Will this return partially invalidated pages back to the CMA allocator without 
restoring the linear mapping? Subsequent allocations of these pages could 
trigger direct map faults.

[ ... ]

> +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: Low]
Is there an off-by-one error here? tegra_vpr_find_free_region_in_range() 
expects end_page as an exclusive upper bound, but passing vpr->num_pages - 1 
means an allocation perfectly reaching the end of the region will be 
incorrectly rejected.

[ ... ]

> +static struct tegra_vpr_buffer *
> +tegra_vpr_buffer_allocate(struct tegra_vpr *vpr, size_t size)
> +{
[ ... ]
> +		buffer->pages = kvmalloc_array(buffer->num_pages,
> +					       sizeof(*buffer->pages),
> +					       GFP_KERNEL);
> +		if (!buffer->pages) {
> +			err = -ENOMEM;
> +			goto free;
> +		}
> +
> +		for (i = 0; i < buffer->num_pages; i++)
> +			buffer->pages[i] = &vpr->start_page[pageno + i];

[Severity: High]
Is it safe to perform direct pointer arithmetic on struct page arrays across 
potentially massive VPR regions? On platforms using the SPARSEMEM memory model 
without VMEMMAP, struct page objects are only contiguous within a single memory 
section. A 2 GiB VPR region crosses many section boundaries, so this arithmetic 
could point into unmapped memory and crash the kernel when accessed.

[ ... ]

> +static void tegra_vpr_buffer_release(struct tegra_vpr_buffer *buffer)
> +{
[ ... ]
> +	kfree(buffer->pages);
> +	kfree(buffer);
> +}

[Severity: High]
buffer->pages is allocated using kvmalloc_array(), which may fall back to 
vmalloc space. Shouldn't this be freed with kvfree()? Calling kfree() on a 
vmalloc'd pointer will cause a SLUB panic or memory corruption.

[ ... ]

> +static int tegra_vpr_attach(struct dma_buf *buf,
> +			    struct dma_buf_attachment *attachment)
> +{
[ ... ]
> +	} else {
> +		if (sg_alloc_table(&attach->sgt, 1, GFP_KERNEL)) {
> +			err = -ENOMEM;
> +			goto free;
> +		}
> +
> +		sg_set_page(attach->sgt.sgl, NULL, buffer->size, 0);
> +		sg_dma_address(attach->sgt.sgl) = buffer->start;
> +		sg_dma_len(attach->sgt.sgl) = buffer->size;
> +	}

[Severity: High]
For static VPR setups, the SG list is populated with a NULL page. When 
tegra_vpr_map_dma_buf() later calls dma_map_sgtable(), the IOMMU DMA mapping 
code relies on sg_phys(), which calculates page_to_phys(NULL) + offset. This 
usually results in physical address 0. The IOMMU will be programmed to map 
address 0 instead of the actual VPR hardware address, breaking DMA.

[ ... ]

> +static void tegra_vpr_recycle(struct tegra_vpr *vpr)
> +{
> +	DECLARE_BITMAP(dirty, vpr->num_chunks);
> +	unsigned int i;
> +	int err;

[Severity: Low]
Here is another DECLARE_BITMAP using vpr->num_chunks that creates a VLA.

[ ... ]

> +		err = tegra_vpr_chunk_deactivate(chunk);

[ ... ]

> +	if (!bitmap_empty(dirty, vpr->num_chunks)) {
> +		err = tegra_vpr_resize(vpr);

[Severity: High]
tegra_vpr_chunk_deactivate() is called before tegra_vpr_resize(). This restores 
the CPU's linear mapping to the memory while the hardware firewall is still 
active. Does this inverted ordering create a race window where speculative CPU 
accesses to the direct map could trigger a fatal bus fault / SError before 
the hardware firewall is shrunk?

[ ... ]

> +static struct dma_buf *tegra_vpr_allocate(struct dma_heap *heap,
> +					  unsigned long len, u32 fd_flags,
> +					  u64 heap_flags)
> +{
> +	struct tegra_vpr *vpr = dma_heap_get_drvdata(heap);
> +	struct tegra_vpr_buffer *buffer, *entry;
> +	size_t size = ALIGN(len, vpr->align);

[Severity: High]
Could ALIGN(len, vpr->align) overflow to 0 if an unprivileged user requests a 
massive len? A 0-sized allocation might succeed and eventually underflow 
vpr->last when it computes vpr->last = limit - 1 with limit = 0. This would 
permanently corrupt the heap tracking bounds and break future clustered 
allocations.

[ ... ]

> +static void tegra_vpr_debugfs_show_chunks(struct tegra_vpr *vpr,
> +					  struct seq_file *s)
> +{
> +	struct tegra_vpr_buffer *buffer;
> +	unsigned int i;
> +	char buf[16];
> +
> +	for (i = 0; i < vpr->num_chunks; i++) {
> +		const struct tegra_vpr_chunk *chunk = &vpr->chunks[i];
> +
> +		string_get_size(chunk->size, 1, STRING_UNITS_2, buf,
> +				sizeof(buf));
> +		seq_printf(s, "  %pap-%pap (%s) (%s, %u buffers)\n",
> +			   &chunk->start, &chunk->limit, buf,
> +			   chunk->active ? "active" : "inactive",
> +			   chunk->num_buffers);
> +	}
> +
> +	list_for_each_entry(buffer, &vpr->buffers, list) {

[Severity: Medium]
This iterates over the vpr->buffers linked list without holding vpr->lock. 
If a user reads this debugfs file concurrently with heap allocations or frees, 
could this race with list mutations and cause use-after-free or list 
corruption panics?

[ ... ]

> +static int tegra_vpr_add_heap(struct reserved_mem *rmem,
> +			      struct device_node *np)
> +{
[ ... ]
> +	vpr->dev_node = of_node_get(np);
[ ... ]
> +	if (vpr->resizable)
> +		err = tegra_vpr_setup_chunks(vpr, rmem->name);
> +	else
> +		err = tegra_vpr_setup_static(vpr);
> +
> +	if (err < 0)
> +		goto free;
[ ... ]
> +free:
> +	bitmap_free(vpr->bitmap);
> +	kfree(vpr);
> +	return err;
> +}

[Severity: Medium]
There appear to be multiple leaks in the error paths here. If 
tegra_vpr_setup_chunks() fails, it jumps to free without calling 
of_node_put(vpr->dev_node). Additionally, if dma_heap_add() fails, the cleanup 
path calls tegra_vpr_free_chunks(), which correctly frees the chunks array but 
forgets to free the vpr->active bitmap allocated during setup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-tegra-vpr-v6-0-79042cfa8de5@nvidia.com?part=9