Re: [PATCH v4 09/10] arm64: tegra: Hook up VPR to host1x

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference in tegra_vpr_device_init on deferral and driverless paths.
- [High] VPR heap initialization fails for sizes in the lower half of an order due to 0-sized chunks.
- [Medium] Off-by-one error prevents allocating the final page of the VPR or a single max-sized buffer.
--

--- Patch [9]: [PATCH v4 09/10] arm64: tegra: Hook up VPR to host1x ---
commit 04120e646b04a85555bacc81f097911f705ea117
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";
>  			};
>  
>  			nvdec@15480000 {
> @@ -4497,6 +4500,9 @@ nvdec@15480000 {
>  				iommus = <&smmu_niso1 TEGRA234_SID_NVDEC>;
>  				dma-coherent;
>  
> +				memory-region = <&vpr>;
> +				memory-region-names = "protected";
> +
>  				nvidia,memory-controller = <&mc>;
>  
>  				/*

With the VPR region now hooked up, we noticed a few issues in the tegra-vpr
heap driver that might be triggered by this change.

[Severity: High]
Does this code risk a NULL pointer dereference on the deferral or driverless
paths?

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_device_init() {
	struct dma_heap *heap = rmem->priv;
	struct tegra_vpr *vpr = dma_heap_get_drvdata(heap);
	const struct dev_pm_ops *pm = dev->driver->pm;

	if (!rmem->priv)
		return -EPROBE_DEFER;
}

The heap pointer (rmem->priv) and dev->driver appear to be unconditionally
dereferenced before the check for !rmem->priv happens. Could this cause a
kernel panic if the VPR heap driver is not yet initialized or if dev->driver
is NULL?


[Severity: High]
Can the VPR heap initialization fail for sizes in the lower half of an order
due to 0-sized chunks?

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_setup_chunks() {
		size = min_t(size_t, size, max_size);
		end = start + size - 1;

		err = tegra_vpr_chunk_init(vpr, &vpr->chunks[i], start, size,
					   order, name);
}

When max_size is artificially large due to power-of-2 rounding, size could
reach 0 in the final loop iterations. Would this lead to an invalid CMA
region creation when cma_create() is called with a size of 0?


[Severity: Medium]
Does this off-by-one error prevent allocating the final page of the VPR or a
single max-sized buffer?

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_find_free_region() {
	return tegra_vpr_find_free_region_in_range(vpr, 0, vpr->num_pages - 1,
						   num_pages, align);
}

The end_page parameter is given the inclusive upper bound (vpr->num_pages - 1).

drivers/dma-buf/heaps/tegra-vpr.c:tegra_vpr_find_free_region_in_range() {
	for (pos = next; pos + num_pages <= end_page; pos = next) {
}

However, the loop condition checks pos + num_pages <= end_page, meaning
end_page acts as an exclusive bound. Does this cause allocations that span
the final page to fail with -ENOSPC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9