linux-kernel - Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

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]

Message-ID: <CAPeXnHt+urPvRJKDmYHEAuLvkTe7mhp+0_yFD5VboQzt_-cRnA@mail.gmail.com>
Date:   Wed, 4 Jan 2017 13:59:26 -0600
From:   Matthew Garrett <mjg59@...eos.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Lukas Wunner <lukas@...ner.de>,
        Madalin Bucur <madalin.bucur@....com>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Russell King <rmk+kernel@....linux.org.uk>,
        Petr Tesarik <ptesarik@...e.com>, linux-pm@...r.kernel.org,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH] Allow userspace control of runtime disabling/enabling of
 driver probing

On Wed, Jan 4, 2017 at 1:46 PM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Wed, Jan 04, 2017 at 12:10:04PM -0600, Matthew Garrett wrote:
>> On Wed, Jan 4, 2017 at 3:32 AM, Greg Kroah-Hartman
>> <gregkh@...uxfoundation.org> wrote:
>> > Ick, hiding this in the power management code?  That's messy, and
>> > complex, as Rafael pointed out.
>>
>> It's in code that's used in the power management layer, not in power
>> management code. This is all in the driver core.
>
> Yes, but it's in the power management layer, not the "main" portion of
> the driver bind/unbind logic.  It's kind of hidden, don't you think?

No? It's in dd.c, and defer_all_probes is checked in really_probe()
which is part of the normal probe path. The only user currently is in
the power management code, but there's nothing directly power
management related going on here.

>> > Turning on and off at random times "new devices can not be bound, wait,
>> > now they can!" is ripe for loads of issues and is going to be a pain to
>> > properly maintain over time.
>>
>> What kind of issues?
>
> How are you going to test this to ensure it continues to work properly?

It's code that's already used over every suspend and resume. What kind
of breakage are you expecting? How do we test any feature to ensure it
continues to work properly?

>> > What's wrong with the facility that the USB layer provides today to
>> > allow only "authenticated" devices to be enabled?  That has been used
>> > for a few years now to prevent these "don't allow random devices that
>> > are plugged into the computer to be enabled" type attacks.  Doing much
>> > the same thing, in a different manner, is ripe for problems (how do the
>> > two interact now?)
>>
>> The USB authentication feature was intended for handling wireless USB
>> devices - it can be reused for this, but the code isn't generic enough
>> to apply to other bus types. The two interact in exactly the way you'd
>> expect, ie they don't. If you use both, then you need to handle both.
>
> The feature was originally created for wireless USB devices, but it will
> work for any USB device, and has been successfully used for this type of
> thing (i.e. protecting kiosk-systems) for a while now.

It will work for any USB device, but doesn't have the desired
semantics and the implementation is directly tied to USB.

>> > If you are worried about PCI devices, why not just implement what USB
>> > did for PCI?  Or better yet, move the USB functionality into the driver
>> > core, adding that type of authentication ability to any bus that wishes
>> > to have it (and not break existing working systems that are using the
>> > USB solution today.)
>>
>> The USB approach requires userspace to keep track of which devices
>> were added while the session was locked, whereas the kernel already
>> has the logic to do all of this. All the complexity already exists and
>> is used every time anybody suspends and resumes, so why add additional
>> complexity?
>
> The kernel knows nothing about a "session", you know that.  Userspace
> can disable enabling any new USB device when a session is "locked" today
> quite easily, why not do that?

Because the existing feature doesn't allow you to identify what kind
of device has been plugged in, which means it's not usable.

>> I'm not clear on how this patch breaks anybody using the existing USB
>> solution?
>
> You now have two different ways to enable access to a USB device, please
> let's keep to only one type of path.

So it won't break the existing USB solution?