Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Storing `struct vm_area_struct` by value leads to a Use-After-Free during cache flushing. - [High] Shared `ctx->rectangles` buffer causes data corruption for concurrent command submissions. -- commit 78e2389302cd2c7d31509fbbf3d0fedaa8b301e2 Author: Cyrille Pitchen <[email protected]> ARM: dts: microchip: sam9x7: Add GFX2D GPU This patch enables the GFX2D GPU for the sam9x7 SoC by adding its device tree node. > diff --git a/arch/arm/boot/dts/microchip/sam9x7.dtsi > b/arch/arm/boot/dts/microchip/sam9x7.dtsi > index c680a5033b6b4..5450fddf89dda 100644 > --- a/arch/arm/boot/dts/microchip/sam9x7.dtsi > +++ b/arch/arm/boot/dts/microchip/sam9x7.dtsi > @@ -292,6 +292,14 @@ AT91_XDMAC_DT_PERID(26))>, > status = "disabled"; > }; > > + gpu: gpu@f0018000 { > + compatible = "microchip,sam9x7-gfx2d"; > + reg = <0xf0018000 0x100>; > + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; > + clocks = <&pmc PMC_TYPE_PERIPHERAL 36>; > + status = "disabled"; > + }; > + [Severity: High] This is a pre-existing issue, but I noticed a potential use-after-free when this device is used. When a GEM buffer is mapped, mchp_gfx2d_gem_object_mmap() makes a shallow copy of the kernel-managed vm_area_struct: drivers/gpu/drm/microchip/mchp_gfx2d_gem.c:mchp_gfx2d_gem_object_mmap() { ... if (ret) drm_gem_vm_close(vma); else memcpy(&gfx2d_obj->vma, vma, sizeof(gfx2d_obj->vma)); return ret; } If the mapping is torn down, the original VMA is destroyed. A subsequent call to mchp_gfx2d_ioctl_sync_for_gpu() passes this stale VMA copy to flush_cache_range(): drivers/gpu/drm/microchip/mchp_gfx2d_ioctl.c:mchp_gfx2d_ioctl_sync_for_gpu() { ... dir = gfx2d_obj->direction; if (valid_dma_direction(dir)) { struct vm_area_struct *vma = &gfx2d_obj->vma; flush_cache_range(vma, vma->vm_start, vma->vm_end); } ... } Additionally, since the stored vm_start/vm_end are userspace virtual addresses from the original process, calling this ioctl from another process sharing the GEM object will attempt to flush memory in the wrong address space. Does this sequence cause a use-after-free or cross-process memory corruption when an unprivileged user calls the DRM sync ioctl? [Severity: High] This isn't a bug introduced by this patch, but there appears to be a data corruption issue with concurrent command submissions. In mchp_gfx2d_alloc_rectangles(), if the requested number of rectangles is small enough, the driver assigns the asynchronous command state to use a single shared context buffer: drivers/gpu/drm/microchip/mchp_gfx2d_cmd.c:mchp_gfx2d_alloc_rectangles() { ... cmd->num_rects = num_rectangles; if (num_rectangles <= ARRAY_SIZE(cmd->ctx->rectangles)) { cmd->can_free_rects = false; cmd->rects = cmd->ctx->rectangles; return 0; } ... } This ctx array is stored in the file_priv->driver_priv context, which is shared among all threads using the same DRM file descriptor. Later in mchp_gfx2d_submit(), copy_from_user() writes directly into this shared array without holding any locks: drivers/gpu/drm/microchip/mchp_gfx2d_cmd.c:mchp_gfx2d_submit() { ... ret = mchp_gfx2d_alloc_rectangles(cmd, args->num_rectangles); if (ret) goto err_release_command; ret = copy_from_user(cmd->rects, u64_to_user_ptr(args->rectangles), args->num_rectangles * sizeof(*cmd->rects)); ... } If multiple threads concurrently submit commands on the same file descriptor, will they race and overwrite each other's rectangles, leading to corrupted instructions being sent to the GPU? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
