linux-kernel - Re: [PATCH] drivers/staging/comedi/drivers/ni

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]

Date:	Fri, 11 Jul 2014 15:30:15 +0300
From:	Andrey Utkin <andrey.krieger.utkin@...il.com>
To:	Ian Abbott <abbotti@....co.uk>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	kernel-janitors@...r.kernel.org,
	OSUOSL Drivers <devel@...verdev.osuosl.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	hsweeten@...ionengravers.com, dcb314@...mail.com
Subject: Re: [PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove
 pointless condition

2014-07-11 15:01 GMT+03:00 Ian Abbott <abbotti@....co.uk>:
> On 2014-07-11 11:13, Andrey Utkin wrote:
>>
>> The issue was discovered with static analysis and has two instances in
>> this file. The code looks like this
>> if (x < 65536000) {
>>         ...
>> } else if (x < 655360000) {
>>         ...
>> } else if (x <= 0xffffffff /* 6553600000 */) {
>>         ...
>> } else if (x <= 0xffffffff /* 65536000000 */) {
>>         ...
>> }
>>
>> The meaning of this block is to select appropriate clock frequency for
>> interval timer basing on "x", which is amount of time.
>>
>> Notes:
>> 1. That last condition matches previous one - that's the issue.
>> 2. Decimal numbers in comments don't match hex numbers in expressions.
>> But in first case the numbers have same order, while in the second case
>> the hex number is the same, and the decimal one is 10 times bigger.
>> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
>> not obviously known.
>> 4. There's no "else" block.
>>
>> So it makes sense to make an "else" block from last "else if" case. The
>> code inside the block seems correct for such usage.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
>> Reported-by: David Binderman <dcb314@...mail.com>
>> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@...il.com>
>> ---
>>   drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> index 6ad27f5..895b56d 100644
>> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> @@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
>>         } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
>>                 base_clock = CLOCK_10_KHZ;
>>                 timer = cmd->convert_arg / 100000;
>> -       } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
>> +       } else {
>>                 base_clock = CLOCK_1_KHZ;
>>                 timer = cmd->convert_arg / 1000000;
>>         }
>
>
> Since 0xffffffff is the maximum value 'cmd->convert_arg' can be,

Could you please substantiate this? I see that convert_arg has type
"unsigned int" which may be 8 bytes on 64-bit platform. I haven't
tracked where from actual values come, if the values are limited to 4
bytes, maybe we need to set the type to u32.

> the final
> else can be moved to the 'base_clock = CLOCK_10_KHZ' block and the
> 'base_clock = CLOCK_1_KHZ' block can be removed altogether.

Feel free to prepare such patch, i'm not really keen on the subject.

-- 
Andrey Utkin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/