linux-kernel - Re: [PATCH 2/2] input - leds: fix input_led

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:   Wed, 20 Dec 2017 16:11:00 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Samuel Thibault <samuel.thibault@...-lyon.org>,
        Peter Hutterer <peter.hutterer@...-t.net>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 2/2] input - leds: fix input_led_disconnect path

On Sat, Dec 16, 2017 at 1:48 AM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> Hi Benjamin,
>
> On Thu, Dec 14, 2017 at 02:25:22PM +0100, Benjamin Tissoires wrote:
>> Before unregistering the led classes, we have to be sure there is no
>> more events in the input pipeline.
>> Closing the input node before removing the led classes flushes the
>> pipeline and this prevents segfaults.
>
> I do not think this actually the issue. input_leds_event() is an empty
> stub, it does not really do anything with events it receives form input
> core, and it does not reference the led structures. The way input leds

Right. Actually, we could simply drop the stub as input_to_handler()
checks for the validity of .event().

> work is that input driver owns the hardware led and is responsible for
> communicating with it. The LED subsystem is a secondary interface,
> requests coming from /sys/class/led/... are being forwarded to the input
> core and then to the input hardware driver by the way of:
>
> input_leds_brightness_set() ->
>         input_inject_event() ->
>                 input_handle_event()->
>                         disposition & INPUT_PASS_TO_DEVICE
>                                 dev->event() (in atkbd or hid-input)
>                                         actual control of LED
>
> I do not see how stopping event flow from input core to input-leds would
> help here.

I wonder if this solution in this patch is not just masking the race
condition by forcing the sync.

After further researches, I realized that the patch is actually not
needed. In the upstream repo of Peter, the code inject events and
closes the device ASAP, triggering races with udev.
Adding the proper udev stubs seem to solve the issues.
I still have other random crashes, but I can't seem to reproduce the
error of https://bugzilla.kernel.org/show_bug.cgi?id=197679 now.

Anyway, I can't explain the backtrace of the kernel bug, it is as if
we are calling the unregister function without having properly
registered the device. But this can not happen because of the mutexes
in place.
The race seems to be udev and probably the led class accesses...

BTW, if the handler doesn't use the events coming in from the input
subsystem, is there any benefits of opening the device from the
handler?
I tried without, and it seemed to be working fine, but I wonder if
there is no hidden dependency I haven't see.

Cheers,
Benjamin