Re: [PATCH 6/6] media: amlogic-c3: Add validations for ae and awb config

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Ricardo

On Wed, Apr 29, 2026 at 09:15:59AM +0200, Ricardo Ribalda wrote:
> Hi Jacopo
>
> On Wed, 29 Apr 2026 at 08:55, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Ricardo
> >
> > On Wed, Apr 29, 2026 at 08:44:11AM +0200, Ricardo Ribalda wrote:
> > > Hi Jacopo
> > >
> > > On Wed, 29 Apr 2026 at 08:15, Jacopo Mondi
> > > <jacopo.mondi@ideasonboard.com> wrote:
> > > >
> > > > Hello
> > > >
> > > >    thank you Ricardo for the fix
> > > >
> > > > On Tue, Apr 28, 2026 at 03:49:49PM +0200, Ricardo Ribalda wrote:
> > > > > On Tue, 28 Apr 2026 at 15:26, Laurent Pinchart
> > > > > <laurent.pinchart@ideasonboard.com> wrote:
> > > > > >
> > > > > > On Tue, Apr 28, 2026 at 03:14:21PM +0200, Ricardo Ribalda wrote:
> > > > > > > On Tue, 28 Apr 2026 at 15:10, Laurent Pinchart wrote:
> > > > > > > > On Tue, Apr 28, 2026 at 12:41:12PM +0000, Ricardo Ribalda wrote:
> > > > > > > > > Avoid invalid memory access if the zones_num is bigger than
> > > > > > > > > zone_weight.
> > > > > > > > >
> > > > > > > > > This patch fixes the following smatch errors:
> > > > > > > > > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> > > > > > > > > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> > > > > > > > > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> > > > > > > > > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/media/platform/amlogic/c3/isp/c3-isp-params.c | 4 ++++
> > > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > > > > > > > > index 6f9ca7a7dd88..42d780f684d1 100644
> > > > > > > > > --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > > > > > > > > +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > > > > > > > > @@ -104,6 +104,8 @@ static void c3_isp_params_awb_wt(struct c3_isp_device *isp,
> > > > > > > > >       c3_isp_write(isp, ISP_AWB_BLK_WT_ADDR, 0);
> > > > > > > > >
> > > > > > > > >       zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> > > > > > > > > +     if (WARN_ON(zones_num > C3_ISP_AWB_MAX_ZONES))
> > > > > > > >
> > > > > > > > This is triggerable by userspace, it shouldn't result in a WARN_ON().
> > > > > > > > Ideally the horiz_zones_num and vert_zones_num should be validated at
> > > > > > > > buf prepare time, and an error should be returned to userspace. That
> > > > > > > > will likely not fix your smatch issue though, I don't think it will be
> > > > > > > > able to understand that the values have been validated.
> > > > > > >
> > > > > > > Based on the warnings from the other drivers I also suspect that if
> > > > > > > you have validated the data somewhere else smatch will understand it.
> > > > > > >
> > > > > > > Even if you add a validate function I would suggest to keep the
> > > > > > > WARN_ON(), ideally it should never trigger, and if it triggers it will
> > > > > > > get a lot more attention to get it fixed.
> > > > > >
> > > > > > We could keep the WARN_ON() if we first validate the data, but the
> > > > > > driver doesn't currently :-/ I expect there could be more similar
> > > > > > issues.
> > > > >
> > > > > Yep, I got that. I will let you or Jacopo figure out the best way to
> > > > > implement the validation in buf_prepare. If you do not have time to
> > > > > implement it now I will just remove the WARN_ON in the interim... but
> > > > > from my experience we only fix stuff if we get an oops.
> > > > >
> > > > > Regards!
> > > > >
> > > > > >
> > > > > > > > Jacopo, do we need to add a validate function pointer to
> > > > > > > > v4l2_isp_params_block_type_info ?
> > > >
> > > > To allow drivers to provide an additional per-block validation
> > > > function ? I think it could be nice indeed.
> > > >
> > > > Ricardo, could you spare this patch for the moment ? I think we can
> > > > WARN_ON() to please smatch but we should pre-validate the buffer (without
> > > > spamming the system log in case of errors) to make sure we actually
> > > > never hit the WARN_ON() :)
> > > >
> > > > I have some patches in the pipe for v4l2-isp to add support for
> > > > extensible stats, I could pile up a few more to give drivers a space
> > > > where to implement additional per-block validations
> > >
> > > Do you have any idea of the timeline for this?
> > >
> >
> > Give the change will likely come on top of extensible stats it might
> > slip this cycle
> >
> > > I would really like to land this in this cycle. If it is going to take
> > > long maybe i can just
> > >
> > > if (zones_num > C3_ISP_AE_MAX_ZONES)
> >
> > Fine by me
> >
> > >
> > > and then when you add your checks you can promote it to:
> > >
> > > if (WARN_ON(zones_num > C3_ISP_AE_MAX_ZONES))
> >
> > To be honest, if we pre-validate and silence the smatch warning with
> > the above
> >
> >         if (zones_num > C3_ISP_AE_MAX_ZONES)
> >
> > then there shouldn't be any need to WARN_ON() ?
>
> I like WARN_ON in "sanity checks" because when they fail they give you
> a pretty verbose warning that will lead to resolution.
> But if you do not need/like it, it is also fine by me.

once we add validation, it should never be triggered, so WARN_ON is
fine with me.

In general, whenever an error is triggerable by userspace imho it
shouldn't get to the system's log by default. If someone hits an error
while developing a userspace component it should enable debug on the module
to make a more verbose error description visibile ?

>
>
> >
> > >
> > > ?
> > >
> > > Also it would be much easier to backport this change than a change in v4l2-isp.
> > >
> > > Regards!
> > >
> > > >
> > > > > > > >
> > > > > > > > > +             zones_num = C3_ISP_AWB_MAX_ZONES;
> > > > > > > > >
> > > > > > > > >       /* Need to write 8 weights at once */
> > > > > > > > >       for (i = 0; i < zones_num / 8; i++) {
> > > > > > > > > @@ -220,6 +222,8 @@ static void c3_isp_params_ae_wt(struct c3_isp_device *isp,
> > > > > > > > >       c3_isp_write(isp, ISP_AE_BLK_WT_ADDR, 0);
> > > > > > > > >
> > > > > > > > >       zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> > > > > > > > > +     if (WARN_ON(zones_num > C3_ISP_AE_MAX_ZONES))
> > > > > > > > > +             zones_num = C3_ISP_AE_MAX_ZONES;
> > > > > > > > >
> > > > > > > > >       /* Need to write 8 weights at once */
> > > > > > > > >       for (i = 0; i < zones_num / 8; i++) {
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > >
> > > > > > Laurent Pinchart
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Ricardo Ribalda
> > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
>
>
>
> --
> Ricardo Ribalda