Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Passing `iobj->bar->addr` to `io_mapping_map_wc` without truncating the VMM base address causes an out-of-bounds pointer calculation on 64-bit systems. - [High] Unconditional io_mapping_init_wc() of the entire NVKM_BAR2_INST breaks driver probe on constrained architectures and completely bypasses the existing graceful BAR0 fallback when BAR2 is missing or unmappable. - [High] The patch introduces a type confusion vulnerability and potential resource leak in `nv50_instmem_dtor` due to structural assumptions. --
commit f429f6c462c04142327023f718eaa2eb0469d838 Author: Dave Airlie <[email protected]> nouveau/instmem: use iomapping interface for instmem handling This patch transitions the nouveau driver's instmem handling to use the io_mapping API. Instead of individually mapping and unmapping instobjs with ioremap_wc(), it maps the entire BAR2 region upfront during initialization and maps offsets as needed. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c > index 6fbaa1e5876dc..929246bde72d5 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c [ ... ] > @@ -172,8 +175,7 @@ nv50_instobj_kmap(struct nv50_instobj *iobj, struct > nvkm_vmm *vmm) > > /* Make the mapping visible to the host. */ > iobj->bar = bar; > - iobj->map = ioremap_wc(device->func->resource_addr(device, > NVKM_BAR2_INST) + > - (u32)iobj->bar->addr, size); > + iobj->map = io_mapping_map_wc(&imem->iomap, iobj->bar->addr, size); [Severity: Critical] Is it possible this leads to an out-of-bounds pointer calculation on 64-bit systems? The bar2 VMM is initialized with a starting address of 0x0100000000ULL (4GB), meaning iobj->bar->addr returns an address >= 4GB. Before this patch, the u32 cast implicitly truncated the 4GB base to extract just the internal offset. Since io_mapping_map_wc() takes an unsigned long offset, passing the full 64-bit u64 value will preserve the 4GB base, resulting in a pointer that is out of bounds by at least 4GB. > if (!iobj->map) { > nvkm_warn(subdev, "PRAMIN ioremap failed\n"); > nvkm_vmm_put(vmm, &iobj->bar); [ ... ] > @@ -409,7 +411,10 @@ nv50_instmem_fini(struct nvkm_instmem *base) > static void * > nv50_instmem_dtor(struct nvkm_instmem *base) > { > - return nv50_instmem(base); > + struct nv50_instmem *imem = nv50_instmem(base); > + > + io_mapping_fini(&imem->iomap); > + return imem; [Severity: High] What happens when this destructor is called for derived classes? In nv50_instmem_new(), if nvkm_gsp_rm() is enabled, the creation is delegated to r535_instmem_new(&nv50_instmem, ...). This reuses the nv50_instmem function table (which contains this destructor) but allocates a different structure. If the base pointer does not belong to a struct nv50_instmem, won't calling io_mapping_fini(&imem->iomap) result in type confusion and cause iounmap() to operate on garbage memory? > } [ ... ] > @@ -433,8 +438,16 @@ nv50_instmem_new_(const struct nvkm_instmem_func *func, > > if (!(imem = kzalloc_obj(*imem))) > return -ENOMEM; > + > + if (!io_mapping_init_wc(&imem->iomap, > device->func->resource_addr(device, NVKM_BAR2_INST), > + device->func->resource_size(device, > NVKM_BAR2_INST))) { > + kfree(imem); > + return -ENOMEM; > + } > + [Severity: High] Will this unconditional initialization break driver probe on systems where the full BAR2 region cannot be mapped? Previously, small ioremap_wc() allocations were made on demand, and if they failed, the driver gracefully fell back to accessing memory safely via the BAR0 window. By attempting to map the entire NVKM_BAR2_INST resource upfront and aborting on failure, it seems the fallback mechanism is bypassed entirely, which could cause initialization failures on 32-bit architectures or if BAR2 is exceptionally large. > nvkm_instmem_ctor(func, device, type, inst, &imem->base); > INIT_LIST_HEAD(&imem->lru); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
