Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers

On 11/26/2015 06:29 PM, Greg KH wrote:

On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:

quoted

On 11/25/2015 04:45 PM, Emilio López wrote:

quoted

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

quoted

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;))

Adding this option might be nice.

quoted

quoted

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?

You can't, and why would you ever want to, as you can't tell what an app
"should" or "should not" do.  If you really care about this, then use a
LSM policy to prevent this.

Well, an app can declare what it does and what it needs in it's manifest 
file (or some equivalent of this) and the platform should ensure that 
app can do only what it has declared.

I would really like to use LSM policy in here but currently it is 
impossible as one device node represents whole device. Permissions (even 
those from LSM) are being checked only on open() not on each ioctl() so 
as far as I know there is nothing which prevents any owner of opened fd 
to claim all available (not taken by someone else) interfaces and LSM 
policy is unable to filter those calls (unless we add some LSM hooks 
over there).

quoted

Moreover I'm not convinced to this patch as it hardcodes the *policy* in
kernel code.

What policy is that?

It's a policy which defines set of ioctls which cannot be issued in 
"restricted mode".

quoted

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

I've been asking for that for well over a decade, but no one ever did
the work.  I think if you work through the options, it ends up not being
a viable solution...

I'm not surprised that no one ever did this as it looks like quite a lot 
of work and current interface is still working;) Do you have some link 
to a discussion or sth which shows why it's not a good solution?

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics