Re: [PATCH 2/2] media: mali-c55: Check the statistics buffer address before filling

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi David

On Sun, Sep 06, 2026 at 10:24:25AM +0100, David Carlier wrote:
> mali_c55_stats_cpu_read() copies the metering registers into the buffer
> returned by vb2_plane_vaddr() without checking it. As for the parameters
> queue, a DMABUF whose exporter cannot be vmapped yields NULL, and the
> memcpy_fromio() then dereferences it from the threaded interrupt
> handler.
>
> Report the failure to mali_c55_stats_fill_buffer() and complete the
> buffer with VB2_BUF_STATE_ERROR.
>
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: David Carlier <devnexen@gmail.com>

Same as the previous one
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  .../platform/arm/mali-c55/mali-c55-stats.c     | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-stats.c b/drivers/media/platform/arm/mali-c55/mali-c55-stats.c
> index 655e52a5f288..a9c28b090b2a 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-stats.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-stats.c
> @@ -200,9 +200,9 @@ static const struct vb2_ops mali_c55_stats_vb2_ops = {
>  	.stop_streaming = mali_c55_stats_stop_streaming,
>  };
>
> -static void mali_c55_stats_cpu_read(struct mali_c55_stats *stats,
> -				    struct mali_c55_stats_buf *buf,
> -				    enum mali_c55_config_spaces cfg_space)
> +static int mali_c55_stats_cpu_read(struct mali_c55_stats *stats,
> +				   struct mali_c55_stats_buf *buf,
> +				   enum mali_c55_config_spaces cfg_space)
>  {
>  	struct mali_c55 *mali_c55 = stats->mali_c55;
>  	const void __iomem *src;
> @@ -211,12 +211,18 @@ static void mali_c55_stats_cpu_read(struct mali_c55_stats *stats,
>
>  	src = mali_c55->base + MALI_C55_REG_1024BIN_HIST;
>  	dst = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
> +
> +	if (!dst)
> +		return -EFAULT;
> +
>  	memcpy_fromio(dst, src, MALI_C55_1024BIN_HIST_SIZE);
>
>  	src = mali_c55->base + metering_space_addrs[cfg_space];
>  	dst += MALI_C55_1024BIN_HIST_SIZE;
>  	length = sizeof(struct mali_c55_stats_buffer) - MALI_C55_1024BIN_HIST_SIZE;
>  	memcpy_fromio(dst, src, length);
> +
> +	return 0;
>  }
>
>  void mali_c55_stats_fill_buffer(struct mali_c55 *mali_c55,
> @@ -224,6 +230,7 @@ void mali_c55_stats_fill_buffer(struct mali_c55 *mali_c55,
>  {
>  	struct mali_c55_stats *stats = &mali_c55->stats;
>  	struct mali_c55_stats_buf *buf = NULL;
> +	int ret;
>
>  	spin_lock(&stats->buffers.lock);
>  	if (!list_empty(&stats->buffers.queue)) {
> @@ -239,8 +246,9 @@ void mali_c55_stats_fill_buffer(struct mali_c55 *mali_c55,
>  	buf->vb.sequence = mali_c55->isp.frame_sequence;
>  	buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>
> -	mali_c55_stats_cpu_read(stats, buf, cfg_space);
> -	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +	ret = mali_c55_stats_cpu_read(stats, buf, cfg_space);
> +	vb2_buffer_done(&buf->vb.vb2_buf,
> +			ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>  }
>
>  void mali_c55_unregister_stats(struct mali_c55 *mali_c55)
> --
> 2.55.0
>
>