Re: [PATCH v5] drm/tiny: trigger5: Add MCT Trigger 5 USB display driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/tiny: trigger5: unclipped src rect in trigger5_plane_atomic_update()
--

commit 56412e719e880a97ad42bea1ca53d34f2a12fdb6
Author: Ho Jie Feng <hjf3108@gmail.com>

drm/tiny: trigger5: Add MCT Trigger 5 USB display driver

This commit adds a DRM driver for USB 3 to HDMI and VGA display adapters based
on the MCT Trigger 5 chipset, implementing modesetting, USB framebuffer
updates, connector detection, and EDID retrieval.

> diff --git a/drivers/gpu/drm/tiny/trigger5.c b/drivers/gpu/drm/tiny/trigger5.c
> new file mode 100644
> index 0000000000000..d0ab06bab8d73
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/trigger5.c
[ ... ]
> +static enum drm_mode_status
> +trigger5_crtc_mode_valid(struct drm_crtc *crtc,
> +			 const struct drm_display_mode *mode)
> +{
> +	struct trigger5_pll pll;
> +	u64 err, ppm;
> +
> +	if (mode->hsync_end <= mode->hsync_start ||
> +	    mode->htotal <= mode->hsync_end ||
> +	    mode->htotal > U16_MAX + 1)
> +		return MODE_H_ILLEGAL;
> +
> +	if (mode->vsync_end <= mode->vsync_start ||
> +	    mode->vtotal <= mode->vsync_end ||
> +	    mode->vtotal > U16_MAX + 1)
> +		return MODE_V_ILLEGAL;
> +
> +	if (trigger5_mode_buf_len(mode) > TRIGGER5_TRANSFER_BUF_SIZE)
> +		return MODE_MEM;
[ ... ]
> +static int trigger5_plane_atomic_check(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
[ ... ]
> +	ret = drm_atomic_helper_check_plane_state(new_plane_state,
> +						  new_crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, false);
> +	if (ret || !new_plane_state->visible)
> +		return ret;
[ ... ]
> +static void trigger5_plane_atomic_update(struct drm_plane *plane,
> +					 struct drm_atomic_commit *atomic_state)
> +{
[ ... ]
> +	current_transfer =
> +		&trigger5->transfers[trigger5->current_transfer];
> +	previous_transfer = &trigger5->transfers[1 - trigger5->current_transfer];
> +
> +	src_rect = drm_plane_state_src(state);
> +
> +	/* Match drm_atomic_helper_damage_iter_init() rounding. */
> +	src_rect.x1 >>= 16;
> +	src_rect.y1 >>= 16;
> +	src_rect.x2 = (src_rect.x2 >> 16) + !!(src_rect.x2 & 0xffff);
> +	src_rect.y2 = (src_rect.y2 >> 16) + !!(src_rect.y2 & 0xffff);
> +
> +	/* Latency reduction: requeue with the latest frame data. */
> +	if (cancel_work(&previous_transfer->transfer_work)) {
> +		complete(&previous_transfer->frame_complete);
> +
> +		trigger5_merge_rect(&current_rect, &previous_transfer->transfer_rect);
> +
> +		current_transfer = previous_transfer;
> +		trigger5->current_transfer = !trigger5->current_transfer;
> +	}
> +
> +	/* Damage deferred by an earlier failed update. */
> +	trigger5_merge_rect(&current_rect, &trigger5->pending_rect);
> +	trigger5_clear_rect(&trigger5->pending_rect);
> +
> +	/* Clip merged damage to the new resolution. */
> +	if (!drm_rect_intersect(&current_rect, &src_rect))
> +		goto exit;
> +
> +	width = drm_rect_width(&current_rect);
> +	height = drm_rect_height(&current_rect);
> +	payload_len = array3_size(width, height, 3);
> +	frame_len = size_add(payload_len, sizeof(*header));
> +
> +	if (drm_WARN_ON_ONCE(plane->dev, frame_len > current_transfer->buf.len))
> +		goto exit;

[Severity: Medium]
Can src_rect in trigger5_plane_atomic_update() exceed the active CRTC mode
dimensions and overflow the 8 MiB limit checked in trigger5_crtc_mode_valid()?

In trigger5_plane_atomic_check(), drm_atomic_helper_check_plane_state() clips
new_plane_state->dst to (0, 0)-(mode->hdisplay, mode->vdisplay) and stores the
clipped source coordinates in new_plane_state->src, while leaving the
unclipped new_plane_state->src_x, src_y, src_w, and src_h unchanged. Because
drm_atomic_helper_check_plane_state() checks drm_rect_equals(dst, &clip) after
clipping dst, a plane update on a larger framebuffer with crtc_x <= 0,
crtc_y <= 0, and unclipped src_w/src_h larger than mode->hdisplay and
mode->vdisplay passes validation.

In addition, drm_atomic_helper_check_plane_state() does not reject non-zero
16.16 fixed-point fractional bits in src_x or src_y when panning within a
larger framebuffer. Rounding src_rect.x1/y1 down and src_rect.x2/y2 up expands
the dimensions of src_rect by an extra pixel to mode->hdisplay + 1 and
mode->vdisplay + 1.

Because trigger5_plane_atomic_update() builds src_rect from the unclipped
drm_plane_state_src(state) rather than the clipped state->src, current_rect
after drm_rect_intersect(&current_rect, &src_rect) can exceed mode->hdisplay x
mode->vdisplay. When frame_len exceeds current_transfer->buf.len, this
triggers drm_WARN_ON_ONCE() and jumps to exit after trigger5->pending_rect was
already cleared and previous_transfer->transfer_work was potentially
cancelled, dropping that damage. When frame_len still fits within
current_transfer->buf.len, the bulk header is sent with width and height
larger than the configured mode and offsets computed relative to the unclipped
off-screen src_rect.x1/y1.

Should trigger5_plane_atomic_update() use state->src instead of
drm_plane_state_src(state), and should trigger5_plane_atomic_check() reject
fractional source coordinates or unclipped planes?

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