RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[<Mario.Limonciello@dell.com>]

> -----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.