| Message ID | 83dc225624f1cde5ee1797aebc945732444ded91.1507733291.git.mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 11 Oct 2017 11:27:36 -0500 Mario Limonciello <mario.limonciello@dell.com> wrote: > There are some categories of tokens and SMBIOS calls that it makes > sense to protect userspace from accessing. These are calls that > may write to one time use fields or activate hardware debugging > capabilities. They are not intended for general purpose use. > > This same functionality may be be later extended to also intercept > calls that may cause kernel functionality to get out of sync if > the same functions are used by other drivers. This doesn't work. You are creating an API. If you then have to remove parts of the API because it messes stuff up you break your API guarantee. As I said before, this needs to be a whitelist of stuff that is safe, and it needs to have a security model. When you make a feature available you can't nicely take it away again as you'll break people's user space. Start with a whitelist of the ones you know people want to use, or that your existing tooling you want to enable uses. Add others as needed in future releases of the kernel. Alan
> -----Original Message----- > From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk] > Sent: Thursday, October 12, 2017 5:09 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; > pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; Greg > KH <greg@kroah.com> > Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability > for requests > > On Wed, 11 Oct 2017 11:27:36 -0500 > Mario Limonciello <mario.limonciello@dell.com> wrote: > > > There are some categories of tokens and SMBIOS calls that it makes > > sense to protect userspace from accessing. These are calls that > > may write to one time use fields or activate hardware debugging > > capabilities. They are not intended for general purpose use. > > > > This same functionality may be be later extended to also intercept > > calls that may cause kernel functionality to get out of sync if > > the same functions are used by other drivers. > > This doesn't work. You are creating an API. If you then have to remove > parts of the API because it messes stuff up you break your API guarantee. This potential "problem" already exists in that dcdbas provides a userspace interface that can manipulate this same data as used by some kernel drivers. The kernel drivers can be brought out of sync then. We discussed this a while back, you may not be aware of the discussion. The jist of it came down to that if a driver in the kernel decides to implement the same functionality that is available through the calling interface but with a different interface that the call could be filtered through the calling interface. Intercepting/filtering a call doesn't mean breaking API. It can be handled two ways that I can see and both are valid: 1) Return an error code. The calling interface already API allows for error return codes. Plenty of calls _already_ return errors. Returning one for a filtered call is no different. This would be an improvement over how dcdbas and existing kernel modules handle it. 2) Inspect the buffer and see that it's supposed to be handled by one of the existing kernel modules. Pass the call on to the (kernel) driver that already handles it, and return the result to userspace. This will keep the kernel driver and the userspace return values in sync. > > As I said before, this needs to be a whitelist of stuff that is safe, and > it needs to have a security model. From the BIOS perspective if no BIOS administrator password is set the OS and any tools can access any item in the calling interface. If a BIOS administrator password has been configured, many calls will return error codes unless the application has done a security key exchange with BIOS. This is both how dcdbas works today and how this interface that I provided to replace it works. Within Linux the security model is that items accessible through this interface are only accessible by root. If userspace tools choose to make items in this interface more widely accessible to say authenticated console users that's their prerogative. > When you make a feature available you > can't nicely take it away again as you'll break people's user space. As I said above -ENODEV or -EINVAL is not the same thing as taking a feature away. The character device API doesn't change when something is filtered. It's an error scenario. > > Start with a whitelist of the ones you know people want to use, or that > your existing tooling you want to enable uses. Add others as needed in > future releases of the kernel. > > Alan The existing dcdbas calling interface tooling (libsmbios) expects to be able to access all calls and all tokens. *The kernel doesn't filter any of it.* I understand the ask to filter some calls and that's why patch 10/15 exists, but please let me remind you this patch series is intended to /replace and deprecate/ dcdbas userspace access.
On Thursday 12 October 2017 13:23:08 Mario.Limonciello@dell.com wrote: > The existing dcdbas calling interface tooling (libsmbios) expects to be able > to access all calls and all tokens. *The kernel doesn't filter any of it.* It does not mean that API/ABI was designed correctly or incorrectly. Existing old API/ABI is there and we are not going to change it... > I understand the ask to filter some calls and that's why patch 10/15 exists, > but please let me remind you this patch series is intended to /replace and > deprecate/ dcdbas userspace access. Now when there is a proposal for a new API/ABI, it should be designed correctly without need to redesign it again in future and address all problems which are found during review.
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Thursday, October 12, 2017 9:34 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: gnomes@lxorguk.ukuu.org.uk; 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 > Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability > for requests > > On Thursday 12 October 2017 13:23:08 Mario.Limonciello@dell.com wrote: > > The existing dcdbas calling interface tooling (libsmbios) expects to be able > > to access all calls and all tokens. *The kernel doesn't filter any of it.* > > It does not mean that API/ABI was designed correctly or incorrectly. > Existing old API/ABI is there and we are not going to change it... > > > I understand the ask to filter some calls and that's why patch 10/15 exists, > > but please let me remind you this patch series is intended to /replace and > > deprecate/ dcdbas userspace access. > > Now when there is a proposal for a new API/ABI, it should be designed > correctly without need to redesign it again in future and address all > problems which are found during review. > Well sure I also would hate to have to redesign this again in the future. I believe that this is sufficient now. I'm looking up commands that FW claims can be supported and filtering the rest. There's your whitelist. I addressed the concern on the perceived dangerous calls (write once, debugging, manufacturing use only etc) and those are now filtered. There's your blacklist. Other than the minor errors that kbuild test robot caught from v6 and the s/desc_buffer/buffer/ in an earlier patch, what's left?
On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote: > On Wed, 11 Oct 2017 11:27:36 -0500 > Mario Limonciello <mario.limonciello@dell.com> wrote: > > > There are some categories of tokens and SMBIOS calls that it makes > > sense to protect userspace from accessing. These are calls that > > may write to one time use fields or activate hardware debugging > > capabilities. They are not intended for general purpose use. > > > > This same functionality may be be later extended to also intercept > > calls that may cause kernel functionality to get out of sync if > > the same functions are used by other drivers. > > This doesn't work. You are creating an API. If you then have to remove > parts of the API because it messes stuff up you break your API guarantee. > > As I said before, this needs to be a whitelist of stuff that is safe, and > it needs to have a security model. When you make a feature available you > can't nicely take it away again as you'll break people's user space. > > Start with a whitelist of the ones you know people want to use, or that > your existing tooling you want to enable uses. Add others as needed in > future releases of the kernel. Hi Alan, I've attempted to get a focused discussion on this topic a few times, but have failed to get it the focus in really needs. Thanks for bringing it up. I look forward to your thoughts on the following: The core of the issue is we are trying to achieve feature parity with Windows using the Windows Management Instrumentation (WMI) mechanism. I'm trying to support vendors like Dell in their attempts to do so, and acknowledging that this involves supporting a platform which was designed according to the norms and standards of the windows ecosystem. This WMI mechanism was designed to enable vendors to create management tools which could talk to interfaces their firmware exposed for this purpose without any platform specific OS driver or vendor specific knowledge on the part of the OS driver [1]. The result of this design decision is as you would expect: there is no consistency in design, no guarantee of interfaces with functional boundaries, across the various WMI interface implementations across vendors (or even within the interface of a given vendor on a given platform). The result is a mechanism which is fundamentally incompatible with standard Linux kernel approach to isolating userspace from the firmware. WMI enumerates available data, methods, and events, and shepherds a buffer of bits back and forth between firmware and userspace through these methods. Over the last few months, I've worked with Mario, Pali, Andy L, and others to try and find an acceptable way to support Dell in their efforts, while respecting Linux's design principles. The first concession was to agree not to automatically publish a WMI interface to userspace. This series includes the mechanism which requires a GUID-specific driver to be written and to provide the file ops structure, explicitly requesting the character device. This creates a WMI GUID granular whitelist. I'm willing to deny any such driver which is sent for review without the explicit collaboration of the vendor, to ensure they are doing their due diligence with respect to their firmware implementation. There has been some discussion in this thread regarding Dell's firmware testing, and the access these interfaces have to critical hardware resources. The second concession was to acknowledge that some features implemented in WMI are rightly the domain of the Linux kernel. LEDs, RFKill, Backlight control, hotkey support, etc. When an exported GUID is used for these purposes in addition to whatever purpose userspace requires (see my comment above about a lack of functional boundaries in the exported interfaces), we will provide a way to filter these usages. As Mario said, we can either return an error, or we can attempt to handle the request via the appropriate Linux kernel subsystem connections. From the Linux kernel side, the ask is that we acknowledge that it is not practical to audit every WMI interface to present a set of functional knobs to userspace - because they were specifically designed without that requirement. The platform was designed in this way specifically to optimize the software support effort on the part of the vendor. They don't write Windows OS drivers for these mechanisms, it is unrealistic to expect them to write them for Linux. I believe we need to arrive at a compromise which allows a vendor to work upstream like Mario is doing on behalf of Dell to fully enable their platforms as they were designed on Linux, while ensuring we don't adversely affect the functionality or security of other platforms. I believe the whitelist and blacklist above provides for this compromise. Perhaps an additional concession we should consider is to make exporting WMI interfaces a configuration option. If not set, the drivers will continue to work, but the WMI bus driver will not create the character device, even if requested. This would allow vendors to create a fully supported platform using upstream code, while allowing individuals the control to disable WMI userspace functionality if they don't trust the mechanism. The alternative seems to be that we accept these features will not be available on Linux for these platforms, or that vendors will develop a single wmi mapping driver out of tree, likely without the noted concessions. 1. https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx Specifically: "ACPI-to-WMI Mapper Goals for Windows Instrumentation"
On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote: > On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote: > > On Wed, 11 Oct 2017 11:27:36 -0500 > > Mario Limonciello <mario.limonciello@dell.com> wrote: > > > > > There are some categories of tokens and SMBIOS calls that it makes > > > sense to protect userspace from accessing. These are calls that > > > may write to one time use fields or activate hardware debugging > > > capabilities. They are not intended for general purpose use. > > > > > > This same functionality may be be later extended to also intercept > > > calls that may cause kernel functionality to get out of sync if > > > the same functions are used by other drivers. > > > > This doesn't work. You are creating an API. If you then have to remove > > parts of the API because it messes stuff up you break your API guarantee. > > > > As I said before, this needs to be a whitelist of stuff that is safe, and > > it needs to have a security model. When you make a feature available you > > can't nicely take it away again as you'll break people's user space. > > > > Start with a whitelist of the ones you know people want to use, or that > > your existing tooling you want to enable uses. Add others as needed in > > future releases of the kernel. > > Hi Alan, > > I've attempted to get a focused discussion on this topic a few times, > but have failed to get it the focus in really needs. Thanks for bringing > it up. I look forward to your thoughts on the following: > > The core of the issue is we are trying to achieve feature parity with > Windows using the Windows Management Instrumentation (WMI) mechanism. > I'm trying to support vendors like Dell in their attempts to do so, and > acknowledging that this involves supporting a platform which was > designed according to the norms and standards of the windows ecosystem. Note, we really don't care what Windows does, as I really doubt they care what we do, so this isn't a valid decision. Do we also care about OS-X and how it handles WMI? :) > This WMI mechanism was designed to enable vendors to create management > tools which could talk to interfaces their firmware exposed for this > purpose without any platform specific OS driver or vendor specific > knowledge on the part of the OS driver [1]. That was how Microsoft designed it, due to totally different requirements of their ecosystem. You really can't compare the two for obvious reasons, and I think that's the major disconnect here. > The result of this design decision is as you would expect: there is no > consistency in design, no guarantee of interfaces with functional > boundaries, across the various WMI interface implementations across > vendors (or even within the interface of a given vendor on a given > platform). See, different ecosystem, I like ours better :) > The result is a mechanism which is fundamentally incompatible with > standard Linux kernel approach to isolating userspace from the firmware. > WMI enumerates available data, methods, and events, and shepherds a > buffer of bits back and forth between firmware and userspace through > these methods. > > Over the last few months, I've worked with Mario, Pali, Andy L, and > others to try and find an acceptable way to support Dell in their > efforts, while respecting Linux's design principles. > > The first concession was to agree not to automatically publish a WMI > interface to userspace. This series includes the mechanism which > requires a GUID-specific driver to be written and to provide the file > ops structure, explicitly requesting the character device. This creates > a WMI GUID granular whitelist. I'm willing to deny any such driver which > is sent for review without the explicit collaboration of the vendor, to > ensure they are doing their due diligence with respect to their firmware > implementation. There has been some discussion in this thread regarding > Dell's firmware testing, and the access these interfaces have to > critical hardware resources. How are you going to prevent out-of-tree drivers from using this same api to directly get access to the WMI interface without using any type of whitelist or review? > The second concession was to acknowledge that some features implemented > in WMI are rightly the domain of the Linux kernel. LEDs, RFKill, > Backlight control, hotkey support, etc. When an exported GUID is used > for these purposes in addition to whatever purpose userspace requires > (see my comment above about a lack of functional boundaries in the > exported interfaces), we will provide a way to filter these usages. As > Mario said, we can either return an error, or we can attempt to handle > the request via the appropriate Linux kernel subsystem connections. That's great, but I don't see that in the patches posted, did I miss it? > From the Linux kernel side, the ask is that we acknowledge that it is > not practical to audit every WMI interface to present a set of > functional knobs to userspace - because they were specifically designed > without that requirement. Not true, we can audit whatever we want. whitelists are good for this very reason. Just because a vendor is used to replacing any part of the OS with their own custom api direct to the hardware in other operating systems, doesn't mean that we have to also emulate that crazy api as well, especially given that it can be easily abused, as we discussed before. > The platform was designed in this way specifically to optimize the > software support effort on the part of the vendor. They don't write > Windows OS drivers for these mechanisms, it is unrealistic to expect > them to write them for Linux. Note, we write drivers for hardware for free if asked, so is it unrealistic to require a kernel driver if we offer to do it for them? :) > I believe we need to arrive at a compromise which allows a vendor to > work upstream like Mario is doing on behalf of Dell to fully enable > their platforms as they were designed on Linux, while ensuring we don't > adversely affect the functionality or security of other platforms. I > believe the whitelist and blacklist above provides for this compromise. > > Perhaps an additional concession we should consider is to make exporting > WMI interfaces a configuration option. If not set, the drivers will > continue to work, but the WMI bus driver will not create the character > device, even if requested. This would allow vendors to create a fully > supported platform using upstream code, while allowing individuals the > control to disable WMI userspace functionality if they don't trust the > mechanism. config options do not work, distros have to turn them all on, you know that... > The alternative seems to be that we accept these features will not be > available on Linux for these platforms, or that vendors will develop a > single wmi mapping driver out of tree, likely without the noted > concessions. So you are saying they are blackmailing us that if we do not provide these wide-open api, then they just will take their toys and go home? In other words, back where we are today? :) I understand the goal here of getting this to all work properly, but note that this is a different type of an operating system, and for some things, maybe we just do not allow direct userspace access for the obvious reasons (security, maintance, auditability, long-term-support, etc.) A whitelist of known-good commands sounds like a good place to start, why not start with that and go from there? thanks, greg k-h
On Friday 13 October 2017 11:43:14 Greg KH wrote: > I understand the goal here of getting this to all work properly, but > note that this is a different type of an operating system, and for some > things, maybe we just do not allow direct userspace access for the > obvious reasons (security, maintance, auditability, long-term-support, > etc.) A whitelist of known-good commands sounds like a good place to > start, why not start with that and go from there? So, I understood that Greg want to rather see explicit whitelist in allowed functionality from userspace. And for such thing generic WMI API is not a best option. Creating generic API for kernel drivers for filtering WMI requests based on some white list is hard. If we want to choose this option, then it would be easier to create Dell specific API for setting SMBIOS tokens and in kernel just add list of whitelisted tokens, which userspace can set/unset. Parsing SMBIOS requests in WMI buffer is harder, it leads to more complicated code and basically serves same as "function specific API" which is easier to implement and audit. WMI is just "RPC" for function calls and inspecting it for security and access is harder as designing new simple API which would mimic what is "hidden" in that "RPC".
> Within Linux the security model is that items accessible through this interface > are only accessible by root. "root" has not been a security concept in the Linux kernel since about 2.0. If you are relying on file permissions then at best you are using CAP_SYS_DAC which is too weak for this. If you are allowing near unchecked communication with a third party entity that the user doesn't trust too much you should be requiring CAP_SYS_RAWIO. In fact it's a fair argument hat if you require CAP_SYS_RAWIO and have a module option you have to set to allow it that with the module loaded with say insmod dell_smbios factory=1 does even blacklisted stuff then you are ok, because a process with CAP_SYS_RAWIO has enough power to totally own the machine anyway including taking over and doing the WMI call itself by hand in user space or loading its own module. Alan
> -----Original Message----- > From: Greg KH [mailto:greg@kroah.com] > Sent: Friday, October 13, 2017 4:43 AM > To: Darren Hart <dvhart@infradead.org> > Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>; Limonciello, Mario > <Mario_Limonciello@Dell.com>; 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; pali.rohar@gmail.com; rjw@rjwysocki.net; > mjg59@google.com; hch@lst.de > Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability > for requests > > On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote: > > On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote: > > > On Wed, 11 Oct 2017 11:27:36 -0500 > > > Mario Limonciello <mario.limonciello@dell.com> wrote: > > > > > > > There are some categories of tokens and SMBIOS calls that it makes > > > > sense to protect userspace from accessing. These are calls that > > > > may write to one time use fields or activate hardware debugging > > > > capabilities. They are not intended for general purpose use. > > > > > > > > This same functionality may be be later extended to also intercept > > > > calls that may cause kernel functionality to get out of sync if > > > > the same functions are used by other drivers. > > > > > > This doesn't work. You are creating an API. If you then have to remove > > > parts of the API because it messes stuff up you break your API guarantee. > > > > > > As I said before, this needs to be a whitelist of stuff that is safe, and > > > it needs to have a security model. When you make a feature available you > > > can't nicely take it away again as you'll break people's user space. > > > > > > Start with a whitelist of the ones you know people want to use, or that > > > your existing tooling you want to enable uses. Add others as needed in > > > future releases of the kernel. > > > > Hi Alan, > > > > I've attempted to get a focused discussion on this topic a few times, > > but have failed to get it the focus in really needs. Thanks for bringing > > it up. I look forward to your thoughts on the following: > > > > The core of the issue is we are trying to achieve feature parity with > > Windows using the Windows Management Instrumentation (WMI) mechanism. > > I'm trying to support vendors like Dell in their attempts to do so, and > > acknowledging that this involves supporting a platform which was > > designed according to the norms and standards of the windows ecosystem. > > Note, we really don't care what Windows does, as I really doubt they > care what we do, so this isn't a valid decision. Do we also care about > OS-X and how it handles WMI? :) If you want to be able to offer tools that do the same thing on all OSes that hardware ships with it's a good idea to do it through the same interface in both places and to use the same code base on both if possible. Otherwise that's just asking for bugs. Maybe Dell is alone in this area in trying to improve this, I don't know. > > > This WMI mechanism was designed to enable vendors to create management > > tools which could talk to interfaces their firmware exposed for this > > purpose without any platform specific OS driver or vendor specific > > knowledge on the part of the OS driver [1]. > > That was how Microsoft designed it, due to totally different > requirements of their ecosystem. You really can't compare the two for > obvious reasons, and I think that's the major disconnect here. > > > The result of this design decision is as you would expect: there is no > > consistency in design, no guarantee of interfaces with functional > > boundaries, across the various WMI interface implementations across > > vendors (or even within the interface of a given vendor on a given > > platform). > > See, different ecosystem, I like ours better :) > > > The result is a mechanism which is fundamentally incompatible with > > standard Linux kernel approach to isolating userspace from the firmware. > > WMI enumerates available data, methods, and events, and shepherds a > > buffer of bits back and forth between firmware and userspace through > > these methods. > > > > Over the last few months, I've worked with Mario, Pali, Andy L, and > > others to try and find an acceptable way to support Dell in their > > efforts, while respecting Linux's design principles. > > > > The first concession was to agree not to automatically publish a WMI > > interface to userspace. This series includes the mechanism which > > requires a GUID-specific driver to be written and to provide the file > > ops structure, explicitly requesting the character device. This creates > > a WMI GUID granular whitelist. I'm willing to deny any such driver which > > is sent for review without the explicit collaboration of the vendor, to > > ensure they are doing their due diligence with respect to their firmware > > implementation. There has been some discussion in this thread regarding > > Dell's firmware testing, and the access these interfaces have to > > critical hardware resources. > > How are you going to prevent out-of-tree drivers from using this same > api to directly get access to the WMI interface without using any type > of whitelist or review? Out of tree drivers is really a distraction in this context. An out of tree driver could also just as easily create its own character device without using the WMI bus. > > > The second concession was to acknowledge that some features implemented > > in WMI are rightly the domain of the Linux kernel. LEDs, RFKill, > > Backlight control, hotkey support, etc. When an exported GUID is used > > for these purposes in addition to whatever purpose userspace requires > > (see my comment above about a lack of functional boundaries in the > > exported interfaces), we will provide a way to filter these usages. As > > Mario said, we can either return an error, or we can attempt to handle > > the request via the appropriate Linux kernel subsystem connections. > > That's great, but I don't see that in the patches posted, did I miss it? You are correct, as of v7 this specific filtering of stuff already done in kernel is not present. Unless there is more opposition to this approach I'll modify to return errors for those. > > > From the Linux kernel side, the ask is that we acknowledge that it is > > not practical to audit every WMI interface to present a set of > > functional knobs to userspace - because they were specifically designed > > without that requirement. > > Not true, we can audit whatever we want. whitelists are good for this > very reason. Just because a vendor is used to replacing any part of the > OS with their own custom api direct to the hardware in other operating > systems, doesn't mean that we have to also emulate that crazy api as > well, especially given that it can be easily abused, as we discussed > before. > > > The platform was designed in this way specifically to optimize the > > software support effort on the part of the vendor. They don't write > > Windows OS drivers for these mechanisms, it is unrealistic to expect > > them to write them for Linux. > > Note, we write drivers for hardware for free if asked, so is it > unrealistic to require a kernel driver if we offer to do it for them? :) > > > I believe we need to arrive at a compromise which allows a vendor to > > work upstream like Mario is doing on behalf of Dell to fully enable > > their platforms as they were designed on Linux, while ensuring we don't > > adversely affect the functionality or security of other platforms. I > > believe the whitelist and blacklist above provides for this compromise. > > > > Perhaps an additional concession we should consider is to make exporting > > WMI interfaces a configuration option. If not set, the drivers will > > continue to work, but the WMI bus driver will not create the character > > device, even if requested. This would allow vendors to create a fully > > supported platform using upstream code, while allowing individuals the > > control to disable WMI userspace functionality if they don't trust the > > mechanism. > > config options do not work, distros have to turn them all on, you know > that... > > > The alternative seems to be that we accept these features will not be > > available on Linux for these platforms, or that vendors will develop a > > single wmi mapping driver out of tree, likely without the noted > > concessions. > > So you are saying they are blackmailing us that if we do not provide > these wide-open api, then they just will take their toys and go home? > In other words, back where we are today? :) I think within all of this discussion the spirit of intent is getting lost. There is an interface that exists today in Windows as a dell signed kernel driver and in Linux as dcdbas that lets various tools access interesting pieces of data from the BIOS. Dell is deprecating this interface and moving to a driverless model on Windows and this patch series represents the parity of the "driverless" solution for Linux. Certainly I can advocate internally that all this effort is a waste of time and something can be carried out of tree and shipped with tools, but I don't want people using our tools to have to use tainted kernels. > > I understand the goal here of getting this to all work properly, but > note that this is a different type of an operating system, and for some > things, maybe we just do not allow direct userspace access for the > obvious reasons (security, maintance, auditability, long-term-support, > etc.) A whitelist of known-good commands sounds like a good place to > start, why not start with that and go from there? > Take off your "kernel" hat and put on a "customer" hat for a few moments while I try to put this in practical terms why the whitelist approach doesn't scale for what I'm trying to do. Let's say hypothetically a future version of this series that has only whitelisted commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL release etc. Hardware coming out about that time works fine, you can control the various knobs. Then later that year some new headless hardware is released that has zigbee controllers that work with inbox kernel drivers but you can't turn them on and off any way BUT through the manageability interface (it's headless!). In the manageability interface we also offer a new class/select or token that can control the GPIO that turns on/off these radios. A patch could be submitted upstream to whitelist that new class/select or token, but then you run into the problem that anyone on these stable LTS type distros won't pick it up. You can submit it to linux-stable and that might help Ubuntu, but a distro like RHEL doesn't track linux-stable. So that means now you're stuck in a place that tools don't work with the interface solely because of a whitelist that the kernel community hasn't yet blessed that it's ok to turn on/off zigbee controllers through the OEM's manageability interface. You now have introduced a ton of red tape that will artificially slow down supporting HW with Linux for something as simple as controlling a GPIO in a manageability interface. Tools that could otherwise work in many kernel versions will be gimped to only offer full functionality in the latest kernel versions. As a naïve customer why Does Dell's tool for managing my zigbee radios not work in RHEL 7.x but only in RHEL 7.x+1? No one will understand that. So considering the above isn't offering stuff like this a decision better made by the OEM? If the OEM doen't want customers to be able to modify something we don't offer it in the manageability interface. If the kernel community doesn't want people to be modifying something the OEM does offer, it can just as well be blacklisted in the kernel driver like the current filtering approach offers.
On Fri, 13 Oct 2017 15:03:10 +0000 > Take off your "kernel" hat and put on a "customer" hat for a few moments > while I try to put this in practical terms why the whitelist approach doesn't > scale for what I'm trying to do. As a customer I'm more worried about someone trashing my system or breaking my security. > So considering the above isn't offering stuff like this a decision better made by the OEM? > If the OEM doen't want customers to be able to modify something we don't offer it in the > manageability interface. If the kernel community doesn't want people to be > modifying something the OEM does offer, it can just as well be blacklisted in the > kernel driver like the current filtering approach offers. So you implement the rule if (whitelisted & (capabilities && whitelist->capability_need) == whitelist->capability_need)) return ALLOWED; if (capable(CAP_SYS_RAWIO)) return ALLOWED; return NO This puts you in the position where - known tools work and can sometimes be unprivileged. Privileged tools with enough priv to screw the machien can work anyway. Which is better than the starting point You could further enhance this by having a CAP_SYS_RAWIO interface to add whitelist entries, or to add an eBPF filter that can also make decisions for you. Now you've got the ability to push a policy update. Alan
> -----Original Message----- > From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk] > Sent: Friday, October 13, 2017 10:20 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: greg@kroah.com; dvhart@infradead.org; andy.shevchenko@gmail.com; > linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; > luto@kernel.org; quasisec@google.com; pali.rohar@gmail.com; > rjw@rjwysocki.net; mjg59@google.com; hch@lst.de > Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability > for requests > > On Fri, 13 Oct 2017 15:03:10 +0000 > > Take off your "kernel" hat and put on a "customer" hat for a few moments > > while I try to put this in practical terms why the whitelist approach doesn't > > scale for what I'm trying to do. > > As a customer I'm more worried about someone trashing my system or > breaking my security. > > > So considering the above isn't offering stuff like this a decision better made by > the OEM? > > If the OEM doen't want customers to be able to modify something we don't > offer it in the > > manageability interface. If the kernel community doesn't want people to be > > modifying something the OEM does offer, it can just as well be blacklisted in > the > > kernel driver like the current filtering approach offers. > > So you implement the rule > > if (whitelisted & (capabilities && whitelist->capability_need) == > whitelist->capability_need)) > return ALLOWED; > > if (capable(CAP_SYS_RAWIO)) > return ALLOWED; > > return NO > > This puts you in the position where - known tools work and can sometimes > be unprivileged. Privileged tools with enough priv to screw the machien > can work anyway. Which is better than the starting point > > > You could further enhance this by having a CAP_SYS_RAWIO interface to add > whitelist entries, or to add an eBPF filter that can also make decisions > for you. > > Now you've got the ability to push a policy update. > > Alan Thanks for this idea, I think it's productive in working towards a solution. I'll give it some more thought on what items I feel should be whitelisted to unprivileged processes. I feel like the number of entries that match this will be fairly low. I think I'd actually like to meld this with your other ideas and what I've currently got. What do you think of this approach: /* kernel community doesn't feel userspace should have access at all * or other kernel drivers use this */ if (blacklisted) return NO; /* unprivileged access allowed */ if (whitelisted & (capabilities && whitelist->capability_need) == whitelist->capability_need)) return ALLOWED; /* not yet in whitelist, or need privs to do */ if (capable(CAP_SYS_RAWIO)) return ALLOWED; return NO
Just want to respond to this part first: On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote: > Take off your "kernel" hat and put on a "customer" hat for a few moments > while I try to put this in practical terms why the whitelist approach doesn't > scale for what I'm trying to do. Heh, you do know my background in running an enterprise kernel team, right? :) > Let's say hypothetically a future version of this series that has only whitelisted > commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL > release etc. > Hardware coming out about that time works fine, you can control the various > knobs. Then later that year some new headless hardware is released that has > zigbee controllers that work with inbox kernel drivers but you can't turn them > on and off any way BUT through the manageability interface (it's headless!). > In the manageability interface we also offer a new class/select or token that > can control the GPIO that turns on/off these radios. It's the "later" that you are missing here. We only have code today for hardware we have today. If you come out with new hardware, you need new kernel drivers for it, and as such, "old" enterprise kernels will just not work properly. It's always been that way, this is nothing new, we can't predict the future, and is one big reason why I think the whole "enterprise" distro market is wrong and going to fail in the end :) Same goes for that new device id for the wifi chip, or the video camera or the fingerprint reader. Those have to be added to the kernel, and if the distro so desires, backported to their old and crufty version. This has been happening for two decades now, somehow coming up with a "raw" pipe from userspace to the kernel to control the hardware just because you don't want to update the kernel code, isn't going to solve the issue here (hint, you now have to update your userspace code, why is that suddenly easier than the kernel?) I know hardware companies want to stop writing software for their new hardware designs. I too want a pony :) Sorry, this argument isn't going to fly. greg k-h
On Fri, Oct 13, 2017 at 11:43:14AM +0200, Greg KH wrote: Hi Greg, first off - thanks for the response and the time you spent on it. My reply is longer than I'd like, but I'm not having much luck trimming it down without omitting things I think need to be noted. And as I try, the thread keeps getting longer. Ahhhhh :-) So I'm sending this now, and will continue with the rest of the thread. Yikes. > On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote: > > On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote: > > > On Wed, 11 Oct 2017 11:27:36 -0500 > > > Mario Limonciello <mario.limonciello@dell.com> wrote: > > > > > > > There are some categories of tokens and SMBIOS calls that it makes > > > > sense to protect userspace from accessing. These are calls that > > > > may write to one time use fields or activate hardware debugging > > > > capabilities. They are not intended for general purpose use. > > > > > > > > This same functionality may be be later extended to also intercept > > > > calls that may cause kernel functionality to get out of sync if > > > > the same functions are used by other drivers. > > > > > > This doesn't work. You are creating an API. If you then have to remove > > > parts of the API because it messes stuff up you break your API guarantee. > > > > > > As I said before, this needs to be a whitelist of stuff that is safe, and > > > it needs to have a security model. When you make a feature available you > > > can't nicely take it away again as you'll break people's user space. > > > > > > Start with a whitelist of the ones you know people want to use, or that > > > your existing tooling you want to enable uses. Add others as needed in > > > future releases of the kernel. > > > > Hi Alan, > > > > I've attempted to get a focused discussion on this topic a few times, > > but have failed to get it the focus in really needs. Thanks for bringing > > it up. I look forward to your thoughts on the following: > > > > The core of the issue is we are trying to achieve feature parity with > > Windows using the Windows Management Instrumentation (WMI) mechanism. > > I'm trying to support vendors like Dell in their attempts to do so, and > > acknowledging that this involves supporting a platform which was > > designed according to the norms and standards of the windows ecosystem. > > Note, we really don't care what Windows does, as I really doubt they > care what we do, so this isn't a valid decision. Do we also care about > OS-X and how it handles WMI? :) > > > This WMI mechanism was designed to enable vendors to create management > > tools which could talk to interfaces their firmware exposed for this > > purpose without any platform specific OS driver or vendor specific > > knowledge on the part of the OS driver [1]. > > That was how Microsoft designed it, due to totally different > requirements of their ecosystem. You really can't compare the two for > obvious reasons, and I think that's the major disconnect here. > > > The result of this design decision is as you would expect: there is no > > consistency in design, no guarantee of interfaces with functional > > boundaries, across the various WMI interface implementations across > > vendors (or even within the interface of a given vendor on a given > > platform). > > See, different ecosystem, I like ours better :) > I'm not sure if you're agreeing with me or arguing with me :-) The point I'm trying to make is that the this platform (and most laptops and PCs) was designed for the windows ecosystem, and is part of that ecosystem. They are very different, as you say above, and that means that it isn't reasonable to expect that platform to fit in nicely with the Linux ecosystem when it was designed for a very different one. I think that needs to be acknowledged as we try to support these platforms. Round hole, square peg... legos and tinker toys... whatever metaphor works. > > The result is a mechanism which is fundamentally incompatible with > > standard Linux kernel approach to isolating userspace from the firmware. > > WMI enumerates available data, methods, and events, and shepherds a > > buffer of bits back and forth between firmware and userspace through > > these methods. > > > > Over the last few months, I've worked with Mario, Pali, Andy L, and > > others to try and find an acceptable way to support Dell in their > > efforts, while respecting Linux's design principles. > > > > The first concession was to agree not to automatically publish a WMI > > interface to userspace. This series includes the mechanism which > > requires a GUID-specific driver to be written and to provide the file > > ops structure, explicitly requesting the character device. This creates > > a WMI GUID granular whitelist. I'm willing to deny any such driver which > > is sent for review without the explicit collaboration of the vendor, to > > ensure they are doing their due diligence with respect to their firmware > > implementation. There has been some discussion in this thread regarding > > Dell's firmware testing, and the access these interfaces have to > > critical hardware resources. > > How are you going to prevent out-of-tree drivers from using this same > api to directly get access to the WMI interface without using any type > of whitelist or review? I don't think that's the right question. The interface we're proposing here is more complex than a simple wmi-mapping driver would be. If someone were to create an out of tree driver, I suspect they would start by writing that, and the rest is moot. Map all WMI calls to userspace, export the MOF, done. Just like Windows. No need to write any platform specific drivers at all. I'm not saying it's right or even a good idea, but it's the shortest path to do what they want, and it's consistent with what they already do, and would simplify the porting of the userspace management applications. > > The second concession was to acknowledge that some features implemented > > in WMI are rightly the domain of the Linux kernel. LEDs, RFKill, > > Backlight control, hotkey support, etc. When an exported GUID is used > > for these purposes in addition to whatever purpose userspace requires > > (see my comment above about a lack of functional boundaries in the > > exported interfaces), we will provide a way to filter these usages. As > > Mario said, we can either return an error, or we can attempt to handle > > the request via the appropriate Linux kernel subsystem connections. > > That's great, but I don't see that in the patches posted, did I miss it? > New in v7. Patch 10/15 (this one). I would expect to see more of this in the other and future *-wmi drivers where a MOF is available and methods are used for both Linux internal things (LEDs, hotkeys) and for management tasks. Hopefully we don't see a lot of those oversubscribed GUIDs, but nothing in the spec discourages that - which means we'll see more than we'd like I'm sure :-( > > From the Linux kernel side, the ask is that we acknowledge that it is > > not practical to audit every WMI interface to present a set of > > functional knobs to userspace - because they were specifically designed > > without that requirement. > > Not true, we can audit whatever we want. whitelists are good for this > very reason. Just because a vendor is used to replacing any part of the > OS with their own custom api direct to the hardware in other operating > systems, doesn't mean that we have to also emulate that crazy api as > well, especially given that it can be easily abused, as we discussed > before. It's technically feasible, sure. My argument is that it isn't practical. Meaning vendors won't do it. It's actively contrary to the intent of the WMI mechanism, which means the interfaces won't have been designed to facilitate audit by the OS, and these audits have the potential to turn into incredibly complex bit checks. I discussed this previously here: https://lkml.org/lkml/2017/6/13/147 | > > And filter layer which will accept only WMI calls which are safe for | > > currently loaded/used kernel modules seems like a sane idea to ensure | > > functionality of kernel plus allow userspace to do other things. | > | > My biggest concern with this approach is maintenance. Because we would be doing | > something unforeseen by the specification, the various vendor implemented WMI | > APIs are not likely to be amenable to filtering. I can see these filters | > getting extremely complicated. They are "high touch", by which I mean each | > generation of platform may require subtle tweaks, which will be difficult to | > verify they don't break past generations. | | Agreed. As mentioned before I think the only sensible approach is | white listing GUIDs that have a valid userspace use case. And use | the dynamic IDs approach to add them for debugging and reverse | engineering. But, see the link for Christoph's full take on the subject :-) > > The platform was designed in this way specifically to optimize the > > software support effort on the part of the vendor. They don't write > > Windows OS drivers for these mechanisms, it is unrealistic to expect > > them to write them for Linux. > > Note, we write drivers for hardware for free if asked, so is it > unrealistic to require a kernel driver if we offer to do it for them? :) In my opinion, yes, it is. The system was designed to be driver-less. No company is going to place their product delivery schedule in the hands of an unpaid contractor who gets to dictate to them how their platform should be designed from the perspective of an OS with a tiny fraction of their install base. This is fundamentally different from how their entire organization is structured, and it's unrealistic to expect them to change that to support Linux on a laptop. Even if there were a few success stories, the model obviously can't scale. Vendors need to be able to design and implement their platform support on their own schedule and as independently as possible. Note, from the same link above, Christoph disagrees with me strongly on this point: | Hell no! The last thing we need on Linux is systems that once support | us added don't just work out of the box because you're missing your | vendor blob. Note that Mario has already posted links to the sources for the tools interacting with this interface, and the previous mechanism (libsmbios) source have also been publicly available. Perhaps we continue to make this a requirement to expose WMI features - an open source userspace client which uses them (seems we do that for other interfaces anyway). What I would tell Intel teams when trying to get them to work upstream to enable customers to build designs with their products was: Enable them to enable themselves, and get out of their way. I think a similar approach is required here. > > I believe we need to arrive at a compromise which allows a vendor to > > work upstream like Mario is doing on behalf of Dell to fully enable > > their platforms as they were designed on Linux, while ensuring we don't > > adversely affect the functionality or security of other platforms. I > > believe the whitelist and blacklist above provides for this compromise. > > > > Perhaps an additional concession we should consider is to make exporting > > WMI interfaces a configuration option. If not set, the drivers will > > continue to work, but the WMI bus driver will not create the character > > device, even if requested. This would allow vendors to create a fully > > supported platform using upstream code, while allowing individuals the > > control to disable WMI userspace functionality if they don't trust the > > mechanism. > > config options do not work, distros have to turn them all on, you know > that... > As I understand it, the distros will also take out of tree drivers and patches to support hardware platforms for partners, so I think that battle is already lost. I'd much rather have them integrating upstream code we have some influence over. If a CONFIG option is not acceptable, perhaps a runtime sysfs toggle? Or something like the factory=1 module parameter Alan mentioned to Mario. > > The alternative seems to be that we accept these features will not be > > available on Linux for these platforms, or that vendors will develop a > > single wmi mapping driver out of tree, likely without the noted > > concessions. > > So you are saying they are blackmailing us that if we do not provide > these wide-open api, then they just will take their toys and go home? > In other words, back where we are today? :) > That's a fun sound byte, but not what I said. ;-) I'm trying to address a practical reality. We're at a disadvantage because we're a minority OS for these types of systems. I think we'd all like stronger support for these platforms, and getting the vendors involved will help that. If all they have to do on Windows to support these features is update their management application, while on Linux they have to write a driver for every platform and GUID, and on top of that perform an audit in the kernel driver - all of which pushes more high maintenance platform specific knowledge into the Linux kernel... (effort which duplicates whatever they have done within the platform firmware already) I just don't believe any vendor will be able to justify the expense. > I understand the goal here of getting this to all work properly, but > note that this is a different type of an operating system, and for some > things, maybe we just do not allow direct userspace access for the > obvious reasons (security, maintance, auditability, long-term-support, > etc.) A whitelist of known-good commands sounds like a good place to > start, why not start with that and go from there? This was my initial position on this as well. A clearly defined functional interface from firmware which we can pick and choose how to export to userspace would be the ideal. Unfortunately, that just isn't what WMI is. As I mentioned above, at its core, WMI methods accept an arbitrarily sized bucket of bits. The notion of a "command" doesn't exist in the specification. I'm also not confident we can be sure these interfaces don't change over time from platform to platform (just another possible artifact from the design and intent of WMI). I forgot to mention the third concession yesterday. For future drivers for which the firmware provides a MOF (this one doesn't), we plan to do the MOF parsing within the kernel instead of just in userspace to provide some level of automated audit by the WMI subsystem - but it isn't yet clear to me how useful that will really be (suspect we can check types, but not values or ranges). I understand our (kernel maintainers') desire to mediate between userspace and firmware. I'm wondering if Alan's point about CAP_SYS_RAWIO is something that needs more consideration. Windows has a number of "namespace access settings" which limit the use of WMI, perhaps this would provide a similar parallel? https://msdn.microsoft.com/en-us/library/aa822575(v=vs.85).aspx I also understand the vendors' position of owning their platforms and using a common mechanism across OSes. In this case, and given the level of involvement from Dell in particular and their publication of the userspace tools and the whitelist and blacklist concessions, I think the right thing, and the only practical way to see WMI supported through upstream code, is to allow for WMI to be implemented in Linux in a way that is compatible with how they designed their platforms.
> -----Original Message----- > From: Greg KH [mailto:greg@kroah.com] > Sent: Friday, October 13, 2017 10:56 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; gnomes@lxorguk.ukuu.org.uk; > andy.shevchenko@gmail.com; linux-kernel@vger.kernel.org; platform-driver- > x86@vger.kernel.org; luto@kernel.org; quasisec@google.com; > pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de > Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability > for requests > > Just want to respond to this part first: > > On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote: > > Take off your "kernel" hat and put on a "customer" hat for a few moments > > while I try to put this in practical terms why the whitelist approach doesn't > > scale for what I'm trying to do. > > Heh, you do know my background in running an enterprise kernel team, > right? :) > > > Let's say hypothetically a future version of this series that has only whitelisted > > commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL > > release etc. > > Hardware coming out about that time works fine, you can control the various > > knobs. Then later that year some new headless hardware is released that has > > zigbee controllers that work with inbox kernel drivers but you can't turn them > > on and off any way BUT through the manageability interface (it's headless!). > > In the manageability interface we also offer a new class/select or token that > > can control the GPIO that turns on/off these radios. > > It's the "later" that you are missing here. We only have code today for > hardware we have today. If you come out with new hardware, you need new > kernel drivers for it, and as such, "old" enterprise kernels will just > not work properly. > > It's always been that way, this is nothing new, we can't predict the > future, and is one big reason why I think the whole "enterprise" distro > market is wrong and going to fail in the end :) > > Same goes for that new device id for the wifi chip, or the video camera > or the fingerprint reader. Those have to be added to the kernel, and if > the distro so desires, backported to their old and crufty version. > There's plenty of cases you're absolutely right and I have a hate/hate relationship regarding in this area. I was very careful in my example to pick something that it's just the manageability interface that changes. > This has been happening for two decades now, somehow coming up with a > "raw" pipe from userspace to the kernel to control the hardware just > because you don't want to update the kernel code, isn't going to solve > the issue here (hint, you now have to update your userspace code, why is > that suddenly easier than the kernel?) > It's easier because you can control your own destiny. Changing the kernel code means submitting something upstream, submitting something to -stable, convincing the enterprise distro folks to pull in your fix, adding code to your tool to test whether the kernel supports your newly whitelisted function adding support for this function to the tool and then posting that tool somewhere for consumption. Changing userspace code means changing userspace code, posting it somewhere and you're done. As Darren mentioned most companies are built around large organizations with specific touch points. The people who decide what the next features will be for manageability aren't the same people who write the tools, who aren't the same people that implement them in the BIOS and aren't the same people that would be submitting any of this into the kernel. Asking every new option to be whitelisted in the kernel and getting that translated into upstream, into stable, into distros, updating tools for etc is a giant process undertaking that companies will look at and say uh why? I think Alan's suggestions are leading this the right way though, whitelisting for certain unprivileged actions, blacklisting certain actions, looking for capabilities but still letting tools with the right capabilities or permissions do what they want to do. It's a balancing act.
> I think I'd actually like to meld this with your other ideas and what I've > currently got. What do you think of this approach: > > /* kernel community doesn't feel userspace should have access at all > * or other kernel drivers use this > */ > if (blacklisted) > return NO; > > /* unprivileged access allowed */ > if (whitelisted & (capabilities && whitelist->capability_need) == > whitelist->capability_need)) > return ALLOWED; > > /* not yet in whitelist, or need privs to do */ > if (capable(CAP_SYS_RAWIO)) > return ALLOWED; > > return NO > This looks sensible to me. Note that the middle case isn't necessarily 'unprviliged'. If the entyr is whitelisted and the capability_need is 0 then it means 'anyone' but you can also set any other appropriate capability (eg CAP_NET_ADMIN for a WMI call that does stuff to the wifi). Alan
On Fri, Oct 13, 2017 at 08:46:11PM +0100, One Thousand Gnomes wrote: > > I think I'd actually like to meld this with your other ideas and what I've > > currently got. What do you think of this approach: > > > > /* kernel community doesn't feel userspace should have access at all > > * or other kernel drivers use this > > */ > > if (blacklisted) > > return NO; > > > > /* unprivileged access allowed */ > > if (whitelisted & (capabilities && whitelist->capability_need) == > > whitelist->capability_need)) > > return ALLOWED; > > > > /* not yet in whitelist, or need privs to do */ > > if (capable(CAP_SYS_RAWIO)) > > return ALLOWED; > > > > return NO > > > > This looks sensible to me. Note that the middle case isn't necessarily > 'unprviliged'. If the entyr is whitelisted and the capability_need is 0 > then it means 'anyone' but you can also set any other appropriate > capability (eg CAP_NET_ADMIN for a WMI call that does stuff to the wifi). Thank you Alan. This model appears consistent in intent with some of the higher level WMI access privileges in the Windows OS, and translate fairly well to Linux. This seems like a good model at least for the dell smbios driver in this thread. I do have some concerns about the ability to audit the buffer in the general case. Dell uses a sensible buffer format with 'command' and 'class' fields, but the WMI spec explicitly makes no requirement on the format of the buffer. This may prove much harder to implement for other vendors depending on their format - but perhaps we accept that and deal with that as it comes up.
On Fri, Oct 13, 2017 at 05:56:03PM +0200, Greg KH wrote: > Just want to respond to this part first: > > On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote: > > Take off your "kernel" hat and put on a "customer" hat for a few moments > > while I try to put this in practical terms why the whitelist approach doesn't > > scale for what I'm trying to do. > > Heh, you do know my background in running an enterprise kernel team, > right? :) > > > Let's say hypothetically a future version of this series that has only whitelisted > > commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL > > release etc. > > Hardware coming out about that time works fine, you can control the various > > knobs. Then later that year some new headless hardware is released that has > > zigbee controllers that work with inbox kernel drivers but you can't turn them > > on and off any way BUT through the manageability interface (it's headless!). > > In the manageability interface we also offer a new class/select or token that > > can control the GPIO that turns on/off these radios. > > It's the "later" that you are missing here. We only have code today for > hardware we have today. If you come out with new hardware, you need new > kernel drivers for it, and as such, "old" enterprise kernels will just > not work properly. > > It's always been that way, this is nothing new, we can't predict the > future, and is one big reason why I think the whole "enterprise" distro > market is wrong and going to fail in the end :) Just because it's always been that way, doesn't mean it has to continue to be that way. We have a few examples where we support implementing what was formerly kernel space in userspace, such as FUSE and libusb. The details are different, but the idea is the same. Allow people to do things they need to do without having to modify the kernel. > > Same goes for that new device id for the wifi chip, or the video camera > or the fingerprint reader. Those have to be added to the kernel, and if > the distro so desires, backported to their old and crufty version. > > This has been happening for two decades now, somehow coming up with a > "raw" pipe from userspace to the kernel to control the hardware just > because you don't want to update the kernel code, isn't going to solve > the issue here (hint, you now have to update your userspace code, why is > that suddenly easier than the kernel?) Even if updating the kernel was just as easy as updating a management application they control, that's still twice the work. > I know hardware companies want to stop writing software for their new > hardware designs. I too want a pony :) > > Sorry, this argument isn't going to fly. Well... I don't think reducing the amount of software required to support a new platform is a bad thing. I'm curious for your take on Alan's CAPs recommendation.
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c index a2afd7cb24fc..f21ec4e60e5b 100644 --- a/drivers/platform/x86/dell-smbios.c +++ b/drivers/platform/x86/dell-smbios.c @@ -24,6 +24,7 @@ #include <linux/platform_device.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,57 @@ struct smbios_device { int (*call_fn)(struct calling_interface_buffer *); }; + +/* calls that should be blacklisted. may contain diagnostics, + * debugging information or are write once functions + */ +struct smbios_call { + int class; + int select; +}; + +static struct smbios_call call_blacklist[] = { + {01, 07}, /* manufacturing use */ + {06, 05}, /* manufacturing use */ + {11, 03}, /* write once */ + {11, 07}, /* write once */ + {11, 11}, /* write once */ + {19, -1}, /* diagnostics */ +}; + +/* tokens corresponding to diagnostics or write once locations */ +struct token_range { + u16 exact_value; + u16 min; + u16 max; +}; + +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 */ + {0x02E3, 0x0000, 0x0000}, /* manufacturing use */ + {0x02FF, 0x0000, 0x0000}, /* manufacturing use */ + {0x0000, 0x0300, 0x0302}, /* manufacturing use */ + {0x0000, 0x0325, 0x0326}, /* manufacturing use */ + {0x0000, 0x0332, 0x0335}, /* fan control */ + {0x0350, 0x0000, 0x0000}, /* manufacturing use */ + {0x0363, 0x0000, 0x0000}, /* manufacturing use */ + {0x0368, 0x0000, 0x0000}, /* 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 */ +}; + static LIST_HEAD(smbios_device_list); int dell_smbios_error(int value) @@ -90,6 +142,72 @@ 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) +{ + int i; + u16 t = 0; + + /* can't make calls over 30 */ + if (buffer->class > 30) { + dev_dbg(d, "buffer->class too big: %d\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: %d/%d\n", + buffer->class, buffer->select); + return -EINVAL; + } + + /* if a token call, find token ID */ + if ((buffer->class == 0 && buffer->select < 3) || + (buffer->class == 1 && buffer->select < 3)) { + for (i = 0; i < da_num_tokens; i++) { + /* find the matching token ID */ + if (da_tokens[i].location != buffer->input[0]) + continue; + t = da_tokens[i].tokenID; + break; + } + /* not a token call */ + } else + return 0; + + /* token call; but token didn't exist */ + if (!t) { + dev_dbg(d, "token at location %u 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].exact_value && + t == token_blacklist[i].exact_value) + return -EINVAL; + if (!token_blacklist[i].min || !token_blacklist[i].max) + continue; + if (t >= token_blacklist[i].min && t <= token_blacklist[i].max) + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL_GPL(dell_smbios_call_filter); + int dell_smbios_call(struct calling_interface_buffer *buffer) { int (*call_fn)(struct calling_interface_buffer *) = NULL; @@ -113,6 +231,13 @@ int dell_smbios_call(struct calling_interface_buffer *buffer) goto out_smbios_call; } + if (dell_smbios_call_filter(selected_dev, buffer)) { + ret = -EINVAL; + dev_err(selected_dev, "Invalid call %d/%d:%8x\n", + buffer->class, buffer->select, buffer->input[0]); + goto out_smbios_call; + } + ret = call_fn(buffer); out_smbios_call: @@ -168,6 +293,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 911916be73d4..98950298fc51 100644 --- a/drivers/platform/x86/dell-smbios.h +++ b/drivers/platform/x86/dell-smbios.h @@ -51,6 +51,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);
There are some categories of tokens and SMBIOS calls that it makes sense to protect userspace from accessing. These are calls that may write to one time use fields or activate hardware debugging capabilities. They are not intended for general purpose use. This same functionality may be be later extended to also intercept calls that may cause kernel functionality to get out of sync if the same functions are used by other drivers. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- drivers/platform/x86/dell-smbios.c | 127 +++++++++++++++++++++++++++++++++++++ drivers/platform/x86/dell-smbios.h | 2 + 2 files changed, 129 insertions(+)