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

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Calling `dma_map_sgtable()` on a scatterlist with a `NULL` page pointer will overwrite the manual DMA address and cause a kernel panic for static VPRs.
- [High] Memory allocated with `kvmalloc_array()` is incorrectly freed with `kfree()`, which will cause a SLUB BUG or kernel panic if it falls back to `vmalloc()`.
- [High] Signed integer overflow in physical address calculation for VPR sizes exceeding ~2 GiB.
- [High] Unconditional CMA initialization on `no-map` reserved memory will cause a kernel panic.
- [High] SMCCC calling convention truncates 64-bit physical addresses to 32 bits.
- [High] Race condition and framework abuse by calling `pm_generic_freeze()` at runtime without acquiring the device lock.
- [High] Dereferencing `dev->driver` when calling `tegra_vpr_device_init()` will cause a boot-time kernel panic.
- [Medium] Missing locking in `tegra_vpr_debugfs_show_chunks` leads to use-after-free and list corruption.
- [Medium] Memory and OF node leaks on the error path of `tegra_vpr_add_heap()`.
- [Medium] Off-by-one error prevents allocation of the last VPR page.
- [Low] Missing Kconfig dependencies for `ARM_SMCCC` will cause compilation and link errors on unsupported architectures.
- [Low] The use of `DECLARE_BITMAP(dirty, vpr->num_chunks)` creates a Variable Length Array (VLA), which is forbidden in the kernel.
--

--- Patch [7]: [PATCH v5 07/10] dma-buf: heaps: Add support for Tegra VPR ---

commit 4503ba247ad23e5d232fb07fec29334ba71260ab
Author: Thierry Reding [off-list ref]
Subject: dma-buf: heaps: Add support for Tegra VPR

This commit introduces a new DMA-BUF heap for the NVIDIA Tegra
Video-Protection-Region (VPR). It exposes VPR memory, which is restricted
from CPU access, to drivers and applications that handle content-protected
video. The heap supports both static and resizable VPR configurations.

[Severity: Low]
Is it possible for this driver to be compiled on unsupported architectures?

The driver uses arm_smccc_smc(), which relies on __arm_smccc_smc. This is
only implemented on ARM/ARM64. 

Without an ARM_SMCCC or ARCH_TEGRA dependency, this could lead to unresolved
symbol linker failures on other architectures like x86.

[ ... ]

[Severity: High]
Does the use of ARM_SMCCC_SMC_32 truncate the physical addresses passed to
the secure monitor? 

When passing 64-bit phys_addr_t arguments for base and size in
tegra_vpr_set(), the SMC32 convention truncates them to 32 bits.

This might cause the secure monitor to program the VPR to an incorrect
memory location.

[ ... ]

[Severity: High]
Is it safe to call pm_generic_freeze() here without acquiring the device
lock? 

Calling it directly without holding device_lock() can race with concurrent
runtime PM transitions, device removal, or unbind operations. 

Could this lead to crashes if dev->driver is cleared concurrently?

[ ... ]

[Severity: Low]
Does this declaration create a Variable Length Array (VLA)? 

Because vpr->num_chunks is a struct member evaluated at runtime, this
creates a VLA on the stack. Since the kernel is compiled with -Werror=vla,
this will result in a build failure.

[ ... ]

[Severity: Medium]
Will passing vpr->num_pages - 1 prevent the allocation of the last VPR page?

Looking at tegra_vpr_find_free_region_in_range(), the loop condition is
pos + num_pages <= end_page. 

If end_page is decremented by 1, it seems it will incorrectly exclude the
final page from ever being allocated.

[ ... ]

[Severity: High]
Can this calculation overflow for VPR sizes exceeding 2 GiB? 

Because pageno is a signed 32-bit int, pageno << PAGE_SHIFT can overflow
and become a negative value before being promoted and added to the 64-bit
phys_addr_t vpr->base. 

This could yield a completely incorrect DMA address.

[ ... ]

[ ... ]

[Severity: High]
Should this be kvfree() instead of kfree()? 

Memory allocated with kvmalloc_array() might fall back to vmalloc(). 
If that happens, calling kfree() on a vmalloc address will cause a SLUB BUG
or kernel panic during virt_to_page() translation.

[ ... ]

[Severity: High]
Similarly, should this teardown path also use kvfree() to release
buffer->pages since it was allocated using kvmalloc_array()?

[ ... ]

[Severity: High]
Can this crash for static VPR configurations? 

In the static VPR case, the scatterlist is populated with a NULL page
pointer. 

Unconditionally calling dma_map_sgtable() here will cause the DMA framework
to iterate over the scatterlist and dereference the NULL page pointer. This
could corrupt the DMA address or cause a kernel panic.

[ ... ]

[Severity: High]
This appears to have the same issue as the mapping path. Calling
dma_unmap_sgtable() unconditionally attempts to unmap a NULL-paged
scatterlist, which could lead to a panic.

[ ... ]

[Severity: Low]
Does this declaration also create a Variable Length Array (VLA) on the
stack? This will similarly cause a build failure under -Werror=vla.

[ ... ]

[Severity: Medium]
Should this list iteration be protected by vpr->lock? 

Iterating over vpr->buffers without the lock can race with concurrent
dma-buf allocations or frees, potentially leading to a use-after-free or a
panic from list corruption.

[ ... ]

[Severity: Medium]
Are there a couple of resource leaks on this error path? 

If dma_heap_add() fails, tegra_vpr_free_chunks() is called, but it looks
like the vpr->active bitmap is never freed. 

Additionally, it seems the device node reference acquired via of_node_get()
is not released with of_node_put(vpr->dev_node).

[ ... ]

[Severity: High]
Will this cause a panic for static VPRs? 

Static VPRs use the "no-map" property, which removes the memory from the
linear map and prevents struct page creation. 

However, this calls cma_init_reserved_mem() unconditionally, and CMA
expects struct page to be present when initializing pageblocks.

[ ... ]

[Severity: High]
Can this unconditionally dereference a NULL pointer? 

This function acts as the device_init callback for reserved memory
operations. When devices are instantiated and memory regions are attached
via of_dma_configure_id(), this callback is invoked before a driver is
matched or bound to the device.

This means dev->driver would be NULL here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-tegra-vpr-v5-0-71832b5d0246@nvidia.com?part=7