linux-kernel - Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register read

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:	Tue, 17 May 2011 23:46:00 +0200
From:	Éric Piel <Eric.Piel@...mplin-utc.net>
To:	Christian Lamparter <chunkeey@...glemail.com>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register
 read

Op 16-05-11 23:36, Christian Lamparter schreef:
> On Monday 16 May 2011 13:16:46 Éric Piel wrote:
>> Op 16-05-11 00:46, Christian Lamparter schreef:
>>>  From my POV, it looks like the hardware is not working as expected
>>> and returns a bogus data rate. The driver doesn't check the result
>>> and directly uses it as some sort of divisor in some places:
>>>
>>> msleep(lis3->pwron_delay / lis3lv02d_get_odr());
>>>
>>> Under this circumstances, this could very well cause the
>>> "divide by zero" exception from above.
>>>
>> However, I'd fix it a bit differently: let lis3lv02d_get_odr() return
>> the raw data, and create a special function
>> lis3lv02d_get_pwron_delay_ms() which does the "lis3->pwron_delay /
>> lis3lv02d_get_odr()" with special handling for 0 (returning a large
>> value and also sending a printk_once() ).
> Do you know how "volatile" this data rate is? If it never changes
> [at least it doesn't here?] then why not read it once in init_device
> and store it in the device context?
It is not normally changing, normally it is set just at init/unsuspend 
(where the bios can also interfere sometimes) and when the user changes 
it. So definitely within the same function it's not going to suddenly 
change. We could avoid calculating/checking it twice in 
lis3lv02d_selftest(). Care to do a third version with this little clean up?

>
> Anyway, I updated the code... But instead of returning a "large value"
> I went for the -ENXIO to bail-out early, so now we won't continue if
> something went bad [after resume for instance?].
Sounds even better than using a conservative value!

>
>> As you have noted, we might want to check other parts of the driver to
>> validate the data from the device. So far, all the code considers that
>> the device is flawless :-S
> well:
>    CHECK   drivers/misc/lis3lv02d/lis3lv02d.c
> drivers/misc/lis3lv02d/lis3lv02d.c:170:52: warning: cast to restricted __le16
This is not introduced by your patch, right? So it's fine for now :-)

Thanks,
Eric
--
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/