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 <[email protected]>
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.

> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index bb729e91545c4..8909330bfaa2a 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -20,3 +20,15 @@ config DMABUF_HEAPS_CMA
> +config DMABUF_HEAPS_TEGRA_VPR
> +	bool "NVIDIA Tegra Video-Protected-Region DMA-BUF Heap"
> +	depends on DMABUF_HEAPS && DMA_CMA

[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.

> diff --git a/drivers/dma-buf/heaps/tegra-vpr.c b/drivers/dma-buf/heaps/tegra-vpr.c
> new file mode 100644
> index 0000000000000..c8d9f9c66dded
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/tegra-vpr.c
[ ... ]
> @@ -112,6 +112,6 @@ struct tegra_vpr_attachment {
>  
> +#define ARM_SMCCC_VENDOR_SIP_TE_PROGRAM_VPR_FUNC_ID		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
> +			   ARM_SMCCC_SMC_32,			\
> +			   ARM_SMCCC_OWNER_SIP,			\
> +			   ARM_SMCCC_TE_FUNC_PROGRAM_VPR)

[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.

[ ... ]
> @@ -166,5 +166,5 @@ 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]
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?

[ ... ]
> @@ -365,3 +365,3 @@ static int tegra_vpr_activate_chunks(struct tegra_vpr *vpr,
> +{
> +	DECLARE_BITMAP(dirty, vpr->num_chunks);

[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.

[ ... ]
> @@ -498,4 +498,4 @@ static int tegra_vpr_find_free_region(struct tegra_vpr *vpr,
> +{
> +	return tegra_vpr_find_free_region_in_range(vpr, 0, vpr->num_pages - 1,
> +						   num_pages, align);

[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.

[ ... ]
> @@ -642,3 +642,3 @@ tegra_vpr_buffer_allocate(struct tegra_vpr *vpr, size_t size)
> +	INIT_LIST_HEAD(&buffer->list);
> +	mutex_init(&buffer->lock);
> +	buffer->start = vpr->base + (pageno << PAGE_SHIFT);

[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.

[ ... ]
> @@ -654,4 +654,4 @@ 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) {
[ ... ]
> @@ -672,3 +672,3 @@ free:
> +	kfree(buffer->pages);
> +release:

[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.

[ ... ]
> @@ -728,4 +728,4 @@ 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]
Similarly, should this teardown path also use kvfree() to release
buffer->pages since it was allocated using kvmalloc_array()?

[ ... ]
> @@ -785,4 +785,4 @@ tegra_vpr_map_dma_buf(struct dma_buf_attachment *attachment,
> +
> +	err = dma_map_sgtable(attachment->dev, sgt, direction,
> +			      DMA_ATTR_SKIP_CPU_SYNC);

[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.

[ ... ]
> @@ -796,4 +796,4 @@ static void tegra_vpr_unmap_dma_buf(struct dma_buf_attachment *attachment,
> +{
> +	dma_unmap_sgtable(attachment->dev, sgt, direction,
> +			  DMA_ATTR_SKIP_CPU_SYNC);

[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.

[ ... ]
> @@ -800,3 +800,3 @@ static void tegra_vpr_recycle(struct tegra_vpr *vpr)
> +{
> +	DECLARE_BITMAP(dirty, vpr->num_chunks);

[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.

[ ... ]
> @@ -1048,4 +1048,4 @@ static void tegra_vpr_debugfs_show_chunks(struct tegra_vpr *vpr,
> +
> +	list_for_each_entry(buffer, &vpr->buffers, list) {
> +		string_get_size(buffer->size, 1, STRING_UNITS_2, buf,

[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.

[ ... ]
> @@ -1199,6 +1199,6 @@ static int tegra_vpr_add_heap(struct reserved_mem *rmem,
> +
> +cleanup:
> +	if (vpr->resizable)
> +		tegra_vpr_free_chunks(vpr);
> +free:
> +	bitmap_free(vpr->bitmap);
> +	kfree(vpr);
> +	return err;

[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).

[ ... ]
> @@ -1305,4 +1305,4 @@ static int __init tegra_vpr_node_init(unsigned long offset,
> +
> +	err = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name,
> +				    &cma);

[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.

[ ... ]
> @@ -1359,3 +1359,3 @@ static int tegra_vpr_device_init(struct reserved_mem *rmem, struct device *dev)
> +{
> +	const struct dev_pm_ops *pm = dev->driver->pm;

[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/[email protected]?part=7