[PATCH 1/2] drm/vkms: Cache composer line buffers in vkms_output

[PATCH 1/2] drm/vkms: Cache composer line buffers in vkms_output

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

compose_active_planes() kvmalloc/kvfree two line buffers every vblank
frame, but their size depends only on hdisplay which changes only on
modeset.  Cache them in vkms_output and reallocate only when the width
changes, avoiding repeated alloc/free overhead.

Register a drmm action to free the buffers when the device is released.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 44 ++++++++++++++++------------
 drivers/gpu/drm/vkms/vkms_crtc.c     | 12 ++++++++
 drivers/gpu/drm/vkms/vkms_drv.h      |  7 +++++
 3 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 83d217085ad0..f3fb203946e8 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -541,11 +541,11 @@ static int check_iosys_map(struct vkms_crtc_state *crtc_state)
 
 static int compose_active_planes(struct vkms_writeback_job *active_wb,
 				 struct vkms_crtc_state *crtc_state,
+				 struct vkms_output *out,
 				 u32 *crc32)
 {
 	size_t line_width, pixel_size = sizeof(struct pixel_argb_u16);
 	struct line_buffer output_buffer, stage_buffer;
-	int ret = 0;
 
 	/*
 	 * This check exists so we can call `crc32_le` for the entire line
@@ -565,27 +565,35 @@ static int compose_active_planes(struct vkms_writeback_job *active_wb,
 	stage_buffer.n_pixels = line_width;
 	output_buffer.n_pixels = line_width;
 
-	stage_buffer.pixels = kvmalloc(line_width * pixel_size, GFP_KERNEL);
-	if (!stage_buffer.pixels) {
-		DRM_ERROR("Cannot allocate memory for the output line buffer");
-		return -ENOMEM;
-	}
+	if (out->composer_buffer_width != line_width) {
+		kvfree(out->composer_stage_buffer);
+		kvfree(out->composer_output_buffer);
+		out->composer_buffer_width = 0;
+
+		out->composer_stage_buffer = kvmalloc(line_width * pixel_size, GFP_KERNEL);
+		if (!out->composer_stage_buffer) {
+			DRM_ERROR("Cannot allocate memory for the output line buffer");
+			return -ENOMEM;
+		}
+
+		out->composer_output_buffer = kvmalloc(line_width * pixel_size, GFP_KERNEL);
+		if (!out->composer_output_buffer) {
+			DRM_ERROR("Cannot allocate memory for intermediate line buffer");
+			kvfree(out->composer_stage_buffer);
+			out->composer_stage_buffer = NULL;
+			return -ENOMEM;
+		}
 
-	output_buffer.pixels = kvmalloc(line_width * pixel_size, GFP_KERNEL);
-	if (!output_buffer.pixels) {
-		DRM_ERROR("Cannot allocate memory for intermediate line buffer");
-		ret = -ENOMEM;
-		goto free_stage_buffer;
+		out->composer_buffer_width = line_width;
 	}
 
+	stage_buffer.pixels = out->composer_stage_buffer;
+	output_buffer.pixels = out->composer_output_buffer;
+
 	blend(active_wb, crtc_state, crc32, &stage_buffer,
 	      &output_buffer, line_width * pixel_size);
 
-	kvfree(output_buffer.pixels);
-free_stage_buffer:
-	kvfree(stage_buffer.pixels);
-
-	return ret;
+	return 0;
 }
 
 /**
@@ -644,9 +652,9 @@ void vkms_composer_worker(struct work_struct *work)
 		return;
 
 	if (wb_pending)
-		ret = compose_active_planes(active_wb, crtc_state, &crc32);
+		ret = compose_active_planes(active_wb, crtc_state, out, &crc32);
 	else
-		ret = compose_active_planes(NULL, crtc_state, &crc32);
+		ret = compose_active_planes(NULL, crtc_state, out, &crc32);
 
 	if (ret)
 		return;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 079abfba427d..6e03c9a1fbd8 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -13,6 +13,14 @@
 
 #include "vkms_drv.h"
 
+static void vkms_composer_buffers_release(struct drm_device *dev, void *data)
+{
+	struct vkms_output *out = data;
+
+	kvfree(out->composer_stage_buffer);
+	kvfree(out->composer_output_buffer);
+}
+
 static bool vkms_crtc_handle_vblank_timeout(struct drm_crtc *crtc)
 {
 	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
@@ -237,5 +245,9 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
 	if (IS_ERR(vkms_out->composer_workq))
 		return ERR_CAST(vkms_out->composer_workq);
 
+	ret = drmm_add_action_or_reset(dev, vkms_composer_buffers_release, vkms_out);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return vkms_out;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 0933e4ce0ff0..e6d58b396451 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -214,6 +214,9 @@ struct vkms_crtc_state {
  *		      writeback)
  * @composer_state: Protected by @lock, current state of this VKMS output
  * @composer_lock: Lock used internally to protect @composer_state members
+ * @composer_stage_buffer: Cached line buffer for plane pixel read and pre-blend transform
+ * @composer_output_buffer: Cached line buffer for blending output
+ * @composer_buffer_width: Current allocated width of the cached buffers (in pixels)
  */
 struct vkms_output {
 	struct drm_crtc crtc;
@@ -226,6 +229,10 @@ struct vkms_output {
 	struct vkms_crtc_state *composer_state;
 
 	spinlock_t composer_lock;
+
+	struct pixel_argb_u16 *composer_stage_buffer;
+	struct pixel_argb_u16 *composer_output_buffer;
+	size_t composer_buffer_width;
 };
 
 struct vkms_config;
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH] drm/vkms: Fix gamma_lut size check

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

The gamma_lut size check in vkms_atomic_check() uses
sizeof(struct drm_color_lut *) instead of sizeof(struct drm_color_lut).
On 64-bit both happen to be 8 bytes, so the check works by accident.
On 32-bit sizeof(pointer) is 4 bytes while sizeof(struct drm_color_lut)
is 8 bytes, which would incorrectly reject valid 256-entry LUTs.

Fixes: db1f254f2cfa ("drm/vkms: Add support to 1D gamma LUT")

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 5a640b531d88..2db3971f72c3 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -112,7 +112,7 @@ static int vkms_atomic_check(struct drm_device *dev, struct drm_atomic_commit *s
 		if (!new_crtc_state->gamma_lut || !new_crtc_state->color_mgmt_changed)
 			continue;
 
-		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut *)
+		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut)
 		    > VKMS_LUT_SIZE)
 			return -EINVAL;
 	}
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH] drm/vkms: Fix vertical read step for R1/R2/R4 formats

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

get_block_step_bytes() returns pitches * block_w as the vertical step for
READ_TOP_TO_BOTTOM and READ_BOTTOM_TO_TOP, but consecutive rows are
pitches * block_h bytes apart (block_h is 1 for R1/R2/R4). Vertical reads
therefore sample every block_w-th row instead of every row, and walk past
the end of the framebuffer, when 90/270-rotating or reflecting R1/R2/R4
planes. Use the block height instead, matching packed_pixels_offset().

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index dfb8e13cba87..1e3a6af9cea5 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -103,11 +103,11 @@ static int get_block_step_bytes(struct drm_framebuffer *fb, enum pixel_read_dire
 	case READ_RIGHT_TO_LEFT:
 		return -fb->format->char_per_block[plane_index];
 	case READ_TOP_TO_BOTTOM:
-		return (int)fb->pitches[plane_index] * drm_format_info_block_width(fb->format,
-										   plane_index);
-	case READ_BOTTOM_TO_TOP:
-		return -(int)fb->pitches[plane_index] * drm_format_info_block_width(fb->format,
+		return (int)fb->pitches[plane_index] * drm_format_info_block_height(fb->format,
 										    plane_index);
+	case READ_BOTTOM_TO_TOP:
+		return -(int)fb->pitches[plane_index] * drm_format_info_block_height(fb->format,
+										     plane_index);
 	}
 
 	return 0;
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH 2/2] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

pre_blend_color_transform() traverses the colorop pipeline and unpacks
each pixel into s32 for every line, even when all colorops are bypassed
or the pipeline is empty.  Add a line-level check to skip the entire
function in that case, avoiding per-pixel overhead.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index f3fb203946e8..1806c249da6f 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -190,13 +190,29 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
 	}
 }
 
+static bool pipeline_all_bypassed(struct drm_colorop *colorop)
+{
+	while (colorop) {
+		struct drm_colorop_state *colorop_state = colorop->state;
+
+		if (!colorop_state || !colorop_state->bypass)
+			return false;
+		colorop = colorop->next;
+	}
+	return true;
+}
+
 static void pre_blend_color_transform(const struct vkms_plane_state *plane_state,
 				      struct line_buffer *output_buffer)
 {
+	struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
 	struct pixel_argb_s32 pixel;
 
+	if (!colorop || pipeline_all_bypassed(colorop))
+		return;
+
 	for (size_t x = 0; x < output_buffer->n_pixels; x++) {
-		struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
+		colorop = plane_state->base.base.color_pipeline;
 
 		/*
 		 * Some operations, such as applying a BT709 encoding matrix,
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH 1/2] drm/vkms: Cache composer line buffers in vkms_output

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The device teardown path frees the composer line buffers before the composer workqueue is flushed, leading to a Use-After-Free (UAF).
- [High] A double-free vulnerability exists if the allocation for the stage buffer fails during reallocation, leaving a dangling pointer.

Pre-existing issues:
- [High] Preexisting Use-After-Free of `vkms_crtc_state` when destroying the CRTC state while a composer worker is pending.
--

--- Patch [1]: [PATCH 1/2] drm/vkms: Cache composer line buffers in vkms_output ---
commit 169a243140521e33c116ce0ae779021e7e4e4b76
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Cache composer line buffers in vkms_output

compose_active_planes() kvmalloc/kvfree two line buffers every vblank
frame, but their size depends only on hdisplay which changes only on
modeset.  Cache them in vkms_output and reallocate only when the width
changes, avoiding repeated alloc/free overhead.

Register a drmm action to free the buffers when the device is released.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..f3fb203946e83 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
[ ... ]
> @@ -565,27 +565,35 @@ static int compose_active_planes(struct vkms_writeback_job *active_wb,
>  	stage_buffer.n_pixels = line_width;
>  	output_buffer.n_pixels = line_width;
>  
> -	stage_buffer.pixels = kvmalloc(line_width * pixel_size, GFP_KERNEL);
> -	if (!stage_buffer.pixels) {
> -		DRM_ERROR("Cannot allocate memory for the output line buffer");
> -		return -ENOMEM;
> -	}
> +	if (out->composer_buffer_width != line_width) {
> +		kvfree(out->composer_stage_buffer);
> +		kvfree(out->composer_output_buffer);
> +		out->composer_buffer_width = 0;
> +
> +		out->composer_stage_buffer = kvmalloc(line_width * pixel_size, GFP_KERNEL);
> +		if (!out->composer_stage_buffer) {
> +			DRM_ERROR("Cannot allocate memory for the output line buffer");
> +			return -ENOMEM;
> +		}

[Severity: High]
If this kvmalloc() fails, does it leave out->composer_output_buffer pointing
to freed memory?

When the line width changes, both buffers are freed. If the allocation for the
stage buffer fails, the function returns -ENOMEM without clearing
out->composer_output_buffer.

Later, during device teardown, vkms_composer_buffers_release() will call
kvfree() on out->composer_output_buffer again. Could this lead to a
double-free?

> +
> +		out->composer_output_buffer = kvmalloc(line_width * pixel_size, GFP_KERNEL);
> +		if (!out->composer_output_buffer) {
> +			DRM_ERROR("Cannot allocate memory for intermediate line buffer");
> +			kvfree(out->composer_stage_buffer);
> +			out->composer_stage_buffer = NULL;
> +			return -ENOMEM;
> +		}
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 079abfba427d0..6e03c9a1fbd82 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
[ ... ]
> @@ -237,5 +245,9 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
>  	if (IS_ERR(vkms_out->composer_workq))
>  		return ERR_CAST(vkms_out->composer_workq);
>  
> +	ret = drmm_add_action_or_reset(dev, vkms_composer_buffers_release, vkms_out);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	return vkms_out;
>  }

[Severity: High]
Does this setup create a use-after-free during device teardown?

The DRM managed resource API executes release actions in LIFO order. Here, the
workqueue is allocated first (which registers its destruction/flush action),
and then the buffer release action is registered.

During teardown, the buffers will be freed first. Then the workqueue is
destroyed, which flushes any pending vkms_composer_worker items. If a worker
is pending and executes compose_active_planes(), won't it access the already
freed buffer memory?


[Severity: High]
This is a pre-existing issue, but I noticed another potential use-after-free
in vkms_atomic_crtc_destroy_state().

When the CRTC state is destroyed, it issues a warning if the composer work is
pending:

    WARN_ON(work_pending(&vkms_state->composer_work));
    kfree(vkms_state->active_planes);
    kfree(vkms_state);

Since it doesn't synchronize or cancel the work (e.g., using
cancel_work_sync()), if the worker executes after this point, won't it
access the freed vkms_state and its planes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-1-oushixiong1025@163.com?part=1

Re: [PATCH] drm/vkms: Fix gamma_lut size check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the vkms_composer_worker accesses the currently committed CRTC state (crtc->state) asynchronously without locks, leading to a Time-of-Check to Time-of-Use (TOCTOU) race condition and Use-After-Free (UAF).
--

--- Patch [1]: [PATCH] drm/vkms: Fix gamma_lut size check ---
commit a52b4df7305f28acb171652e76fbc632383dbfb6
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Fix gamma_lut size check
    
The gamma_lut size check in vkms_atomic_check() uses
sizeof(struct drm_color_lut *) instead of sizeof(struct drm_color_lut).
On 64-bit both happen to be 8 bytes, so the check works by accident.
On 32-bit sizeof(pointer) is 4 bytes while sizeof(struct drm_color_lut)
is 8 bytes, which would incorrectly reject valid 256-entry LUTs.

> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 5a640b531d88f..2db3971f72c31 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -112,7 +112,7 @@ static int vkms_atomic_check(struct drm_device *dev, struct drm_atomic_commit *s
>  		if (!new_crtc_state->gamma_lut || !new_crtc_state->color_mgmt_changed)
>  			continue;
>  
> -		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut *)
> +		if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut)
>  		    > VKMS_LUT_SIZE)
>  			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but since we are looking at the
gamma_lut, is it possible for vkms_composer_worker() to hit a use-after-free
when accessing the LUT blob data?

In drivers/gpu/drm/vkms/vkms_composer.c:vkms_composer_worker():

	if (crtc->state->gamma_lut) {
		s64 max_lut_index_fp;
		s64 u16_max_fp = drm_int2fixp(0xffff);

		crtc_state->gamma_lut.base = (struct drm_color_lut *)crtc->state->gamma_lut->data;
		crtc_state->gamma_lut.lut_length =
			crtc->state->gamma_lut->length / sizeof(struct drm_color_lut);

The worker accesses the currently committed CRTC state (crtc->state)
asynchronously without locks, rather than using its own atomic state snapshot.

If userspace rapidly issues atomic commits that update or remove the CRTC's
gamma LUT, couldn't a concurrent atomic commit swap crtc->state and free
the LUT blob while the worker is executing?

This seems like it could create a large use-after-free window during the
blending phase in compose_active_planes(), or a direct NULL pointer
dereference in the setup phase.

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-2-oushixiong1025@163.com?part=1

Re: [PATCH] drm/vkms: Fix vertical read step for R1/R2/R4 formats

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer overflow in `packed_pixels_offset()` causes a massive out-of-bounds kernel memory read.
--

--- Patch [1]: [PATCH] drm/vkms: Fix vertical read step for R1/R2/R4 formats ---
commit 27fa0b8f7b4814c63740fbad03cf74d10431bd82
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Fix vertical read step for R1/R2/R4 formats

This commit fixes the vertical read step for R1/R2/R4 formats by using the
block height instead of block width to calculate byte offsets. This prevents
out-of-bounds reads and incorrect sampling when rotating or reflecting planes.

> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index dfb8e13cba87a..1e3a6af9cea52 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -103,11 +103,11 @@ static int get_block_step_bytes(struct drm_framebuffer *fb, enum pixel_read_dire
>  	case READ_RIGHT_TO_LEFT:
>  		return -fb->format->char_per_block[plane_index];
>  	case READ_TOP_TO_BOTTOM:
> -		return (int)fb->pitches[plane_index] * drm_format_info_block_width(fb->format,
> -										   plane_index);
> -	case READ_BOTTOM_TO_TOP:
> -		return -(int)fb->pitches[plane_index] * drm_format_info_block_width(fb->format,
> +		return (int)fb->pitches[plane_index] * drm_format_info_block_height(fb->format,
>  										    plane_index);
> +	case READ_BOTTOM_TO_TOP:
> +		return -(int)fb->pitches[plane_index] * drm_format_info_block_height(fb->format,
> +										     plane_index);
>  	}
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential integer
overflow in packed_pixels_offset() in the same file.

If a user configures a framebuffer with a specific height and pitch (e.g.,
height = 3 and pitch = 0x40000000), this satisfies the bounds check in
framebuffer_check(), but might cause an issue during VKMS software composition:

drivers/gpu/drm/vkms/vkms_formats.c:packed_pixels_offset() {
...
	int block_y = y / drm_format_info_block_height(format, plane_index);
	int block_pitch = fb->pitches[plane_index] * drm_format_info_block_height(format,
										  plane_index);
	*rem_x = x % drm_format_info_block_width(format, plane_index);
	*rem_y = y % drm_format_info_block_height(format, plane_index);
	*offset = fb->offsets[plane_index] +
		  block_y * block_pitch +
		  block_x * format->char_per_block[plane_index];
...
}

Since block_pitch and block_y are signed 32-bit integers, could the
multiplication of block_y * block_pitch overflow into a negative value?

This negative offset propagates to *offset, and later in packed_pixels_addr():

drivers/gpu/drm/vkms/vkms_formats.c:packed_pixels_addr() {
...
	packed_pixels_offset(frame_info, x, y, plane_index, &offset, rem_x, rem_y);
	*addr = (u8 *)frame_info->map[0].vaddr + offset;
}

Could adding this signed 32-bit integer offset to the 64-bit virtual address
pointer trigger sign-extension, effectively subtracting a large amount from
the base address and allowing an out-of-bounds kernel memory read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-3-oushixiong1025@163.com?part=1

Re: [PATCH 2/2] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The VKMS asynchronous composer worker accesses the global `colorop->state` directly without locking or using a snapshotted atomic state, leading to Use-After-Free (UAF) and screen tearing.
--

--- Patch [2]: [PATCH 2/2] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed ---
commit 8ff3a905b0e8440dde34c1e49a7de87eb5e0a637
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

This commit skips the per-pixel unpacking and processing loop in
pre_blend_color_transform() when the color pipeline is completely empty
or all color operations are bypassed. It adds a pipeline_all_bypassed()
helper to check the bypass state.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index f3fb203946e83..1806c249da6fc 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
[ ... ]
> @@ -190,13 +190,29 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
>  	}
>  }
>  
> +static bool pipeline_all_bypassed(struct drm_colorop *colorop)
> +{
> +	while (colorop) {
> +		struct drm_colorop_state *colorop_state = colorop->state;

[Severity: High]
This is a pre-existing issue, but does accessing the global colorop->state
locklessly in the async composer worker risk a use-after-free?

If the composer worker reads colorop->state while userspace queues new
non-blocking commits, the following sequence seems possible:

1. The composer worker reads colorop->state (pointing to commit C2's state).
2. The worker is preempted.
3. Userspace queues another commit (C3), making C2 the old state.
4. C3 finishes its tail work and frees C2's state via cleanup_planes().
5. The preempted worker resumes and dereferences the now-freed C2 state.

The existing per-pixel loop in pre_blend_color_transform() in vkms_composer.c
already reads colorop->state directly, so this architectural flaw is
pre-existing. However, this new pipeline_all_bypassed() helper repeats the
pattern. Should the worker be using a snapshotted atomic state instead of
reading the global colorop->state?

> +
> +		if (!colorop_state || !colorop_state->bypass)
> +			return false;
> +		colorop = colorop->next;
> +	}
> +	return true;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-1-oushixiong1025@163.com?part=2