* [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-25 15:45 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2015-11-25 15:45 UTC (permalink / raw)
To: gregkh, stern, kborer
Cc: reillyg, keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter, Emilio López
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". 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.
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.
This patch is currently being used in Chrome OS; I have updated it
to be in line with changes in v4.4-rc.
Cheers!
Emilio
Reilly Grant (1):
usb: devio: Add ioctl to disallow detaching kernel USB drivers.
drivers/usb/core/devio.c | 50 +++++++++++++++++++++++++++++++++++----
include/uapi/linux/usbdevice_fs.h | 1 +
2 files changed, 47 insertions(+), 4 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-25 15:45 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2015-11-25 15:45 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w
Cc: reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA, Emilio López
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". 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.
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.
This patch is currently being used in Chrome OS; I have updated it
to be in line with changes in v4.4-rc.
Cheers!
Emilio
Reilly Grant (1):
usb: devio: Add ioctl to disallow detaching kernel USB drivers.
drivers/usb/core/devio.c | 50 +++++++++++++++++++++++++++++++++++----
include/uapi/linux/usbdevice_fs.h | 1 +
2 files changed, 47 insertions(+), 4 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
2015-11-25 15:45 ` Emilio López
(?)
@ 2015-11-25 15:45 ` Emilio López
2015-11-26 8:59 ` Peter Chen
-1 siblings, 1 reply; 54+ messages in thread
From: Emilio López @ 2015-11-25 15:45 UTC (permalink / raw)
To: gregkh, stern, kborer
Cc: reillyg, keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter, Emilio López
From: Reilly Grant <reillyg@chromium.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
Signed-off-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
[emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
drivers/usb/core/devio.c | 50 +++++++++++++++++++++++++++++++++++----
include/uapi/linux/usbdevice_fs.h | 1 +
2 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 38ae877c..a5ccc50 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,7 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
};
struct async {
@@ -878,7 +879,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -911,11 +912,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1215,6 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ if (ps->privileges_dropped) {
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+
+ /* Don't touch the device if any interfaces are claimed. It
+ * could interfere with other drivers' operations and this
+ * process has dropped its privileges to do such things.
+ */
+ if (actconfig) {
+ int i;
+
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ if (usb_interface_claimed(
+ actconfig->interface[i])) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by"
+ " %s while '%s'"
+ " resets device\n",
+ actconfig->interface[i]
+ ->cur_altsetting
+ ->desc.bInterfaceNumber,
+ actconfig->interface[i]
+ ->dev.driver->name,
+ current->comm);
+ return -EACCES;
+ }
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1922,6 +1949,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2084,6 +2114,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
sizeof(dc.driver)) == 0)
return -EBUSY;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
dev_dbg(&intf->dev, "disconnect by usbfs\n");
usb_driver_release_interface(driver, intf);
}
@@ -2130,6 +2163,12 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps)
+{
+ ps->privileges_dropped = true;
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2318,6 +2357,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..99c296b 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -187,5 +187,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IO('U', 30)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
2015-11-25 15:45 ` [PATCH v1] usb: devio: Add " Emilio López
@ 2015-11-26 8:59 ` Peter Chen
0 siblings, 0 replies; 54+ messages in thread
From: Peter Chen @ 2015-11-26 8:59 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, stern, kborer, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
On Wed, Nov 25, 2015 at 12:45:34PM -0300, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> [emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>
> ---
>
> drivers/usb/core/devio.c | 50 +++++++++++++++++++++++++++++++++++----
> include/uapi/linux/usbdevice_fs.h | 1 +
> 2 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..a5ccc50 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,7 @@ struct usb_dev_state {
> unsigned long ifclaimed;
> u32 secid;
> u32 disabled_bulk_eps;
> + bool privileges_dropped;
> };
>
> struct async {
> @@ -878,7 +879,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
> int ret;
>
> ret = -ENOMEM;
> - ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
> + ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
> if (!ps)
> goto out_free_ps;
>
> @@ -911,11 +912,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
> INIT_LIST_HEAD(&ps->async_pending);
> INIT_LIST_HEAD(&ps->async_completed);
> init_waitqueue_head(&ps->wait);
> - ps->discsignr = 0;
> ps->disc_pid = get_pid(task_pid(current));
> ps->cred = get_current_cred();
> - ps->disccontext = NULL;
> - ps->ifclaimed = 0;
The above changes seem to be not related with this change.
> security_task_getsecid(current, &ps->secid);
> smp_wmb();
> list_add_tail(&ps->list, &dev->filelist);
> @@ -1215,6 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>
> static int proc_resetdevice(struct usb_dev_state *ps)
> {
> + if (ps->privileges_dropped) {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> +
> + /* Don't touch the device if any interfaces are claimed. It
> + * could interfere with other drivers' operations and this
> + * process has dropped its privileges to do such things.
> + */
> + if (actconfig) {
> + int i;
> +
> + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> + if (usb_interface_claimed(
> + actconfig->interface[i])) {
> + dev_warn(&ps->dev->dev,
> + "usbfs: interface %d claimed by"
> + " %s while '%s'"
> + " resets device\n",
> + actconfig->interface[i]
> + ->cur_altsetting
> + ->desc.bInterfaceNumber,
The length of line is 80 characters
> + actconfig->interface[i]
> + ->dev.driver->name,
> + current->comm);
> + return -EACCES;
> + }
> + }
> + }
> + }
> +
> return usb_reset_device(ps->dev);
> }
>
> @@ -1922,6 +1949,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
> struct usb_interface *intf = NULL;
> struct usb_driver *driver = NULL;
>
> + if (ps->privileges_dropped)
> + return -EACCES;
> +
> /* alloc buffer */
> size = _IOC_SIZE(ctl->ioctl_code);
> if (size > 0) {
> @@ -2084,6 +2114,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
> sizeof(dc.driver)) == 0)
> return -EBUSY;
>
> + if (ps->privileges_dropped)
> + return -EACCES;
> +
> dev_dbg(&intf->dev, "disconnect by usbfs\n");
> usb_driver_release_interface(driver, intf);
> }
> @@ -2130,6 +2163,12 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
> return r;
> }
>
> +static int proc_drop_privileges(struct usb_dev_state *ps)
> +{
> + ps->privileges_dropped = true;
> + return 0;
> +}
> +
> /*
> * NOTE: All requests here that have interface numbers as parameters
> * are assuming that somehow the configuration has been prevented from
> @@ -2318,6 +2357,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
> case USBDEVFS_FREE_STREAMS:
> ret = proc_free_streams(ps, p);
> break;
> + case USBDEVFS_DROP_PRIVILEGES:
> + ret = proc_drop_privileges(ps);
> + break;
> }
>
> done:
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..99c296b 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -187,5 +187,6 @@ struct usbdevfs_streams {
> #define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
> #define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
> #define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
> +#define USBDEVFS_DROP_PRIVILEGES _IO('U', 30)
>
> #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2015-11-26 8:59 ` Peter Chen
0 siblings, 0 replies; 54+ messages in thread
From: Peter Chen @ 2015-11-26 8:59 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, stern, kborer, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
On Wed, Nov 25, 2015 at 12:45:34PM -0300, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> [emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>
> ---
>
> drivers/usb/core/devio.c | 50 +++++++++++++++++++++++++++++++++++----
> include/uapi/linux/usbdevice_fs.h | 1 +
> 2 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..a5ccc50 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,7 @@ struct usb_dev_state {
> unsigned long ifclaimed;
> u32 secid;
> u32 disabled_bulk_eps;
> + bool privileges_dropped;
> };
>
> struct async {
> @@ -878,7 +879,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
> int ret;
>
> ret = -ENOMEM;
> - ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
> + ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
> if (!ps)
> goto out_free_ps;
>
> @@ -911,11 +912,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
> INIT_LIST_HEAD(&ps->async_pending);
> INIT_LIST_HEAD(&ps->async_completed);
> init_waitqueue_head(&ps->wait);
> - ps->discsignr = 0;
> ps->disc_pid = get_pid(task_pid(current));
> ps->cred = get_current_cred();
> - ps->disccontext = NULL;
> - ps->ifclaimed = 0;
The above changes seem to be not related with this change.
> security_task_getsecid(current, &ps->secid);
> smp_wmb();
> list_add_tail(&ps->list, &dev->filelist);
> @@ -1215,6 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>
> static int proc_resetdevice(struct usb_dev_state *ps)
> {
> + if (ps->privileges_dropped) {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> +
> + /* Don't touch the device if any interfaces are claimed. It
> + * could interfere with other drivers' operations and this
> + * process has dropped its privileges to do such things.
> + */
> + if (actconfig) {
> + int i;
> +
> + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> + if (usb_interface_claimed(
> + actconfig->interface[i])) {
> + dev_warn(&ps->dev->dev,
> + "usbfs: interface %d claimed by"
> + " %s while '%s'"
> + " resets device\n",
> + actconfig->interface[i]
> + ->cur_altsetting
> + ->desc.bInterfaceNumber,
The length of line is 80 characters
> + actconfig->interface[i]
> + ->dev.driver->name,
> + current->comm);
> + return -EACCES;
> + }
> + }
> + }
> + }
> +
> return usb_reset_device(ps->dev);
> }
>
> @@ -1922,6 +1949,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
> struct usb_interface *intf = NULL;
> struct usb_driver *driver = NULL;
>
> + if (ps->privileges_dropped)
> + return -EACCES;
> +
> /* alloc buffer */
> size = _IOC_SIZE(ctl->ioctl_code);
> if (size > 0) {
> @@ -2084,6 +2114,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
> sizeof(dc.driver)) == 0)
> return -EBUSY;
>
> + if (ps->privileges_dropped)
> + return -EACCES;
> +
> dev_dbg(&intf->dev, "disconnect by usbfs\n");
> usb_driver_release_interface(driver, intf);
> }
> @@ -2130,6 +2163,12 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
> return r;
> }
>
> +static int proc_drop_privileges(struct usb_dev_state *ps)
> +{
> + ps->privileges_dropped = true;
> + return 0;
> +}
> +
> /*
> * NOTE: All requests here that have interface numbers as parameters
> * are assuming that somehow the configuration has been prevented from
> @@ -2318,6 +2357,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
> case USBDEVFS_FREE_STREAMS:
> ret = proc_free_streams(ps, p);
> break;
> + case USBDEVFS_DROP_PRIVILEGES:
> + ret = proc_drop_privileges(ps);
> + break;
> }
>
> done:
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..99c296b 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -187,5 +187,6 @@ struct usbdevfs_streams {
> #define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
> #define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
> #define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
> +#define USBDEVFS_DROP_PRIVILEGES _IO('U', 30)
>
> #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-26 9:19 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-26 9:19 UTC (permalink / raw)
To: Emilio López, gregkh, stern, kborer
Cc: reillyg, keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-26 9:19 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-26 9:19 UTC (permalink / raw)
To: Emilio López, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w
Cc: reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
2015-11-26 8:59 ` Peter Chen
(?)
@ 2015-11-26 9:20 ` Dan Carpenter
-1 siblings, 0 replies; 54+ messages in thread
From: Dan Carpenter @ 2015-11-26 9:20 UTC (permalink / raw)
To: Peter Chen
Cc: Emilio López, gregkh, stern, kborer, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo
On Thu, Nov 26, 2015 at 04:59:25PM +0800, Peter Chen wrote:
> On Wed, Nov 25, 2015 at 12:45:34PM -0300, Emilio López wrote:
> > From: Reilly Grant <reillyg@chromium.org>
> >
> > The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> > relinquish the ability to issue other ioctls that may interfere with
> > other processes and drivers that have claimed an interface on the
> > device.
> >
> > Signed-off-by: Reilly Grant <reillyg@chromium.org>
> > Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > [emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
> > Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> >
> > ---
> >
> > drivers/usb/core/devio.c | 50 +++++++++++++++++++++++++++++++++++----
> > include/uapi/linux/usbdevice_fs.h | 1 +
> > 2 files changed, 47 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index 38ae877c..a5ccc50 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -77,6 +77,7 @@ struct usb_dev_state {
> > unsigned long ifclaimed;
> > u32 secid;
> > u32 disabled_bulk_eps;
> > + bool privileges_dropped;
> > };
> >
> > struct async {
> > @@ -878,7 +879,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
> > int ret;
> >
> > ret = -ENOMEM;
> > - ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
> > + ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
> > if (!ps)
> > goto out_free_ps;
> >
> > @@ -911,11 +912,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
> > INIT_LIST_HEAD(&ps->async_pending);
> > INIT_LIST_HEAD(&ps->async_completed);
> > init_waitqueue_head(&ps->wait);
> > - ps->discsignr = 0;
> > ps->disc_pid = get_pid(task_pid(current));
> > ps->cred = get_current_cred();
> > - ps->disccontext = NULL;
> > - ps->ifclaimed = 0;
>
> The above changes seem to be not related with this change.
>
It is though. They added a new struct member and thought that now it
was cleaner to use kzalloc() instead of initializing the new member to
zero.
> > security_task_getsecid(current, &ps->secid);
> > smp_wmb();
> > list_add_tail(&ps->list, &dev->filelist);
> > @@ -1215,6 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
> >
> > static int proc_resetdevice(struct usb_dev_state *ps)
> > {
> > + if (ps->privileges_dropped) {
> > + struct usb_host_config *actconfig = ps->dev->actconfig;
> > +
> > + /* Don't touch the device if any interfaces are claimed. It
> > + * could interfere with other drivers' operations and this
> > + * process has dropped its privileges to do such things.
> > + */
> > + if (actconfig) {
> > + int i;
> > +
> > + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> > + if (usb_interface_claimed(
> > + actconfig->interface[i])) {
> > + dev_warn(&ps->dev->dev,
> > + "usbfs: interface %d claimed by"
> > + " %s while '%s'"
> > + " resets device\n",
> > + actconfig->interface[i]
> > + ->cur_altsetting
> > + ->desc.bInterfaceNumber,
>
> The length of line is 80 characters
>
> > + actconfig->interface[i]
> > + ->dev.driver->name,
> > + current->comm);
> > + return -EACCES;
> > + }
> > + }
> > + }
> > + }
Yeah. Better to flip this around:
static int proc_resetdevice(struct usb_dev_state *ps)
{
struct usb_host_config *actconfig = ps->dev->actconfig;
struct usb_interface *interface;
int i;
if (ps->privileges_dropped && actconfig) {
for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
interface = actconfig->interface[i];
if (usb_interface_claimed(interface)) {
dev_warn(&ps->dev->dev,
"usbfs: interface %d claimed by %s while '%s' resets device\n",
interface->cur_altsetting->desc.bInterfaceNumber,
interface->dev.driver->name,
current->comm);
return -EACCES;
}
}
}
return usb_reset_device(ps->dev);
}
It still goes over the 80 character limit, but that's cleaner than
breaking up the variable.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
2015-11-26 9:19 ` Krzysztof Opasiak
(?)
@ 2015-11-26 17:29 ` Greg KH
2015-11-27 8:44 ` Krzysztof Opasiak
-1 siblings, 1 reply; 54+ messages in thread
From: Greg KH @ 2015-11-26 17:29 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Emilio López, stern, kborer, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
>
>
> 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;))
Adding this option might be nice.
> >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.
> Moreover I'm not convinced to this patch as it hardcodes the *policy* in
> kernel code.
What policy is that?
> 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...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-27 8:44 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-27 8:44 UTC (permalink / raw)
To: Greg KH
Cc: Emilio López, stern, kborer, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
On 11/26/2015 06:29 PM, Greg KH wrote:
> On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
>>
>>
>> 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;))
>
> Adding this option might be nice.
>
>>> 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).
>
>> 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".
>
>> 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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-27 8:44 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-27 8:44 UTC (permalink / raw)
To: Greg KH
Cc: Emilio López, stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, reillyg-F7+t8E8rja9g9hUCZPvPmw,
keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On 11/26/2015 06:29 PM, Greg KH wrote:
> On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
>>
>>
>> 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;))
>
> Adding this option might be nice.
>
>>> 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).
>
>> 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".
>
>> 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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-28 2:39 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2015-11-28 2:39 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Emilio López, stern, kborer, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
On Fri, Nov 27, 2015 at 09:44:45AM +0100, Krzysztof Opasiak wrote:
>
>
> On 11/26/2015 06:29 PM, Greg KH wrote:
> >On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
> >>
> >>
> >>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;))
> >
> >Adding this option might be nice.
> >
> >>>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.
"should"? Depending on what? :)
> 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).
Yes, it's tough, I know, good luck.
Also deal with multiple devices, busses that are ordered differently
depending on the phase of the moon, and other fun things with dynamic
devices and ioctls. It's a loosing battle :)
> >>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".
>
> >
> >>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?
Dig through the archives, I think the last time this was brought up was
way before USB 3 came out. As for "not a good solution", you have to
map endpoints together somehow in some way in userspace, which gets
messy fast, and then you would have to somehow modify all userspace
programs to use the new model.
Good luck!
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-28 2:39 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2015-11-28 2:39 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Emilio López, stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, reillyg-F7+t8E8rja9g9hUCZPvPmw,
keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Fri, Nov 27, 2015 at 09:44:45AM +0100, Krzysztof Opasiak wrote:
>
>
> On 11/26/2015 06:29 PM, Greg KH wrote:
> >On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
> >>
> >>
> >>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;))
> >
> >Adding this option might be nice.
> >
> >>>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.
"should"? Depending on what? :)
> 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).
Yes, it's tough, I know, good luck.
Also deal with multiple devices, busses that are ordered differently
depending on the phase of the moon, and other fun things with dynamic
devices and ioctls. It's a loosing battle :)
> >>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".
>
> >
> >>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?
Dig through the archives, I think the last time this was brought up was
way before USB 3 came out. As for "not a good solution", you have to
map endpoints together somehow in some way in userspace, which gets
messy fast, and then you would have to somehow modify all userspace
programs to use the new model.
Good luck!
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 9:08 ` Oliver Neukum
0 siblings, 0 replies; 54+ messages in thread
From: Oliver Neukum @ 2015-11-30 9:08 UTC (permalink / raw)
To: Greg KH
Cc: Krzysztof Opasiak, Emilio López, stern, kborer, reillyg,
keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter
On Fri, 2015-11-27 at 18:39 -0800, Greg KH wrote:
> Yes, it's tough, I know, good luck.
>
> Also deal with multiple devices, busses that are ordered differently
> depending on the phase of the moon, and other fun things with dynamic
> devices and ioctls. It's a loosing battle :)
IMHO the fundamental problem is using one device node per device.
And I think it should be tackled. Yet I have no idea how to do
this in a compatible manner.
Regards
Oliver
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 9:08 ` Oliver Neukum
0 siblings, 0 replies; 54+ messages in thread
From: Oliver Neukum @ 2015-11-30 9:08 UTC (permalink / raw)
To: Greg KH
Cc: Krzysztof Opasiak, Emilio López,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, reillyg-F7+t8E8rja9g9hUCZPvPmw,
keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Fri, 2015-11-27 at 18:39 -0800, Greg KH wrote:
> Yes, it's tough, I know, good luck.
>
> Also deal with multiple devices, busses that are ordered differently
> depending on the phase of the moon, and other fun things with dynamic
> devices and ioctls. It's a loosing battle :)
IMHO the fundamental problem is using one device node per device.
And I think it should be tackled. Yet I have no idea how to do
this in a compatible manner.
Regards
Oliver
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
2015-11-27 8:44 ` Krzysztof Opasiak
(?)
(?)
@ 2015-11-30 16:16 ` Alan Stern
2015-11-30 17:12 ` Krzysztof Opasiak
-1 siblings, 1 reply; 54+ messages in thread
From: Alan Stern @ 2015-11-30 16:16 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Greg KH, Emilio López, kborer, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
> >> 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).
How about this approach? Once a process has dropped its usbfs
privileges, it's not allowed to claim any interfaces (either explicitly
or implicitly). Instead, it or some manager program must claim the
appropriate interfaces before dropping privileges.
Alan Stern
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 17:12 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-30 17:12 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, Emilio López, kborer, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
On 11/30/2015 05:16 PM, Alan Stern wrote:
> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>
>>>> 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).
>
> How about this approach? Once a process has dropped its usbfs
> privileges, it's not allowed to claim any interfaces (either explicitly
> or implicitly). Instead, it or some manager program must claim the
> appropriate interfaces before dropping privileges.
>
I agree that restricting interface claiming only to privileged process
is a good idea. Unfortunately this generates a problem when program
needs more than one interface (like in cdc - data + control for
example). We need to declare both of them in first call to "usb-manager"
or reopen the dev node at second call and claim all interfaces claimed
using this fd till now and claim one more and then drop privileges and
send a new fd.
Maybe better option would be to add optional argument to claim interface
ioctl() and allow to claim interface for other fd than the current one?
So "usb-manager" could have fd with full control and claim interfaces
for apps which have fds with restricted privileges.
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 17:12 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-30 17:12 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, Emilio López, kborer-Re5JQEeQqe8AvxtiuMwx3w,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On 11/30/2015 05:16 PM, Alan Stern wrote:
> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>
>>>> 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).
>
> How about this approach? Once a process has dropped its usbfs
> privileges, it's not allowed to claim any interfaces (either explicitly
> or implicitly). Instead, it or some manager program must claim the
> appropriate interfaces before dropping privileges.
>
I agree that restricting interface claiming only to privileged process
is a good idea. Unfortunately this generates a problem when program
needs more than one interface (like in cdc - data + control for
example). We need to declare both of them in first call to "usb-manager"
or reopen the dev node at second call and claim all interfaces claimed
using this fd till now and claim one more and then drop privileges and
send a new fd.
Maybe better option would be to add optional argument to claim interface
ioctl() and allow to claim interface for other fd than the current one?
So "usb-manager" could have fd with full control and claim interfaces
for apps which have fds with restricted privileges.
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 17:20 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2015-11-30 17:20 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Alan Stern, Emilio López, kborer, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo, dan.carpenter
On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>
>
> On 11/30/2015 05:16 PM, Alan Stern wrote:
> >On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
> >
> >>>>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).
> >
> >How about this approach? Once a process has dropped its usbfs
> >privileges, it's not allowed to claim any interfaces (either explicitly
> >or implicitly). Instead, it or some manager program must claim the
> >appropriate interfaces before dropping privileges.
> >
>
> I agree that restricting interface claiming only to privileged process is a
> good idea. Unfortunately this generates a problem when program needs more
> than one interface (like in cdc - data + control for example). We need to
> declare both of them in first call to "usb-manager" or reopen the dev node
> at second call and claim all interfaces claimed using this fd till now and
> claim one more and then drop privileges and send a new fd.
Have you seen such a device that is controlled this way in userspace?
Don't over-engineer something that is probably pretty rare...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 17:20 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2015-11-30 17:20 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Alan Stern, Emilio López, kborer-Re5JQEeQqe8AvxtiuMwx3w,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>
>
> On 11/30/2015 05:16 PM, Alan Stern wrote:
> >On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
> >
> >>>>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).
> >
> >How about this approach? Once a process has dropped its usbfs
> >privileges, it's not allowed to claim any interfaces (either explicitly
> >or implicitly). Instead, it or some manager program must claim the
> >appropriate interfaces before dropping privileges.
> >
>
> I agree that restricting interface claiming only to privileged process is a
> good idea. Unfortunately this generates a problem when program needs more
> than one interface (like in cdc - data + control for example). We need to
> declare both of them in first call to "usb-manager" or reopen the dev node
> at second call and claim all interfaces claimed using this fd till now and
> claim one more and then drop privileges and send a new fd.
Have you seen such a device that is controlled this way in userspace?
Don't over-engineer something that is probably pretty rare...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 18:48 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-30 18:48 UTC (permalink / raw)
To: Greg KH
Cc: Alan Stern, Emilio López, kborer, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo, dan.carpenter
On 11/30/2015 06:20 PM, Greg KH wrote:
> On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>>
>>
>> On 11/30/2015 05:16 PM, Alan Stern wrote:
>>> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>>>
>>>>>> 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).
>>>
>>> How about this approach? Once a process has dropped its usbfs
>>> privileges, it's not allowed to claim any interfaces (either explicitly
>>> or implicitly). Instead, it or some manager program must claim the
>>> appropriate interfaces before dropping privileges.
>>>
>>
>> I agree that restricting interface claiming only to privileged process is a
>> good idea. Unfortunately this generates a problem when program needs more
>> than one interface (like in cdc - data + control for example). We need to
>> declare both of them in first call to "usb-manager" or reopen the dev node
>> at second call and claim all interfaces claimed using this fd till now and
>> claim one more and then drop privileges and send a new fd.
>
> Have you seen such a device that is controlled this way in userspace?
> Don't over-engineer something that is probably pretty rare...
>
Yes I have seen such devices (not cdc of course) and they were driven
using libusb (vendor specific service + "driver" to bypass publishing
protocol code due to kernel's GPL). I have even seen an android app
written in java which claims and uses multiple interfaces using
android's USB API, so it's real;)
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2015-11-30 18:48 ` Krzysztof Opasiak
0 siblings, 0 replies; 54+ messages in thread
From: Krzysztof Opasiak @ 2015-11-30 18:48 UTC (permalink / raw)
To: Greg KH
Cc: Alan Stern, Emilio López, kborer-Re5JQEeQqe8AvxtiuMwx3w,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On 11/30/2015 06:20 PM, Greg KH wrote:
> On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>>
>>
>> On 11/30/2015 05:16 PM, Alan Stern wrote:
>>> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>>>
>>>>>> 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).
>>>
>>> How about this approach? Once a process has dropped its usbfs
>>> privileges, it's not allowed to claim any interfaces (either explicitly
>>> or implicitly). Instead, it or some manager program must claim the
>>> appropriate interfaces before dropping privileges.
>>>
>>
>> I agree that restricting interface claiming only to privileged process is a
>> good idea. Unfortunately this generates a problem when program needs more
>> than one interface (like in cdc - data + control for example). We need to
>> declare both of them in first call to "usb-manager" or reopen the dev node
>> at second call and claim all interfaces claimed using this fd till now and
>> claim one more and then drop privileges and send a new fd.
>
> Have you seen such a device that is controlled this way in userspace?
> Don't over-engineer something that is probably pretty rare...
>
Yes I have seen such devices (not cdc of course) and they were driven
using libusb (vendor specific service + "driver" to bypass publishing
protocol code due to kernel's GPL). I have even seen an android app
written in java which claims and uses multiple interfaces using
android's USB API, so it's real;)
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2016-01-19 16:39 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-19 16:39 UTC (permalink / raw)
To: Krzysztof Opasiak, Greg KH, Alan Stern
Cc: kborer, reillyg, keescook, linux-api, linux-usb, linux-kernel,
jorgelo, dan.carpenter
Hi,
I'm reviving this thread as there's no consensus so far, and I'd like to
see this supported in some way in mainline.
El 30/11/15 a las 15:48, Krzysztof Opasiak escribió:
>
>
> On 11/30/2015 06:20 PM, Greg KH wrote:
>> On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>>>
>>>
>>> On 11/30/2015 05:16 PM, Alan Stern wrote:
>>>> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>>>>
>>>>>>> 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).
>>>>
>>>> How about this approach? Once a process has dropped its usbfs
>>>> privileges, it's not allowed to claim any interfaces (either explicitly
>>>> or implicitly). Instead, it or some manager program must claim the
>>>> appropriate interfaces before dropping privileges.
>>>>
>>>
>>> I agree that restricting interface claiming only to privileged
>>> process is a
>>> good idea. Unfortunately this generates a problem when program needs
>>> more
>>> than one interface (like in cdc - data + control for example). We
>>> need to
>>> declare both of them in first call to "usb-manager" or reopen the dev
>>> node
>>> at second call and claim all interfaces claimed using this fd till
>>> now and
>>> claim one more and then drop privileges and send a new fd.
I talked with Reilly some time back and they proposed using an extra
parameter in the ioctl. This parameter would be a mask that specifies
which interfaces the unprivileged process is allowed to claim (but
doesn't necessarily have to do so). Providing a mask of ~0 would retain
the current patch behaviour, and allow existing programs using
out-of-tree implementations to continue working with little changes.
Now, if this were to be used on a system that knew better, via an app
manifest or similar, the manager process would generate a suitable mask
based on the interfaces required by the unprivileged process and use it
as a parameter when dropping privileges. This way, the unprivileged
process would be unable to claim interfaces it is not supposed to claim,
while allowing it the liberty to do as it wishes with the interfaces it
is allowed to use.
Krzysztof et al, would you find such an interface suitable? If you think
it could be a way forward I'll gladly write the code and send a new patch.
Cheers,
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2016-01-19 16:39 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-19 16:39 UTC (permalink / raw)
To: Krzysztof Opasiak, Greg KH, Alan Stern
Cc: kborer-Re5JQEeQqe8AvxtiuMwx3w, reillyg-F7+t8E8rja9g9hUCZPvPmw,
keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
Hi,
I'm reviving this thread as there's no consensus so far, and I'd like to
see this supported in some way in mainline.
El 30/11/15 a las 15:48, Krzysztof Opasiak escribió:
>
>
> On 11/30/2015 06:20 PM, Greg KH wrote:
>> On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>>>
>>>
>>> On 11/30/2015 05:16 PM, Alan Stern wrote:
>>>> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>>>>
>>>>>>> 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).
>>>>
>>>> How about this approach? Once a process has dropped its usbfs
>>>> privileges, it's not allowed to claim any interfaces (either explicitly
>>>> or implicitly). Instead, it or some manager program must claim the
>>>> appropriate interfaces before dropping privileges.
>>>>
>>>
>>> I agree that restricting interface claiming only to privileged
>>> process is a
>>> good idea. Unfortunately this generates a problem when program needs
>>> more
>>> than one interface (like in cdc - data + control for example). We
>>> need to
>>> declare both of them in first call to "usb-manager" or reopen the dev
>>> node
>>> at second call and claim all interfaces claimed using this fd till
>>> now and
>>> claim one more and then drop privileges and send a new fd.
I talked with Reilly some time back and they proposed using an extra
parameter in the ioctl. This parameter would be a mask that specifies
which interfaces the unprivileged process is allowed to claim (but
doesn't necessarily have to do so). Providing a mask of ~0 would retain
the current patch behaviour, and allow existing programs using
out-of-tree implementations to continue working with little changes.
Now, if this were to be used on a system that knew better, via an app
manifest or similar, the manager process would generate a suitable mask
based on the interfaces required by the unprivileged process and use it
as a parameter when dropping privileges. This way, the unprivileged
process would be unable to claim interfaces it is not supposed to claim,
while allowing it the liberty to do as it wishes with the interfaces it
is allowed to use.
Krzysztof et al, would you find such an interface suitable? If you think
it could be a way forward I'll gladly write the code and send a new patch.
Cheers,
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2016-01-19 18:07 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2016-01-19 18:07 UTC (permalink / raw)
To: Emilio López
Cc: Krzysztof Opasiak, Alan Stern, kborer, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo, dan.carpenter
On Tue, Jan 19, 2016 at 01:39:59PM -0300, Emilio López wrote:
> Hi,
>
> I'm reviving this thread as there's no consensus so far, and I'd like to see
> this supported in some way in mainline.
I don't see any patches to comment on here :)
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers
@ 2016-01-19 18:07 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2016-01-19 18:07 UTC (permalink / raw)
To: Emilio López
Cc: Krzysztof Opasiak, Alan Stern, kborer-Re5JQEeQqe8AvxtiuMwx3w,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Tue, Jan 19, 2016 at 01:39:59PM -0300, Emilio López wrote:
> Hi,
>
> I'm reviving this thread as there's no consensus so far, and I'd like to see
> this supported in some way in mainline.
I don't see any patches to comment on here :)
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-21 23:54 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-21 23:54 UTC (permalink / raw)
To: gregkh, stern, kborer, k.opasiak
Cc: reillyg, keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter, Emilio López
From: Reilly Grant <reillyg@chromium.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
Signed-off-by: Reilly Grant <reillyg@chromium.org>
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
I guess code talks more than words :) This patch is only build-tested,
and is meant to showcase the approach. The basic idea is to allow
limiting the interface claims via an extra parameter to resolve
Krzysztof's worries.
A longer paragraph explaining the idea can be seen at
http://www.spinics.net/lists/linux-usb/msg135271.html
Cheers!
Emilio
Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
process is allowed to claim.
- Drop Tested-by's
drivers/usb/core/devio.c | 65 ++++++++++++++++++++++++++++++++++++---
include/uapi/linux/usbdevice_fs.h | 5 +++
2 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 38ae877c..bf40aa6 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
+ unsigned long interface_allowed_mask;
};
struct async {
@@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
if (test_bit(ifnum, &ps->ifclaimed))
return 0;
+ if (ps->privileges_dropped) {
+ if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
+ return -EINVAL;
+
+ if (!test_bit(ifnum, &ps->interface_allowed_mask))
+ return -EACCES;
+ }
+
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
@@ -878,7 +888,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -911,11 +921,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1215,6 +1222,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+ struct usb_interface *interface;
+ int i, number;
+
+ /* Don't touch the device if any interfaces are claimed. It
+ * could interfere with other drivers' operations and this
+ * process has dropped its privileges to do such things.
+ */
+ if (ps->privileges_dropped && actconfig) {
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ interface = actconfig->interface[i];
+ number = interface->cur_altsetting->desc.bInterfaceNumber;
+ if (usb_interface_claimed(interface)) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by %s while '%s' resets device\n",
+ number, interface->dev.driver->name, current->comm);
+ return -EACCES;
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1922,6 +1950,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2074,6 +2105,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
if (intf->dev.driver) {
struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+ if (ps->privileges_dropped)
+ return -EACCES;
+
if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
strncmp(dc.driver, intf->dev.driver->name,
sizeof(dc.driver)) != 0)
@@ -2130,6 +2164,26 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+ struct usbdevfs_drop_privs data;
+
+ if (copy_from_user(&data, arg, sizeof(data)))
+ return -EFAULT;
+
+ /* This is a one way operation. Once privileges were dropped,
+ * you cannot do it again (Otherwise unprivileged processes
+ * would be able to change their allowed interfaces mask)
+ */
+ if (ps->privileges_dropped)
+ return -EACCES;
+
+ ps->interface_allowed_mask = data.interface_allowed_mask;
+ ps->privileges_dropped = true;
+
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2318,6 +2372,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps, p);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..9abcb34 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -154,6 +154,10 @@ struct usbdevfs_streams {
unsigned char eps[0];
};
+struct usbdevfs_drop_privs {
+ unsigned long interface_allowed_mask;
+};
+
#define USBDEVFS_CONTROL _IOWR('U', 0, struct usbdevfs_ctrltransfer)
#define USBDEVFS_CONTROL32 _IOWR('U', 0, struct usbdevfs_ctrltransfer32)
#define USBDEVFS_BULK _IOWR('U', 2, struct usbdevfs_bulktransfer)
@@ -187,5 +191,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IOR('U', 30, struct usbdevfs_drop_privs)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-21 23:54 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-21 23:54 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ
Cc: reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA, Emilio López
From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---
I guess code talks more than words :) This patch is only build-tested,
and is meant to showcase the approach. The basic idea is to allow
limiting the interface claims via an extra parameter to resolve
Krzysztof's worries.
A longer paragraph explaining the idea can be seen at
http://www.spinics.net/lists/linux-usb/msg135271.html
Cheers!
Emilio
Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
process is allowed to claim.
- Drop Tested-by's
drivers/usb/core/devio.c | 65 ++++++++++++++++++++++++++++++++++++---
include/uapi/linux/usbdevice_fs.h | 5 +++
2 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 38ae877c..bf40aa6 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
+ unsigned long interface_allowed_mask;
};
struct async {
@@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
if (test_bit(ifnum, &ps->ifclaimed))
return 0;
+ if (ps->privileges_dropped) {
+ if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
+ return -EINVAL;
+
+ if (!test_bit(ifnum, &ps->interface_allowed_mask))
+ return -EACCES;
+ }
+
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
@@ -878,7 +888,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -911,11 +921,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1215,6 +1222,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+ struct usb_interface *interface;
+ int i, number;
+
+ /* Don't touch the device if any interfaces are claimed. It
+ * could interfere with other drivers' operations and this
+ * process has dropped its privileges to do such things.
+ */
+ if (ps->privileges_dropped && actconfig) {
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ interface = actconfig->interface[i];
+ number = interface->cur_altsetting->desc.bInterfaceNumber;
+ if (usb_interface_claimed(interface)) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by %s while '%s' resets device\n",
+ number, interface->dev.driver->name, current->comm);
+ return -EACCES;
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1922,6 +1950,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2074,6 +2105,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
if (intf->dev.driver) {
struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+ if (ps->privileges_dropped)
+ return -EACCES;
+
if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
strncmp(dc.driver, intf->dev.driver->name,
sizeof(dc.driver)) != 0)
@@ -2130,6 +2164,26 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+ struct usbdevfs_drop_privs data;
+
+ if (copy_from_user(&data, arg, sizeof(data)))
+ return -EFAULT;
+
+ /* This is a one way operation. Once privileges were dropped,
+ * you cannot do it again (Otherwise unprivileged processes
+ * would be able to change their allowed interfaces mask)
+ */
+ if (ps->privileges_dropped)
+ return -EACCES;
+
+ ps->interface_allowed_mask = data.interface_allowed_mask;
+ ps->privileges_dropped = true;
+
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2318,6 +2372,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps, p);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..9abcb34 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -154,6 +154,10 @@ struct usbdevfs_streams {
unsigned char eps[0];
};
+struct usbdevfs_drop_privs {
+ unsigned long interface_allowed_mask;
+};
+
#define USBDEVFS_CONTROL _IOWR('U', 0, struct usbdevfs_ctrltransfer)
#define USBDEVFS_CONTROL32 _IOWR('U', 0, struct usbdevfs_ctrltransfer32)
#define USBDEVFS_BULK _IOWR('U', 2, struct usbdevfs_bulktransfer)
@@ -187,5 +191,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IOR('U', 30, struct usbdevfs_drop_privs)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-22 9:41 ` Bjørn Mork
0 siblings, 0 replies; 54+ messages in thread
From: Bjørn Mork @ 2016-01-22 9:41 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, stern, kborer, k.opasiak, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
Emilio López <emilio.lopez@collabora.co.uk> writes:
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..bf40aa6 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,8 @@ struct usb_dev_state {
> unsigned long ifclaimed;
> u32 secid;
> u32 disabled_bulk_eps;
> + bool privileges_dropped;
> + unsigned long interface_allowed_mask;
> };
>
> struct async {
> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> if (test_bit(ifnum, &ps->ifclaimed))
> return 0;
>
> + if (ps->privileges_dropped) {
> + if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
> + return -EINVAL;
I don't think you need this runtime test. You can just make sure that
sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
time.
I do find this variable and arbitrary limit a bit confusing, but that's
not your fault - I guess it is an indication that ifnums > 31 are rare
:)
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..9abcb34 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
> unsigned char eps[0];
> };
>
> +struct usbdevfs_drop_privs {
> + unsigned long interface_allowed_mask;
> +};
> +
"unsigned long" isn't a very good choice here, is it?
Bjørn
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-22 9:41 ` Bjørn Mork
0 siblings, 0 replies; 54+ messages in thread
From: Bjørn Mork @ 2016-01-22 9:41 UTC (permalink / raw)
To: Emilio López
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org> writes:
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..bf40aa6 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,8 @@ struct usb_dev_state {
> unsigned long ifclaimed;
> u32 secid;
> u32 disabled_bulk_eps;
> + bool privileges_dropped;
> + unsigned long interface_allowed_mask;
> };
>
> struct async {
> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> if (test_bit(ifnum, &ps->ifclaimed))
> return 0;
>
> + if (ps->privileges_dropped) {
> + if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
> + return -EINVAL;
I don't think you need this runtime test. You can just make sure that
sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
time.
I do find this variable and arbitrary limit a bit confusing, but that's
not your fault - I guess it is an indication that ifnums > 31 are rare
:)
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..9abcb34 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
> unsigned char eps[0];
> };
>
> +struct usbdevfs_drop_privs {
> + unsigned long interface_allowed_mask;
> +};
> +
"unsigned long" isn't a very good choice here, is it?
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-22 16:10 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-01-22 16:10 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, kborer, k.opasiak, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
On Thu, 21 Jan 2016, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> static int proc_resetdevice(struct usb_dev_state *ps)
> {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> + struct usb_interface *interface;
> + int i, number;
> +
> + /* Don't touch the device if any interfaces are claimed. It
> + * could interfere with other drivers' operations and this
> + * process has dropped its privileges to do such things.
> + */
This comment should be rephrased. It should say something like:
"Don't allow if the process has dropped its privilege to do such
things and any of the interfaces are claimed."
You also might consider allowing the reset if the interfaces are
claimed only by the current process (or more precisely, by ps).
> +static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
> +{
> + struct usbdevfs_drop_privs data;
> +
> + if (copy_from_user(&data, arg, sizeof(data)))
> + return -EFAULT;
> +
> + /* This is a one way operation. Once privileges were dropped,
> + * you cannot do it again (Otherwise unprivileged processes
> + * would be able to change their allowed interfaces mask)
> + */
If you're going to keep a mask of claimable interfaces then there's no
reason this has to be a one-time operation. Processes should always be
allowed to shrink the mask, just not to grow it.
Alan Stern
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-22 16:10 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-01-22 16:10 UTC (permalink / raw)
To: Emilio López
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Thu, 21 Jan 2016, Emilio López wrote:
> From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> static int proc_resetdevice(struct usb_dev_state *ps)
> {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> + struct usb_interface *interface;
> + int i, number;
> +
> + /* Don't touch the device if any interfaces are claimed. It
> + * could interfere with other drivers' operations and this
> + * process has dropped its privileges to do such things.
> + */
This comment should be rephrased. It should say something like:
"Don't allow if the process has dropped its privilege to do such
things and any of the interfaces are claimed."
You also might consider allowing the reset if the interfaces are
claimed only by the current process (or more precisely, by ps).
> +static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
> +{
> + struct usbdevfs_drop_privs data;
> +
> + if (copy_from_user(&data, arg, sizeof(data)))
> + return -EFAULT;
> +
> + /* This is a one way operation. Once privileges were dropped,
> + * you cannot do it again (Otherwise unprivileged processes
> + * would be able to change their allowed interfaces mask)
> + */
If you're going to keep a mask of claimable interfaces then there's no
reason this has to be a one-time operation. Processes should always be
allowed to shrink the mask, just not to grow it.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 1:40 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-25 1:40 UTC (permalink / raw)
To: Bjørn Mork
Cc: gregkh, stern, kborer, k.opasiak, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
Hi Bjørn,
El 22/01/16 a las 06:41, Bjørn Mork escribió:
> Emilio López <emilio.lopez@collabora.co.uk> writes:
>
>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>> index 38ae877c..bf40aa6 100644
>> --- a/drivers/usb/core/devio.c
>> +++ b/drivers/usb/core/devio.c
>> @@ -77,6 +77,8 @@ struct usb_dev_state {
>> unsigned long ifclaimed;
>> u32 secid;
>> u32 disabled_bulk_eps;
>> + bool privileges_dropped;
>> + unsigned long interface_allowed_mask;
>> };
>>
>> struct async {
>> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>> if (test_bit(ifnum, &ps->ifclaimed))
>> return 0;
>>
>> + if (ps->privileges_dropped) {
>> + if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
>> + return -EINVAL;
>
>
> I don't think you need this runtime test. You can just make sure that
> sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
> time.
>
> I do find this variable and arbitrary limit a bit confusing, but that's
> not your fault - I guess it is an indication that ifnums > 31 are rare
> :)
>
>
>> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
>> index 019ba1e..9abcb34 100644
>> --- a/include/uapi/linux/usbdevice_fs.h
>> +++ b/include/uapi/linux/usbdevice_fs.h
>> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>> unsigned char eps[0];
>> };
>>
>> +struct usbdevfs_drop_privs {
>> + unsigned long interface_allowed_mask;
>> +};
>> +
>
> "unsigned long" isn't a very good choice here, is it?
I went with a type matching ifclaimed on struct usb_dev_state to keep
the limit the same, but I guess it's not the best idea for an ioctl. I
can switch it to __u32, keeping the runtime check above as is, or use
__u64. Which one would you prefer?
Thanks for the review!
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 1:40 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-25 1:40 UTC (permalink / raw)
To: Bjørn Mork
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
Hi Bjørn,
El 22/01/16 a las 06:41, Bjørn Mork escribió:
> Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org> writes:
>
>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>> index 38ae877c..bf40aa6 100644
>> --- a/drivers/usb/core/devio.c
>> +++ b/drivers/usb/core/devio.c
>> @@ -77,6 +77,8 @@ struct usb_dev_state {
>> unsigned long ifclaimed;
>> u32 secid;
>> u32 disabled_bulk_eps;
>> + bool privileges_dropped;
>> + unsigned long interface_allowed_mask;
>> };
>>
>> struct async {
>> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>> if (test_bit(ifnum, &ps->ifclaimed))
>> return 0;
>>
>> + if (ps->privileges_dropped) {
>> + if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
>> + return -EINVAL;
>
>
> I don't think you need this runtime test. You can just make sure that
> sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
> time.
>
> I do find this variable and arbitrary limit a bit confusing, but that's
> not your fault - I guess it is an indication that ifnums > 31 are rare
> :)
>
>
>> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
>> index 019ba1e..9abcb34 100644
>> --- a/include/uapi/linux/usbdevice_fs.h
>> +++ b/include/uapi/linux/usbdevice_fs.h
>> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>> unsigned char eps[0];
>> };
>>
>> +struct usbdevfs_drop_privs {
>> + unsigned long interface_allowed_mask;
>> +};
>> +
>
> "unsigned long" isn't a very good choice here, is it?
I went with a type matching ifclaimed on struct usb_dev_state to keep
the limit the same, but I guess it's not the best idea for an ioctl. I
can switch it to __u32, keeping the runtime check above as is, or use
__u64. Which one would you prefer?
Thanks for the review!
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
2016-01-22 16:10 ` Alan Stern
(?)
@ 2016-01-25 2:01 ` Emilio López
-1 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-01-25 2:01 UTC (permalink / raw)
To: Alan Stern
Cc: gregkh, kborer, k.opasiak, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
Hi Alan,
El 22/01/16 a las 13:10, Alan Stern escribió:
> On Thu, 21 Jan 2016, Emilio López wrote:
>
>> From: Reilly Grant <reillyg@chromium.org>
>>
>> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
>> relinquish the ability to issue other ioctls that may interfere with
>> other processes and drivers that have claimed an interface on the
>> device.
>>
>> Signed-off-by: Reilly Grant <reillyg@chromium.org>
>> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>
>
>> static int proc_resetdevice(struct usb_dev_state *ps)
>> {
>> + struct usb_host_config *actconfig = ps->dev->actconfig;
>> + struct usb_interface *interface;
>> + int i, number;
>> +
>> + /* Don't touch the device if any interfaces are claimed. It
>> + * could interfere with other drivers' operations and this
>> + * process has dropped its privileges to do such things.
>> + */
>
> This comment should be rephrased. It should say something like:
> "Don't allow if the process has dropped its privilege to do such
> things and any of the interfaces are claimed."
I have replaced it with the following now
/* Don't allow a device reset if the process has dropped the
* privilege to do such things and any of the interfaces are
* currently claimed.
*/
> You also might consider allowing the reset if the interfaces are
> claimed only by the current process (or more precisely, by ps).
>
>> +static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
>> +{
>> + struct usbdevfs_drop_privs data;
>> +
>> + if (copy_from_user(&data, arg, sizeof(data)))
>> + return -EFAULT;
>> +
>> + /* This is a one way operation. Once privileges were dropped,
>> + * you cannot do it again (Otherwise unprivileged processes
>> + * would be able to change their allowed interfaces mask)
>> + */
>
> If you're going to keep a mask of claimable interfaces then there's no
> reason this has to be a one-time operation. Processes should always be
> allowed to shrink the mask, just not to grow it.
Good point, I've changed this to look like the following
/* This is an one way operation. Once privileges are
* dropped, you cannot regain them. You may however reissue
* this ioctl to shrink the allowed interfaces mask.
*/
if (ps->privileges_dropped)
ps->interface_allowed_mask &= data.interface_allowed_mask;
else
ps->interface_allowed_mask = data.interface_allowed_mask;
ps->privileges_dropped = true;
Or maybe I could change the default mask to ~0 and simplify this a bit, hm.
Thank you for the review!
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
2016-01-25 1:40 ` Emilio López
(?)
@ 2016-01-25 8:39 ` Bjørn Mork
2016-01-25 15:21 ` Alan Stern
-1 siblings, 1 reply; 54+ messages in thread
From: Bjørn Mork @ 2016-01-25 8:39 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, stern, kborer, k.opasiak, reillyg, keescook, linux-api,
linux-usb, linux-kernel, jorgelo, dan.carpenter
Emilio López <emilio.lopez@collabora.co.uk> writes:
>>> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
>>> index 019ba1e..9abcb34 100644
>>> --- a/include/uapi/linux/usbdevice_fs.h
>>> +++ b/include/uapi/linux/usbdevice_fs.h
>>> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>>> unsigned char eps[0];
>>> };
>>>
>>> +struct usbdevfs_drop_privs {
>>> + unsigned long interface_allowed_mask;
>>> +};
>>> +
>>
>> "unsigned long" isn't a very good choice here, is it?
>
> I went with a type matching ifclaimed on struct usb_dev_state to keep
> the limit the same, but I guess it's not the best idea for an ioctl. I
> can switch it to __u32, keeping the runtime check above as is, or use
> __u64. Which one would you prefer?
I don't feel much like an expert here, but I can certainly make up an
opinion anyway :)
Since 64bits kernels allow usb devio with interface numbers up to 63, I
guess you need __u64 to avoid limiting the range? Limiting will create
all sorts of followup problems, so it's definitely easiest to just go
with __u64.
Bjørn
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 15:21 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-01-25 15:21 UTC (permalink / raw)
To: Bjørn Mork
Cc: Emilio López, gregkh, kborer, k.opasiak, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo, dan.carpenter
On Mon, 25 Jan 2016, Bjørn Mork wrote:
> I don't feel much like an expert here, but I can certainly make up an
> opinion anyway :)
>
> Since 64bits kernels allow usb devio with interface numbers up to 63, I
> guess you need __u64 to avoid limiting the range? Limiting will create
> all sorts of followup problems, so it's definitely easiest to just go
> with __u64.
But the Linux USB stack only allows up to 32 interfaces (see
include/linux/usb.h):
/* this maximum is arbitrary */
#define USB_MAXINTERFACES 32
So there's no point using a 64-bit value.
On the other hand, this value is supposed to be the same size as
ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(),
and test_bit(). Those routines require unsigned long.
Alan Stern
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 15:21 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-01-25 15:21 UTC (permalink / raw)
To: Bjørn Mork
Cc: Emilio López, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Mon, 25 Jan 2016, Bjørn Mork wrote:
> I don't feel much like an expert here, but I can certainly make up an
> opinion anyway :)
>
> Since 64bits kernels allow usb devio with interface numbers up to 63, I
> guess you need __u64 to avoid limiting the range? Limiting will create
> all sorts of followup problems, so it's definitely easiest to just go
> with __u64.
But the Linux USB stack only allows up to 32 interfaces (see
include/linux/usb.h):
/* this maximum is arbitrary */
#define USB_MAXINTERFACES 32
So there's no point using a 64-bit value.
On the other hand, this value is supposed to be the same size as
ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(),
and test_bit(). Those routines require unsigned long.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 15:32 ` Bjørn Mork
0 siblings, 0 replies; 54+ messages in thread
From: Bjørn Mork @ 2016-01-25 15:32 UTC (permalink / raw)
To: Alan Stern
Cc: Emilio López, gregkh, kborer, k.opasiak, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo, dan.carpenter
Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 25 Jan 2016, Bjørn Mork wrote:
>
>> I don't feel much like an expert here, but I can certainly make up an
>> opinion anyway :)
>>
>> Since 64bits kernels allow usb devio with interface numbers up to 63, I
>> guess you need __u64 to avoid limiting the range? Limiting will create
>> all sorts of followup problems, so it's definitely easiest to just go
>> with __u64.
>
> But the Linux USB stack only allows up to 32 interfaces (see
> include/linux/usb.h):
>
> /* this maximum is arbitrary */
> #define USB_MAXINTERFACES 32
Ah, I totally missed that. Thanks
> So there's no point using a 64-bit value.
>
> On the other hand, this value is supposed to be the same size as
> ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(),
> and test_bit(). Those routines require unsigned long.
Maybe the input to these should be clamped to USB_MAXINTERFACES?
Bjørn
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 15:32 ` Bjørn Mork
0 siblings, 0 replies; 54+ messages in thread
From: Bjørn Mork @ 2016-01-25 15:32 UTC (permalink / raw)
To: Alan Stern
Cc: Emilio López, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> writes:
> On Mon, 25 Jan 2016, Bjørn Mork wrote:
>
>> I don't feel much like an expert here, but I can certainly make up an
>> opinion anyway :)
>>
>> Since 64bits kernels allow usb devio with interface numbers up to 63, I
>> guess you need __u64 to avoid limiting the range? Limiting will create
>> all sorts of followup problems, so it's definitely easiest to just go
>> with __u64.
>
> But the Linux USB stack only allows up to 32 interfaces (see
> include/linux/usb.h):
>
> /* this maximum is arbitrary */
> #define USB_MAXINTERFACES 32
Ah, I totally missed that. Thanks
> So there's no point using a 64-bit value.
>
> On the other hand, this value is supposed to be the same size as
> ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(),
> and test_bit(). Those routines require unsigned long.
Maybe the input to these should be clamped to USB_MAXINTERFACES?
Bjørn
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 15:46 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-01-25 15:46 UTC (permalink / raw)
To: Bjørn Mork
Cc: Emilio López, gregkh, kborer, k.opasiak, reillyg, keescook,
linux-api, linux-usb, linux-kernel, jorgelo, dan.carpenter
On Mon, 25 Jan 2016, Bjørn Mork wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Mon, 25 Jan 2016, Bjørn Mork wrote:
> >
> >> I don't feel much like an expert here, but I can certainly make up an
> >> opinion anyway :)
> >>
> >> Since 64bits kernels allow usb devio with interface numbers up to 63, I
> >> guess you need __u64 to avoid limiting the range? Limiting will create
> >> all sorts of followup problems, so it's definitely easiest to just go
> >> with __u64.
> >
> > But the Linux USB stack only allows up to 32 interfaces (see
> > include/linux/usb.h):
> >
> > /* this maximum is arbitrary */
> > #define USB_MAXINTERFACES 32
>
>
> Ah, I totally missed that. Thanks
>
>
> > So there's no point using a 64-bit value.
> >
> > On the other hand, this value is supposed to be the same size as
> > ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(),
> > and test_bit(). Those routines require unsigned long.
>
> Maybe the input to these should be clamped to USB_MAXINTERFACES?
If you want. Right now it's clamped to 8 * sizeof(ps->ifclaimed),
which ought to be good enough.
Oh yes, there's one other thing to notice. The value passed to these
routines is an interface number (as opposed to an index). According to
the USB spec, interfaces are supposed to be numbered sequentially
starting from 0, but there may be some devices that mess this up. So
it's possible we'll see an interface number which is larger than the
number of interfaces! :-)
Alan Stern
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-01-25 15:46 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-01-25 15:46 UTC (permalink / raw)
To: Bjørn Mork
Cc: Emilio López, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
kborer-Re5JQEeQqe8AvxtiuMwx3w, k.opasiak-Sze3O3UU22JBDgjK7y7TUQ,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Mon, 25 Jan 2016, Bjørn Mork wrote:
> Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> writes:
> > On Mon, 25 Jan 2016, Bjørn Mork wrote:
> >
> >> I don't feel much like an expert here, but I can certainly make up an
> >> opinion anyway :)
> >>
> >> Since 64bits kernels allow usb devio with interface numbers up to 63, I
> >> guess you need __u64 to avoid limiting the range? Limiting will create
> >> all sorts of followup problems, so it's definitely easiest to just go
> >> with __u64.
> >
> > But the Linux USB stack only allows up to 32 interfaces (see
> > include/linux/usb.h):
> >
> > /* this maximum is arbitrary */
> > #define USB_MAXINTERFACES 32
>
>
> Ah, I totally missed that. Thanks
>
>
> > So there's no point using a 64-bit value.
> >
> > On the other hand, this value is supposed to be the same size as
> > ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(),
> > and test_bit(). Those routines require unsigned long.
>
> Maybe the input to these should be clamped to USB_MAXINTERFACES?
If you want. Right now it's clamped to 8 * sizeof(ps->ifclaimed),
which ought to be good enough.
Oh yes, there's one other thing to notice. The value passed to these
routines is an interface number (as opposed to an index). According to
the USB spec, interfaces are supposed to be numbered sequentially
starting from 0, but there may be some devices that mess this up. So
it's possible we'll see an interface number which is larger than the
number of interfaces! :-)
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-04 3:20 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-02-04 3:20 UTC (permalink / raw)
To: gregkh, stern, kborer, bjorn
Cc: reillyg, keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter, Emilio López
From: Reilly Grant <reillyg@chromium.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
Signed-off-by: Reilly Grant <reillyg@chromium.org>
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
Changes in v3:
- Switch ioctl to use a __u32 given the iface qty is capped at 32
- Reword comments as requested by Alan
- Allow callers to shrink the allowed interfaces mask
Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
process is allowed to claim.
- Drop Tested-by's
drivers/usb/core/devio.c | 59 ++++++++++++++++++++++++++++++++++++---
include/uapi/linux/usbdevice_fs.h | 1 +
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 59e7a33..3750255 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
+ unsigned long interface_allowed_mask;
};
struct async {
@@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
if (test_bit(ifnum, &ps->ifclaimed))
return 0;
+ if (ps->privileges_dropped
+ && !test_bit(ifnum, &ps->interface_allowed_mask))
+ return -EACCES;
+
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
@@ -861,7 +867,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -889,16 +895,14 @@ static int usbdev_open(struct inode *inode, struct file *file)
ps->dev = dev;
ps->file = file;
+ ps->interface_allowed_mask = 0xFFFFFFFF; /* 32 bits */
spin_lock_init(&ps->lock);
INIT_LIST_HEAD(&ps->list);
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+ struct usb_interface *interface;
+ int i, number;
+
+ /* Don't allow a device reset if the process has dropped the
+ * privilege to do such things and any of the interfaces are
+ * currently claimed.
+ */
+ if (ps->privileges_dropped && actconfig) {
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ interface = actconfig->interface[i];
+ number = interface->cur_altsetting->desc.bInterfaceNumber;
+ if (usb_interface_claimed(interface)) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by %s while '%s' resets device\n",
+ number, interface->dev.driver->name, current->comm);
+ return -EACCES;
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1915,6 +1940,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2067,6 +2095,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
if (intf->dev.driver) {
struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+ if (ps->privileges_dropped)
+ return -EACCES;
+
if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
strncmp(dc.driver, intf->dev.driver->name,
sizeof(dc.driver)) != 0)
@@ -2123,6 +2154,23 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+ u32 data;
+
+ if (copy_from_user(&data, arg, sizeof(data)))
+ return -EFAULT;
+
+ /* This is an one way operation. Once privileges are
+ * dropped, you cannot regain them. You may however reissue
+ * this ioctl to shrink the allowed interfaces mask.
+ */
+ ps->interface_allowed_mask &= data;
+ ps->privileges_dropped = true;
+
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2311,6 +2359,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps, p);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..2fad9f85 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -187,5 +187,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IOR('U', 30, __u32)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-04 3:20 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-02-04 3:20 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, bjorn-yOkvZcmFvRU
Cc: reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA, Emilio López
From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---
Changes in v3:
- Switch ioctl to use a __u32 given the iface qty is capped at 32
- Reword comments as requested by Alan
- Allow callers to shrink the allowed interfaces mask
Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
process is allowed to claim.
- Drop Tested-by's
drivers/usb/core/devio.c | 59 ++++++++++++++++++++++++++++++++++++---
include/uapi/linux/usbdevice_fs.h | 1 +
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 59e7a33..3750255 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
+ unsigned long interface_allowed_mask;
};
struct async {
@@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
if (test_bit(ifnum, &ps->ifclaimed))
return 0;
+ if (ps->privileges_dropped
+ && !test_bit(ifnum, &ps->interface_allowed_mask))
+ return -EACCES;
+
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
@@ -861,7 +867,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -889,16 +895,14 @@ static int usbdev_open(struct inode *inode, struct file *file)
ps->dev = dev;
ps->file = file;
+ ps->interface_allowed_mask = 0xFFFFFFFF; /* 32 bits */
spin_lock_init(&ps->lock);
INIT_LIST_HEAD(&ps->list);
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+ struct usb_interface *interface;
+ int i, number;
+
+ /* Don't allow a device reset if the process has dropped the
+ * privilege to do such things and any of the interfaces are
+ * currently claimed.
+ */
+ if (ps->privileges_dropped && actconfig) {
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ interface = actconfig->interface[i];
+ number = interface->cur_altsetting->desc.bInterfaceNumber;
+ if (usb_interface_claimed(interface)) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by %s while '%s' resets device\n",
+ number, interface->dev.driver->name, current->comm);
+ return -EACCES;
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1915,6 +1940,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2067,6 +2095,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
if (intf->dev.driver) {
struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+ if (ps->privileges_dropped)
+ return -EACCES;
+
if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
strncmp(dc.driver, intf->dev.driver->name,
sizeof(dc.driver)) != 0)
@@ -2123,6 +2154,23 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+ u32 data;
+
+ if (copy_from_user(&data, arg, sizeof(data)))
+ return -EFAULT;
+
+ /* This is an one way operation. Once privileges are
+ * dropped, you cannot regain them. You may however reissue
+ * this ioctl to shrink the allowed interfaces mask.
+ */
+ ps->interface_allowed_mask &= data;
+ ps->privileges_dropped = true;
+
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2311,6 +2359,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps, p);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..2fad9f85 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -187,5 +187,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IOR('U', 30, __u32)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-04 3:46 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2016-02-04 3:46 UTC (permalink / raw)
To: Emilio López
Cc: stern, kborer, bjorn, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
On Thu, Feb 04, 2016 at 12:20:57AM -0300, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>
> ---
>
> Changes in v3:
> - Switch ioctl to use a __u32 given the iface qty is capped at 32
> - Reword comments as requested by Alan
> - Allow callers to shrink the allowed interfaces mask
No documentation? Proof it works? Test scripts?
This isn't a trivial feature to add, how do we know it is correct?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-04 3:46 ` Greg KH
0 siblings, 0 replies; 54+ messages in thread
From: Greg KH @ 2016-02-04 3:46 UTC (permalink / raw)
To: Emilio López
Cc: stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w, bjorn-yOkvZcmFvRU,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Thu, Feb 04, 2016 at 12:20:57AM -0300, Emilio López wrote:
> From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
>
> ---
>
> Changes in v3:
> - Switch ioctl to use a __u32 given the iface qty is capped at 32
> - Reword comments as requested by Alan
> - Allow callers to shrink the allowed interfaces mask
No documentation? Proof it works? Test scripts?
This isn't a trivial feature to add, how do we know it is correct?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
2016-02-04 3:20 ` Emilio López
@ 2016-02-04 16:27 ` Alan Stern
-1 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-02-04 16:27 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, kborer, bjorn, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
On Thu, 4 Feb 2016, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>
> ---
>
> Changes in v3:
> - Switch ioctl to use a __u32 given the iface qty is capped at 32
> - Reword comments as requested by Alan
> - Allow callers to shrink the allowed interfaces mask
> @@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> if (test_bit(ifnum, &ps->ifclaimed))
> return 0;
>
> + if (ps->privileges_dropped
> + && !test_bit(ifnum, &ps->interface_allowed_mask))
Continuation lines in this file are indented by 2 tab stops, not 1
space.
> @@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>
> static int proc_resetdevice(struct usb_dev_state *ps)
> {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> + struct usb_interface *interface;
> + int i, number;
> +
> + /* Don't allow a device reset if the process has dropped the
> + * privilege to do such things and any of the interfaces are
> + * currently claimed.
> + */
> + if (ps->privileges_dropped && actconfig) {
> + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> + interface = actconfig->interface[i];
> + number = interface->cur_altsetting->desc.bInterfaceNumber;
> + if (usb_interface_claimed(interface)) {
The test should be:
if (usb_interface_claimed(interface) &&
!test_bit(number, &ps->ifclaimed)) {
We don't want to prevent people from resetting a device merely because
they have claimed an interface. Only if someone else has claimed one.
> + dev_warn(&ps->dev->dev,
> + "usbfs: interface %d claimed by %s while '%s' resets device\n",
> + number, interface->dev.driver->name, current->comm);
> + return -EACCES;
> + }
> + }
> + }
> +
> return usb_reset_device(ps->dev);
> }
Also, it wouldn't hurt to change proc_get_capabilities() and
include/uapi/linux/usbdevicefs.h to add a USBDEVFS_CAP_DROP_PRIVILEGES
bit.
Alan Stern
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-04 16:27 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-02-04 16:27 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, kborer, bjorn, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
On Thu, 4 Feb 2016, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>
> ---
>
> Changes in v3:
> - Switch ioctl to use a __u32 given the iface qty is capped at 32
> - Reword comments as requested by Alan
> - Allow callers to shrink the allowed interfaces mask
> @@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> if (test_bit(ifnum, &ps->ifclaimed))
> return 0;
>
> + if (ps->privileges_dropped
> + && !test_bit(ifnum, &ps->interface_allowed_mask))
Continuation lines in this file are indented by 2 tab stops, not 1
space.
> @@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>
> static int proc_resetdevice(struct usb_dev_state *ps)
> {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> + struct usb_interface *interface;
> + int i, number;
> +
> + /* Don't allow a device reset if the process has dropped the
> + * privilege to do such things and any of the interfaces are
> + * currently claimed.
> + */
> + if (ps->privileges_dropped && actconfig) {
> + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> + interface = actconfig->interface[i];
> + number = interface->cur_altsetting->desc.bInterfaceNumber;
> + if (usb_interface_claimed(interface)) {
The test should be:
if (usb_interface_claimed(interface) &&
!test_bit(number, &ps->ifclaimed)) {
We don't want to prevent people from resetting a device merely because
they have claimed an interface. Only if someone else has claimed one.
> + dev_warn(&ps->dev->dev,
> + "usbfs: interface %d claimed by %s while '%s' resets device\n",
> + number, interface->dev.driver->name, current->comm);
> + return -EACCES;
> + }
> + }
> + }
> +
> return usb_reset_device(ps->dev);
> }
Also, it wouldn't hurt to change proc_get_capabilities() and
include/uapi/linux/usbdevicefs.h to add a USBDEVFS_CAP_DROP_PRIVILEGES
bit.
Alan Stern
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-08 1:56 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-02-08 1:56 UTC (permalink / raw)
To: Alan Stern
Cc: gregkh, kborer, bjorn, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
Hello Alan,
El 04/02/16 a las 13:27, Alan Stern escribió:
> On Thu, 4 Feb 2016, Emilio López wrote:
>
>> From: Reilly Grant <reillyg@chromium.org>
>>
>> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
>> relinquish the ability to issue other ioctls that may interfere with
>> other processes and drivers that have claimed an interface on the
>> device.
>>
>> Signed-off-by: Reilly Grant <reillyg@chromium.org>
>> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>>
>> ---
>>
>> Changes in v3:
>> - Switch ioctl to use a __u32 given the iface qty is capped at 32
>> - Reword comments as requested by Alan
>> - Allow callers to shrink the allowed interfaces mask
>
>> @@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>> if (test_bit(ifnum, &ps->ifclaimed))
>> return 0;
>>
>> + if (ps->privileges_dropped
>> + && !test_bit(ifnum, &ps->interface_allowed_mask))
>
> Continuation lines in this file are indented by 2 tab stops, not 1
> space.
Ok, I'll change it.
>> @@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>>
>> static int proc_resetdevice(struct usb_dev_state *ps)
>> {
>> + struct usb_host_config *actconfig = ps->dev->actconfig;
>> + struct usb_interface *interface;
>> + int i, number;
>> +
>> + /* Don't allow a device reset if the process has dropped the
>> + * privilege to do such things and any of the interfaces are
>> + * currently claimed.
>> + */
>> + if (ps->privileges_dropped && actconfig) {
>> + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
>> + interface = actconfig->interface[i];
>> + number = interface->cur_altsetting->desc.bInterfaceNumber;
>> + if (usb_interface_claimed(interface)) {
>
> The test should be:
>
> if (usb_interface_claimed(interface) &&
> !test_bit(number, &ps->ifclaimed)) {
>
> We don't want to prevent people from resetting a device merely because
> they have claimed an interface. Only if someone else has claimed one.
Sounds sensible, now changed as well.
>> + dev_warn(&ps->dev->dev,
>> + "usbfs: interface %d claimed by %s while '%s' resets device\n",
>> + number, interface->dev.driver->name, current->comm);
>> + return -EACCES;
>> + }
>> + }
>> + }
>> +
>> return usb_reset_device(ps->dev);
>> }
>
> Also, it wouldn't hurt to change proc_get_capabilities() and
> include/uapi/linux/usbdevicefs.h to add a USBDEVFS_CAP_DROP_PRIVILEGES
> bit.
Good idea, I've added a bit now. I'm writing some docs and polishing a
program to test this as Greg requested and I'll submit v4 then.
Thanks!
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-08 1:56 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-02-08 1:56 UTC (permalink / raw)
To: Alan Stern
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
kborer-Re5JQEeQqe8AvxtiuMwx3w, bjorn-yOkvZcmFvRU,
reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
Hello Alan,
El 04/02/16 a las 13:27, Alan Stern escribió:
> On Thu, 4 Feb 2016, Emilio López wrote:
>
>> From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>
>> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
>> relinquish the ability to issue other ioctls that may interfere with
>> other processes and drivers that have claimed an interface on the
>> device.
>>
>> Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
>>
>> ---
>>
>> Changes in v3:
>> - Switch ioctl to use a __u32 given the iface qty is capped at 32
>> - Reword comments as requested by Alan
>> - Allow callers to shrink the allowed interfaces mask
>
>> @@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>> if (test_bit(ifnum, &ps->ifclaimed))
>> return 0;
>>
>> + if (ps->privileges_dropped
>> + && !test_bit(ifnum, &ps->interface_allowed_mask))
>
> Continuation lines in this file are indented by 2 tab stops, not 1
> space.
Ok, I'll change it.
>> @@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>>
>> static int proc_resetdevice(struct usb_dev_state *ps)
>> {
>> + struct usb_host_config *actconfig = ps->dev->actconfig;
>> + struct usb_interface *interface;
>> + int i, number;
>> +
>> + /* Don't allow a device reset if the process has dropped the
>> + * privilege to do such things and any of the interfaces are
>> + * currently claimed.
>> + */
>> + if (ps->privileges_dropped && actconfig) {
>> + for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
>> + interface = actconfig->interface[i];
>> + number = interface->cur_altsetting->desc.bInterfaceNumber;
>> + if (usb_interface_claimed(interface)) {
>
> The test should be:
>
> if (usb_interface_claimed(interface) &&
> !test_bit(number, &ps->ifclaimed)) {
>
> We don't want to prevent people from resetting a device merely because
> they have claimed an interface. Only if someone else has claimed one.
Sounds sensible, now changed as well.
>> + dev_warn(&ps->dev->dev,
>> + "usbfs: interface %d claimed by %s while '%s' resets device\n",
>> + number, interface->dev.driver->name, current->comm);
>> + return -EACCES;
>> + }
>> + }
>> + }
>> +
>> return usb_reset_device(ps->dev);
>> }
>
> Also, it wouldn't hurt to change proc_get_capabilities() and
> include/uapi/linux/usbdevicefs.h to add a USBDEVFS_CAP_DROP_PRIVILEGES
> bit.
Good idea, I've added a bit now. I'm writing some docs and polishing a
program to test this as Greg requested and I'll submit v4 then.
Thanks!
Emilio
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v4] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-15 1:41 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-02-15 1:41 UTC (permalink / raw)
To: gregkh, stern, kborer
Cc: reillyg, keescook, linux-api, linux-usb, linux-kernel, jorgelo,
dan.carpenter, Emilio López
From: Reilly Grant <reillyg@chromium.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
This commit also includes a simple utility to be able to test the
ioctl, located at Documentation/usb/usbdevfs-drop-permissions.c
Example (with qemu-kvm's input device):
$ lsusb
...
Bus 001 Device 002: ID 0627:0001 Adomax Technology Co., Ltd
$ usb-devices
...
C: #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA
I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=usbhid
$ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
OK: privileges dropped!
Available options:
[0] Exit now
[1] Reset device. Should fail if device is in use
[2] Claim 4 interfaces. Should succeed where not in use
[3] Narrow interface permission mask
Which option shall I run?: 1
ERROR: USBDEVFS_RESET failed! (1 - Operation not permitted)
Which test shall I run next?: 2
ERROR claiming if 0 (1 - Operation not permitted)
ERROR claiming if 1 (1 - Operation not permitted)
ERROR claiming if 2 (1 - Operation not permitted)
ERROR claiming if 3 (1 - Operation not permitted)
Which test shall I run next?: 0
After unbinding usbhid:
$ usb-devices
...
I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=(none)
$ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
...
Which option shall I run?: 2
OK: claimed if 0
ERROR claiming if 1 (1 - Operation not permitted)
ERROR claiming if 2 (1 - Operation not permitted)
ERROR claiming if 3 (1 - Operation not permitted)
Which test shall I run next?: 1
OK: USBDEVFS_RESET succeeded
Which test shall I run next?: 0
After unbinding usbhid and restricting the mask:
$ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
...
Which option shall I run?: 3
Insert new mask: 0
OK: privileges dropped!
Which test shall I run next?: 2
ERROR claiming if 0 (1 - Operation not permitted)
ERROR claiming if 1 (1 - Operation not permitted)
ERROR claiming if 2 (1 - Operation not permitted)
ERROR claiming if 3 (1 - Operation not permitted)
Signed-off-by: Reilly Grant <reillyg@chromium.org>
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
Changes in v4:
- IOR -> IOW
- Explain the new ioctl on usb Docbook
- Small program to be able to test the new functionality
- New capability flag
- Allow people to reset devices if they are the only ones
claiming interfaces, as suggested by Alan
Changes in v3:
- Switch ioctl to use a __u32 given the iface qty is capped at 32
- Reword comments as requested by Alan
- Allow callers to shrink the allowed interfaces mask
Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
process is allowed to claim.
- Drop Tested-by's
Documentation/DocBook/usb.tmpl | 12 +++
Documentation/usb/usbdevfs-drop-permissions.c | 120 ++++++++++++++++++++++++++
drivers/usb/core/devio.c | 63 ++++++++++++--
include/uapi/linux/usbdevice_fs.h | 2 +
4 files changed, 192 insertions(+), 5 deletions(-)
create mode 100644 Documentation/usb/usbdevfs-drop-permissions.c
diff --git a/Documentation/DocBook/usb.tmpl b/Documentation/DocBook/usb.tmpl
index 4cd5b2c..bc776be0 100644
--- a/Documentation/DocBook/usb.tmpl
+++ b/Documentation/DocBook/usb.tmpl
@@ -732,6 +732,18 @@ usbdev_ioctl (int fd, int ifno, unsigned request, void *param)
or SET_INTERFACE.
</para></warning></listitem></varlistentry>
+ <varlistentry><term>USBDEVFS_DROP_PRIVILEGES</term>
+ <listitem><para>This is used to relinquish the ability
+ to do certain operations which are considered to be
+ privileged on a usbfs file descriptor.
+ This includes claiming arbitrary interfaces, resetting
+ a device on which there are currently claimed interfaces
+ from other users, and issuing USBDEVFS_IOCTL calls.
+ The ioctl parameter is a 32 bit mask of interfaces
+ the user is allowed to claim on this file descriptor.
+ You may issue this ioctl more than one time to narrow
+ said mask.
+ </para></listitem></varlistentry>
</variablelist>
</sect2>
diff --git a/Documentation/usb/usbdevfs-drop-permissions.c b/Documentation/usb/usbdevfs-drop-permissions.c
new file mode 100644
index 0000000..efc948a
--- /dev/null
+++ b/Documentation/usb/usbdevfs-drop-permissions.c
@@ -0,0 +1,120 @@
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <errno.h>
+#include <string.h>
+#include <inttypes.h>
+#include <unistd.h>
+
+#include <linux/usbdevice_fs.h>
+
+/* For building without an updated set of headers */
+#ifndef USBDEVFS_DROP_PRIVILEGES
+#define USBDEVFS_DROP_PRIVILEGES _IOW('U', 30, __u32)
+#define USBDEVFS_CAP_DROP_PRIVILEGES 0x20
+#endif
+
+void drop_privileges(int fd, uint32_t mask)
+{
+ int res;
+
+ res = ioctl(fd, USBDEVFS_DROP_PRIVILEGES, &mask);
+ if (res)
+ printf("ERROR: USBDEVFS_DROP_PRIVILEGES returned %d\n", res);
+ else
+ printf("OK: privileges dropped!\n");
+}
+
+void reset_device(int fd)
+{
+ int res;
+
+ res = ioctl(fd, USBDEVFS_RESET);
+ if (!res)
+ printf("OK: USBDEVFS_RESET succeeded\n");
+ else
+ printf("ERROR: reset failed! (%d - %s)\n",
+ -res, strerror(-res));
+}
+
+void claim_some_intf(int fd)
+{
+ int i, res;
+
+ for (i = 0; i < 4; i++) {
+ res = ioctl(fd, USBDEVFS_CLAIMINTERFACE, &i);
+ if (!res)
+ printf("OK: claimed if %d\n", i);
+ else
+ printf("ERROR claiming if %d (%d - %s)\n",
+ i, -res, strerror(-res));
+ }
+}
+
+int main(int argc, char *argv[])
+{
+ uint32_t mask, caps;
+ int c, fd;
+
+ fd = open(argv[1], O_RDWR);
+ if (fd < 0) {
+ printf("Failed to open file\n");
+ goto err_fd;
+ }
+
+ /*
+ * check if dropping privileges is supported,
+ * bail on systems where the capability is not present
+ */
+ ioctl(fd, USBDEVFS_GET_CAPABILITIES, &caps);
+ if (!(caps & USBDEVFS_CAP_DROP_PRIVILEGES)) {
+ printf("DROP_PRIVILEGES not supported\n");
+ goto err;
+ }
+
+ /*
+ * Drop privileges but keep the ability to claim all
+ * free interfaces (i.e., those not used by kernel drivers)
+ */
+ drop_privileges(fd, -1U);
+
+ printf("Available options:\n"
+ "[0] Exit now\n"
+ "[1] Reset device. Should fail if device is in use\n"
+ "[2] Claim 4 interfaces. Should succeed where not in use\n"
+ "[3] Narrow interface permission mask\n"
+ "Which option shall I run?: ");
+
+ while (scanf("%d", &c) == 1) {
+ switch (c) {
+ case 0:
+ goto exit;
+ case 1:
+ reset_device(fd);
+ break;
+ case 2:
+ claim_some_intf(fd);
+ break;
+ case 3:
+ printf("Insert new mask: ");
+ scanf("%x", &mask);
+ drop_privileges(fd, mask);
+ break;
+ default:
+ printf("I don't recognize that\n");
+ }
+
+ printf("Which test shall I run next?: ");
+ }
+
+exit:
+ close(fd);
+ return 0;
+
+err:
+ close(fd);
+err_fd:
+ return 1;
+}
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 59e7a33..013484a 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
+ unsigned long interface_allowed_mask;
};
struct async {
@@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
if (test_bit(ifnum, &ps->ifclaimed))
return 0;
+ if (ps->privileges_dropped &&
+ !test_bit(ifnum, &ps->interface_allowed_mask))
+ return -EACCES;
+
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
@@ -861,7 +867,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -889,16 +895,14 @@ static int usbdev_open(struct inode *inode, struct file *file)
ps->dev = dev;
ps->file = file;
+ ps->interface_allowed_mask = 0xFFFFFFFF; /* 32 bits */
spin_lock_init(&ps->lock);
INIT_LIST_HEAD(&ps->list);
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1198,6 +1202,28 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+ struct usb_interface *interface;
+ int i, number;
+
+ /* Don't allow a device reset if the process has dropped the
+ * privilege to do such things and any of the interfaces are
+ * currently claimed.
+ */
+ if (ps->privileges_dropped && actconfig) {
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ interface = actconfig->interface[i];
+ number = interface->cur_altsetting->desc.bInterfaceNumber;
+ if (usb_interface_claimed(interface) &&
+ !test_bit(number, &ps->ifclaimed)) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by %s while '%s' resets device\n",
+ number, interface->dev.driver->name, current->comm);
+ return -EACCES;
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1915,6 +1941,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2040,7 +2069,8 @@ static int proc_get_capabilities(struct usb_dev_state *ps, void __user *arg)
__u32 caps;
caps = USBDEVFS_CAP_ZERO_PACKET | USBDEVFS_CAP_NO_PACKET_SIZE_LIM |
- USBDEVFS_CAP_REAP_AFTER_DISCONNECT;
+ USBDEVFS_CAP_REAP_AFTER_DISCONNECT |
+ USBDEVFS_CAP_DROP_PRIVILEGES;
if (!ps->dev->bus->no_stop_on_short)
caps |= USBDEVFS_CAP_BULK_CONTINUATION;
if (ps->dev->bus->sg_tablesize)
@@ -2067,6 +2097,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
if (intf->dev.driver) {
struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+ if (ps->privileges_dropped)
+ return -EACCES;
+
if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
strncmp(dc.driver, intf->dev.driver->name,
sizeof(dc.driver)) != 0)
@@ -2123,6 +2156,23 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+ u32 data;
+
+ if (copy_from_user(&data, arg, sizeof(data)))
+ return -EFAULT;
+
+ /* This is an one way operation. Once privileges are
+ * dropped, you cannot regain them. You may however reissue
+ * this ioctl to shrink the allowed interfaces mask.
+ */
+ ps->interface_allowed_mask &= data;
+ ps->privileges_dropped = true;
+
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2311,6 +2361,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps, p);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..9b9d4be 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -134,6 +134,7 @@ struct usbdevfs_hub_portinfo {
#define USBDEVFS_CAP_NO_PACKET_SIZE_LIM 0x04
#define USBDEVFS_CAP_BULK_SCATTER_GATHER 0x08
#define USBDEVFS_CAP_REAP_AFTER_DISCONNECT 0x10
+#define USBDEVFS_CAP_DROP_PRIVILEGES 0x20
/* USBDEVFS_DISCONNECT_CLAIM flags & struct */
@@ -187,5 +188,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IOW('U', 30, __u32)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v4] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-15 1:41 ` Emilio López
0 siblings, 0 replies; 54+ messages in thread
From: Emilio López @ 2016-02-15 1:41 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
kborer-Re5JQEeQqe8AvxtiuMwx3w
Cc: reillyg-F7+t8E8rja9g9hUCZPvPmw, keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA, Emilio López
From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.
This commit also includes a simple utility to be able to test the
ioctl, located at Documentation/usb/usbdevfs-drop-permissions.c
Example (with qemu-kvm's input device):
$ lsusb
...
Bus 001 Device 002: ID 0627:0001 Adomax Technology Co., Ltd
$ usb-devices
...
C: #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA
I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=usbhid
$ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
OK: privileges dropped!
Available options:
[0] Exit now
[1] Reset device. Should fail if device is in use
[2] Claim 4 interfaces. Should succeed where not in use
[3] Narrow interface permission mask
Which option shall I run?: 1
ERROR: USBDEVFS_RESET failed! (1 - Operation not permitted)
Which test shall I run next?: 2
ERROR claiming if 0 (1 - Operation not permitted)
ERROR claiming if 1 (1 - Operation not permitted)
ERROR claiming if 2 (1 - Operation not permitted)
ERROR claiming if 3 (1 - Operation not permitted)
Which test shall I run next?: 0
After unbinding usbhid:
$ usb-devices
...
I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=(none)
$ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
...
Which option shall I run?: 2
OK: claimed if 0
ERROR claiming if 1 (1 - Operation not permitted)
ERROR claiming if 2 (1 - Operation not permitted)
ERROR claiming if 3 (1 - Operation not permitted)
Which test shall I run next?: 1
OK: USBDEVFS_RESET succeeded
Which test shall I run next?: 0
After unbinding usbhid and restricting the mask:
$ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
...
Which option shall I run?: 3
Insert new mask: 0
OK: privileges dropped!
Which test shall I run next?: 2
ERROR claiming if 0 (1 - Operation not permitted)
ERROR claiming if 1 (1 - Operation not permitted)
ERROR claiming if 2 (1 - Operation not permitted)
ERROR claiming if 3 (1 - Operation not permitted)
Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---
Changes in v4:
- IOR -> IOW
- Explain the new ioctl on usb Docbook
- Small program to be able to test the new functionality
- New capability flag
- Allow people to reset devices if they are the only ones
claiming interfaces, as suggested by Alan
Changes in v3:
- Switch ioctl to use a __u32 given the iface qty is capped at 32
- Reword comments as requested by Alan
- Allow callers to shrink the allowed interfaces mask
Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
process is allowed to claim.
- Drop Tested-by's
Documentation/DocBook/usb.tmpl | 12 +++
Documentation/usb/usbdevfs-drop-permissions.c | 120 ++++++++++++++++++++++++++
drivers/usb/core/devio.c | 63 ++++++++++++--
include/uapi/linux/usbdevice_fs.h | 2 +
4 files changed, 192 insertions(+), 5 deletions(-)
create mode 100644 Documentation/usb/usbdevfs-drop-permissions.c
diff --git a/Documentation/DocBook/usb.tmpl b/Documentation/DocBook/usb.tmpl
index 4cd5b2c..bc776be0 100644
--- a/Documentation/DocBook/usb.tmpl
+++ b/Documentation/DocBook/usb.tmpl
@@ -732,6 +732,18 @@ usbdev_ioctl (int fd, int ifno, unsigned request, void *param)
or SET_INTERFACE.
</para></warning></listitem></varlistentry>
+ <varlistentry><term>USBDEVFS_DROP_PRIVILEGES</term>
+ <listitem><para>This is used to relinquish the ability
+ to do certain operations which are considered to be
+ privileged on a usbfs file descriptor.
+ This includes claiming arbitrary interfaces, resetting
+ a device on which there are currently claimed interfaces
+ from other users, and issuing USBDEVFS_IOCTL calls.
+ The ioctl parameter is a 32 bit mask of interfaces
+ the user is allowed to claim on this file descriptor.
+ You may issue this ioctl more than one time to narrow
+ said mask.
+ </para></listitem></varlistentry>
</variablelist>
</sect2>
diff --git a/Documentation/usb/usbdevfs-drop-permissions.c b/Documentation/usb/usbdevfs-drop-permissions.c
new file mode 100644
index 0000000..efc948a
--- /dev/null
+++ b/Documentation/usb/usbdevfs-drop-permissions.c
@@ -0,0 +1,120 @@
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <errno.h>
+#include <string.h>
+#include <inttypes.h>
+#include <unistd.h>
+
+#include <linux/usbdevice_fs.h>
+
+/* For building without an updated set of headers */
+#ifndef USBDEVFS_DROP_PRIVILEGES
+#define USBDEVFS_DROP_PRIVILEGES _IOW('U', 30, __u32)
+#define USBDEVFS_CAP_DROP_PRIVILEGES 0x20
+#endif
+
+void drop_privileges(int fd, uint32_t mask)
+{
+ int res;
+
+ res = ioctl(fd, USBDEVFS_DROP_PRIVILEGES, &mask);
+ if (res)
+ printf("ERROR: USBDEVFS_DROP_PRIVILEGES returned %d\n", res);
+ else
+ printf("OK: privileges dropped!\n");
+}
+
+void reset_device(int fd)
+{
+ int res;
+
+ res = ioctl(fd, USBDEVFS_RESET);
+ if (!res)
+ printf("OK: USBDEVFS_RESET succeeded\n");
+ else
+ printf("ERROR: reset failed! (%d - %s)\n",
+ -res, strerror(-res));
+}
+
+void claim_some_intf(int fd)
+{
+ int i, res;
+
+ for (i = 0; i < 4; i++) {
+ res = ioctl(fd, USBDEVFS_CLAIMINTERFACE, &i);
+ if (!res)
+ printf("OK: claimed if %d\n", i);
+ else
+ printf("ERROR claiming if %d (%d - %s)\n",
+ i, -res, strerror(-res));
+ }
+}
+
+int main(int argc, char *argv[])
+{
+ uint32_t mask, caps;
+ int c, fd;
+
+ fd = open(argv[1], O_RDWR);
+ if (fd < 0) {
+ printf("Failed to open file\n");
+ goto err_fd;
+ }
+
+ /*
+ * check if dropping privileges is supported,
+ * bail on systems where the capability is not present
+ */
+ ioctl(fd, USBDEVFS_GET_CAPABILITIES, &caps);
+ if (!(caps & USBDEVFS_CAP_DROP_PRIVILEGES)) {
+ printf("DROP_PRIVILEGES not supported\n");
+ goto err;
+ }
+
+ /*
+ * Drop privileges but keep the ability to claim all
+ * free interfaces (i.e., those not used by kernel drivers)
+ */
+ drop_privileges(fd, -1U);
+
+ printf("Available options:\n"
+ "[0] Exit now\n"
+ "[1] Reset device. Should fail if device is in use\n"
+ "[2] Claim 4 interfaces. Should succeed where not in use\n"
+ "[3] Narrow interface permission mask\n"
+ "Which option shall I run?: ");
+
+ while (scanf("%d", &c) == 1) {
+ switch (c) {
+ case 0:
+ goto exit;
+ case 1:
+ reset_device(fd);
+ break;
+ case 2:
+ claim_some_intf(fd);
+ break;
+ case 3:
+ printf("Insert new mask: ");
+ scanf("%x", &mask);
+ drop_privileges(fd, mask);
+ break;
+ default:
+ printf("I don't recognize that\n");
+ }
+
+ printf("Which test shall I run next?: ");
+ }
+
+exit:
+ close(fd);
+ return 0;
+
+err:
+ close(fd);
+err_fd:
+ return 1;
+}
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 59e7a33..013484a 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
unsigned long ifclaimed;
u32 secid;
u32 disabled_bulk_eps;
+ bool privileges_dropped;
+ unsigned long interface_allowed_mask;
};
struct async {
@@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
if (test_bit(ifnum, &ps->ifclaimed))
return 0;
+ if (ps->privileges_dropped &&
+ !test_bit(ifnum, &ps->interface_allowed_mask))
+ return -EACCES;
+
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
@@ -861,7 +867,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
int ret;
ret = -ENOMEM;
- ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+ ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
if (!ps)
goto out_free_ps;
@@ -889,16 +895,14 @@ static int usbdev_open(struct inode *inode, struct file *file)
ps->dev = dev;
ps->file = file;
+ ps->interface_allowed_mask = 0xFFFFFFFF; /* 32 bits */
spin_lock_init(&ps->lock);
INIT_LIST_HEAD(&ps->list);
INIT_LIST_HEAD(&ps->async_pending);
INIT_LIST_HEAD(&ps->async_completed);
init_waitqueue_head(&ps->wait);
- ps->discsignr = 0;
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- ps->disccontext = NULL;
- ps->ifclaimed = 0;
security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
@@ -1198,6 +1202,28 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
static int proc_resetdevice(struct usb_dev_state *ps)
{
+ struct usb_host_config *actconfig = ps->dev->actconfig;
+ struct usb_interface *interface;
+ int i, number;
+
+ /* Don't allow a device reset if the process has dropped the
+ * privilege to do such things and any of the interfaces are
+ * currently claimed.
+ */
+ if (ps->privileges_dropped && actconfig) {
+ for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ interface = actconfig->interface[i];
+ number = interface->cur_altsetting->desc.bInterfaceNumber;
+ if (usb_interface_claimed(interface) &&
+ !test_bit(number, &ps->ifclaimed)) {
+ dev_warn(&ps->dev->dev,
+ "usbfs: interface %d claimed by %s while '%s' resets device\n",
+ number, interface->dev.driver->name, current->comm);
+ return -EACCES;
+ }
+ }
+ }
+
return usb_reset_device(ps->dev);
}
@@ -1915,6 +1941,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
+ if (ps->privileges_dropped)
+ return -EACCES;
+
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
@@ -2040,7 +2069,8 @@ static int proc_get_capabilities(struct usb_dev_state *ps, void __user *arg)
__u32 caps;
caps = USBDEVFS_CAP_ZERO_PACKET | USBDEVFS_CAP_NO_PACKET_SIZE_LIM |
- USBDEVFS_CAP_REAP_AFTER_DISCONNECT;
+ USBDEVFS_CAP_REAP_AFTER_DISCONNECT |
+ USBDEVFS_CAP_DROP_PRIVILEGES;
if (!ps->dev->bus->no_stop_on_short)
caps |= USBDEVFS_CAP_BULK_CONTINUATION;
if (ps->dev->bus->sg_tablesize)
@@ -2067,6 +2097,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
if (intf->dev.driver) {
struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+ if (ps->privileges_dropped)
+ return -EACCES;
+
if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
strncmp(dc.driver, intf->dev.driver->name,
sizeof(dc.driver)) != 0)
@@ -2123,6 +2156,23 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
return r;
}
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+ u32 data;
+
+ if (copy_from_user(&data, arg, sizeof(data)))
+ return -EFAULT;
+
+ /* This is an one way operation. Once privileges are
+ * dropped, you cannot regain them. You may however reissue
+ * this ioctl to shrink the allowed interfaces mask.
+ */
+ ps->interface_allowed_mask &= data;
+ ps->privileges_dropped = true;
+
+ return 0;
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2311,6 +2361,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_FREE_STREAMS:
ret = proc_free_streams(ps, p);
break;
+ case USBDEVFS_DROP_PRIVILEGES:
+ ret = proc_drop_privileges(ps, p);
+ break;
}
done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..9b9d4be 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -134,6 +134,7 @@ struct usbdevfs_hub_portinfo {
#define USBDEVFS_CAP_NO_PACKET_SIZE_LIM 0x04
#define USBDEVFS_CAP_BULK_SCATTER_GATHER 0x08
#define USBDEVFS_CAP_REAP_AFTER_DISCONNECT 0x10
+#define USBDEVFS_CAP_DROP_PRIVILEGES 0x20
/* USBDEVFS_DISCONNECT_CLAIM flags & struct */
@@ -187,5 +188,6 @@ struct usbdevfs_streams {
#define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES _IOW('U', 30, __u32)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
--
2.5.0
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v4] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-18 18:44 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-02-18 18:44 UTC (permalink / raw)
To: Emilio López
Cc: gregkh, kborer, reillyg, keescook, linux-api, linux-usb,
linux-kernel, jorgelo, dan.carpenter
On Sun, 14 Feb 2016, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> This commit also includes a simple utility to be able to test the
> ioctl, located at Documentation/usb/usbdevfs-drop-permissions.c
>
> Example (with qemu-kvm's input device):
>
> $ lsusb
> ...
> Bus 001 Device 002: ID 0627:0001 Adomax Technology Co., Ltd
>
> $ usb-devices
> ...
> C: #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA
> I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=usbhid
>
> $ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
> OK: privileges dropped!
> Available options:
> [0] Exit now
> [1] Reset device. Should fail if device is in use
> [2] Claim 4 interfaces. Should succeed where not in use
> [3] Narrow interface permission mask
> Which option shall I run?: 1
> ERROR: USBDEVFS_RESET failed! (1 - Operation not permitted)
> Which test shall I run next?: 2
> ERROR claiming if 0 (1 - Operation not permitted)
> ERROR claiming if 1 (1 - Operation not permitted)
> ERROR claiming if 2 (1 - Operation not permitted)
> ERROR claiming if 3 (1 - Operation not permitted)
> Which test shall I run next?: 0
>
> After unbinding usbhid:
>
> $ usb-devices
> ...
> I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=(none)
>
> $ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
> ...
> Which option shall I run?: 2
> OK: claimed if 0
> ERROR claiming if 1 (1 - Operation not permitted)
> ERROR claiming if 2 (1 - Operation not permitted)
> ERROR claiming if 3 (1 - Operation not permitted)
> Which test shall I run next?: 1
> OK: USBDEVFS_RESET succeeded
> Which test shall I run next?: 0
>
> After unbinding usbhid and restricting the mask:
>
> $ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
> ...
> Which option shall I run?: 3
> Insert new mask: 0
> OK: privileges dropped!
> Which test shall I run next?: 2
> ERROR claiming if 0 (1 - Operation not permitted)
> ERROR claiming if 1 (1 - Operation not permitted)
> ERROR claiming if 2 (1 - Operation not permitted)
> ERROR claiming if 3 (1 - Operation not permitted)
>
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4] usb: devio: Add ioctl to disallow detaching kernel USB drivers.
@ 2016-02-18 18:44 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2016-02-18 18:44 UTC (permalink / raw)
To: Emilio López
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
kborer-Re5JQEeQqe8AvxtiuMwx3w, reillyg-F7+t8E8rja9g9hUCZPvPmw,
keescook-F7+t8E8rja9g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jorgelo-F7+t8E8rja9g9hUCZPvPmw,
dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA
On Sun, 14 Feb 2016, Emilio López wrote:
> From: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
>
> This commit also includes a simple utility to be able to test the
> ioctl, located at Documentation/usb/usbdevfs-drop-permissions.c
>
> Example (with qemu-kvm's input device):
>
> $ lsusb
> ...
> Bus 001 Device 002: ID 0627:0001 Adomax Technology Co., Ltd
>
> $ usb-devices
> ...
> C: #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=100mA
> I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=usbhid
>
> $ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
> OK: privileges dropped!
> Available options:
> [0] Exit now
> [1] Reset device. Should fail if device is in use
> [2] Claim 4 interfaces. Should succeed where not in use
> [3] Narrow interface permission mask
> Which option shall I run?: 1
> ERROR: USBDEVFS_RESET failed! (1 - Operation not permitted)
> Which test shall I run next?: 2
> ERROR claiming if 0 (1 - Operation not permitted)
> ERROR claiming if 1 (1 - Operation not permitted)
> ERROR claiming if 2 (1 - Operation not permitted)
> ERROR claiming if 3 (1 - Operation not permitted)
> Which test shall I run next?: 0
>
> After unbinding usbhid:
>
> $ usb-devices
> ...
> I: If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=02 Driver=(none)
>
> $ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
> ...
> Which option shall I run?: 2
> OK: claimed if 0
> ERROR claiming if 1 (1 - Operation not permitted)
> ERROR claiming if 2 (1 - Operation not permitted)
> ERROR claiming if 3 (1 - Operation not permitted)
> Which test shall I run next?: 1
> OK: USBDEVFS_RESET succeeded
> Which test shall I run next?: 0
>
> After unbinding usbhid and restricting the mask:
>
> $ sudo ./usbdevfs-drop-permissions /dev/bus/usb/001/002
> ...
> Which option shall I run?: 3
> Insert new mask: 0
> OK: privileges dropped!
> Which test shall I run next?: 2
> ERROR claiming if 0 (1 - Operation not permitted)
> ERROR claiming if 1 (1 - Operation not permitted)
> ERROR claiming if 2 (1 - Operation not permitted)
> ERROR claiming if 3 (1 - Operation not permitted)
>
> Signed-off-by: Reilly Grant <reillyg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Emilio López <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2016-02-18 18:44 UTC | newest]
Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-25 15:45 [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Emilio López
2015-11-25 15:45 ` Emilio López
2015-11-25 15:45 ` [PATCH v1] usb: devio: Add " Emilio López
2015-11-26 8:59 ` Peter Chen
2015-11-26 8:59 ` Peter Chen
2015-11-26 9:20 ` Dan Carpenter
2015-11-26 9:19 ` [PATCH v1 0/1] " Krzysztof Opasiak
2015-11-26 9:19 ` Krzysztof Opasiak
2015-11-26 17:29 ` Greg KH
2015-11-27 8:44 ` Krzysztof Opasiak
2015-11-27 8:44 ` Krzysztof Opasiak
2015-11-28 2:39 ` Greg KH
2015-11-28 2:39 ` Greg KH
2015-11-30 9:08 ` Oliver Neukum
2015-11-30 9:08 ` Oliver Neukum
2015-11-30 16:16 ` Alan Stern
2015-11-30 17:12 ` Krzysztof Opasiak
2015-11-30 17:12 ` Krzysztof Opasiak
2015-11-30 17:20 ` Greg KH
2015-11-30 17:20 ` Greg KH
2015-11-30 18:48 ` Krzysztof Opasiak
2015-11-30 18:48 ` Krzysztof Opasiak
2016-01-19 16:39 ` Emilio López
2016-01-19 16:39 ` Emilio López
2016-01-19 18:07 ` Greg KH
2016-01-19 18:07 ` Greg KH
2016-01-21 23:54 ` [PATCH v2] usb: devio: Add " Emilio López
2016-01-21 23:54 ` Emilio López
2016-01-22 9:41 ` Bjørn Mork
2016-01-22 9:41 ` Bjørn Mork
2016-01-25 1:40 ` Emilio López
2016-01-25 1:40 ` Emilio López
2016-01-25 8:39 ` Bjørn Mork
2016-01-25 15:21 ` Alan Stern
2016-01-25 15:21 ` Alan Stern
2016-01-25 15:32 ` Bjørn Mork
2016-01-25 15:32 ` Bjørn Mork
2016-01-25 15:46 ` Alan Stern
2016-01-25 15:46 ` Alan Stern
2016-01-22 16:10 ` Alan Stern
2016-01-22 16:10 ` Alan Stern
2016-01-25 2:01 ` Emilio López
2016-02-04 3:20 ` [PATCH v3] " Emilio López
2016-02-04 3:20 ` Emilio López
2016-02-04 3:46 ` Greg KH
2016-02-04 3:46 ` Greg KH
2016-02-04 16:27 ` Alan Stern
2016-02-04 16:27 ` Alan Stern
2016-02-08 1:56 ` Emilio López
2016-02-08 1:56 ` Emilio López
2016-02-15 1:41 ` [PATCH v4] " Emilio López
2016-02-15 1:41 ` Emilio López
2016-02-18 18:44 ` Alan Stern
2016-02-18 18:44 ` Alan Stern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.