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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D0C7D12-C645-4766-B7B1-0B34B2129579@kernel.org>
Date: Fri, 18 Oct 2024 07:37:52 -0700
From: Kees Cook <kees@...nel.org>
To: Philipp Stanner <pstanner@...hat.com>,
 Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
CC: Hans Verkuil <hverkuil@...all.nl>, Kevin Hao <haokexin@...il.com>,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v2 04/13] media: dvb_frontend: don't play tricks with underflow values



On October 18, 2024 4:44:20 AM PDT, Philipp Stanner <pstanner@...hat.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@...nel.org>
>
>Anyways, this in my eyes does what it's intended to do:
>
>Reviewed-by: Philipp Stanner <pstanner@...hat.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