[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()

[Jose A. Perez de Azpillaga]

On Mon, Mar 30, 2026 at 11:59:24AM +0300, Andy Shevchenko wrote:
> 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?
>

not really, I don't have the hardware, but while reading the code, I found
it to be logically inconsistent. imo, the comment is misplaced since
delay_frames can be null earlier.

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


you are right here. I should've done a deeper analysis instead of focusing only
on that function. looking more closely at how the pipeline is built, I found that
these frames are intentionally skipped during allocation to save memory when a
specific feature isn't enabled.

the configuration path was just ignoring those enable flags and trying to use the frames
anyway. instead of a NULL check, maybe I should try gating these calls behind the actual
feature flags in the binary info.

if this is a better approach, I'll send a v2 with these changes. :)

...

regards,
jose a. p-a

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

[Andy Shevchenko]

On Tue, Mar 31, 2026 at 8:57 AM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
> On Mon, Mar 30, 2026 at 11:59:24AM +0300, Andy Shevchenko wrote:
> > 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?

> not really, I don't have the hardware, but while reading the code, I found
> it to be logically inconsistent. imo, the comment is misplaced since
> delay_frames can be null earlier.

Don't forget to mention this in the cover letter.

...

> > >         /*
> > > -        * 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.
>
> you are right here. I should've done a deeper analysis instead of focusing only
> on that function. looking more closely at how the pipeline is built, I found that
> these frames are intentionally skipped during allocation to save memory when a
> specific feature isn't enabled.
>
> the configuration path was just ignoring those enable flags and trying to use the frames
> anyway. instead of a NULL check, maybe I should try gating these calls behind the actual
> feature flags in the binary info.
>
> if this is a better approach, I'll send a v2 with these changes. :)

Sounds like a plan! We would appreciate this kind of patch over some
mechanical cleanups that flooded recently to this driver.

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

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

[Jose A. Perez de Azpillaga]

On Mon, Mar 30, 2026 at 12:35:30PM +0300, Dan Carpenter wrote:
> 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.
>

yes, I misunderstood the comment. my bad. I read more carefully.

...

regards,
jose a. p-a

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

[Jose A. Perez de Azpillaga]

On Tue, Mar 31, 2026 at 08:07:43AM +0200, Jose A. Perez de Azpillaga wrote:
> On Mon, Mar 30, 2026 at 12:35:30PM +0300, Dan Carpenter wrote:
> > 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.
> >
>
> yes, I misunderstood the comment. my bad. I read more carefully.
>

edit: I'll read more carefully.

> ...
>
> regards,
> jose a. p-a

[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()

[Jose A. Perez de Azpillaga]

On Mon, Mar 30, 2026 at 12:01:21PM +0300, Andy Shevchenko wrote:
> 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.
>

mhm... my bad for that,

the ia_css_output0_configure() function acts as a configuration setter.
it populates a struct ia_css_output0_configuration from the frame info
and caches it in the binary parameters. calling it twice with the same
out_frame[0] pointer merely overwrites the exact same state with
identical values. it has no cumulative state, either does its order
matter relative to ia_css_copy_output_configure().

...

regards,
jose a. p-a

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

[Andy Shevchenko]

On Tue, Mar 31, 2026 at 9:03 AM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
> On Mon, Mar 30, 2026 at 12:01:21PM +0300, Andy Shevchenko wrote:
> > 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.
>
> mhm... my bad for that,
>
> the ia_css_output0_configure() function acts as a configuration setter.
> it populates a struct ia_css_output0_configuration from the frame info
> and caches it in the binary parameters. calling it twice with the same
> out_frame[0] pointer merely overwrites the exact same state with
> identical values. it has no cumulative state, either does its order
> matter relative to ia_css_copy_output_configure().

So, no call in between accesses that parameter or relies on its state?
With this analysis in the commit message the change looks good.
Please, amend it in v2.

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

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

[Jose A. Perez de Azpillaga]

On Mon, Mar 30, 2026 at 12:35:34PM +0300, Dan Carpenter wrote:
> 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?
>

my response here would be something similar to Andy's one.

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

ouch.

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

okay, I wasn't aware of that. I'll try to add that comment or better
yet, not trying to fix things without the actual hardware. thanks.

...

regards,
jose a. p-a