[PATCH 1/2] media: mali-c55: Check the parameters buffer address before copying

[PATCH 1/2] media: mali-c55: Check the parameters buffer address before copying

[David Carlier]

mali_c55_params_buf_prepare() copies the parameters buffer supplied by
userspace into the driver's scratch buffer, using the result of
vb2_plane_vaddr() as the source without checking it. The queue accepts
VB2_DMABUF, and vb2_dc_vaddr() returns NULL for an exporter that cannot
be vmapped, so the memcpy() dereferences NULL.

Reject the buffer at prepare time instead.

Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
Cc: stable@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
index 70106276b7e4..16e895e3488e 100644
--- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
@@ -710,6 +710,9 @@ static int mali_c55_params_buf_prepare(struct vb2_buffer *vb)
 	struct mali_c55 *mali_c55 = params->mali_c55;
 	int ret;
 
+	if (!config)
+		return -EFAULT;
+
 	ret = v4l2_isp_params_validate_buffer_size(mali_c55->dev, vb,
 			v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
 	if (ret)
-- 
2.55.0

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

[David Carlier]

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>
---
 .../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

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

[Jacopo Mondi]

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
>
>

Re: [PATCH 1/2] media: mali-c55: Check the parameters buffer address before copying

[Jacopo Mondi]

Hi David

On Sun, Sep 06, 2026 at 10:24:24AM +0100, David Carlier wrote:
> mali_c55_params_buf_prepare() copies the parameters buffer supplied by
> userspace into the driver's scratch buffer, using the result of
> vb2_plane_vaddr() as the source without checking it. The queue accepts
> VB2_DMABUF, and vb2_dc_vaddr() returns NULL for an exporter that cannot
> be vmapped, so the memcpy() dereferences NULL.
>
> Reject the buffer at prepare time instead.
>
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: David Carlier <devnexen@gmail.com>

Quite a corner case I would say, but it's anyway good to check.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> index 70106276b7e4..16e895e3488e 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> @@ -710,6 +710,9 @@ static int mali_c55_params_buf_prepare(struct vb2_buffer *vb)
>  	struct mali_c55 *mali_c55 = params->mali_c55;
>  	int ret;
>
> +	if (!config)
> +		return -EFAULT;
> +
>  	ret = v4l2_isp_params_validate_buffer_size(mali_c55->dev, vb,
>  			v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
>  	if (ret)
> --
> 2.55.0
>
>