Re: [PATCH v8 4/5] ARM: dts: microchip: sam9x7: Add GFX2D GPU

[sashiko-bot@kernel.org]

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 <cyrille.pitchen@microchip.com>

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/20260828-cpitchen-mainline_gfx2d-v8-0-378374559738@microchip.com?part=4