Re: [PATCH v6 10/12] arm64: tegra: Add VPR placeholder node on Tegra234

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The newly introduced `cma_alloc_at()` API does not validate if the requested allocation range exceeds the bounds of the CMA region and mismatches bit/page units, leading to an out-of-bounds read and write in the CMA bitmap.
- [High] The `tegra_vpr_set()` function passes 64-bit physical addresses and sizes to a 32-bit SMC call, which truncates them, misconfiguring hardware protection.
- [High] Missing mutex lock in `tegra_vpr_debugfs_show_chunks()` when iterating over the `vpr->buffers` list.
- [High] A negative left shift occurs in `tegra_vpr_setup_chunks()` if the configured VPR size is small, leading to undefined behavior.
--

commit 4d64d0d8b755f60422c65bdb111c671e4013a472
Author: Thierry Reding <treding@nvidia.com>

arm64: tegra: Add VPR placeholder node on Tegra234

This node contains two sets of properties, one for the case where the
VPR is resizable (in which case the VPR region will be dynamically
allocated at boot time) and another case where the VPR is fixed in size
and initialized by early firmware.

> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index 5e29316a4d759..6c4739efc8a5d 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -29,6 +29,45 @@ aliases {
>  		i2c8 = &dp_aux_ch3_i2c;
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		vpr: protected {
> +			compatible = "nvidia,tegra-video-protection-region";

[Severity: High]
Is it possible for the shift operand to become negative when the VPR size is
small?

If the VPR size configured in the device tree is small (e.g., 4KB or 8KB),
get_order(vpr->size) is 0 or 1. Since vpr->num_chunks is hardcoded to 4,
ilog2(4) is 2. The shift operand can evaluate without bounds checking:

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_setup_chunks() {
	...
	max_size = PAGE_SIZE << (get_order(vpr->size) - ilog2(vpr->num_chunks));
	...
}

Can this evaluate to a negative number (e.g., 0 - 2 or 1 - 2) and cause a
negative shift during driver initialization based on device tree inputs?
On arm64, a negative shift count often wraps around, resulting in a very
large max_size.

[Severity: High]
Could passing these 64-bit addresses to the SMC32 call cause truncation?

The 64-bit parameters base and size are passed directly into an SMC32 call:

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_set() {
	...
	arm_smccc_smc(ARM_SMCCC_VENDOR_SIP_TE_PROGRAM_VPR_FUNC_ID, base, size,
		      0, 0, 0, 0, 0, &res);
	...
}

The ARM_SMCCC_VENDOR_SIP_TE_PROGRAM_VPR_FUNC_ID uses ARM_SMCCC_SMC_32,
meaning the SMC instruction adheres to the 32-bit calling convention, natively
truncating the upper 32 bits of base and size inside the secure monitor. Since
CMA regions on arm64 frequently reside above the 4GB boundary, can this
misconfigure hardware protection by locking down the wrong physical memory?

[Severity: High]
Is it safe to iterate the vpr->buffers list here without acquiring a lock?

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_debugfs_show_chunks() {
	...
	list_for_each_entry(buffer, &vpr->buffers, list) {
	...
}

This iteration accesses a shared, mutable linked list without acquiring
vpr->lock. If another process performs a DMA-BUF allocation or release
concurrently, it modifies the vpr->buffers list under vpr->lock. Can this
result in list corruption or a use-after-free if modified or freed nodes are
encountered mid-traversal?

[Severity: Critical]
Are the offset and count units mismatched when calling cma_range_alloc()?

The exported cma_alloc_at API passes page-based offset and count to
cma_range_alloc():

mm/cma.c:cma_alloc_at() {
	...
	page = cma_alloc_at_frozen(cma, offset, count, no_warn);
	...
}

In cma_range_alloc(), this sets the max limit for the bitmap scan to end
without bounds checking:

mm/cma.c:cma_range_alloc() {
	...
	bitmap_maxno = (end > start) ? end : cma_bitmap_maxno(cma, cmr);
	...
}

cma_alloc_at_frozen() passes offset and offset + count (which are in pages)
to cma_range_alloc(), but cma_range_alloc() incorrectly assumes end is always
passed in bits instead of pages. Any driver calling cma_alloc_at() with an
offset + count exceeding the CMA region size could bypass length validation.

Will this scan past the end of the allocated CMA bitmap and allow bitmap_set()
to write out-of-bounds?

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