Messages in this thread | | | Subject | Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. | From | Emilio López <> | Date | Sun, 24 Jan 2016 23:01:29 -0300 |
| |
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
| |