Re: [PATCH 2/2] media: qcom: camss: vfe-17x: silence wm-done IRQ on unmapped WMs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving the `wm_output_map` validation outside the `output_lock` spinlock introduces a TOCTOU race condition against stream stopping, leading to Use-After-Free and double vb2 buffer completions.

Pre-existing issues:
- [High] Pre-existing bug in `vfe_buf_done` where an out-of-bounds line pointer is computed locklessly and later dereferenced.
--

commit 98cdb54eb2a601721b4658e4c2704449a2aa63dd
Author: Herman van Hazendonk <github.com@herrie.org>

media: qcom: camss: vfe-17x: silence wm-done IRQ on unmapped WMs

This commit aims to silence a rate-limited error message when handling
Write Master IRQs for unmapped indices. It moves the check for VFE_LINE_NONE
outside of the output_lock spinlock, intending to return early and avoid log
spam.

> vfe_isr_wm_done() handles the per-write-master "frame complete" IRQ.
> On VFE 1.7x configurations where a Write Master is allocated to a
> secondary purpose (e.g. statistics or DMI buffers) it is not mapped
> to any output line, so vfe->wm_output_map[wm] is VFE_LINE_NONE.
> 
> Today the function takes the output spinlock first and only then
> checks for VFE_LINE_NONE, emitting a rate-limited "Received wm done
> for unmapped index" error each time. The error is harmless but it
> spams dmesg when a stats path is configured.
> 
> Move the VFE_LINE_NONE test in front of the spinlock and return
> silently: an unmapped WM simply has nothing to deliver to userspace.
> While at it, hoist the wm_output_map[] dereference into a local for
> readability.
> 
> No functional change for mapped Write Masters.
> 
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> --- a/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> @@ -507,21 +507,36 @@ static void vfe_isr_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
>   */
>  static void vfe_isr_wm_done(struct vfe_device *vfe, u8 wm)
>  {
> -	struct vfe_line *line = &vfe->line[vfe->wm_output_map[wm]];

[Severity: High]
This isn't a bug introduced by this patch, but a similar pattern exists in
vfe_buf_done() in drivers/media/platform/qcom/camss/camss-vfe.c:

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_buf_done()
	struct vfe_line *line = &vfe->line[vfe->wm_output_map[wm]];
	...
	spin_lock_irqsave(&vfe->output_lock, flags);

	if (vfe->wm_output_map[wm] == VFE_LINE_NONE) {
        ...

If wm_output_map[wm] is initially VFE_LINE_NONE (-1), line is assigned an
out-of-bounds pointer locklessly. If a concurrent thread maps the WM to a
valid index before vfe_buf_done() acquires output_lock, the internal check
passes. The function will then pass the stale, out-of-bounds line pointer
to ops->vfe_wm_update(), which dereferences it. Should this also be updated
to prevent reading from an out-of-bounds pointer?

> +	struct vfe_line *line;
>  	struct camss_buffer *ready_buf;
>  	struct vfe_output *output;
>  	unsigned long flags;
> +	int wm_output;
>  	u32 index;
>  	u64 ts = ktime_get_ns();
>  
> +	/*
> +	 * Some VFE modes route data to secondary WMs that aren't mapped
> +	 * to output lines. Silently ignore their IRQs.
> +	 *
> +	 * vfe->wm_output_map[wm] is written from the stream stop/start
> +	 * paths under output_lock held as a mutex; this ISR runs in
> +	 * atomic context and cannot take it. Snapshot the value once
> +	 * with READ_ONCE() so the check below and the array index that
> +	 * follows operate on the same value: otherwise a concurrent
> +	 * write of VFE_LINE_NONE (-1) between the two loads would let
> +	 * the function pass the check and then dereference
> +	 * &vfe->line[-1].
> +	 */
> +	wm_output = READ_ONCE(vfe->wm_output_map[wm]);
> +	if (wm_output == VFE_LINE_NONE)
> +		return;
> +
> +	line = &vfe->line[wm_output];
> +
>  	spin_lock_irqsave(&vfe->output_lock, flags);
>  
> -	if (vfe->wm_output_map[wm] == VFE_LINE_NONE) {
> -		dev_err_ratelimited(vfe->camss->dev,
> -				    "Received wm done for unmapped index\n");
> -		goto out_unlock;
> -	}
> -	output = &vfe->line[vfe->wm_output_map[wm]].output;
> +	output = &line->output;

[Severity: High]
Does moving this check outside the output_lock introduce a race condition with
stream stopping?

The new comment mentions the ISR runs in atomic context and cannot take the
lock, but the ISR proceeds to acquire output_lock immediately after this check.

If the ISR locklessly reads a valid wm_output here, but stalls before acquiring
the output_lock:

CPU1
vfe_isr_wm_done()
   wm_output = READ_ONCE(vfe->wm_output_map[wm]);

CPU2 (stream teardown)
vfe_put_output()
   spin_lock_irqsave(&vfe->output_lock, flags);
   vfe->wm_output_map[wm] = VFE_LINE_NONE;
   vfe_flush_buffers(vfe, &line->output);
   spin_unlock_irqrestore(&vfe->output_lock, flags);

CPU1
   spin_lock_irqsave(&vfe->output_lock, flags);
   output = &line->output;
   ready_buf = output->buf[0];

vfe_flush_buffers() completes all buffers via vb2_buffer_done() but leaves
output->buf[0] non-NULL. When CPU1 resumes and acquires the lock, it will no
longer re-verify wm_output_map. It then modifies the already completed buffer
(ready_buf->vb.vb2_buf.timestamp = ts) and double-completes it via
vb2_buffer_done(), which would corrupt the vb2 state.

>  	ready_buf = output->buf[0];
>  	if (!ready_buf) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603054238.562902-1-github.com@herrie.org?part=2