Re: [PATCH v7 09/19] media: meson: vdec: Fix vp9 header update failure on invalid payloads

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Negative error code converted to massive positive integer bypasses validation, leading to out-of-bounds DMA fetch and memory corruption.
- [High] Early return from `esparser_isr` bypasses hardware interrupt clearing, leading to an unrecoverable IRQ storm and system lockup.
- [High] Using `break` inside `scoped_guard` escapes only the guard mechanism, causing an infinite busy-wait loop when the worker queue is empty.

Pre-existing issues:
- [Critical] Implicit integer promotion of VP9 frame sizes allows crafting negative sizes to bypass bounds checks, causing a massive out-of-bounds heap copy.
- [High] esparser worker accesses hardware registers and waitqueues without taking the global lock, causing hardware state corruption across concurrent sessions.
- [High] VP9 frame size validation checks against maximum buffer capacity instead of actual payload size, allowing information disclosure of uninitialized kernel memory.
--

--- Patch [9]: [PATCH v7 09/19] media: meson: vdec: Fix vp9 header update failure on invalid payloads ---
commit e42c61ef1ea850aa0156d3301824f682854a42da
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Fix vp9 header update failure on invalid payloads

Ensure vp9_update_header() returns an explicit error code on invalid or
malformed buffer payloads instead of silently returning zero.

> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -97,11 +97,15 @@ static int vp9_update_header(struct amvdec_core *core, struct vb2_buffer *buf)
>  	unsigned char *old_header = NULL;
>  
>  	dp = (uint8_t *)vb2_plane_vaddr(buf, 0);
> +	if (!dp)
> +		return -EINVAL;

[Severity: Critical]
Does returning a negative error code here cause an issue in the caller?

Looking at esparser_queue(), it assigns the return value of 
vp9_update_header() to payload_size and checks if (payload_size == 0). If 
this returns -EINVAL and the caller assigns the result to a u32 variable, 
could the negative error code underflow to a massive positive integer (like 
4294967274)? 

This might completely bypass validation and be passed to 
esparser_pad_start_code(), causing an out-of-bounds memset and out-of-bounds 
DMA commands to the hardware.

>  	dsize = vb2_get_plane_payload(buf, 0);
>  
> -	if (dsize == vb2_plane_size(buf, 0)) {
> -		dev_warn(core->dev, "%s: unable to update header\n", __func__);
> -		return 0;
> +	if (dsize <= 0 || dsize > vb2_plane_size(buf, 0)) {
> +		dev_warn(core->dev, "%s: invalid payload size %d\n",
> +			 __func__, dsize);
> +		return -EINVAL;
>  	}
>  

[ ... ]

> @@ -109,13 +113,16 @@ static int vp9_update_header(struct amvdec_core *core, struct vb2_buffer *buf)
>  		num_frames = (marker & 0x7) + 1;
>  		mag = ((marker >> 3) & 0x3) + 1;
>  		mag_ptr = dsize - mag * num_frames - 2;
> -		if (dp[mag_ptr] != marker)
> -			return 0;
> +		if (mag_ptr < 0 || dp[mag_ptr] != marker)
> +			return -EINVAL;
>  
>  		mag_ptr++;
>  		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));

[Severity: Critical]
This isn't a bug introduced by this patch, but does this shift operation 
cause an implicit sign extension?

Because dp is a pointer to u8, the byte at dp[mag_ptr] undergoes integer 
promotion to a signed int before shifting. If cur_mag is 3 and the byte is 
128 or larger, shifting it by 24 bits produces a negative value due to the 
sign bit.

Could this allow crafting combinations of large positive and negative 
frame_size values so their sum wraps around to a small positive integer, 
bypassing the bounds check later on and causing a massive out-of-bounds 
memmove()?

>  				mag_ptr++;
>  			}

[ ... ]

> @@ -140,7 +147,7 @@ static int vp9_update_header(struct amvdec_core *core, struct vb2_buffer *buf)
>  
>  	if (new_frame_size >= vb2_plane_size(buf, 0)) {
>  		dev_warn(core->dev, "%s: unable to update header\n", __func__);
> -		return 0;
> +		return -ENOMEM;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does validating against the 
total buffer capacity instead of the actual payload size allow information 
disclosure?

If the check uses vb2_plane_size(buf, 0) instead of the actual received 
payload length in dsize, could a valid short payload with a crafted 
superframe index declare frame sizes that exceed the payload length but 
remain under the total buffer capacity?

If so, memmove() could copy trailing uninitialized or stale kernel memory 
from the buffer into the active payload stream.


Outside of the immediate diff, there are a few other questions about the 
esparser implementation that may be worth reviewing:

[Severity: High]
This isn't a bug introduced by this patch, but does esparser_queue_all_src() 
queue hardware operations without acquiring the global hardware lock?

The worker runs concurrently for multiple sessions because the work queue is 
initialized per-session. The worker locks the per-session sess->lock but 
does not appear to acquire the global core->lock or verify hardware ownership 
before calling esparser_write_data():

esparser_queue_all_src() {
    ...
    ret = esparser_queue(sess, vbuf);
    ...
}

Could multiple sessions invoke esparser_write_data() simultaneously, causing 
conflicting writes to the hardware parser registers and corruption of the 
global search_done waitqueue flag?

[Severity: High]
Could the early return in esparser_isr() bypass clearing the hardware 
interrupt?

The interrupt service routine starts with a check:

esparser_isr() {
    if (!sess)
        return IRQ_HANDLED;
    ...
}

If a hardware interrupt asserts just as the session is torn down, returning 
early leaves the hardware interrupt un-cleared. Would this cause the 
hardware to hold the interrupt line asserted indefinitely, leading to an 
unrecoverable IRQ storm and system lockup?

[Severity: High]
Does the break statement inside esparser_queue_all_src() properly exit the 
worker loop?

When checking if a buffer is available inside a scoped_guard():

esparser_queue_all_src() {
    ...
    vbuf = v4l2_m2m_next_src_buf(sess->m2m_ctx);
    if (!vbuf)
        break;
    ...
}

Since scoped_guard() is implemented internally as a for loop, breaking here 
would only escape the guard's inner loop. If processed_frame remains false, 
the outer while (1) loop proceeds past its own exit condition:

    if (processed_frame)
        break;
    /* Give other threads and IRQ routines a window ... */
    cond_resched();

Could this pin a CPU core at 100% in an infinite busy-wait loop when the 
queue is empty?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713120840.17427-1-linux.amoon@gmail.com?part=9

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic