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