Re: [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

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:

quoted

From: Reilly Grant <redacted>

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 <redacted>
Reviewed-by: Jorge Lucangeli Obes <redacted>
Reviewed-by: Kees Cook <redacted>
[emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
Signed-off-by: Emilio López <redacted>

---

 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.

quoted

 	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

quoted

+						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