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 <[email protected]> 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/[email protected]?part=9
