Re: [PATCH v3 3/5] media: rkisp2: Add statistics capture video node

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing V4L2 block type info for big histogram 2, causing an infinite loop in userspace parsers.
- [High] Unacknowledged hardware 3A interrupts cause an IRQ storm when no stats buffers are queued.
- [High] Return value of rkisp2_stats_init_vb2_queue() is ignored.
- [High] mutex_destroy() is called while file descriptors might still be open, leading to Use-After-Free.
- [High] Unprotected initialization of stats->icr in start_streaming races with the ISR.
- [High] Measurement window dimensions are not clamped in the statistics configuration handlers, which can cause ISP hardware hangs.
- [Medium] Uninitialized stats block exposes stale memory to userspace when hardware status bits are unset.
- [Medium] Truncated programming of big histogram weights.
- [Medium] Missing SRAM/WRAM address pointer reset before reading or writing AWB statistics and parameters.

Pre-existing issues:
- [Critical] Time-of-check to time-of-use (TOCTOU) vulnerability in parameters buffer validation.
--

commit a35565a8e85636bb5290bad9e33a1d9d66a1f24a
Author: Paul Elder <paul.elder@ideasonboard.com>

media: rkisp2: Add statistics capture video node

This commit adds support for capturing statistics out of the ISP by dequeueing
statistics buffers from rkisp2. Support for configuring the statistics engines
in the ISP is also added, implemented as configurations in parameter buffers.

> diff --git a/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c b/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c
> --- a/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c
> +++ b/drivers/media/platform/rockchip/rkisp2/rkisp2-params.c
[ ... ]
> @@ -84,6 +100,12 @@ rkisp2_params_handlers[] = {
>  	RKISP2_PARAMS_BLOCK_HANDLER_INFO(GOC, goc, PRE),
>  	RKISP2_PARAMS_BLOCK_HANDLER_INFO(LSC, lsc, POST),
>  	RKISP2_PARAMS_BLOCK_HANDLER_INFO(CROP, crop, PRE),
> +	RKISP2_PARAMS_BLOCK_HANDLER_INFO(AE_LITE, ae_lite, PRE),
> +	RKISP2_PARAMS_BLOCK_HANDLER_INFO(HIST_LITE, hist_lite, PRE),
> +	RKISP2_PARAMS_BLOCK_HANDLER_INFO(HIST_BIG0, hist_big, PRE),
> +	RKISP2_PARAMS_BLOCK_HANDLER_INFO(HIST_BIG1, hist_big, PRE),
> +	RKISP2_PARAMS_BLOCK_HANDLER_INFO(HIST_BIG2, hist_big, PRE),
> +	RKISP2_PARAMS_BLOCK_HANDLER_INFO(AWB_MEAS, awb_meas, PRE),
>  };

[Severity: Critical]
This is a pre-existing issue, but can malicious userspace bypass block validation to trigger an out-of-bounds read on the rkisp2_params_handlers array?

In rkisp2_params_vb2_buf_prepare(), the buffer is copied to params_buf->cfg, but v4l2_isp_params_validate_buffer() is mistakenly called on the userspace-accessible buffer (cfg) instead of the snapshot. 

If userspace concurrently changes block.header->type in the shared memory after memcpy() completes, rkisp2_params_configure() will read the unvalidated type from the snapshot, resulting in an out-of-bounds index when accessing info = &rkisp2_params_handlers[block.header->type] and potentially executing an arbitrary function pointer.

[ ... ]
> +static void rkisp2_params_ae_lite(struct rkisp2_params *params,
> +				  union rkisp2_params_block block)
> +{
> +	const struct rkisp2_params_ae_lite *arg = block.ae_lite;
> +	u32 control;
[ ... ]
> +	rkisp2_write(params->rkisp2, ISP_RAWAE_LITE_BLK_SIZ,
> +		     ISP3X_RAWAE_LITE_H(arg->meas_window.h_size) |
> +		     ISP3X_RAWAE_LITE_V(arg->meas_window.v_size));

[Severity: High]
Is it safe to write these user-controlled window sizes directly to the ISP registers without bounds checking? If a userspace program provides extremely large dimensions that exceed the sensor frame size, could it cause the ISP hardware to hang indefinitely while waiting for lines that will never arrive?

[ ... ]
> +static void rkisp2_params_hist_big(struct rkisp2_params *params,
> +				   union rkisp2_params_block block)
> +{
[ ... ]
> +	for (i = 0; i < 16; i++) {
> +		rkisp2_write(params->rkisp2, reg_base + ISP_RAWHIST_BIG_WEIGHT_BASE + 4 * i,
> +			     ISP_RAWHIST_BIG_WEIGHT_WND0(arg->weights[5 * i + 0]) |
> +			     ISP_RAWHIST_BIG_WEIGHT_WND1(arg->weights[5 * i + 1]) |
> +			     ISP_RAWHIST_BIG_WEIGHT_WND2(arg->weights[5 * i + 2]) |
> +			     ISP_RAWHIST_BIG_WEIGHT_WND3(arg->weights[5 * i + 3]) |
> +			     ISP_RAWHIST_BIG_WEIGHT_WND4(arg->weights[5 * i + 4]));
> +	}

[Severity: Medium]
Is this loop intentionally truncated? The rkisp2_params_hist_big struct defines the weights array size as RKISP2_ISP_HIST_WEIGHT_GRIDS_SIZE_BIG (which is 225), but this loop only runs 16 times, writing 80 weights. Could this leave 145 weights unprogrammed and corrupt the histogram output?

[ ... ]
> +static void rkisp2_params_awb_meas(struct rkisp2_params *params,
> +				   union rkisp2_params_block block)
> +{
[ ... ]
> +	for (i = 0; i < RKISP2_ISP_AWB_COUNTS_SIZE / 5; i++) {
> +		val = (arg->weights[5 * i] & 0x3f)
> +		    | ((arg->weights[5 * i + 1] & 0x3f) << 6)
> +		    | ((arg->weights[5 * i + 2] & 0x3f) << 12)
> +		    | ((arg->weights[5 * i + 3] & 0x3f) << 18)
> +		    | ((arg->weights[5 * i + 4] & 0x3f) << 24);
> +		rkisp2_write(params->rkisp2, ISP21_RAWAWB_WRAM_DATA_BASE, val);
> +	}

[Severity: Medium]
Should the internal WRAM address pointer be reset before writing these values? Since data is written sequentially to ISP21_RAWAWB_WRAM_DATA_BASE, the pointer might start at whatever leftover value it had from previous operations if ISP21_RAWAWB_RAM_CTRL isn't cleared first, potentially misaligning the parameters.

> diff --git a/drivers/media/platform/rockchip/rkisp2/rkisp2-stats.c b/drivers/media/platform/rockchip/rkisp2/rkisp2-stats.c
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkisp2/rkisp2-stats.c
> @@ -0,0 +1,509 @@
[ ... ]
> +static const struct v4l2_isp_stats_block_type_info
> +rkisp2_stats_blocks_info[] = {
> +	RKISP2_STATS_BLOCK_INFO(AE_LITE, ae_lite),
> +	RKISP2_STATS_BLOCK_INFO(HIST_LITE, hist),
> +	RKISP2_STATS_BLOCK_INFO(HIST_BIG0, hist),
> +	RKISP2_STATS_BLOCK_INFO(HIST_BIG1, hist),
> +	RKISP2_STATS_BLOCK_INFO(HIST_BIG1, hist),
> +	RKISP2_STATS_BLOCK_INFO(AWB, awb),
> +};

[Severity: High]
Is the duplicate HIST_BIG1 entry here a typo? It looks like HIST_BIG2 is missing, which leaves its block type uninitialized and implicitly sized at 0. 

If rkisp2_stats_hist_read() processes RKISP2_STATS_BLOCK_HIST_BIG2, would adding a block with a size of 0 cause userspace parsers to spin in an infinite loop since their block offset logic will never advance?

[ ... ]
> +static int rkisp2_stats_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> +	struct rkisp2_stats *stats = vq->drv_priv;
> +
> +	stats->icr = 0;
> +	stats->cur_buf = NULL;
> +	stats->awb_window_offset = 0;
> +
> +	return 0;
> +}

[Severity: High]
Does initializing stats->icr to 0 here need to be protected by stats->lock?

If userspace calls VIDIOC_STREAMON while the ISP is already running and generating EOF interrupts, could this race with rkisp2_stats_isr_eof()? This might clobber the bits accumulated by the ISR, causing it to write 0 to ISP_ISP3A_ICR and fail to clear the pending hardware interrupts, leading to an IRQ storm.

[ ... ]
> +static void rkisp2_stats_get_rawae_lite(struct rkisp2_stats *stats,
> +					u32 status,
> +					struct v4l2_isp_buffer *pbuf)
> +{
[ ... ]
> +	block = rkisp2_init_stats_block(rkisp2, pbuf,
> +					RKISP2_STATS_BLOCK_AE_LITE);
> +	if (IS_ERR(block))
> +		return;
> +
> +	ae_lite = &block->ae_lite;
> +
> +	/* TODO figure out what the other channels are for */
> +	if (!(status & ISP2X_3A_RAWAE_CH0))
> +		return;

[Severity: Medium]
Could this early return expose uninitialized kernel memory to userspace?

Since rkisp2_init_stats_block() allocates the block and increments buf->data_size, returning here without filling the payload or initializing the done flag might cause userspace to consume stale or poisoned statistics data from the buffer. This same pattern appears to be present in rkisp2_stats_get_rawawb() below.

[ ... ]
> +static void rkisp2_stats_get_rawawb(struct rkisp2_stats *stats,
> +				    u32 status,
> +				    struct v4l2_isp_buffer *pbuf)
> +{
[ ... ]
> +	for (i = 0; i < RKISP2_ISP_AWB_COUNTS_SIZE; i++) {
> +		val1 = rkisp2_read(rkisp2, ISP21_RAWAWB_RAM_DATA_BASE);
> +		val2 = rkisp2_read(rkisp2, ISP21_RAWAWB_RAM_DATA_BASE);
> +		awb->counts_r[i] = (val2 >> 4) & 0x3ffff;
> +		awb->counts_g[i] = (val1 >> 18) | ((val2 & 0xf) << 14);
> +		awb->counts_b[i] = val1 & 0x3ffff;
> +		awb->counts_w[i] = val2 >> 22;
> +	}

[Severity: Medium]
Should the internal address pointer be reset before reading these AWB counts? It appears this reads from the data port sequentially without first writing 0 to ISP21_RAWAWB_RAM_CTRL, which might cause the read pointer to start at a leftover value from previous operations and corrupt the stats output.

[ ... ]
> +static void rkisp2_stats_send_measurement_3a(struct rkisp2_stats *stats, u32 status)
> +{
> +	struct v4l2_isp_buffer *cur_stat_buf;
> +	struct rkisp2_buffer *cur_buf;
> +
> +	cur_buf = rkisp2_stats_get_buf(stats);
> +	if (!cur_buf)
> +		return;

[Severity: High]
If no V4L2 stats buffers are queued, does this early return bypass the hardware interrupt acknowledgment routine?

If the ISP is running and asserting 3A interrupts on EOF, skipping the write to ISP_ISP3A_ICR could leave the IRQ line asserted indefinitely, resulting in an infinite IRQ storm and system hang.

[ ... ]
> +int rkisp2_stats_register(struct rkisp2_device *rkisp2)
> +{
[ ... ]
> +	vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> +	vdev->vfl_dir =  VFL_DIR_RX;
> +	rkisp2_stats_init_vb2_queue(vdev->queue, stats);
> +	rkisp2_init_stats(stats);
> +	video_set_drvdata(vdev, stats);

[Severity: High]
Is it safe to ignore the return value of rkisp2_stats_init_vb2_queue() here? If the queue initialization fails due to memory pressure, allowing the registration to proceed could register the video node with a broken vb2_queue, potentially leading to crashes when queue ioctls are invoked.

[ ... ]
> +void rkisp2_stats_unregister(struct rkisp2_device *rkisp2)
> +{
> +	struct rkisp2_stats *stats = &rkisp2->stats;
> +	struct rkisp2_vdev_node *node = &stats->vnode;
> +	struct video_device *vdev = &node->vdev;
> +
> +	if (!stats->rkisp2)
> +		return;
> +
> +	vb2_video_unregister_device(vdev);
> +	media_entity_cleanup(&vdev->entity);
> +	mutex_destroy(&node->vlock);
> +}

[Severity: High]
Can destroying the mutex here lead to a use-after-free? 

The mutex_destroy() is being called synchronously during device unregistration. If a userspace process still holds an open file descriptor to the stats node, it could later call vb2_fop_release() upon closing the file descriptor, which will attempt to acquire this destroyed mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-rkisp2-v3-0-e91ff0a604d6@ideasonboard.com?part=3