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