Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing explicit `of_reserved_mem_device_init()` API call for VPR devices. - [High] Omitted `memory-region` assignment on the `host1x` parent node. - [Low] Dead self-assignment of a zero-initialized pointer member in `tegra_vpr_attach()`. --
commit d66858609809fa2e5e478b229dab43684ed2e277 Author: Thierry Reding <[email protected]> arm64: tegra: Hook up VPR to host1x This commit adds the memory-region property to the vic and nvdec device tree nodes in tegra234.dtsi. It intends to hook up the video protection region to host1x and its clients. > The host1x needs access to the VPR region, so make sure to reference it > via the memory-region property. [Severity: High] Are we missing the memory-region assignment on the host1x parent node? The commit message explicitly states that host1x needs access to the vpr region and should reference it via the memory-region property. However, this commit omits adding the memory-region property to the host1x@13e00000 parent node itself, only applying it to the child nodes (vic and nvdec). Since the host1x dma engines fetch command streams from memory, if those command buffers reside in the vpr region, host1x must be tracked as a vpr device and frozen during vpr resizes. Omitting the property prevents host1x from being added to the vpr devices list, leaving the system vulnerable to hardware crashes if a resize is triggered. > diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi > b/arch/arm64/boot/dts/nvidia/tegra234.dtsi > index 52ff11873580d..38637e8e6fc92 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 this node assignment lack the explicit of_reserved_mem_device_init() call required in the driver? The generic of_dma_configure_id() logic only automatically invokes of_reserved_mem_device_init_by_idx() for reserved regions compatible with restricted-dma-pool. Since the vpr region uses nvidia,tegra-video-protection-region, this automatic initialization is skipped. The drivers for vic and nvdec do not explicitly call of_reserved_mem_device_init() to initialize the reserved memory. As a result, the tegra_vpr_device_init() callback is never triggered, and the devices are never added to the vpr->devices list. Since they are missing from the list, they will not be suspended by tegra_vpr_resize() during vpr resizing. Because untrusted userspace applications can allocate memory from the vpr dma-buf heap to trigger a resize, an active hardware workload on the gpu, vic, or nvdec during the resize will lead to memory corruption, hardware faults, or a system crash. > }; > > nvdec@15480000 { > @@ -4497,6 +4500,9 @@ nvdec@15480000 { > iommus = <&smmu_niso1 TEGRA234_SID_NVDEC>; > dma-coherent; > > + memory-region = <&vpr>; > + memory-region-names = "protected"; > + [Severity: High] Could this assignment also lead to a hardware crash for similar reasons? The generic of_dma_configure_id() logic skips automatic initialization for nvidia,tegra-video-protection-region. The nvdec driver does not explicitly call of_reserved_mem_device_init(), so the device is never added to the vpr->devices list and will not be suspended during vpr resizes, leading to potential hardware faults or crashes when the region is resized. > nvidia,memory-controller = <&mc>; Additionally, looking at the driver changes introduced in this series, there is a regression in tegra_vpr_attach(): drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_attach() { [ ... ] if (err < 0) goto free; attach->dev = attach->dev; INIT_LIST_HEAD(&attach->list); [ ... ] } [Severity: Low] Is this intended to capture attachment->dev instead? tegra_vpr_attach() allocates the attach structure using kzalloc_obj(), which zero-initializes the memory. It then executes the dead assignment attach->dev = attach->dev, which effectively assigns null to the struct member. While this doesn't cause an immediate crash because the driver uses attachment->dev directly in tegra_vpr_map_dma_buf() and leaves the member unread, it appears to be a logic flaw introduced by this patch series. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
