Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [High] Calling `dma_map_sgtable()` on a scatterlist with a `NULL` page pointer will overwrite the manual DMA address and cause a kernel panic for static VPRs. - [High] Memory allocated with `kvmalloc_array()` is incorrectly freed with `kfree()`, which will cause a SLUB BUG or kernel panic if it falls back to `vmalloc()`. - [High] Signed integer overflow in physical address calculation for VPR sizes exceeding ~2 GiB. - [High] Unconditional CMA initialization on `no-map` reserved memory will cause a kernel panic. - [High] SMCCC calling convention truncates 64-bit physical addresses to 32 bits. - [High] Race condition and framework abuse by calling `pm_generic_freeze()` at runtime without acquiring the device lock. - [High] Dereferencing `dev->driver` when calling `tegra_vpr_device_init()` will cause a boot-time kernel panic. - [Medium] Missing locking in `tegra_vpr_debugfs_show_chunks` leads to use-after-free and list corruption. - [Medium] Memory and OF node leaks on the error path of `tegra_vpr_add_heap()`. - [Medium] Off-by-one error prevents allocation of the last VPR page. - [Low] Missing Kconfig dependencies for `ARM_SMCCC` will cause compilation and link errors on unsupported architectures. - [Low] The use of `DECLARE_BITMAP(dirty, vpr->num_chunks)` creates a Variable Length Array (VLA), which is forbidden in the kernel. --
--- Patch [7]: [PATCH v5 07/10] dma-buf: heaps: Add support for Tegra VPR --- commit 4503ba247ad23e5d232fb07fec29334ba71260ab Author: Thierry Reding <[email protected]> Subject: dma-buf: heaps: Add support for Tegra VPR This commit introduces a new DMA-BUF heap for the NVIDIA Tegra Video-Protection-Region (VPR). It exposes VPR memory, which is restricted from CPU access, to drivers and applications that handle content-protected video. The heap supports both static and resizable VPR configurations. > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index bb729e91545c4..8909330bfaa2a 100644 > --- a/drivers/dma-buf/heaps/Kconfig > +++ b/drivers/dma-buf/heaps/Kconfig > @@ -20,3 +20,15 @@ config DMABUF_HEAPS_CMA > +config DMABUF_HEAPS_TEGRA_VPR > + bool "NVIDIA Tegra Video-Protected-Region DMA-BUF Heap" > + depends on DMABUF_HEAPS && DMA_CMA [Severity: Low] Is it possible for this driver to be compiled on unsupported architectures? The driver uses arm_smccc_smc(), which relies on __arm_smccc_smc. This is only implemented on ARM/ARM64. Without an ARM_SMCCC or ARCH_TEGRA dependency, this could lead to unresolved symbol linker failures on other architectures like x86. > diff --git a/drivers/dma-buf/heaps/tegra-vpr.c > b/drivers/dma-buf/heaps/tegra-vpr.c > new file mode 100644 > index 0000000000000..c8d9f9c66dded > --- /dev/null > +++ b/drivers/dma-buf/heaps/tegra-vpr.c [ ... ] > @@ -112,6 +112,6 @@ struct tegra_vpr_attachment { > > +#define ARM_SMCCC_VENDOR_SIP_TE_PROGRAM_VPR_FUNC_ID \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_SMC_32, \ > + ARM_SMCCC_OWNER_SIP, \ > + ARM_SMCCC_TE_FUNC_PROGRAM_VPR) [Severity: High] Does the use of ARM_SMCCC_SMC_32 truncate the physical addresses passed to the secure monitor? When passing 64-bit phys_addr_t arguments for base and size in tegra_vpr_set(), the SMC32 convention truncates them to 32 bits. This might cause the secure monitor to program the VPR to an incorrect memory location. [ ... ] > @@ -166,5 +166,5 @@ static int tegra_vpr_resize(struct tegra_vpr *vpr) > > + list_for_each_entry(node, &vpr->devices, node) { > + err = pm_generic_freeze(node->dev); [Severity: High] Is it safe to call pm_generic_freeze() here without acquiring the device lock? Calling it directly without holding device_lock() can race with concurrent runtime PM transitions, device removal, or unbind operations. Could this lead to crashes if dev->driver is cleared concurrently? [ ... ] > @@ -365,3 +365,3 @@ static int tegra_vpr_activate_chunks(struct tegra_vpr > *vpr, > +{ > + DECLARE_BITMAP(dirty, vpr->num_chunks); [Severity: Low] Does this declaration create a Variable Length Array (VLA)? Because vpr->num_chunks is a struct member evaluated at runtime, this creates a VLA on the stack. Since the kernel is compiled with -Werror=vla, this will result in a build failure. [ ... ] > @@ -498,4 +498,4 @@ static int tegra_vpr_find_free_region(struct tegra_vpr > *vpr, > +{ > + return tegra_vpr_find_free_region_in_range(vpr, 0, vpr->num_pages - 1, > + num_pages, align); [Severity: Medium] Will passing vpr->num_pages - 1 prevent the allocation of the last VPR page? Looking at tegra_vpr_find_free_region_in_range(), the loop condition is pos + num_pages <= end_page. If end_page is decremented by 1, it seems it will incorrectly exclude the final page from ever being allocated. [ ... ] > @@ -642,3 +642,3 @@ tegra_vpr_buffer_allocate(struct tegra_vpr *vpr, size_t > size) > + INIT_LIST_HEAD(&buffer->list); > + mutex_init(&buffer->lock); > + buffer->start = vpr->base + (pageno << PAGE_SHIFT); [Severity: High] Can this calculation overflow for VPR sizes exceeding 2 GiB? Because pageno is a signed 32-bit int, pageno << PAGE_SHIFT can overflow and become a negative value before being promoted and added to the 64-bit phys_addr_t vpr->base. This could yield a completely incorrect DMA address. [ ... ] > @@ -654,4 +654,4 @@ tegra_vpr_buffer_allocate(struct tegra_vpr *vpr, size_t > size) > + buffer->pages = kvmalloc_array(buffer->num_pages, > + sizeof(*buffer->pages), > + GFP_KERNEL); > + if (!buffer->pages) { [ ... ] > @@ -672,3 +672,3 @@ free: > + kfree(buffer->pages); > +release: [Severity: High] Should this be kvfree() instead of kfree()? Memory allocated with kvmalloc_array() might fall back to vmalloc(). If that happens, calling kfree() on a vmalloc address will cause a SLUB BUG or kernel panic during virt_to_page() translation. [ ... ] > @@ -728,4 +728,4 @@ static void tegra_vpr_buffer_release(struct > tegra_vpr_buffer *buffer) > + bitmap_clear(vpr->bitmap, buffer->pageno, buffer->num_pages); > + list_del(&buffer->list); > + kfree(buffer->pages); [Severity: High] Similarly, should this teardown path also use kvfree() to release buffer->pages since it was allocated using kvmalloc_array()? [ ... ] > @@ -785,4 +785,4 @@ tegra_vpr_map_dma_buf(struct dma_buf_attachment > *attachment, > + > + err = dma_map_sgtable(attachment->dev, sgt, direction, > + DMA_ATTR_SKIP_CPU_SYNC); [Severity: High] Can this crash for static VPR configurations? In the static VPR case, the scatterlist is populated with a NULL page pointer. Unconditionally calling dma_map_sgtable() here will cause the DMA framework to iterate over the scatterlist and dereference the NULL page pointer. This could corrupt the DMA address or cause a kernel panic. [ ... ] > @@ -796,4 +796,4 @@ static void tegra_vpr_unmap_dma_buf(struct > dma_buf_attachment *attachment, > +{ > + dma_unmap_sgtable(attachment->dev, sgt, direction, > + DMA_ATTR_SKIP_CPU_SYNC); [Severity: High] This appears to have the same issue as the mapping path. Calling dma_unmap_sgtable() unconditionally attempts to unmap a NULL-paged scatterlist, which could lead to a panic. [ ... ] > @@ -800,3 +800,3 @@ static void tegra_vpr_recycle(struct tegra_vpr *vpr) > +{ > + DECLARE_BITMAP(dirty, vpr->num_chunks); [Severity: Low] Does this declaration also create a Variable Length Array (VLA) on the stack? This will similarly cause a build failure under -Werror=vla. [ ... ] > @@ -1048,4 +1048,4 @@ static void tegra_vpr_debugfs_show_chunks(struct > tegra_vpr *vpr, > + > + list_for_each_entry(buffer, &vpr->buffers, list) { > + string_get_size(buffer->size, 1, STRING_UNITS_2, buf, [Severity: Medium] Should this list iteration be protected by vpr->lock? Iterating over vpr->buffers without the lock can race with concurrent dma-buf allocations or frees, potentially leading to a use-after-free or a panic from list corruption. [ ... ] > @@ -1199,6 +1199,6 @@ static int tegra_vpr_add_heap(struct reserved_mem *rmem, > + > +cleanup: > + if (vpr->resizable) > + tegra_vpr_free_chunks(vpr); > +free: > + bitmap_free(vpr->bitmap); > + kfree(vpr); > + return err; [Severity: Medium] Are there a couple of resource leaks on this error path? If dma_heap_add() fails, tegra_vpr_free_chunks() is called, but it looks like the vpr->active bitmap is never freed. Additionally, it seems the device node reference acquired via of_node_get() is not released with of_node_put(vpr->dev_node). [ ... ] > @@ -1305,4 +1305,4 @@ static int __init tegra_vpr_node_init(unsigned long > offset, > + > + err = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name, > + &cma); [Severity: High] Will this cause a panic for static VPRs? Static VPRs use the "no-map" property, which removes the memory from the linear map and prevents struct page creation. However, this calls cma_init_reserved_mem() unconditionally, and CMA expects struct page to be present when initializing pageblocks. [ ... ] > @@ -1359,3 +1359,3 @@ static int tegra_vpr_device_init(struct reserved_mem > *rmem, struct device *dev) > +{ > + const struct dev_pm_ops *pm = dev->driver->pm; [Severity: High] Can this unconditionally dereference a NULL pointer? This function acts as the device_init callback for reserved memory operations. When devices are instantiated and memory regions are attached via of_dma_configure_id(), this callback is invoked before a driver is matched or bound to the device. This means dev->driver would be NULL here. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
