MIME-version: 1.0
Content-type: text/plain; charset=utf-8; format=flowed
Content-transfer-encoding: 8BIT
Subject: Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
To: =?UTF-8?Q?Emilio_L=c3=b3pez?= <emilio.lopez@collabora.co.uk>,
        gregkh@linuxfoundation.org, stern@rowland.harvard.edu,
        kborer@gmail.com
References: <1448466334-21346-1-git-send-email-emilio.lopez@collabora.co.uk>
Cc: reillyg@chromium.org, keescook@chromium.org, linux-api@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        jorgelo@chromium.org, dan.carpenter@oracle.com
From: Krzysztof Opasiak <k.opasiak@samsung.com>
Message-id: <5656CEA1.9010203@samsung.com>
Date: Thu, 26 Nov 2015 10:19:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.2.0
In-reply-to: <1448466334-21346-1-git-send-email-emilio.lopez@collabora.co.uk>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3087
Lines: 72


On 11/25/2015 04:45 PM, Emilio López wrote:
> Hi everyone,
>
> This patch introduces a new ioctl, USBDEVFS_DROP_PRIVILEGES,
> to voluntarily forgo the ability to issue ioctls which may
> interfere with other users of the USB device.
>
> This feature allows a privileged process (in the case of Chrome OS,
> permission_broker) to open a USB device node and then drop a number
> of capabilities that are considered "privileged".

We had the same idea in Tizen but for now we didn't have time to 
implement it.

  These privileges
> include the ability to reset the device if there are other users
> (most notably a kernel driver) or to disconnect a kernel driver
> from the device. The file descriptor can then be passed to an
> unprivileged process.

And how about switching configuration? This can be also harmful even if 
the are no other users for any interface in this configuration.
(Just imagine the situation in which only second config contains an HID 
function and when app switch configuration it is activated without user 
knowing about this;))

>
> This is useful for granting a process access to a device with
> multiple functions. It won't be able to use its access to one
> function to disrupt or take over control of another function.

I run through your code and as far as I understand above is not exactly 
true. Your patch allows only to prevent userspace from accessing 
interfaces which has kernel drivers, there is no way to stop an 
application from taking control over all free interfaces.

Let's say that your device has 3 interfaces. First of them has a kernel 
driver but second and third doesn't. You have 2 apps. One should 
communicate using second interface and another one third. But first app 
is malicious and it claims all free interfaces of received device (your 
patch doesn't prevent this). And when second app starts it is unable to 
do anything with the device because all interfaces are taken. How would 
you like to handle this?

Moreover I'm not convinced to this patch as it hardcodes the *policy* in 
kernel code. Generally our approach (with passing fd from broker to 
unprivileged process) was similar but we found out that if we would like 
to do this correctly there is much more things to filter than in this 
patch. We had two main ideas:

- implement some LSM hooks in ioctls() but this leads to a lot of 
additional callbacks in lsm ops struct which even now is very big. But 
as a benefit we would get a very flexible policy consistent with other 
system policies

- split single usb device node into multiple files which could represent 
single endpoins only for io and separate control file for privileged but 
it's quite a lot of work and I don't know if any one is going to accept 
such a change

Best regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/