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

[Kees Cook <kees@kernel.org>]

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.

>
>> 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.

And I actually think this could be refractored to avoid needing "ready" at all?

-Kees

-- 
Kees Cook