[PATCH v1 0/2] media: atomisp: harden and clean up isp configuration

[PATCH v1 0/2] media: atomisp: harden and clean up isp configuration

[Jose A. Perez de Azpillaga]

This series addresses stability issues and technical debt within the
ISP configuration path of the AtomISP driver.

Jose A. Perez de Azpillaga (2):
  media: atomisp: fix potential NULL pointer dereference in
    configure_isp_from_args()
  media: atomisp: remove redundant call to ia_css_output0_configure()

 drivers/staging/media/atomisp/pci/sh_css_sp.c | 48 +++++++++++--------
 1 file changed, 28 insertions(+), 20 deletions(-)

--
2.53.0

[PATCH v1 1/2] media: atomisp: fix potential NULL pointer dereference in configure_isp_from_args()

[Jose A. Perez de Azpillaga]

The function configure_isp_from_args() incorrectly dereferences
args->delay_frames[0] to configure cropping without checking if the
pointer is valid. However, as noted in a FIXME comment later in the
same function, delay_frames can be NULL in certain pipeline
configurations.

Add defensive checks for both delay_frames and tnr_frames before passing
them to their respective configuration functions. This ensures that
optional frames are only processed if they were actually allocated,
preventing a kernel NULL pointer dereference.

Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css_sp.c | 44 ++++++++++++-------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index 6da151e7a873..2904455b35f7 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -775,9 +775,17 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 	ret = ia_css_fpn_configure(binary,  &binary->in_frame_info);
 	if (ret)
 		return ret;
-	ret = ia_css_crop_configure(binary, ia_css_frame_get_info(args->delay_frames[0]));
-	if (ret)
-		return ret;
+
+	/*
+	 * Only configure crop if delay_frames are present. Accessing
+	 * delay_frames[0] without this check would result in a NULL deference.
+	 */
+	if (args->delay_frames[0]) {
+		ret = ia_css_crop_configure(binary, ia_css_frame_get_info(args->delay_frames[0]));
+		if (ret)
+			return ret;
+	}
+
 	ret = ia_css_qplane_configure(pipeline, binary, &binary->in_frame_info);
 	if (ret)
 		return ret;
@@ -808,21 +816,23 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 		return ret;
 
 	/*
-	 * FIXME: args->delay_frames can be NULL here
-	 *
-	 * Somehow, the driver at the Intel Atom Yocto tree doesn't seem to
-	 * suffer from the same issue.
-	 *
-	 * Anyway, the function below should now handle a NULL delay_frames
-	 * without crashing, but the pipeline should likely be built without
-	 * adding it at the first place (or there are a hidden bug somewhere)
+	 * Safely handle pipelines built without delay_frames
 	 */
-	ret = ia_css_ref_configure(binary, args->delay_frames, pipeline->dvs_frame_delay);
-	if (ret)
-		return ret;
-	ret = ia_css_tnr_configure(binary, args->tnr_frames);
-	if (ret)
-		return ret;
+	if (args->delay_frames[0]) {
+		ret = ia_css_ref_configure(binary, args->delay_frames, pipeline->dvs_frame_delay);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Safely handle TNR frames as well
+	 */
+	if (args->tnr_frames[0]) {
+		ret = ia_css_tnr_configure(binary, args->tnr_frames);
+		if (ret)
+			return ret;
+	}
+
 	return ia_css_bayer_io_config(binary, args);
 }
 
-- 
2.53.0

Re: [PATCH v1 1/2] media: atomisp: fix potential NULL pointer dereference in configure_isp_from_args()

[Andy Shevchenko]

On Sat, Mar 28, 2026 at 9:27 PM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
>
> The function configure_isp_from_args() incorrectly dereferences
> args->delay_frames[0] to configure cropping without checking if the
> pointer is valid. However, as noted in a FIXME comment later in the
> same function, delay_frames can be NULL in certain pipeline
> configurations.
>
> Add defensive checks for both delay_frames and tnr_frames before passing
> them to their respective configuration functions. This ensures that
> optional frames are only processed if they were actually allocated,
> preventing a kernel NULL pointer dereference.

Have you experienced bugs IRL?

...

>         /*
> -        * FIXME: args->delay_frames can be NULL here
> -        *
> -        * Somehow, the driver at the Intel Atom Yocto tree doesn't seem to
> -        * suffer from the same issue.
> -        *
> -        * Anyway, the function below should now handle a NULL delay_frames
> -        * without crashing, but the pipeline should likely be built without
> -        * adding it at the first place (or there are a hidden bug somewhere)
> +        * Safely handle pipelines built without delay_frames
>          */

This comment suggests something different. What the proposed change is
doing is just skipping the invalid data without actual understanding
of the root cause.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] media: atomisp: fix potential NULL pointer dereference in configure_isp_from_args()

[Dan Carpenter]

On Sat, Mar 28, 2026 at 08:21:37PM +0100, Jose A. Perez de Azpillaga wrote:
> The function configure_isp_from_args() incorrectly dereferences
> args->delay_frames[0] to configure cropping without checking if the
> pointer is valid. However, as noted in a FIXME comment later in the
> same function, delay_frames can be NULL in certain pipeline
> configurations.

The comment comes later in the function and it says "FIXME:
args->delay_frames can be NULL here".  "args->delay_frames" is
different from "args->delay_frames[0]".  Obviously
args->delay_frames can't actually be NULL there since we
dereference it here so the comment is wrong.

If the correct response to the FIXME were just to add a NULL
check then the original author probably would have done that.

regards,
dan carpenter

[PATCH v1 2/2] media: atomisp: remove redundant call to ia_css_output0_configure()

[Jose A. Perez de Azpillaga]

The function configure_isp_from_args() contained a duplicate call to
ia_css_output0_configure() using the same output frame index. Remove
the redundant call to simplify the configuration path.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css_sp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index 2904455b35f7..1612bb2fd8b0 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -796,9 +796,6 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 	if (ret)
 		return ret;
 	ret = ia_css_copy_output_configure(binary, args->copy_output);
-	if (ret)
-		return ret;
-	ret = ia_css_output0_configure(binary, ia_css_frame_get_info(args->out_frame[0]));
 	if (ret)
 		return ret;
 	ret = ia_css_iterator_configure(binary, ia_css_frame_get_info(args->in_frame));
-- 
2.53.0

Re: [PATCH v1 2/2] media: atomisp: remove redundant call to ia_css_output0_configure()

[Andy Shevchenko]

On Sat, Mar 28, 2026 at 9:27 PM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
>
> The function configure_isp_from_args() contained a duplicate call to
> ia_css_output0_configure() using the same output frame index. Remove
> the redundant call to simplify the configuration path.

This requires more information, in particular to explain if the order
has no side effects. It might be that double configuration has side
effects and removal (wrong) one may lead to other currently hidden
issues.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] media: atomisp: remove redundant call to ia_css_output0_configure()

[Dan Carpenter]

On Sat, Mar 28, 2026 at 08:21:38PM +0100, Jose A. Perez de Azpillaga wrote:
> The function configure_isp_from_args() contained a duplicate call to
> ia_css_output0_configure() using the same output frame index. Remove
> the redundant call to simplify the configuration path.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> ---

This feels like a guess work patch.  Sure, it looks like duplicate
code but duplicate code isn't always wrong.  How do you know which
call to remove?  How do you know that it's not a copy and paste error
and the right fix is just to change the code instead of deleting it?

Patch 1 felt like an AI patch, and this patch feels even more strongly
like an AI patch.

Please don't send guess work patches or if you do add a giant comment
at the bottom saying --- "This patch is a GUESS.  Review carefully!
Untested"

regards,
dan carpenter