linux-kernel - Incorrect processing of "1" raw_event() return value inside hid_input

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHh1A6kKmqW_4DqOsxKXVuMY_+gf8R1M5fmL717F9AaRjXNx4Q@mail.gmail.com>
Date:	Thu, 11 Feb 2016 01:22:18 +0300
From:	Dmitry Semyonov <dmitry.semyonov@...entembedded.com>
To:	Jiri Kosina <jkosina@...e.cz>
Cc:	linux-kernel@...r.kernel.org
Subject: Incorrect processing of "1" raw_event() return value inside hid_input_report()

Hello Jiri,

Please, refer to the commit b1a1442a23776756b254b69786848a94d92445ba
and to the second part of
http://thread.gmane.org/gmane.linux.kernel.input/39802. (The message
author was right but hesitated to insist. Therefore, I have to re-open
the issue):

>>         if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
>>                 ret = hdrv->raw_event(hid, report, data, size);
>> -               if (ret != 0) {
>> +               if (ret < 0) {
>>                         ret = ret < 0 ? ret : 0;
>>                         goto unlock;
>>                 }
>> --------------------------------------------------
>>
>> So now it does not matter whether the raw_event method returns 0 or 1.
>> The documentation says:

(The hid_driver structure documentation in linux/hid.h)

>>     raw_event and event should return 0 on no action performed, 1 when
>>     no further processing should be done and negative on error
 [...]
> This change was done so that we don't call hid_report_raw_event() when
> ->raw_event() callback encountered error during parsing (and therefore
> returned negative value).

I cannot accept this answer because the processing of negative return
values was not affected by the above change.

What is worse, I cannot accept the change itself because it breaks the
contract established by the hid_driver structure documentation for HID
drivers.

Ubuntu guys had to revert it[1] due to changed behavior that uncovered
another bug[2]. A colleague of mine re-discovered the same root cause
while investigating a different issue, and had to revert the change as
well because of some existing proprietary keypad driver was using the
"1" value to indicate that no further processing is needed, (exactly
as specified in the documentation).

[1]https://lists.ubuntu.com/archives/kernel-team/2013-October/032351.html
[2]http://www.kernelhub.org/?p=2&msg=380901

A lot of HID drivers in the kernel source tree use negative, 0, and 1
return values in their *_raw_event() functions. Some of them may be
using the values incorrectly but at least they were verified by the
original developers, and I doubt you had a chance to properly re-test
*all* the drivers before introducing the breaking API change.
Therefore, I strongly recommend to revert the following two related
patches, and fix the specific driver(s) instead if there are any
issues left:

b1a1442a HID: core: fix reporting of raw events
556483e2 HID: remove self-assignment from hid_input_report

-- 
...Bye..Dmitry.