Re: [PATCH v8 2/5] drm/microchip: add a driver for the Microchip GFX2D GPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 17 potential issue(s) to consider:
- [Critical] Concurrent WAIT ioctls on the same GEM object corrupt `priv->wlist` leading to a double `list_del()` kernel panic.
- [Critical] Lack of synchronization around static `next_id` causes duplicate command IDs and premature Use-After-Free.
- [High] Concurrent SUBMIT ioctls on the same file descriptor corrupt the shared per-file `ctx->rectangles` buffer.
- [High] Shallow copying `struct vm_area_struct` by value leads to Use-After-Free and API misuse during cache flushing.
- [High] Hardware exception (bus fault) due to unsafe MMIO register access while the peripheral clock is disabled.
- [High] `IMPORT_BUFFER` unconditionally overwrites shared GEM buffer metadata.
- [High] `SYNC_FOR_CPU` ioctl fails to invalidate CPU caches for noncoherent buffers.
- [High] Invalid use of `virt_to_page()` on DMA addresses in `mchp_gfx2d_gem_object_mmap`.
- [High] Integer truncation allows userspace to bypass ROP4 mask validation and trigger a GPU fault.
- [High] Double-subtraction of `drm_vma_node_start` causes mmap to permanently fail with `-ENXIO`.
- [High] Integer overflow in `round_up(size, PAGE_SIZE)` allows 0-sized allocation bypassing limits.
- [High] Use-After-Free in `mchp_gfx2d_ioctl_alloc_buffer` due to exposing the GEM handle before object initialization is complete.
- [High] The driver fails to reinitialize hardware registers after waking up from a system suspend, leading to a GPU hang.
- [Medium] Use of `__kernel_size_t` and implicit padding in UAPI structures breaks 32-bit compatibility.
- [Medium] Silent overwrite of `BLEND_SREG` when hardware lacks `DREG` for BLEND instructions.
- [Medium] Missing upper bound check on `num_rectangles` allows arbitrary large memory allocations.
- [Medium] The interrupt handler silently acknowledges and ignores all GPU hardware error interrupts.
--

commit 9a29a32449f034a19c84420d0dbfd7fc866315ff
Author: Cyrille Pitchen <cyrille.pitchen@microchip.com>

drm/microchip: add a driver for the Microchip GFX2D GPU

This commit adds a DRM driver for the Microchip GFX2D 2D GPU, supporting
basic graphics instructions like FILL, COPY, BLEND, and ROP. The driver
handles queueing instructions into a ring buffer and tracks completion
via a LDR/STR command ID trick to a mailbox in system memory.

> diff --git a/drivers/gpu/drm/microchip/mchp_gfx2d_cmd.c b/drivers/gpu/drm/microchip/mchp_gfx2d_cmd.c
> index 0000000000000..5b2cc704a336f
> --- /dev/null
> +++ b/drivers/gpu/drm/microchip/mchp_gfx2d_cmd.c
> @@ -96,15 +96,12 @@ static int mchp_gfx2d_alloc_rectangles(struct mchp_gfx2d_command *cmd,
>  				       size_t num_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;
>  	}

[Severity: High]
Can this corrupt graphics instructions if multiple threads use the same file
descriptor? 

Unprivileged userspace can spawn threads that concurrently invoke the submit
ioctl on the same fd. Blindly assigning the shared per-file ctx rectangles
buffer without synchronization could allow concurrent copy operations to
overwrite each other's data, leading to arbitrary GPU execution or faults.

[ ... ]

> @@ -616,11 +616,10 @@ static void mchp_gfx2d_run_command(struct mchp_gfx2d_device *priv,
>  		if (cmd->blend.flags & GFX2D_BLEND_SET_DST_COLOR) {
>  			enum mchp_gfx2d_reg_id reg = priv->caps->has_dreg ?
>  				BLEND_DREG :
>  				BLEND_SREG;
>  			mchp_gfx2d_ldr(priv, reg, cmd->blend.dst_color);
>  			mchp_gfx2d_trigger(priv);
>  			cmd->blend.flags &= ~GFX2D_BLEND_SET_DST_COLOR;
>  		}
>  		if (cmd->blend.flags & GFX2D_BLEND_SET_SRC_COLOR) {
>  			mchp_gfx2d_ldr(priv, BLEND_SREG, cmd->blend.src_color);

[Severity: Medium]
If hardware lacks a dedicated destination register, won't this silently
overwrite the destination color?

The register gets populated with the destination color above, but if the
source color flag is also set, the exact same register is immediately
overwritten, causing the operation to execute without the correct destination
color.

[ ... ]

> @@ -1242,15 +1242,14 @@ static int
>  mchp_gfx2d_set_rop_params(struct mchp_gfx2d_device *priv,
>  			  struct drm_file *file_priv,
>  			  const struct drm_mchp_gfx2d_submit *args,
>  			  struct mchp_gfx2d_command *cmd)
>  {
>  	uint32_t handle = args->rop.mask_handle;
>  
>  	cmd->rop.mode = (u8)args->rop.mode;
>  	if (cmd->rop.mode > DRM_MCHP_GFX2D_ROP4)
>  		return -EINVAL;
>  
>  	if (args->rop.mode == DRM_MCHP_GFX2D_ROP2 && args->rop.low > 15)
>  		return -EINVAL;
>  
>  	if (((args->rop.mode == DRM_MCHP_GFX2D_ROP4) && !handle) ||
>  	    ((args->rop.mode != DRM_MCHP_GFX2D_ROP4) && handle))
>  		return -EINVAL;

[Severity: High]
Does the integer truncation of the mode allow userspace to bypass mask
validation? 

The 32-bit mode is truncated to an 8-bit integer for the bounds check.
If a user passes 0x10000002, it truncates to 2 (ROP4) and passes the check.
However, the subsequent validation for a missing mask uses the untruncated
32-bit value. This could allow a ROP4 command into the pipeline with a null
mask pointer, potentially hard-faulting the GPU.

[ ... ]

> @@ -1291,15 +1291,14 @@ static struct mchp_gfx2d_command *
>  mchp_gfx2d_alloc_command(struct mchp_gfx2d_device *priv)
>  {
>  	static uint32_t next_id;
>  	struct mchp_gfx2d_command *cmd = NULL;

[ ... ]

>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->id = next_id++;
>  
>  	return cmd;
>  }

[Severity: Critical]
Can this race if multiple threads submit graphics instructions concurrently? 

Since the identifier is a static variable updated without synchronization,
concurrent ioctls might assign identical identifiers to different commands.
If the hardware mailbox reports the most recently completed identifier,
compleosing the first command could incorrectly signal that both are finished.
This could cause the driver to free the second command and its GEM references
while the GPU is still executing it.

[ ... ]

> @@ -1513,11 +1513,10 @@ void mchp_gfx2d_cleanup_command_queue(struct mchp_gfx2d_device *priv)
>  {
>  	struct device *dev = priv->drm.dev;
>  	struct mchp_gfx2d_command *cmd, *tmp;
>  	uint32_t i;
>  
>  	writel(GFX2D_IRQ_EXEND, priv->regs + GFX2D_ID);

[Severity: High]
Does this safely handle hardware accesses if the peripheral clock is gated?

This unconditionally writes to registers, which could trigger a hardware bus
fault if the clock failed to enable or was disabled by runtime suspend.

[ ... ]

> @@ -1568,14 +1568,13 @@ int mchp_gfx2d_submit(struct mchp_gfx2d_device *priv,
>  		      struct drm_file *file_priv,
>  		      const struct drm_mchp_gfx2d_submit *args)
>  {
>  	struct mchp_gfx2d_file *ctx = file_priv->driver_priv;
>  	struct mchp_gfx2d_command *cmd;
>  	size_t i, num_sources;
>  	int ret;
>  
>  	if (!args->num_rectangles)
>  		return -EINVAL;
>  
>  	cmd = mchp_gfx2d_alloc_command(priv);
>  	if (!cmd)
>  		return -ENOMEM;
>  
>  	cmd->ctx = ctx;
>  	cmd->operation = (u8)args->operation;
>  	drm_dbg(&priv->drm, "cmd %u: allocate command (operation = %u)\n",
>  		cmd->id, cmd->operation);
>  
>  	ret = mchp_gfx2d_alloc_rectangles(cmd, args->num_rectangles);

[Severity: Medium]
Is there a missing upper bound check for the number of rectangles here?

Since this untrusted user value is passed directly to the memory allocation
routines, an unprivileged user could pass an extremely large value to exhaust
kernel memory or trigger the OOM killer.

>  	if (ret)
>  		goto err_release_command;
>  
>  	ret = copy_from_user(cmd->rects, u64_to_user_ptr(args->rectangles),
>  			     args->num_rectangles * sizeof(*cmd->rects));

[Severity: High]
Can this copy operations corrupt the shared context array?

If multiple threads use the same file descriptor and the shared context buffer
is assigned without synchronization, concurrent copies here would overwrite each
other's data.

[ ... ]

> @@ -1689,17 +1689,16 @@ int mchp_gfx2d_wait(struct mchp_gfx2d_device *priv,
>  		    struct mchp_gfx2d_gem_object *gfx2d_obj,
>  		    const struct drm_mchp_timespec *timeout)
>  {
>  	struct device *dev = priv->drm.dev;
>  	int ret;
>  
>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret)
>  		return ret;
>  
>  	(void)mchp_gfx2d_progress(priv);
>  
>  	drm_dbg(&priv->drm, "obj %u: poll object\n", gfx2d_obj->id);
>  
>  	ret = mchp_gfx2d_gem_wait(gfx2d_obj, NULL);
>  	if (!ret || !timeout)
>  		goto put_autosuspend;
>  
>  	drm_dbg(&priv->drm, "obj %u: wait for object\n", gfx2d_obj->id);
>  
>  	mutex_lock(&priv->wlist_mutex);
>  	list_add_tail(&gfx2d_obj->w_node, &priv->wlist);
>  	mutex_unlock(&priv->wlist_mutex);
>  
>  	mchp_gfx2d_enable_exend(priv);
>  
>  	ret = mchp_gfx2d_gem_wait(gfx2d_obj, timeout);
>  
>  	mutex_lock(&priv->wlist_mutex);
>  	list_del(&gfx2d_obj->w_node);
>  	mutex_unlock(&priv->wlist_mutex);

[Severity: Critical]
Can this corrupt the linked list? 

Unprivileged userspace can spawn multiple threads invoking a wait on the same
GEM object concurrently. They will sequentially acquire the mutex and blindly
call list_add_tail on the same embedded node. Inserting an already-linked node
corrupts the pointers, and when the threads later attempt to remove it, a
double-delete kernel panic will occur.

> diff --git a/drivers/gpu/drm/microchip/mchp_gfx2d_drv.c b/drivers/gpu/drm/microchip/mchp_gfx2d_drv.c
> index 0000000000000..ab8df683f3e00
> --- /dev/null
> +++ b/drivers/gpu/drm/microchip/mchp_gfx2d_drv.c
> @@ -69,14 +69,13 @@ static const struct drm_driver mchp_gfx2d_drm_driver = {
>  
>  static irqreturn_t mchp_gfx2d_interrupt(int irq, void *dev_id)
>  {
>  	struct mchp_gfx2d_device *priv = dev_id;
>  	u32 status, mask, pending;
>  
>  	status = readl(priv->regs + GFX2D_IS);

[Severity: High]
Does this safely handle spurious interrupts while the peripheral clock is
gated? 

Reading an MMIO register without checking if the clock is active might cause
a synchronous external abort (bus fault) if the driver is runtime suspended.

>  	mask = readl(priv->regs + GFX2D_IM);
>  	pending = status & mask;
>  
>  	if (!pending)
>  		return IRQ_NONE;
>  
>  	if (pending & GFX2D_IRQ_EXEND)
>  		return IRQ_WAKE_THREAD;
>  
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Does this intentionally silently acknowledge and ignore GPU hardware error
interrupts? 

If the interrupt was triggered by a read error, write error, or illegal
instruction, falling through without waking up waiters or resetting the GPU
might cause waiting processes to hang indefinitely until they time out.

[ ... ]

> @@ -212,12 +212,11 @@ static void mchp_gfx2d_remove(struct platform_device *pdev)
>  {
>  	struct mchp_gfx2d_device *priv = platform_get_drvdata(pdev);
>  	int ret;
>  
>  	drm_dev_unregister(&priv->drm);
>  
>  	ret = pm_runtime_resume_and_get(&pdev->dev);
>  	mchp_gfx2d_cleanup_command_queue(priv);

[Severity: High]
If the runtime resume fails here, does the cleanup function safely handle
hardware accesses? 

The cleanup function unconditionally writes to hardware registers, which could
trigger a hardware bus fault if the clock failed to enable.

[ ... ]

> @@ -279,13 +279,12 @@ static int __maybe_unused mchp_gfx2d_runtime_resume(struct device *dev)
>  {
>  	struct mchp_gfx2d_device *priv = dev_get_drvdata(dev);
>  	int ret;
>  
>  	ret = clk_enable(priv->pclk);
>  	if (ret)
>  		return ret;
>  
>  	mutex_lock(&priv->cmdlist_mutex);

[Severity: High]
Does the hardware retain its configuration after a system suspend (S3)? 

If power is lost, waking up and only enabling the clock without restoring
critical registers like the ring buffer address and global enable flag might
cause the GPU to permanently stall when subsequent commands are submitted.

> diff --git a/drivers/gpu/drm/microchip/mchp_gfx2d_gem.c b/drivers/gpu/drm/microchip/mchp_gfx2d_gem.c
> index 0000000000000..66f22cd6ac981
> --- /dev/null
> +++ b/drivers/gpu/drm/microchip/mchp_gfx2d_gem.c
> @@ -23,17 +23,16 @@ static int mchp_gfx2d_gem_object_mmap(struct drm_gem_object *obj,
>  				      struct vm_area_struct *vma)
>  {
>  	struct mchp_gfx2d_gem_object *gfx2d_obj = to_mchp_gfx2d_bo(obj);
>  	int ret;
>  
>  	/*
>  	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
>  	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
>  	 * the whole buffer.
>  	 */
>  	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);

[Severity: High]
Could this double-subtraction permanently break the mapping?

The core DRM subsystem already translates and subtracts the offset before
invoking the driver callback. Doing it a second time here will underflow the
offset, causing the subsequent mapping call to reject the operation with an
error.

>  	vm_flags_mod(vma, VM_DONTEXPAND, VM_PFNMAP);
>  
>  	if (gfx2d_obj->direction == DMA_NONE) {
>  		ret = dma_mmap_wc(obj->dev->dev, vma, gfx2d_obj->vaddr,
>  				  gfx2d_obj->dma_addr,
>  				  vma->vm_end - vma->vm_start);
>  	} else {
>  		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  
>  		ret = dma_mmap_pages(obj->dev->dev,
>  				     vma, vma->vm_end - vma->vm_start,
>  				     virt_to_page(gfx2d_obj->vaddr));

[Severity: High]
Is virt_to_page() safe to use on DMA addresses here? 

The virtual address is allocated with the noncoherent dma allocator, which
on many architectures returns a non-linear vmalloc address. Using this helper
on non-linear addresses yields a bogus page structure, potentially leading to
page faults or mapping arbitrary physical memory to userspace.

>  	}
>  	if (ret)
>  		drm_gem_vm_close(vma);
>  	else
>  		memcpy(&gfx2d_obj->vma, vma, sizeof(gfx2d_obj->vma));

[Severity: High]
Does this safely handle the lifecycle of the memory management struct? 

Performing a shallow copy using memcpy means that if the original mapping is
unmapped, split, or the process exits, this cached copy will contain stale
pointers that could cause a use-after-free later.

[ ... ]

> @@ -100,12 +100,11 @@ struct mchp_gfx2d_gem_object *
>  mchp_gfx2d_gem_create(struct drm_device *dev, size_t size,
>  		      enum dma_data_direction dir)
>  {
>  	struct mchp_gfx2d_gem_object *gfx2d_obj;
>  
>  	size = round_up(size, PAGE_SIZE);

[Severity: High]
Can this overflow? 

If userspace passes an extremely large size close to U32_MAX, this can silently
wrap to 0. The driver would allocate 0 bytes of physical memory but still
populate the metadata with the huge dimensions, tricking the GPU into
out-of-bounds DMA accesses.

[ ... ]

> @@ -132,14 +131,13 @@ mchp_gfx2d_gem_create_with_handle(struct drm_file *file_priv,
>  				  struct drm_device *dev, size_t size,
>  				  enum dma_data_direction dir,
>  				  uint32_t *handle)
>  {
>  	struct mchp_gfx2d_gem_object *gfx2d_obj;
>  	struct drm_gem_object *obj;
>  	int ret;
>  
>  	gfx2d_obj = mchp_gfx2d_gem_create(dev, size, dir);
>  	if (IS_ERR(gfx2d_obj))
>  		return gfx2d_obj;
>  
>  	obj = &gfx2d_obj->base;
>  
>  	ret = drm_gem_handle_create(file_priv, obj, handle);
>  	/* drop reference from allocate - handle holds it now. */
>  	drm_gem_object_put(obj);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
>  	return gfx2d_obj;
>  }

[Severity: High]
Does dropping the reference here expose the object to being concurrently freed
before initialization is complete? 

Once the handle is registered, it becomes visible to userspace. A concurrent
thread can immediately issue the free ioctl, releasing the memory. When this
function returns and its callers subsequently write to the object's properties,
it would result in a use-after-free.

> diff --git a/drivers/gpu/drm/microchip/mchp_gfx2d_ioctl.c b/drivers/gpu/drm/microchip/mchp_gfx2d_ioctl.c
> index 0000000000000..2a24738577892
> --- /dev/null
> +++ b/drivers/gpu/drm/microchip/mchp_gfx2d_ioctl.c
> @@ -178,16 +178,15 @@ static int mchp_gfx2d_ioctl_import_buffer(struct drm_device *dev, void *data,
>  					  struct drm_file *file_priv)
>  {
>  	struct drm_mchp_gfx2d_import_buffer *args = data;
>  	struct mchp_gfx2d_gem_object *gfx2d_obj;
>  	struct drm_gem_object *obj;
>  	int ret;
>  
>  	if (!mchp_gfx2d_valid_buffer_params(args->width, args->stride, args->format))
>  		return -EINVAL;
>  
>  	ret = drm_gem_prime_fd_to_handle(dev, file_priv, args->fd, &args->handle);
>  	if (ret)
>  		return ret;
>  
>  	obj = drm_gem_object_lookup(file_priv, args->handle);
>  	if (!obj)
>  		return -ENOENT;
>  
>  	if (!mchp_gfx2d_valid_size(args->stride, args->height, obj->size)) {
>  		drm_gem_object_put(obj);
>  		drm_gem_handle_delete(file_priv, args->handle);
>  		return -EINVAL;
>  	}
>  
>  	gfx2d_obj = to_mchp_gfx2d_bo(obj);
>  	gfx2d_obj->width = args->width;
>  	gfx2d_obj->height = args->height;
>  	gfx2d_obj->stride = args->stride;
>  	gfx2d_obj->format = args->format;
>  
>  	drm_gem_object_put(obj);
>  
>  	return 0;
>  }

[Severity: High]
Is it safe to unconditionally overwrite these metadata bounds on an imported
object? 

If a buffer is exported by one process and imported by another, the importing
process could provide arbitrary dimensions here. This modifies the shared
object globally, potentially breaking metadata immutability and permitting
out-of-bounds memory accesses for other processes sharing the object.

[ ... ]

> @@ -206,16 +205,15 @@ static int mchp_gfx2d_ioctl_sync_for_cpu(struct drm_device *dev, void *data,
>  					 struct drm_file *file_priv)
>  {
>  	struct mchp_gfx2d_device *priv = drm_to_dev(dev);
>  	struct drm_mchp_gfx2d_sync_for_cpu *args = data;
>  	const struct drm_mchp_timespec *timeout = &args->timeout;
>  	struct mchp_gfx2d_gem_object *gfx2d_obj;
>  	struct drm_gem_object *obj;
>  	int ret;
>  
>  	if (args->flags & ~DRM_MCHP_GFX2D_WAIT_NONBLOCK)
>  		return -EINVAL;
>  
>  	obj = drm_gem_object_lookup(file_priv, args->handle);
>  	if (!obj)
>  		return -ENOENT;
>  
>  	gfx2d_obj = to_mchp_gfx2d_bo(obj);
>  
>  	if (args->flags & DRM_MCHP_GFX2D_WAIT_NONBLOCK)
>  		timeout = NULL;
>  
>  	ret = mchp_gfx2d_wait(priv, gfx2d_obj, timeout);
>  
>  	drm_gem_object_put(obj);
>  
>  	return ret;
>  }

[Severity: High]
Does this ioctl intentionally skip invalidating CPU caches? 

For buffers allocated via the noncoherent allocator, failing to call the CPU
cache synchronization functions means the CPU might read stale cached data
instead of the final GPU output once the wait completes.

> @@ -236,13 +234,12 @@ static int mchp_gfx2d_ioctl_sync_for_gpu(struct drm_device *dev, void *data,
>  					 struct drm_file *file_priv)
>  {
>  	struct drm_mchp_gfx2d_sync_for_gpu *args = data;
>  	struct mchp_gfx2d_gem_object *gfx2d_obj;
>  	enum dma_data_direction dir;
>  	struct drm_gem_object *obj;
>  
>  	obj = drm_gem_object_lookup(file_priv, args->handle);
>  	if (!obj)
>  		return -ENOENT;
>  
>  	gfx2d_obj = to_mchp_gfx2d_bo(obj);
>  
>  	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);
>  	}

[Severity: High]
Continuing from the shallow copy concern, does the cache flush safely handle
the copied struct? 

The architecture-level cache flush can dereference potentially freed properties
if the original mapping was already unmapped or split by the process, leading
to a use-after-free.

> diff --git a/include/uapi/drm/microchip_drm.h b/include/uapi/drm/microchip_drm.h
> index 0000000000000..6be5ea9edd7cd
> --- /dev/null
> +++ b/include/uapi/drm/microchip_drm.h
> @@ -148,15 +148,14 @@ struct drm_mchp_gfx2d_rop {
>  	__u32 mask_handle;
>  	enum drm_mchp_gfx2d_rop_mode mode;
>  	__u8 high;
>  	__u8 low;
>  };
>  
>  struct drm_mchp_gfx2d_submit {
>  	__u64 rectangles;
>  	__kernel_size_t num_rectangles;

[Severity: Medium]
Does using this type in a UAPI struct break 32-bit compatibility ioctl
translations? 

Its size varies between 32-bit and 64-bit architectures, which typically causes
ABI mismatches.

[ ... ]

> @@ -224,15 +224,14 @@ enum drm_mchp_gfx2d_direction {
>  	DRM_MCHP_GFX2D_DIR_BIDIRECTIONAL,
>  	DRM_MCHP_GFX2D_DIR_TO_DEVICE,
>  	DRM_MCHP_GFX2D_DIR_FROM_DEVICE,
>  	DRM_MCHP_GFX2D_DIR_NONE,
>  };
>  
>  struct drm_mchp_gfx2d_alloc_buffer {
>  	__u32 size;     /* in bytes for mmap() */
>  	__u16 width;
>  	__u16 height;
>  	__u16 stride;
>  	enum drm_mchp_gfx2d_pixel_format format;

[Severity: Medium]
Will this 16-bit field followed by a 32-bit enum create implicit padding?

Leaving uninitialized padding in UAPI structures can lead to information leaks
to userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-cpitchen-mainline_gfx2d-v8-0-378374559738@microchip.com?part=2