Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753336AbbKZJVJ (ORCPT ); Thu, 26 Nov 2015 04:21:09 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:31707 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752643AbbKZJVB (ORCPT ); Thu, 26 Nov 2015 04:21:01 -0500 Date: Thu, 26 Nov 2015 12:20:14 +0300 From: Dan Carpenter To: Peter Chen Cc: Emilio =?iso-8859-1?Q?L=F3pez?= , gregkh@linuxfoundation.org, stern@rowland.harvard.edu, kborer@gmail.com, reillyg@chromium.org, keescook@chromium.org, linux-api@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, jorgelo@chromium.org Subject: Re: [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Message-ID: <20151126092014.GD7289@mwanda> References: <1448466334-21346-1-git-send-email-emilio.lopez@collabora.co.uk> <1448466334-21346-2-git-send-email-emilio.lopez@collabora.co.uk> <20151126085924.GD26397@shlinux2> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20151126085924.GD26397@shlinux2> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4744 Lines: 134 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 > > > > 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 > > Reviewed-by: Jorge Lucangeli Obes > > Reviewed-by: Kees Cook > > [emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix] > > Signed-off-by: Emilio L?pez > > > > --- > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/