Re: [PATCH v6 11/12] arm64: tegra: Hook up VPR to host1x

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The VPR driver assumes `set_direct_map_invalid_noflush()` successfully unmaps memory from the linear map if it returns 0. However, on arm64 with `rodata_full` disabled, the function is a silent no-op (returns 0 without modifying PTEs).
- [High] For static VPR allocations without page-backing, `dma_map_sgtable()` is unconditionally called with a NULL page pointer, which will generate a bogus DMA address and corrupt the mapping.
- [High] Memory allocated with `kvmalloc_array()` is incorrectly freed with `kfree()`, which will cause a kernel panic if the allocation fell back to vmalloc.
- [Medium] The `vpr->active` bitmap leaks on the error path if `dma_heap_add()` fails.
--

commit 41cac70cac0869723195f2732d0c1395995f6880
Author: Thierry Reding <treding@nvidia.com>

arm64: tegra: Hook up VPR to host1x
    
The host1x needs access to the VPR region, so make sure to reference it
via the memory-region property.

> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index 6c4739efc8a5d..c6a5ced069e79 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -4479,6 +4479,9 @@ vic@15340000 {
>  				interconnect-names = "dma-mem", "write";
>  				iommus = <&smmu_niso1 TEGRA234_SID_VIC>;
>  				dma-coherent;
> +
> +				memory-region = <&vpr>;
> +				memory-region-names = "protected";

[Severity: High]
Does hooking up the VPR driver here expose an issue where
set_direct_map_invalid_noflush() is assumed to invalidate memory, but silently
fails on arm64?

In drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_chunk_activate():

	err = set_direct_map_invalid_noflush(chunk->start_page,
					     chunk->num_pages);
	if (err)
		return err;

On arm64, if rodata_full is disabled, can_set_direct_map() returns false,
causing set_direct_map_invalid_noflush() to return 0 without actually
modifying PTEs. Since this returns 0, the driver assumes success and continues.

If the CPU speculatively accesses memory that is supposed to be protected by
the VPR, could the hardware memory controller block the access and fault?

[Severity: High]
Does enabling VPR also trigger an issue where dma_map_sgtable() overwrites
manually crafted SG entries containing NULL page pointers?

In drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_map_dma_buf():

	err = dma_map_sgtable(attachment->dev, sgt, direction,
			      DMA_ATTR_SKIP_CPU_SYNC);

For static VPR allocations without page-backing, the driver creates an SG table
and explicitly sets sg_set_page() to NULL in tegra_vpr_attach(), manually
populating sg_dma_address. 

However, the unconditional call to dma_map_sgtable() will iterate over the SG
table, extract the physical address via sg_page() (which resolves to PA 0), 
and map it, overwriting the driver's manually populated sg_dma_address. 

Can this corrupt the DMA mapping and potentially cause an IOMMU fault?

[Severity: High]
Is it safe to free memory allocated with kvmalloc_array() using kfree()?

In drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_buffer_allocate():

		buffer->pages = kvmalloc_array(buffer->num_pages,
					       sizeof(*buffer->pages),
					       GFP_KERNEL);
...
		err = tegra_vpr_activate_chunks(vpr, buffer);
		if (err < 0) {
			kfree(buffer->pages);
			goto free;
		}

And also in tegra_vpr_buffer_release():

	bitmap_clear(vpr->bitmap, buffer->pageno, buffer->num_pages);
	list_del(&buffer->list);
	kfree(buffer->pages);
	kfree(buffer);

If the kvmalloc_array() allocation falls back to vmalloc(), calling kfree()
on a vmalloc address will trigger a kernel bug. Since video buffers are
typically large, isn't the vmalloc() fallback highly likely?

[Severity: Medium]
Does the vpr->active bitmap leak on the error path if dma_heap_add() fails?

In drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_add_heap(), if dma_heap_add()
fails, it jumps to cleanup:

cleanup:
	if (vpr->resizable)
		tegra_vpr_free_chunks(vpr);

But tegra_vpr_free_chunks() fails to clean up the bitmap allocated in
tegra_vpr_setup_chunks():

	vpr->active = bitmap_zalloc(vpr->num_chunks, GFP_KERNEL);

Should there be a corresponding bitmap_free(vpr->active) in
tegra_vpr_free_chunks()?

>  			};
>  
>  			nvdec@15480000 {
[ ... ]

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