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
