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 22:40:28 -0300 |
| |
Hi Bjørn,
El 22/01/16 a las 06:41, Bjørn Mork escribió: > Emilio López <emilio.lopez@collabora.co.uk> writes: > >> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c >> index 38ae877c..bf40aa6 100644 >> --- a/drivers/usb/core/devio.c >> +++ b/drivers/usb/core/devio.c >> @@ -77,6 +77,8 @@ struct usb_dev_state { >> unsigned long ifclaimed; >> u32 secid; >> u32 disabled_bulk_eps; >> + bool privileges_dropped; >> + unsigned long interface_allowed_mask; >> }; >> >> struct async { >> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum) >> if (test_bit(ifnum, &ps->ifclaimed)) >> return 0; >> >> + if (ps->privileges_dropped) { >> + if (ifnum >= 8*sizeof(ps->interface_allowed_mask)) >> + return -EINVAL; > > > I don't think you need this runtime test. You can just make sure that > sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build > time. > > I do find this variable and arbitrary limit a bit confusing, but that's > not your fault - I guess it is an indication that ifnums > 31 are rare > :) > > >> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h >> index 019ba1e..9abcb34 100644 >> --- a/include/uapi/linux/usbdevice_fs.h >> +++ b/include/uapi/linux/usbdevice_fs.h >> @@ -154,6 +154,10 @@ struct usbdevfs_streams { >> unsigned char eps[0]; >> }; >> >> +struct usbdevfs_drop_privs { >> + unsigned long interface_allowed_mask; >> +}; >> + > > "unsigned long" isn't a very good choice here, is it?
I went with a type matching ifclaimed on struct usb_dev_state to keep the limit the same, but I guess it's not the best idea for an ioctl. I can switch it to __u32, keeping the runtime check above as is, or use __u64. Which one would you prefer?
Thanks for the review! Emilio
| |