linux-kernel - Re: [PATCH 1/3] Input: synaptics-rmi4 - disable the relative position IRQ in the F12 driver

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, 29 Oct 2019 19:54:25 +0000
From:   Andrew Duggan <aduggan@...aptics.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:     "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Christopher Heiny <Cheiny@...aptics.com>,
        Simon Wood <simon@...gewell.org>,
        Nick Dyer <nick@...anahar.org>
Subject: Re: [PATCH 1/3] Input: synaptics-rmi4 - disable the relative position
 IRQ in the F12 driver

Hi Dmitry,

On 10/27/19 10:39 PM, Dmitry Torokhov wrote:
>
> Hi Andrew,
>
> On Fri, Oct 25, 2019 at 12:25:56AM +0000, Andrew Duggan wrote:
>> This patch fixes an issue seen on HID touchpads which report finger
>> positions using RMI4 Function 12. The issue manifests itself as
>> spurious button presses as described in:
>> https://www.spinics.net/lists/linux-input/msg58618.html
>>
>> Commit 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution
>> to irq_domain") switched the RMI4 driver to using an irq_domain to handle
>> RMI4 function interrupts. Functions with more then one interrupt now have
>> each interrupt mapped to their own IRQ and IRQ handler. The result of
>> this change is that the F12 IRQ handler was now getting called twice. Once
>> for the absolute data interrupt and once for the relative data interrupt.
>> For HID devices, calling rmi_f12_attention() a second time causes the
>> attn_data data pointer and size to be set incorrectly. When the touchpad
>> button is pressed, F30 will generate an interrupt and attempt to read the
>> F30 data from the invalid attn_data data pointer and report incorrect
>> button events.
> Maybe we should create only 1 interrupt per function instead of
> multiple? It looks like the functions read their entire block of data on
> any interrupt received.

Yes, we could only create 1 interrupt per function. We could modify the 
rmi_create_function_irq() function to only map a specific IRQ and call 
it from the function driver's probe function instead of 
rmi_function_probe(). I considered that, but it was a bit more of a 
modification then I wanted to make at this point.

>> This patch disables the F12 relative interrupt which prevents
>> rmi_f12_attention() from being called twice.
> Don't we have similar issue with F11, and maybe others?

Currently, F11 prevents this from happening by disabling one of the two 
IRQs on the touch controller (usually the relative IRQ). F11 will decide 
to report abs or rel events based on the F11 query registers. Then it 
will set the IRQ it wants and clear the IRQ it does not in 
rmi_f11_config(). Both IRQs are still mapped in the irq_domain, but the 
touch controller will only generate an IRQ for one of them and 
rmi_f11_attention() will only be called once. I believe F11 and F12 are 
the only RMI functions which report more then one IRQ. Even if 
technically the spec allows up to 6 to be declared. This patch just 
implements the same behavior as F11 for F12.

>
> Also, as far as F12 goes, I see that it may mark sensor as reporting
> relative coordinates, but I do not see where it would actually emit
> relative events. I must be missing something here...

Nope, you are not missing anything. The code to parse the relative data 
from the data9 register was never implemented. I noticed that too when 
creating this patch. This must have been an oversight during the 
original implementation of the F12 driver. I considered adding the code 
to report relative events, but I did not have any hardware readily 
available to test with. It seems unlikely that there would be a device 
using one of our touch controllers supporting F12 which only reported 
relative events. Also, no one seems to have complained about the last of 
reporting relative events in the last few years. Instead, in this patch 
I just clear the relative IRQ to prevent rmi_f12_attention() from being 
called twice. I could add a comment or a warning stating that reporting 
relative events in F12 is unsupported.

Thanks,

Andrew

> Thanks.
>
> --
> Dmitry