linux-kernel - Re: [PATCH] staging: iio: accel: remove impossible condition

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] [day] [month] [year] [list]

Date:	Tue, 31 May 2016 21:53:23 +0100
From:	Luis de Bethencourt <luisbg@....samsung.com>
To:	Jonathan Cameron <jic23@...nel.org>, linux-kernel@...r.kernel.org
CC:	gregkh@...uxfoundation.org, knaack.h@....de, lars@...afoo.de,
	ciorneiioana@...il.com, janani.rvchndrn@...il.com, afd@...com,
	linux-iio@...r.kernel.org, devel@...verdev.osuosl.org
Subject: Re: [PATCH] staging: iio: accel: remove impossible condition

On 31/05/16 21:23, Jonathan Cameron wrote:
> 
> 
> On 31 May 2016 20:47:50 BST, Luis de Bethencourt <luisbg@....samsung.com> wrote:
>> val is set to the value of ret right after ret is checked. If ret is
>> not
>> zero it goes to error_ret. So only value ret can have is zero, which
>> makes
>> the switch (val & 0x03) only match the case 0x00. Removing the switch
>> and
>> since val is only used for this, removing val as well.
> There is clearly an issue here. However it looks like it is that if(ret) which is wrong.
> 
> The code as it stands clearly doesn't work as intended. Fixing the bug would be
> 
>  more useful than removing code that 'should' be accessible.
> 
> I happen to fire the relevant hardware up yesterday for the first time in a while so
> 
> can easily verify the operation of a fix if you want to take another look.
> 
> Jonathan

Jonathan and Andrew are right.

sca3000_read_ctrl_reg() returns a negative number on failure.

So line 597 should be: if (ret < 0)

If everything goes well in sca3000_read_ctrl_reg() it returns st->rx[0], which explains
the switch case.

I am going to send a new patch with this fix.

Thanks so much for the review and sorry for the initial confusion,
Luis