Message ID | 2dfbb122aa11fa9ff73cbfc6a403b82d1bb2e7c8.1508434514.git.mario.limonciello@dell.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Darren Hart |
Headers | show |
I know that this patch is already applied and merged, but I spotted this problem: On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > +/* calls that are explicitly blacklisted */ > +static struct smbios_call call_blacklist[] = { > + {0x0000, 01, 07}, /* manufacturing use */ > + {0x0000, 06, 05}, /* manufacturing use */ > + {0x0000, 11, 03}, /* write once */ > + {0x0000, 11, 07}, /* write once */ Numbers prefixed by zero means that they are in octal notation, right? This can lead to misunderstanding, confusion or problems in future... Can we have all numbers either in hexadecimal or decimal notation? > + {0x0000, 11, 11}, /* write once */ > + {0x0000, 19, -1}, /* diagnostics */ > + /* handled by kernel: dell-laptop */ > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > +};
> -----Original Message----- > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > owner@vger.kernel.org] On Behalf Of Pali Rohár > Sent: Friday, January 5, 2018 5:13 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; Andy > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > <gnomes@lxorguk.ukuu.org.uk> > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > I know that this patch is already applied and merged, but I spotted this > problem: > > On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > > +/* calls that are explicitly blacklisted */ > > +static struct smbios_call call_blacklist[] = { > > + {0x0000, 01, 07}, /* manufacturing use */ > > + {0x0000, 06, 05}, /* manufacturing use */ > > + {0x0000, 11, 03}, /* write once */ > > + {0x0000, 11, 07}, /* write once */ > > Numbers prefixed by zero means that they are in octal notation, right? Is that how the kernel interprets an integer prefix by zero? I prefixed by zero for readability, they're supposed to be decimal. > This can lead to misunderstanding, confusion or problems in future... > > Can we have all numbers either in hexadecimal or decimal notation? Could you elaborate more why this is problematic the way it is? Are you meaning you would rather see this? {0x0000, 1, 7}, /* manufacturing use */ {0x0000, 6, 5}, /* manufacturing use */ {0x0000, 11, 3}, /* write once */ {0x0000, 11, 7}, /* write once */ That seems less readable to me but should interpret the same way. Perhaps it would be better if you submit a patch with what is clearer to you. > > > + {0x0000, 11, 11}, /* write once */ > > + {0x0000, 19, -1}, /* diagnostics */ > > + /* handled by kernel: dell-laptop */ > > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > > +}; > > -- > Pali Rohár > pali.rohar@gmail.com
On Friday 05 January 2018 14:32:54 Mario.Limonciello@dell.com wrote: > > > > -----Original Message----- > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > Sent: Friday, January 5, 2018 5:13 AM > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; Andy > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > <gnomes@lxorguk.ukuu.org.uk> > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > I know that this patch is already applied and merged, but I spotted this > > problem: > > > > On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > > > +/* calls that are explicitly blacklisted */ > > > +static struct smbios_call call_blacklist[] = { > > > + {0x0000, 01, 07}, /* manufacturing use */ > > > + {0x0000, 06, 05}, /* manufacturing use */ > > > + {0x0000, 11, 03}, /* write once */ > > > + {0x0000, 11, 07}, /* write once */ > > > > Numbers prefixed by zero means that they are in octal notation, right? > Is that how the kernel interprets an integer prefix by zero? No, this is how C language define it. See e.g. C11 standard, section 6.4.4.1 Integer constants: decimal-constant: nonzero-digit decimal-constant digit octal-constant: 0 octal-constant octal-digit So in C decimal number cannot start with digit zero. I think the place where octal numbers are used are in permissions (0777) > I prefixed by zero for readability, they're supposed to be decimal. > > > This can lead to misunderstanding, confusion or problems in future... > > > > Can we have all numbers either in hexadecimal or decimal notation? > > Could you elaborate more why this is problematic the way it is? Currently it is not problem as 7 is same number in octal (07) and decimal (7). representation. But e.g. octal 077 is 63 in decimal. > Are you meaning you would rather see this? > {0x0000, 1, 7}, /* manufacturing use */ > {0x0000, 6, 5}, /* manufacturing use */ > {0x0000, 11, 3}, /* write once */ > {0x0000, 11, 7}, /* write once */ Yes, this is better. If you need to achieve alignment then use spaces. Really, not leading zeros. > That seems less readable to me but should interpret the same way. Example: {0x000, 077, 7}, {0x000, 007, 7}, is **not** same as {0x000, 77, 7}, {0x000, 7, 7}, As first number in first section is (decimal) 63, not (decimal) 77. > Perhaps it would be better if you submit a patch with what is clearer to > you. > > > > > > + {0x0000, 11, 11}, /* write once */ > > > + {0x0000, 19, -1}, /* diagnostics */ > > > + /* handled by kernel: dell-laptop */ > > > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > > > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > > > +}; > > > > -- > > Pali Rohár > > pali.rohar@gmail.com
On Fri, Jan 5, 2018 at 4:32 PM, <Mario.Limonciello@dell.com> wrote: >> I know that this patch is already applied and merged, but I spotted this >> problem: >> >> On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: >> > +/* calls that are explicitly blacklisted */ >> > +static struct smbios_call call_blacklist[] = { >> > + {0x0000, 01, 07}, /* manufacturing use */ >> > + {0x0000, 06, 05}, /* manufacturing use */ >> > + {0x0000, 11, 03}, /* write once */ >> > + {0x0000, 11, 07}, /* write once */ >> >> Numbers prefixed by zero means that they are in octal notation, right? > Is that how the kernel interprets an integer prefix by zero? No, compiler. > I prefixed by zero for readability, they're supposed to be decimal. ...which make a confusion. Luckily you don't have 8 and 9 there, otherwise I even don't know if it would be a compilation warning. >> This can lead to misunderstanding, confusion or problems in future... >> >> Can we have all numbers either in hexadecimal or decimal notation? > > Could you elaborate more why this is problematic the way it is? > Are you meaning you would rather see this? > {0x0000, 1, 7}, /* manufacturing use */ > {0x0000, 6, 5}, /* manufacturing use */ > {0x0000, 11, 3}, /* write once */ > {0x0000, 11, 7}, /* write once */ I think something like that. You might use white space for indentation. > > That seems less readable to me but should interpret the same way. > > Perhaps it would be better if you submit a patch with what is clearer to > you.
> -----Original Message----- > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > owner@vger.kernel.org] On Behalf Of Pali Rohár > Sent: Friday, January 5, 2018 8:44 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > quasisec@google.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; > greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > On Friday 05 January 2018 14:32:54 Mario.Limonciello@dell.com wrote: > > > > > > > -----Original Message----- > > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > > Sent: Friday, January 5, 2018 5:13 AM > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > > > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; > Andy > > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > > <gnomes@lxorguk.ukuu.org.uk> > > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > > > I know that this patch is already applied and merged, but I spotted this > > > problem: > > > > > > On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > > > > +/* calls that are explicitly blacklisted */ > > > > +static struct smbios_call call_blacklist[] = { > > > > + {0x0000, 01, 07}, /* manufacturing use */ > > > > + {0x0000, 06, 05}, /* manufacturing use */ > > > > + {0x0000, 11, 03}, /* write once */ > > > > + {0x0000, 11, 07}, /* write once */ > > > > > > Numbers prefixed by zero means that they are in octal notation, right? > > Is that how the kernel interprets an integer prefix by zero? > > No, this is how C language define it. See e.g. C11 standard, section > 6.4.4.1 Integer constants: > > decimal-constant: > nonzero-digit > decimal-constant digit > > octal-constant: > 0 > octal-constant octal-digit > > So in C decimal number cannot start with digit zero. > > I think the place where octal numbers are used are in permissions (0777) > > > I prefixed by zero for readability, they're supposed to be decimal. > > > > > This can lead to misunderstanding, confusion or problems in future... > > > > > > Can we have all numbers either in hexadecimal or decimal notation? > > > > Could you elaborate more why this is problematic the way it is? > > Currently it is not problem as 7 is same number in octal (07) and > decimal (7). representation. But e.g. octal 077 is 63 in decimal. > > > Are you meaning you would rather see this? > > {0x0000, 1, 7}, /* manufacturing use */ > > {0x0000, 6, 5}, /* manufacturing use */ > > {0x0000, 11, 3}, /* write once */ > > {0x0000, 11, 7}, /* write once */ > > Yes, this is better. If you need to achieve alignment then use spaces. > Really, not leading zeros. > > > That seems less readable to me but should interpret the same way. > > Example: > > {0x000, 077, 7}, > {0x000, 007, 7}, > > is **not** same as > > {0x000, 77, 7}, > {0x000, 7, 7}, > > As first number in first section is (decimal) 63, not (decimal) 77. > > > Perhaps it would be better if you submit a patch with what is clearer to > > you. > > > > > > > > > + {0x0000, 11, 11}, /* write once */ > > > > + {0x0000, 19, -1}, /* diagnostics */ > > > > + /* handled by kernel: dell-laptop */ > > > > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > > > > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > > > > +}; > > > > > > -- > > > Pali Rohár > > > pali.rohar@gmail.com > > -- > Pali Rohár > pali.rohar@gmail.com Thanks very much for sharing. I wasn't aware of this. I'll send a patch.
On Friday 05 January 2018 14:48:39 Mario.Limonciello@dell.com wrote: > > > > -----Original Message----- > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > Sent: Friday, January 5, 2018 8:44 AM > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > > quasisec@google.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; > > greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > On Friday 05 January 2018 14:32:54 Mario.Limonciello@dell.com wrote: > > > > > > > > > > -----Original Message----- > > > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > > > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > > > Sent: Friday, January 5, 2018 5:13 AM > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > > > > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; > > Andy > > > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > > > <gnomes@lxorguk.ukuu.org.uk> > > > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > > > > > I know that this patch is already applied and merged, but I spotted this > > > > problem: > > > > > > > > On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > > > > > +/* calls that are explicitly blacklisted */ > > > > > +static struct smbios_call call_blacklist[] = { > > > > > + {0x0000, 01, 07}, /* manufacturing use */ > > > > > + {0x0000, 06, 05}, /* manufacturing use */ > > > > > + {0x0000, 11, 03}, /* write once */ > > > > > + {0x0000, 11, 07}, /* write once */ > > > > > > > > Numbers prefixed by zero means that they are in octal notation, right? > > > Is that how the kernel interprets an integer prefix by zero? > > > > No, this is how C language define it. See e.g. C11 standard, section > > 6.4.4.1 Integer constants: > > > > decimal-constant: > > nonzero-digit > > decimal-constant digit > > > > octal-constant: > > 0 > > octal-constant octal-digit > > > > So in C decimal number cannot start with digit zero. > > > > I think the place where octal numbers are used are in permissions (0777) > > > > > I prefixed by zero for readability, they're supposed to be decimal. > > > > > > > This can lead to misunderstanding, confusion or problems in future... > > > > > > > > Can we have all numbers either in hexadecimal or decimal notation? > > > > > > Could you elaborate more why this is problematic the way it is? > > > > Currently it is not problem as 7 is same number in octal (07) and > > decimal (7). representation. But e.g. octal 077 is 63 in decimal. > > > > > Are you meaning you would rather see this? > > > {0x0000, 1, 7}, /* manufacturing use */ > > > {0x0000, 6, 5}, /* manufacturing use */ > > > {0x0000, 11, 3}, /* write once */ > > > {0x0000, 11, 7}, /* write once */ > > > > Yes, this is better. If you need to achieve alignment then use spaces. > > Really, not leading zeros. > > > > > That seems less readable to me but should interpret the same way. > > > > Example: > > > > {0x000, 077, 7}, > > {0x000, 007, 7}, > > > > is **not** same as > > > > {0x000, 77, 7}, > > {0x000, 7, 7}, > > > > As first number in first section is (decimal) 63, not (decimal) 77. > > > > > Perhaps it would be better if you submit a patch with what is clearer to > > > you. > > > > > > > > > > > > + {0x0000, 11, 11}, /* write once */ > > > > > + {0x0000, 19, -1}, /* diagnostics */ > > > > > + /* handled by kernel: dell-laptop */ > > > > > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > > > > > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > > > > > +}; > > > > > > > > -- > > > > Pali Rohár > > > > pali.rohar@gmail.com > > > > -- > > Pali Rohár > > pali.rohar@gmail.com > > Thanks very much for sharing. I wasn't aware of this. I'll send a patch. Hi! Do you have a patch for it?
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Saturday, January 27, 2018 8:51 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > quasisec@google.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; > greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > On Friday 05 January 2018 14:48:39 Mario.Limonciello@dell.com wrote: > > > > > > > -----Original Message----- > > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > > Sent: Friday, January 5, 2018 8:44 AM > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; > luto@kernel.org; > > > quasisec@google.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; > > > greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > > > On Friday 05 January 2018 14:32:54 Mario.Limonciello@dell.com wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver- > x86- > > > > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > > > > Sent: Friday, January 5, 2018 5:13 AM > > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > > > Cc: dvhart@infradead.org; Andy Shevchenko > <andy.shevchenko@gmail.com>; > > > > > LKML <linux-kernel@vger.kernel.org>; platform-driver- > x86@vger.kernel.org; > > > Andy > > > > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > > > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > > > > <gnomes@lxorguk.ukuu.org.uk> > > > > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering > support > > > > > > > > > > I know that this patch is already applied and merged, but I spotted this > > > > > problem: > > > > > > > > > > On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > > > > > > +/* calls that are explicitly blacklisted */ > > > > > > +static struct smbios_call call_blacklist[] = { > > > > > > + {0x0000, 01, 07}, /* manufacturing use */ > > > > > > + {0x0000, 06, 05}, /* manufacturing use */ > > > > > > + {0x0000, 11, 03}, /* write once */ > > > > > > + {0x0000, 11, 07}, /* write once */ > > > > > > > > > > Numbers prefixed by zero means that they are in octal notation, right? > > > > Is that how the kernel interprets an integer prefix by zero? > > > > > > No, this is how C language define it. See e.g. C11 standard, section > > > 6.4.4.1 Integer constants: > > > > > > decimal-constant: > > > nonzero-digit > > > decimal-constant digit > > > > > > octal-constant: > > > 0 > > > octal-constant octal-digit > > > > > > So in C decimal number cannot start with digit zero. > > > > > > I think the place where octal numbers are used are in permissions (0777) > > > > > > > I prefixed by zero for readability, they're supposed to be decimal. > > > > > > > > > This can lead to misunderstanding, confusion or problems in future... > > > > > > > > > > Can we have all numbers either in hexadecimal or decimal notation? > > > > > > > > Could you elaborate more why this is problematic the way it is? > > > > > > Currently it is not problem as 7 is same number in octal (07) and > > > decimal (7). representation. But e.g. octal 077 is 63 in decimal. > > > > > > > Are you meaning you would rather see this? > > > > {0x0000, 1, 7}, /* manufacturing use */ > > > > {0x0000, 6, 5}, /* manufacturing use */ > > > > {0x0000, 11, 3}, /* write once */ > > > > {0x0000, 11, 7}, /* write once */ > > > > > > Yes, this is better. If you need to achieve alignment then use spaces. > > > Really, not leading zeros. > > > > > > > That seems less readable to me but should interpret the same way. > > > > > > Example: > > > > > > {0x000, 077, 7}, > > > {0x000, 007, 7}, > > > > > > is **not** same as > > > > > > {0x000, 77, 7}, > > > {0x000, 7, 7}, > > > > > > As first number in first section is (decimal) 63, not (decimal) 77. > > > > > > > Perhaps it would be better if you submit a patch with what is clearer to > > > > you. > > > > > > > > > > > > > > > + {0x0000, 11, 11}, /* write once */ > > > > > > + {0x0000, 19, -1}, /* diagnostics */ > > > > > > + /* handled by kernel: dell-laptop */ > > > > > > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > > > > > > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > > > > > > +}; > > > > > > > > > > -- > > > > > Pali Rohár > > > > > pali.rohar@gmail.com > > > > > > -- > > > Pali Rohár > > > pali.rohar@gmail.com > > > > Thanks very much for sharing. I wasn't aware of this. I'll send a patch. > > Hi! Do you have a patch for it? I used the wrong tag so it didn't automatically CC you. Sorry about that. It's here: https://patchwork.kernel.org/patch/10146671/
On Monday 29 January 2018 16:22:07 Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > Sent: Saturday, January 27, 2018 8:51 AM > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > > quasisec@google.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; > > greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > On Friday 05 January 2018 14:48:39 Mario.Limonciello@dell.com wrote: > > > > > > > > > > -----Original Message----- > > > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > > > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > > > Sent: Friday, January 5, 2018 8:44 AM > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > > > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; > > luto@kernel.org; > > > > quasisec@google.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; > > > > greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > > > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering support > > > > > > > > On Friday 05 January 2018 14:32:54 Mario.Limonciello@dell.com wrote: > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver- > > x86- > > > > > > owner@vger.kernel.org] On Behalf Of Pali Rohár > > > > > > Sent: Friday, January 5, 2018 5:13 AM > > > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > > > > > Cc: dvhart@infradead.org; Andy Shevchenko > > <andy.shevchenko@gmail.com>; > > > > > > LKML <linux-kernel@vger.kernel.org>; platform-driver- > > x86@vger.kernel.org; > > > > Andy > > > > > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > > > > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > > > > > <gnomes@lxorguk.ukuu.org.uk> > > > > > > Subject: Re: [PATCH v10 12/15] platform/x86: dell-smbios: Add filtering > > support > > > > > > > > > > > > I know that this patch is already applied and merged, but I spotted this > > > > > > problem: > > > > > > > > > > > > On Thursday 19 October 2017 12:50:15 Mario Limonciello wrote: > > > > > > > +/* calls that are explicitly blacklisted */ > > > > > > > +static struct smbios_call call_blacklist[] = { > > > > > > > + {0x0000, 01, 07}, /* manufacturing use */ > > > > > > > + {0x0000, 06, 05}, /* manufacturing use */ > > > > > > > + {0x0000, 11, 03}, /* write once */ > > > > > > > + {0x0000, 11, 07}, /* write once */ > > > > > > > > > > > > Numbers prefixed by zero means that they are in octal notation, right? > > > > > Is that how the kernel interprets an integer prefix by zero? > > > > > > > > No, this is how C language define it. See e.g. C11 standard, section > > > > 6.4.4.1 Integer constants: > > > > > > > > decimal-constant: > > > > nonzero-digit > > > > decimal-constant digit > > > > > > > > octal-constant: > > > > 0 > > > > octal-constant octal-digit > > > > > > > > So in C decimal number cannot start with digit zero. > > > > > > > > I think the place where octal numbers are used are in permissions (0777) > > > > > > > > > I prefixed by zero for readability, they're supposed to be decimal. > > > > > > > > > > > This can lead to misunderstanding, confusion or problems in future... > > > > > > > > > > > > Can we have all numbers either in hexadecimal or decimal notation? > > > > > > > > > > Could you elaborate more why this is problematic the way it is? > > > > > > > > Currently it is not problem as 7 is same number in octal (07) and > > > > decimal (7). representation. But e.g. octal 077 is 63 in decimal. > > > > > > > > > Are you meaning you would rather see this? > > > > > {0x0000, 1, 7}, /* manufacturing use */ > > > > > {0x0000, 6, 5}, /* manufacturing use */ > > > > > {0x0000, 11, 3}, /* write once */ > > > > > {0x0000, 11, 7}, /* write once */ > > > > > > > > Yes, this is better. If you need to achieve alignment then use spaces. > > > > Really, not leading zeros. > > > > > > > > > That seems less readable to me but should interpret the same way. > > > > > > > > Example: > > > > > > > > {0x000, 077, 7}, > > > > {0x000, 007, 7}, > > > > > > > > is **not** same as > > > > > > > > {0x000, 77, 7}, > > > > {0x000, 7, 7}, > > > > > > > > As first number in first section is (decimal) 63, not (decimal) 77. > > > > > > > > > Perhaps it would be better if you submit a patch with what is clearer to > > > > > you. > > > > > > > > > > > > > > > > > > + {0x0000, 11, 11}, /* write once */ > > > > > > > + {0x0000, 19, -1}, /* diagnostics */ > > > > > > > + /* handled by kernel: dell-laptop */ > > > > > > > + {0x0000, CLASS_INFO, SELECT_RFKILL}, > > > > > > > + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, > > > > > > > +}; > > > > > > > > > > > > -- > > > > > > Pali Rohár > > > > > > pali.rohar@gmail.com > > > > > > > > -- > > > > Pali Rohár > > > > pali.rohar@gmail.com > > > > > > Thanks very much for sharing. I wasn't aware of this. I'll send a patch. > > > > Hi! Do you have a patch for it? > > I used the wrong tag so it didn't automatically CC you. Sorry about that. > It's here: > > https://patchwork.kernel.org/patch/10146671/ Ok, looks good for me.
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c index b540cd4605f7..b2aeb740c04b 100644 --- a/drivers/platform/x86/dell-smbios.c +++ b/drivers/platform/x86/dell-smbios.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include "dell-smbios.h" +static u32 da_supported_commands; static int da_num_tokens; static struct platform_device *platform_device; static struct calling_interface_token *da_tokens; @@ -38,6 +39,91 @@ struct smbios_device { int (*call_fn)(struct calling_interface_buffer *); }; +struct smbios_call { + u32 need_capability; + int class; + int select; +}; + +/* calls that are whitelisted for given capabilities */ +static struct smbios_call call_whitelist[] = { + /* generally tokens are allowed, but may be further filtered or + * restricted by token blacklist or whitelist + */ + {CAP_SYS_ADMIN, CLASS_TOKEN_READ, SELECT_TOKEN_STD}, + {CAP_SYS_ADMIN, CLASS_TOKEN_READ, SELECT_TOKEN_AC}, + {CAP_SYS_ADMIN, CLASS_TOKEN_READ, SELECT_TOKEN_BAT}, + {CAP_SYS_ADMIN, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD}, + {CAP_SYS_ADMIN, CLASS_TOKEN_WRITE, SELECT_TOKEN_AC}, + {CAP_SYS_ADMIN, CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT}, + /* used by userspace: fwupdate */ + {CAP_SYS_ADMIN, CLASS_ADMIN_PROP, SELECT_ADMIN_PROP}, + /* used by userspace: fwupd */ + {CAP_SYS_ADMIN, CLASS_INFO, SELECT_DOCK}, + {CAP_SYS_ADMIN, CLASS_FLASH_INTERFACE, SELECT_FLASH_INTERFACE}, +}; + +/* calls that are explicitly blacklisted */ +static struct smbios_call call_blacklist[] = { + {0x0000, 01, 07}, /* manufacturing use */ + {0x0000, 06, 05}, /* manufacturing use */ + {0x0000, 11, 03}, /* write once */ + {0x0000, 11, 07}, /* write once */ + {0x0000, 11, 11}, /* write once */ + {0x0000, 19, -1}, /* diagnostics */ + /* handled by kernel: dell-laptop */ + {0x0000, CLASS_INFO, SELECT_RFKILL}, + {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT}, +}; + +struct token_range { + u32 need_capability; + u16 min; + u16 max; +}; + +/* tokens that are whitelisted for given capabilities */ +static struct token_range token_whitelist[] = { + /* used by userspace: fwupdate */ + {CAP_SYS_ADMIN, CAPSULE_EN_TOKEN, CAPSULE_DIS_TOKEN}, + /* can indicate to userspace that WMI is needed */ + {0x0000, WSMT_EN_TOKEN, WSMT_DIS_TOKEN} +}; + +/* tokens that are explicitly blacklisted */ +static struct token_range token_blacklist[] = { + {0x0000, 0x0058, 0x0059}, /* ME use */ + {0x0000, 0x00CD, 0x00D0}, /* raid shadow copy */ + {0x0000, 0x013A, 0x01FF}, /* sata shadow copy */ + {0x0000, 0x0175, 0x0176}, /* write once */ + {0x0000, 0x0195, 0x0197}, /* diagnostics */ + {0x0000, 0x01DC, 0x01DD}, /* manufacturing use */ + {0x0000, 0x027D, 0x0284}, /* diagnostics */ + {0x0000, 0x02E3, 0x02E3}, /* manufacturing use */ + {0x0000, 0x02FF, 0x02FF}, /* manufacturing use */ + {0x0000, 0x0300, 0x0302}, /* manufacturing use */ + {0x0000, 0x0325, 0x0326}, /* manufacturing use */ + {0x0000, 0x0332, 0x0335}, /* fan control */ + {0x0000, 0x0350, 0x0350}, /* manufacturing use */ + {0x0000, 0x0363, 0x0363}, /* manufacturing use */ + {0x0000, 0x0368, 0x0368}, /* manufacturing use */ + {0x0000, 0x03F6, 0x03F7}, /* manufacturing use */ + {0x0000, 0x049E, 0x049F}, /* manufacturing use */ + {0x0000, 0x04A0, 0x04A3}, /* disagnostics */ + {0x0000, 0x04E6, 0x04E7}, /* manufacturing use */ + {0x0000, 0x4000, 0x7FFF}, /* internal BIOS use */ + {0x0000, 0x9000, 0x9001}, /* internal BIOS use */ + {0x0000, 0xA000, 0xBFFF}, /* write only */ + {0x0000, 0xEFF0, 0xEFFF}, /* internal BIOS use */ + /* handled by kernel: dell-laptop */ + {0x0000, BRIGHTNESS_TOKEN, BRIGHTNESS_TOKEN}, + {0x0000, KBD_LED_OFF_TOKEN, KBD_LED_AUTO_TOKEN}, + {0x0000, KBD_LED_AC_TOKEN, KBD_LED_AC_TOKEN}, + {0x0000, KBD_LED_AUTO_25_TOKEN, KBD_LED_AUTO_75_TOKEN}, + {0x0000, KBD_LED_AUTO_100_TOKEN, KBD_LED_AUTO_100_TOKEN}, + {0x0000, GLOBAL_MIC_MUTE_ENABLE, GLOBAL_MIC_MUTE_DISABLE}, +}; + static LIST_HEAD(smbios_device_list); int dell_smbios_error(int value) @@ -90,6 +176,108 @@ void dell_smbios_unregister_device(struct device *d) } EXPORT_SYMBOL_GPL(dell_smbios_unregister_device); +int dell_smbios_call_filter(struct device *d, + struct calling_interface_buffer *buffer) +{ + u16 t = 0; + int i; + + /* can't make calls over 30 */ + if (buffer->class > 30) { + dev_dbg(d, "class too big: %u\n", buffer->class); + return -EINVAL; + } + + /* supported calls on the particular system */ + if (!(da_supported_commands & (1 << buffer->class))) { + dev_dbg(d, "invalid command, supported commands: 0x%8x\n", + da_supported_commands); + return -EINVAL; + } + + /* match against call blacklist */ + for (i = 0; i < ARRAY_SIZE(call_blacklist); i++) { + if (buffer->class != call_blacklist[i].class) + continue; + if (buffer->select != call_blacklist[i].select && + call_blacklist[i].select != -1) + continue; + dev_dbg(d, "blacklisted command: %u/%u\n", + buffer->class, buffer->select); + return -EINVAL; + } + + /* if a token call, find token ID */ + if ((buffer->class == CLASS_TOKEN_READ && buffer->select < 3) || + (buffer->class == CLASS_TOKEN_WRITE && buffer->select < 3)) { + /* find the matching token ID */ + for (i = 0; i < da_num_tokens; i++) { + if (da_tokens[i].location != buffer->input[0]) + continue; + t = da_tokens[i].tokenID; + break; + } + + /* token call; but token didn't exist */ + if (!t) { + dev_dbg(d, "token at location %04x doesn't exist\n", + buffer->input[0]); + return -EINVAL; + } + + /* match against token blacklist */ + for (i = 0; i < ARRAY_SIZE(token_blacklist); i++) { + if (!token_blacklist[i].min || !token_blacklist[i].max) + continue; + if (t >= token_blacklist[i].min && + t <= token_blacklist[i].max) + return -EINVAL; + } + + /* match against token whitelist */ + for (i = 0; i < ARRAY_SIZE(token_whitelist); i++) { + if (!token_whitelist[i].min || !token_whitelist[i].max) + continue; + if (t < token_whitelist[i].min || + t > token_whitelist[i].max) + continue; + if (!token_whitelist[i].need_capability || + capable(token_whitelist[i].need_capability)) { + dev_dbg(d, "whitelisted token: %x\n", t); + return 0; + } + + } + } + /* match against call whitelist */ + for (i = 0; i < ARRAY_SIZE(call_whitelist); i++) { + if (buffer->class != call_whitelist[i].class) + continue; + if (buffer->select != call_whitelist[i].select) + continue; + if (!call_whitelist[i].need_capability || + capable(call_whitelist[i].need_capability)) { + dev_dbg(d, "whitelisted capable command: %u/%u\n", + buffer->class, buffer->select); + return 0; + } + dev_dbg(d, "missing capability %d for %u/%u\n", + call_whitelist[i].need_capability, + buffer->class, buffer->select); + + } + + /* not in a whitelist, only allow processes with capabilities */ + if (capable(CAP_SYS_RAWIO)) { + dev_dbg(d, "Allowing %u/%u due to CAP_SYS_RAWIO\n", + buffer->class, buffer->select); + return 0; + } + + return -EACCES; +} +EXPORT_SYMBOL_GPL(dell_smbios_call_filter); + int dell_smbios_call(struct calling_interface_buffer *buffer) { int (*call_fn)(struct calling_interface_buffer *) = NULL; @@ -168,6 +356,8 @@ static void __init parse_da_table(const struct dmi_header *dm) if (dm->length < 17) return; + da_supported_commands = table->supportedCmds; + new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) * sizeof(struct calling_interface_token), GFP_KERNEL); diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h index 956fe9304ed7..23256f79a4b8 100644 --- a/drivers/platform/x86/dell-smbios.h +++ b/drivers/platform/x86/dell-smbios.h @@ -26,9 +26,14 @@ #define SELECT_TOKEN_AC 2 #define CLASS_KBD_BACKLIGHT 4 #define SELECT_KBD_BACKLIGHT 11 +#define CLASS_FLASH_INTERFACE 7 +#define SELECT_FLASH_INTERFACE 3 +#define CLASS_ADMIN_PROP 10 +#define SELECT_ADMIN_PROP 3 #define CLASS_INFO 17 #define SELECT_RFKILL 11 #define SELECT_APP_REGISTRATION 3 +#define SELECT_DOCK 22 /* Tokens used in kernel drivers, any of these * should be filtered from userspace access @@ -44,6 +49,10 @@ #define KBD_LED_AUTO_100_TOKEN 0x02F6 #define GLOBAL_MIC_MUTE_ENABLE 0x0364 #define GLOBAL_MIC_MUTE_DISABLE 0x0365 + +/* tokens whitelisted to userspace use */ +#define CAPSULE_EN_TOKEN 0x0461 +#define CAPSULE_DIS_TOKEN 0x0462 #define WSMT_EN_TOKEN 0x04EC #define WSMT_DIS_TOKEN 0x04ED @@ -80,6 +89,8 @@ int dell_smbios_register_device(struct device *d, void *call_fn); void dell_smbios_unregister_device(struct device *d); int dell_smbios_error(int value); +int dell_smbios_call_filter(struct device *d, + struct calling_interface_buffer *buffer); int dell_smbios_call(struct calling_interface_buffer *buffer); struct calling_interface_token *dell_smbios_find_token(int tokenid);