From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754940AbcAYCG1 (ORCPT ); Sun, 24 Jan 2016 21:06:27 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:58334 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751959AbcAYCGX (ORCPT ); Sun, 24 Jan 2016 21:06:23 -0500 Subject: Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. To: Alan Stern References: Cc: gregkh@linuxfoundation.org, kborer@gmail.com, k.opasiak@samsung.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, dan.carpenter@oracle.com From: =?UTF-8?Q?Emilio_L=c3=b3pez?= Message-ID: <56A581F9.5070707@collabora.co.uk> Date: Sun, 24 Jan 2016 23:01:29 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> 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 >> Signed-off-by: Emilio López > > >> 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