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.
>
>> 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.
And I actually think this could be refractored to avoid needing "ready" at all?
-Kees
--
Kees Cook
