From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754820AbcAYCGM (ORCPT ); Sun, 24 Jan 2016 21:06:12 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:58327 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751959AbcAYCGJ (ORCPT ); Sun, 24 Jan 2016 21:06:09 -0500 Subject: Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. To: =?UTF-8?Q?Bj=c3=b8rn_Mork?= References: <20160119180752.GA10487@kroah.com> <1453420476-26125-1-git-send-email-emilio.lopez@collabora.co.uk> <8760ymdk94.fsf@nemi.mork.no> Cc: gregkh@linuxfoundation.org, stern@rowland.harvard.edu, 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: <56A57D0C.6030308@collabora.co.uk> Date: Sun, 24 Jan 2016 22:40:28 -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: <8760ymdk94.fsf@nemi.mork.no> 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 Bjørn, El 22/01/16 a las 06:41, Bjørn Mork escribió: > Emilio López 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