On Fri, 2024-10-18 at 07:37 -0700, Kees Cook wrote:
>
>
> On October 18, 2024 4:44:20 AM PDT, Philipp Stanner
> <pstanner@xxxxxxxxxx> wrote:
> > On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote:
> > > fepriv->auto_sub_step is unsigned. Setting it to -1 is just a
> > > trick to avoid calling continue, as reported by Coverity.
> > >
> > > It relies to have this code just afterwards:
> > >
> > > if (!ready) fepriv->auto_sub_step++;
> > >
> > > Simplify the code by simply setting it to zero and use
> > > continue to return to the while loop.
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >
> > Oh wow, back to the big-bang-commit ^^'
> >
> > So is this a bug or not? It seems to me that the uint underflows to
> > UINT_MAX, and then wrapps around to 0 again through the ++..
> >
> > I take the liberty of ++CCing Kees, since I heard him talk a lot
> > about
> > overflowing on Plumbers.
> >
> > If it's not a bug, I would not use "Fixes". If it is a bug, it
> > should
> > be backported to stable, agreed?
> >
> > Plus, is there a report-link somewhere by Coverty that could be
> > linked
> > with "Closes: "?
>
> Yeah, this is "avoid currently harmless overflow" fix. It is just
> avoiding depending on the wrapping behavior, which is an improvement
> but not really a "bug fix"; more a code style that will keep future
> work of making the kernel wrapping-safe.
Alright, then it shouldn't be backported, ack?
So I'd drop "Fixes:"
>
> >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> >
> > Anyways, this in my eyes does what it's intended to do:
> >
> > Reviewed-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> >
> > > ---
> > > drivers/media/dvb-core/dvb_frontend.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/dvb-core/dvb_frontend.c
> > > b/drivers/media/dvb-core/dvb_frontend.c
> > > index d48f48fda87c..c9283100332a 100644
> > > --- a/drivers/media/dvb-core/dvb_frontend.c
> > > +++ b/drivers/media/dvb-core/dvb_frontend.c
> > > @@ -443,8 +443,8 @@ static int
> > > dvb_frontend_swzigzag_autotune(struct
> > > dvb_frontend *fe, int check_wra
> > >
> > > default:
> > > fepriv->auto_step++;
> > > - fepriv->auto_sub_step = -1; /* it'll be
> > > incremented to 0 in a moment */
> > > - break;
> > > + fepriv->auto_sub_step = 0;
> > > + continue;
> > > }
> > >
> > > if (!ready) fepriv->auto_sub_step++;
> >
>
> But this change seems incomplete. The above line is no longer needed.
I haven't super duper intensively reviewed it, but wouldn't make that
statement – all the other branches in the switch-case reach this line.
And auto_sub_step might be changed above in the if-check again if
lnb_drift has changed; and it is changed in the switch-case.
>
> And I actually think this could be refractored to avoid needing
> "ready" at all?
Could be. But that'd be indeed some work to get it right without
introducing a subtle bug, and Mauro just seems to want to fix a warning
he encountered on the way.
Thx
P.
>
> -Kees
>
