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

[Dan Carpenter <dan.carpenter@oracle.com>]

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