Re: [PATCH v2 04/13] media: dvb_frontend: don't play tricks with underflow values

[Philipp Stanner <pstanner@redhat.com>]

On Fri, 2024-10-18 at 07:37 -0700, Kees Cook wrote:
> 
> 
> On October 18, 2024 4:44:20 AM PDT, Philipp Stanner
> <pstanner@redhat.com> 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@kernel.org>
> > 
> > Anyways, this in my eyes does what it's intended to do:
> > 
> > Reviewed-by: Philipp Stanner <pstanner@redhat.com>
> > 
> > > ---
> > >  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
>