Re: [PATCH v7 16/19] media: meson: vdec: Configure DMA mask and segment size in probe

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

New issues:
- [High] Hardware interrupts are not masked prior to calling `synchronize_irq()`, leading to a race condition that causes a Use-After-Free of `sess->priv`.
- [High] Misuse of `scoped_guard` inside a `while (1)` loop combined with inverted logic causes the workqueue to busy-wait infinitely when no data is available.
- [Medium] Missing release barriers when publishing `core->cur_sess` allows the lockless ISR to observe an uninitialized or stale session state.

Pre-existing issues:
- [Critical] vp9_update_header trusts user-provided frame sizes to calculate offsets, leading to integer overflows and massive out-of-bounds memory accesses via memmove.
- [High] `meson_canvas_get` retrieves a pointer to the provider's private data but immediately drops its reference to the provider device, leading to a Use-After-Free if the provider driver is unbound.
- [Medium] Potential NULL pointer dereference if the driver is probed without a device tree node.
--

--- Patch [16]: [PATCH v7 16/19] media: meson: vdec: Configure DMA mask and segment size in probe ---
commit 8dd369d994b7e2a6a1fb0db5dcba10a32f46beaa
Author: Anand Moon <[email protected]>

media: meson: vdec: Configure DMA mask and segment size in probe

This commit addresses missing explicit DMA constraints in the vdec probe
routine. It enforces a 32-bit coherent DMA allocation window and maximizes
the contiguous allocation segment boundary constraint to UINT_MAX.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -1186,6 +1186,16 @@ static int vdec_probe(struct platform_device *pdev)
>  	if (IS_ERR(core->canvas))
>  		return PTR_ERR(core->canvas);
>  
> +	/* Enforce strict 32-bit DMA limit to match hardware capabilities */
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set 32-bit DMA mask\n");
> +
> +	ret = vb2_dma_contig_set_max_seg_size(dev, UINT_MAX);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to set DMA max segment size\n");
> +
>  	of_id = of_match_node(vdec_dt_match, dev->of_node);
>  	core->platform = of_id->data;

[Severity: Medium]
This isn't a bug introduced by this patch, but can this lead to a NULL pointer
dereference if the driver is probed without a device tree node? If a user
with privileges binds the driver manually via sysfs without an associated
device tree node, dev->of_node is NULL. The of_match_node() call will
return NULL, causing of_id->data to be dereferenced blindly.

[Severity: High]
Can this regression cause a use-after-free of sess->priv in the interrupt
handler?

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
...
		/* Synchronize and flush pending hardware interrupt service routines */
		synchronize_irq(core->vdec_irq);
		/* Ensure esparser ISR finishes executing */
		synchronize_irq(core->esparser_irq);

		cancel_work_sync(&sess->esparser_queue_work);
		mutex_lock(&core->lock);

		if (core->cur_sess == sess) {
			vdec_poweroff(sess);
...
}

It appears hardware interrupts are not masked prior to calling
synchronize_irq(), which could allow an interrupt to fire immediately
after synchronize_irq() returns. Because vdec_poweroff() is responsible
for masking the hardware interrupts, could the main thread then free
sess->priv while the concurrently executing ISR dereferences it?

[Severity: High]
Does this loop correctly process the buffer queue or does it spin infinitely?

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
...
	while (1) {
		struct vb2_v4l2_buffer *vbuf = NULL;
		bool processed_frame = false;

		scoped_guard(mutex, &sess->lock) {
			/* Safe atomic tracking check: exit loop if session is shutting down */
			if (READ_ONCE(sess->should_stop))
				return;

			/* Queue completely empty: exit work loop cleanly */
			vbuf = v4l2_m2m_next_src_buf(sess->m2m_ctx);
			if (!vbuf)
				break;
...
}

When v4l2_m2m_next_src_buf() returns NULL, the break statement seems to only
exit the inner for loop created by the scoped_guard macro. This bypasses the
subsequent loop exit conditions and repeats the while (1) loop endlessly,
potentially causing a CPU lockup on the workqueue thread. Additionally, if a
buffer is present, could this break out of the loop completely instead of
processing the queue?

[Severity: Medium]
Can the lockless ISR observe an uninitialized or stale session state due to
missing release barriers?

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
...
	/* If already half-initialized, do not re-initialize */
	if (sess->status == STATUS_INIT) {
		mutex_unlock(&core->lock);
		return 0;
	}

	sess->status = STATUS_INIT;
	core->cur_sess = sess;
	mutex_unlock(&core->lock);
...
}

Because core->cur_sess is assigned using a plain store, could the compiler or
CPU reorder this store before earlier initialization steps like
sess->status = STATUS_INIT? If the lockless ISR uses smp_load_acquire(), it
might observe the new session pointer but the old memory state.

[Severity: High]
This is a pre-existing issue, but does this code cause a use-after-free if the
provider driver is unbound?

drivers/soc/amlogic/meson-canvas.c:meson_canvas_get() {
...
	canvas_pdev = of_find_device_by_node(canvas_node);
	of_node_put(canvas_node);
	if (!canvas_pdev)
		return ERR_PTR(-EPROBE_DEFER);
...
	canvas = dev_get_drvdata(&canvas_pdev->dev);
	put_device(&canvas_pdev->dev);
	if (!canvas)
		return ERR_PTR(-EINVAL);

	return canvas;
}

The put_device() call immediately drops the device reference before returning
the pointer. Since no device link is maintained, could an admin or
unprivileged user with sysfs access unbind the amlogic-canvas driver,
freeing the memory while the vdec driver still holds and uses the
dangling pointer?

[Severity: Critical]
This is a pre-existing issue, but does this size calculation prevent integer
overflows and out-of-bounds memory accesses?

drivers/staging/media/meson/vdec/esparser.c:vp9_update_header() {
...
		for (cur_frame = 0; cur_frame < num_frames; cur_frame++) {
			frame_size[cur_frame] = 0;
			for (cur_mag = 0; cur_mag < mag; cur_mag++) {
				if (mag_ptr >= dsize)
					return -EINVAL;

				frame_size[cur_frame] |=
					(dp[mag_ptr] << (cur_mag * 8));
				mag_ptr++;
			}
			if (cur_frame == 0)
				tot_frame_size[cur_frame] =
					frame_size[cur_frame];
			else
				tot_frame_size[cur_frame] =
					tot_frame_size[cur_frame - 1] +
					frame_size[cur_frame];
			total_datasize += frame_size[cur_frame];
		}
...
}

It appears user-provided frame sizes are added blindly without overflow
protection. If a user crafts a V4L2 VP9 buffer payload with two frames
specifying sizes that sum to 0 (e.g., 0x40000000 and 0xC0000000), could
this bypass the new_frame_size bounds check? The driver might then use
one of the huge frame sizes in a memmove operation, leading to
out-of-bounds reads and writes on the kernel heap.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic