Re: [PATCH 2/2] drm/verisilicon: add support for Nuvoton MA35D1 DCUltra Lite display controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The primary plane is never logically disabled in hardware for the DCUltra Lite variant, leading to a Use-After-Free condition.
- [High] The primary plane is enabled before its DMA address and size are configured, causing a hardware race condition.
- [High] The DCUltra Lite IRQ handler fails to acknowledge interrupts and unconditionally returns IRQ_HANDLED, which can cause an interrupt storm.
- [High] Pre-existing NULL pointer dereference in early IRQ handling due to incorrect initialization order.
--

commit 522901c8020d5fe12ddd26c3af8e4d282c2fd2fd
Author: Joey Lu <a0987203069@gmail.com>
Subject: drm/verisilicon: add support for Nuvoton MA35D1 DCUltra Lite display controller

This commit adds support for the Nuvoton MA35D1 DCUltra Lite display
controller to the verisilicon DRM driver. It introduces a vs_dc_info
platform data structure to handle architectural differences between the
DC8000 and DCUltra Lite variants.

> diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c
> index dad9967bc10b8..82a6a26f6d817 100644
> --- a/drivers/gpu/drm/verisilicon/vs_dc.c
> +++ b/drivers/gpu/drm/verisilicon/vs_dc.c

[ ... ]

> @@ -33,6 +57,13 @@ static irqreturn_t vs_dc_irq_handler(int irq, void *private)
>  	struct vs_dc *dc = private;
>  	u32 irqs;
>  
> +	if (dc->info->family == VS_DC_FAMILY_DCULTRA_LITE) {
> +		regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &irqs);
> +		if (irqs & BIT(0))
> +			vs_drm_handle_irq(dc, VSDC_TOP_IRQ_VSYNC(0));
> +		return IRQ_HANDLED;
> +	}

Does this missing ACK cause an interrupt storm?

The DCUltra Lite code path reads the interrupt status but does not appear to
write back to clear the pending hardware interrupt.

Additionally, unconditionally returning IRQ_HANDLED might prevent the spurious
interrupt detector from disabling a faulty IRQ line.

This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference during boot?

In vs_dc_probe(), the IRQ handler is requested before initialization:

	ret = devm_request_irq(dev, irq, vs_dc_irq_handler, 0,
			       dev_name(dev), dc);
	...
	ret = vs_drm_initialize(dc, pdev);

If a leftover hardware interrupt fires immediately, vs_dc_irq_handler() calls
vs_drm_handle_irq(), which accesses dc->drm_dev->crtcs[i].

Since dc->drm_dev is allocated inside vs_drm_initialize(), it appears
an early interrupt would dereference a NULL pointer.

> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index 1f2be41ae496c..197d5d683e224 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c

[ ... ]

> @@ -88,8 +91,9 @@ static void vs_primary_plane_atomic_disable(struct drm_plane *plane,
>  	unsigned int output = vcrtc->id;
>  	struct vs_dc *dc = vcrtc->dc;
>  
> -	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> -			VSDC_FB_CONFIG_EX_FB_EN);
> +	if (dc->info->has_config_ex)
> +		regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> +				VSDC_FB_CONFIG_EX_FB_EN);
>  
>  	vs_primary_plane_commit(dc, output);
>  }

If has_config_ex is false for DCUltra Lite, how is the primary plane disabled
in hardware here?

Since the disable sequence is skipped, will the display controller continue
fetching from the framebuffer memory after it is freed by the DRM core?
This could lead to a use-after-free by the DMA engine.

[ ... ]

> @@ -126,6 +130,11 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
>  			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
>  			   vs_state->format.uv_swizzle);
>  
> +	/* DCUltra Lite requires explicit enable/reset bits in FB_CONFIG */
> +	if (!dc->info->has_config_ex)
> +		regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output),
> +				VSDC_FB_CONFIG_ENABLE | VSDC_FB_CONFIG_RESET);
> +
>  	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
>  
>  	regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
> @@ -133,16 +142,18 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
>  	regmap_write(dc->regs, VSDC_FB_STRIDE(output),
>  		     fb->pitches[0]);

Could enabling the plane before configuring its DMA address and size cause
the hardware to fetch from uninitialized or stale memory?

Since DCUltra Lite does not use a shadow register commit mechanism, it seems
the address and stride should be programmed before setting the
VSDC_FB_CONFIG_ENABLE bit to prevent out-of-bounds memory accesses.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511075142.54752-1-a0987203069@gmail.com?part=2