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.0On 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
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.