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

["Jose A. Perez de Azpillaga" <azpijr@gmail.com>]

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