[v4,13/14] platform/x86: dell-smbios-wmi: introduce userspace interface

Commit Message

Limonciello, Mario Oct. 4, 2017, 10:48 p.m. UTC

This userspace character device will be used to perform SMBIOS calls
from any applications.

It provides an ioctl that will allow passing the 32k WMI calling
interface buffer between userspace and kernel space.

This character device is intended to deprecate the dcdbas kernel module
and the interface that it provides to userspace.

It's important for the driver to provide a R/W ioctl to ensure that
two competing userspace processes don't race to provide or read each
others data.

The API for interacting with this interface is defined in documentation
as well as a uapi header provides the format of the structures.

Signed-off-by: Mario Limonciello <[e-mail]>
---
 Documentation/ABI/testing/dell-smbios-wmi          | 43 ++++++++++
 .../ABI/testing/sysfs-platform-dell-smbios-wmi     | 10 +++
 MAINTAINERS                                        |  1 +
 drivers/platform/x86/dell-smbios-wmi.c             | 98 ++++++++++++++++++++++
 drivers/platform/x86/dell-smbios-wmi.h             | 10 ---
 include/uapi/linux/dell-smbios-wmi.h               | 25 ++++++
 6 files changed, 177 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/dell-smbios-wmi
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
 create mode 100644 include/uapi/linux/dell-smbios-wmi.h

Comments

Greg KH Oct. 5, 2017, 7:23 a.m. UTC | #1

On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications.
> 
> It provides an ioctl that will allow passing the 32k WMI calling
> interface buffer between userspace and kernel space.

{sigh}  Did you really test this?  It feels like it wasn't, due to the
api you are using here.  Did you run it with a 32bit userspace and 64bit
kernel?  32bit kernel/userspace?  How well did your userspace developer
fall down crying when you tried that?  :)

> This character device is intended to deprecate the dcdbas kernel module
> and the interface that it provides to userspace.

At least that driver has a well-documented api to userspace, you are
throwing all of that away here, are you _sure_ you want to do that?
Seems like you just made things much harder.

> It's important for the driver to provide a R/W ioctl to ensure that
> two competing userspace processes don't race to provide or read each
> others data.

The whole goal of this patch is to provide that ioctl, right?  So of
course it is "important" :)

> The API for interacting with this interface is defined in documentation
> as well as a uapi header provides the format of the structures.

Ok, let's _just_ review that api please:

> diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-smbios-wmi.h
> new file mode 100644
> index 000000000000..0d0d09b04021
> --- /dev/null
> +++ b/include/uapi/linux/dell-smbios-wmi.h
> @@ -0,0 +1,25 @@
> +#ifndef _UAPI_DELL_SMBIOS_WMI_H_
> +#define _UAPI_DELL_SMBIOS_WMI_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/wmi.h>
> +
> +struct wmi_calling_interface_buffer {
> +	u16 class;
> +	u16 select;
> +	u32 input[4];
> +	u32 output[4];
> +	u32 argattrib;
> +	u32 blength;
> +	u8 *data;
> +} __packed;

{sigh}

For structures that cross the user/kernel boundry, you _HAVE_ to use the
correct types.  For some things, that is easy, u16 needs to be __u16,
u32 needs to be __u32, but u8*?  Hah, good luck!  Remember what I
mentioned above about 32/64 bit issues?

Why do you need a pointer here at all?  You are providing a huge chunk
of memory to the ioctl, what's the use of a pointer?  How are you
dereferenceing this pointer (remember, it's a userspace pointer, which
you are not saying here, so odds are the kernel code is wrong...)


> +struct wmi_smbios_ioctl {
> +	u32 length;

__u32.

And why not __u64?  Is 32 bits always going to be ok?

> +	struct wmi_calling_interface_buffer *buf;

Another pointer?  2 pointer dereferences in the same ioctl structure?
Crazy, you are wanting to make your life harder than it has to be...


> +};
> +
> +/* only offers on the single instance */
> +#define DELL_WMI_SMBIOS_CMD		WMI_IOWR(0)

I don't understand the comment, please explain it better.

Please sit down and work out your api here, I don't think you have
thought it through properly, given the number of pointers alone.

thanks,

greg k-h

Greg KH Oct. 5, 2017, 7:33 a.m. UTC | #2

On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> +static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	void __user *p = (void __user *) arg;
> +	struct wmi_smbios_ioctl *input;
> +	struct wmi_smbios_priv *priv;
> +	struct wmi_device *wdev;
> +	size_t ioctl_size;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	/* we only operate on first instance */
> +	case DELL_WMI_SMBIOS_CMD:
> +		wdev = get_first_wmi_device();
> +		if (!wdev) {
> +			pr_err("No WMI devices bound\n");

dev_err(), you are a driver, never use "raw" pr_ calls.

> +			return -ENODEV;
> +		}
> +		ioctl_size = sizeof(struct wmi_smbios_ioctl);
> +		priv = dev_get_drvdata(&wdev->dev);
> +		input = kmalloc(ioctl_size, GFP_KERNEL);
> +		if (!input)
> +			return -ENOMEM;
> +		mutex_lock(&wmi_mutex);
> +		if (!access_ok(VERIFY_READ, p, ioctl_size)) {

Hm, any time I see an access_ok() call, I get scared.  You should almost
never need to make that call if you are using the correct kernel apis.

> +			pr_err("Unsafe userspace pointer passed\n");

dev_err().

> +			return -EFAULT;

Memory leak!


> +		}
> +		if (copy_from_user(input, p, ioctl_size)) {
> +			ret = -EFAULT;

So, why did you call access_ok() followed by copy_from_user()?
copy_from/to() handle all of that for you automatically.

> +			goto fail_smbios_cmd;
> +		}
> +		if (input->length != priv->buffer_size) {
> +			pr_err("Got buffer size %d expected %d\n",
> +				input->length, priv->buffer_size);

length is user provided, it can be whatever anyone sets it to.  I don't
understand this error.

> +			ret = -EINVAL;
> +			goto fail_smbios_cmd;
> +		}
> +		if (!access_ok(VERIFY_WRITE, input->buf, priv->buffer_size)) {
> +			pr_err("Unsafe userspace pointer passed\n");

Again, don't need this.

> +			ret = -EFAULT;
> +			goto fail_smbios_cmd;
> +		}
> +		if (copy_from_user(priv->buf, input->buf, priv->buffer_size)) {

Wait, input->buf is a user pointer?  Ick, see my previous email about
your crazy api here being a mess.  This should not be needed.

And as you "know" the buffer size already, why do you have userspace
specify it?  What good is it?

> +			ret = -EFAULT;
> +			goto fail_smbios_cmd;
> +		}
> +		ret = run_smbios_call(wdev);

No other checking of the values in the structure?  You just "trust"
userspace to get it all right?  Hah!

> +		if (ret != 0)
> +			goto fail_smbios_cmd;

You didn't run this through checkpatch :(


> +		if (copy_to_user(input->buf, priv->buf, priv->buffer_size))
> +			ret = -EFAULT;
> +fail_smbios_cmd:
> +		kfree(input);
> +		mutex_unlock(&wmi_mutex);
> +		break;
> +	default:
> +		pr_err("unsupported ioctl: %d.\n", cmd);
> +		ret = -ENOIOCTLCMD;
> +	}
> +	return ret;
> +}
> +
> +static ssize_t buffer_size_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct wmi_smbios_priv *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", priv->buffer_size);
> +}
> +static DEVICE_ATTR_RO(buffer_size);
> +
> +static struct attribute *smbios_wmi_attrs[] = {
> +	&dev_attr_buffer_size.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group smbios_wmi_attribute_group = {
> +	.attrs = smbios_wmi_attrs,
> +};
> +
>  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
>  {
>  	struct wmi_smbios_priv *priv;
> @@ -127,6 +209,11 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
>  	if (!priv->buf)
>  		return -ENOMEM;
>  
> +	ret = sysfs_create_group(&wdev->dev.kobj,
> +				 &smbios_wmi_attribute_group);

Hint, if a driver ever makes a call to sysfs_*(), something is wrong, it
should never be needed.

Also, you just raced with userspace and lost :(

There is a way to fix all of this, in a simple way, I'll leave that as
an exercise for the reader, I've reviewed enough of this code for
today...

> +static const struct file_operations dell_smbios_wmi_fops = {
> +	.owner		= THIS_MODULE,

And who uses that field?  Hint, no one is, which is another issue that I
forgot to review in your previous patch where you use this structure.
What is protecting this module from being unloaded while the ioctl call
is running?  (hint, nothing...)

I need more coffee...

greg k-h

Alan Cox Oct. 5, 2017, 1:59 p.m. UTC | #3

On Wed,  4 Oct 2017 17:48:39 -0500
Mario Limonciello <[e-mail]> wrote:

> This userspace character device will be used to perform SMBIOS calls
> from any applications.
> 
> It provides an ioctl that will allow passing the 32k WMI calling
> interface buffer between userspace and kernel space.

What is your security model for firing 32K of random crap at the BIOS ?
Do you fuzz test the BIOS interface ?

How do we know that between now and the end of the universe every call is
safe to execute as any random user without upsetting other users on the
same PC ?

Right now this patch is scary. U've fuzzed tested BIOS firmware in thepast
and it almost universally ended up in reaching for the power cord because
PC firmware is usually closed and usually crap.

In addition you are assuming that every function you ever provide via
that ioctl has the same securiy model. So if one of them should only be
usable by the user logged in on the console the system would have to
enforce that for all. If you have two conflicting security policies we'd
have to make it root only and owned by a daemon. If a BIOS turns out to
have a hole then we have to make it CAP_SYS_RAWIO.

/dev/dorandomvendorspecificshit is not an API and not a security policy.

Alan

Limonciello, Mario Oct. 5, 2017, 2:22 p.m. UTC | #4

> -----Original Message-----
> From: Alan Cox [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 8:59 AM
> To: Limonciello, Mario <[e-mail]>
> Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> LKML <[e-mail]>; [e-mail];
> Andy Lutomirski <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Greg
> KH <[e-mail]>
> Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> On Wed,  4 Oct 2017 17:48:39 -0500
> Mario Limonciello <[e-mail]> wrote:
> 
> > This userspace character device will be used to perform SMBIOS calls
> > from any applications.
> >
> > It provides an ioctl that will allow passing the 32k WMI calling
> > interface buffer between userspace and kernel space.
> 
> What is your security model for firing 32K of random crap at the BIOS ?

Adding new class and select methods requires a review with the security
team.  They will do STRIDE analysis and threat modeling.

> Do you fuzz test the BIOS interface ?

Yes there has been internal fuzz testing classes and selects used in the 
ACPI interface in the past.  I can't comment on how regularly that is done.
I do think it's interesting is to use the interface in Linux for further fuzz
testing though.

> 
> How do we know that between now and the end of the universe every call is
> safe to execute as any random user without upsetting other users on the
> same PC ?

Any random user shouldn't be executing the ioctl.
Only root should be executing any of these calls.

If there is a particular call that is deemed too dangerous to have available the ioctl
call can be filtered by the kernel module.

> 
> Right now this patch is scary. U've fuzzed tested BIOS firmware in thepast
> and it almost universally ended up in reaching for the power cord because
> PC firmware is usually closed and usually crap.
>

Can you please share more context?

> In addition you are assuming that every function you ever provide via
> that ioctl has the same securiy model. So if one of them should only be
> usable by the user logged in on the console the system would have to
> enforce that for all. If you have two conflicting security policies we'd
> have to make it root only and owned by a daemon. If a BIOS turns out to
> have a hole then we have to make it CAP_SYS_RAWIO.
> 
> /dev/dorandomvendorspecificshit is not an API and not a security policy.
> 
> Alan

All functionality offered through this interface has the same security model.
This should only be made available to root, but I don't see the need for a
daemon to further sit between calls.  The two applications that I see using
this in the short term (fwupd and fwupdate) both run as root and would
directly use this interface.

Greg KH Oct. 5, 2017, 3:44 p.m. UTC | #5

On Thu, Oct 05, 2017 at 02:22:37PM +0000, [e-mail] wrote:
> > -----Original Message-----
> > From: Alan Cox [mailto:[e-mail]]
> > Sent: Thursday, October 5, 2017 8:59 AM
> > To: Limonciello, Mario <[e-mail]>
> > Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> > LKML <[e-mail]>; [e-mail];
> > Andy Lutomirski <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; [e-mail]; Greg
> > KH <[e-mail]>
> > Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> > userspace interface
> > 
> > On Wed,  4 Oct 2017 17:48:39 -0500
> > Mario Limonciello <[e-mail]> wrote:
> > 
> > > This userspace character device will be used to perform SMBIOS calls
> > > from any applications.
> > >
> > > It provides an ioctl that will allow passing the 32k WMI calling
> > > interface buffer between userspace and kernel space.
> > 
> > What is your security model for firing 32K of random crap at the BIOS ?
> 
> Adding new class and select methods requires a review with the security
> team.  They will do STRIDE analysis and threat modeling.
> 
> > Do you fuzz test the BIOS interface ?
> 
> Yes there has been internal fuzz testing classes and selects used in the 
> ACPI interface in the past.  I can't comment on how regularly that is done.
> I do think it's interesting is to use the interface in Linux for further fuzz
> testing though.

That should be simple, start firing random data at this memory location
and see what happens.  Can you brick the box?  Change the
manufactured-date?  Change the serial number?  Normally these types of
BIOS interfaces allow all sorts of "fun" things like this, which is why
we have the kernel "own" the interface, to protect yourself from
breaking the box.

> > How do we know that between now and the end of the universe every call is
> > safe to execute as any random user without upsetting other users on the
> > same PC ?
> 
> Any random user shouldn't be executing the ioctl.
> Only root should be executing any of these calls.

"only root" isn't the best protection method, you should know better :)

You are going to have to do some kind of parsing/whitelisting here,
trust us...

thanks,

greg k-h

Pali Rohár Oct. 5, 2017, 3:56 p.m. UTC | #6

On Thursday 05 October 2017 17:44:45 Greg KH wrote:
> On Thu, Oct 05, 2017 at 02:22:37PM +0000, [e-mail] wrote:
> > > -----Original Message-----
> > > From: Alan Cox [mailto:[e-mail]]
> > > Sent: Thursday, October 5, 2017 8:59 AM
> > > To: Limonciello, Mario <[e-mail]>
> > > Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> > > LKML <[e-mail]>; [e-mail];
> > > Andy Lutomirski <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail]; [e-mail]; Greg
> > > KH <[e-mail]>
> > > Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> > > userspace interface
> > > 
> > > On Wed,  4 Oct 2017 17:48:39 -0500
> > > Mario Limonciello <[e-mail]> wrote:
> > > 
> > > > This userspace character device will be used to perform SMBIOS calls
> > > > from any applications.
> > > >
> > > > It provides an ioctl that will allow passing the 32k WMI calling
> > > > interface buffer between userspace and kernel space.
> > > 
> > > What is your security model for firing 32K of random crap at the BIOS ?
> > 
> > Adding new class and select methods requires a review with the security
> > team.  They will do STRIDE analysis and threat modeling.
> > 
> > > Do you fuzz test the BIOS interface ?
> > 
> > Yes there has been internal fuzz testing classes and selects used in the 
> > ACPI interface in the past.  I can't comment on how regularly that is done.
> > I do think it's interesting is to use the interface in Linux for further fuzz
> > testing though.
> 
> That should be simple, start firing random data at this memory location
> and see what happens.  Can you brick the box?  Change the
> manufactured-date?  Change the serial number?  Normally these types of
> BIOS interfaces allow all sorts of "fun" things like this, which is why
> we have the kernel "own" the interface, to protect yourself from
> breaking the box.

There are also Dell calls to "permanently disable TPM" and similar which
are irreversible. So this can be really a problem.

> > > How do we know that between now and the end of the universe every call is
> > > safe to execute as any random user without upsetting other users on the
> > > same PC ?
> > 
> > Any random user shouldn't be executing the ioctl.
> > Only root should be executing any of these calls.
> 
> "only root" isn't the best protection method, you should know better :)
> 
> You are going to have to do some kind of parsing/whitelisting here,
> trust us...

That is also why I in past suggested to create fully transparent
whitelisting filter for all WMI calls. And properly implement particular
filter in kernel...

But that is of course hard as you need to know all details of internal
structures and data which may be sent via userspace. To prevent e.g.
action "permanently disable TPM" in kernel.

> thanks,
> 
> greg k-h

Greg KH Oct. 5, 2017, 4:28 p.m. UTC | #7

On Thu, Oct 05, 2017 at 05:56:19PM +0200, Pali Rohár wrote:
> On Thursday 05 October 2017 17:44:45 Greg KH wrote:
> > On Thu, Oct 05, 2017 at 02:22:37PM +0000, [e-mail] wrote:
> > > > -----Original Message-----
> > > > From: Alan Cox [mailto:[e-mail]]
> > > > Sent: Thursday, October 5, 2017 8:59 AM
> > > > To: Limonciello, Mario <[e-mail]>
> > > > Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> > > > LKML <[e-mail]>; [e-mail];
> > > > Andy Lutomirski <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail]; [e-mail]; Greg
> > > > KH <[e-mail]>
> > > > Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> > > > userspace interface
> > > > 
> > > > On Wed,  4 Oct 2017 17:48:39 -0500
> > > > Mario Limonciello <[e-mail]> wrote:
> > > > 
> > > > > This userspace character device will be used to perform SMBIOS calls
> > > > > from any applications.
> > > > >
> > > > > It provides an ioctl that will allow passing the 32k WMI calling
> > > > > interface buffer between userspace and kernel space.
> > > > 
> > > > What is your security model for firing 32K of random crap at the BIOS ?
> > > 
> > > Adding new class and select methods requires a review with the security
> > > team.  They will do STRIDE analysis and threat modeling.
> > > 
> > > > Do you fuzz test the BIOS interface ?
> > > 
> > > Yes there has been internal fuzz testing classes and selects used in the 
> > > ACPI interface in the past.  I can't comment on how regularly that is done.
> > > I do think it's interesting is to use the interface in Linux for further fuzz
> > > testing though.
> > 
> > That should be simple, start firing random data at this memory location
> > and see what happens.  Can you brick the box?  Change the
> > manufactured-date?  Change the serial number?  Normally these types of
> > BIOS interfaces allow all sorts of "fun" things like this, which is why
> > we have the kernel "own" the interface, to protect yourself from
> > breaking the box.
> 
> There are also Dell calls to "permanently disable TPM" and similar which
> are irreversible. So this can be really a problem.
> 
> > > > How do we know that between now and the end of the universe every call is
> > > > safe to execute as any random user without upsetting other users on the
> > > > same PC ?
> > > 
> > > Any random user shouldn't be executing the ioctl.
> > > Only root should be executing any of these calls.
> > 
> > "only root" isn't the best protection method, you should know better :)
> > 
> > You are going to have to do some kind of parsing/whitelisting here,
> > trust us...
> 
> That is also why I in past suggested to create fully transparent
> whitelisting filter for all WMI calls. And properly implement particular
> filter in kernel...
> 
> But that is of course hard as you need to know all details of internal
> structures and data which may be sent via userspace. To prevent e.g.
> action "permanently disable TPM" in kernel.

It's not "hard" as userspace would have to know these same things if it
were to just have an clean "pipe" to the BIOS as it has to create and
parse the commands to get the BIOS to do anything.

So I agree with you, whitelisting seems to be the only sane solution,
unless people don't like their TPMs to be erased by root exploits?  :)

thanks,

greg k-h

Limonciello, Mario Oct. 5, 2017, 4:28 p.m. UTC | #8

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 2:23 AM
> To: Limonciello, Mario <[e-mail]>
> Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> LKML <[e-mail]>; [e-mail];
> Andy Lutomirski <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> > This userspace character device will be used to perform SMBIOS calls
> > from any applications.
> >
> > It provides an ioctl that will allow passing the 32k WMI calling
> > interface buffer between userspace and kernel space.
> 
> {sigh}  Did you really test this?  It feels like it wasn't, due to the
> api you are using here.  Did you run it with a 32bit userspace and 64bit
> kernel?  32bit kernel/userspace?  How well did your userspace developer
> fall down crying when you tried that?  :)

I tested 64 bit kernel / 64 bit userspace.

> 
> > This character device is intended to deprecate the dcdbas kernel module
> > and the interface that it provides to userspace.
> 
> At least that driver has a well-documented api to userspace, you are
> throwing all of that away here, are you _sure_ you want to do that?
> Seems like you just made things much harder.

Being a well-documented API isn't the same as a "good" API.  Have you
seen how exactly that driver works to userspace?  It's not pretty.

That BIOS <-> OS interface that we use with it will stop working too on new
machines at some point soon too.  With no new interface available
this will just mean no way to get this data from userspace.

> 
> > It's important for the driver to provide a R/W ioctl to ensure that
> > two competing userspace processes don't race to provide or read each
> > others data.
> 
> The whole goal of this patch is to provide that ioctl, right?  So of
> course it is "important" :)
> 
> > The API for interacting with this interface is defined in documentation
> > as well as a uapi header provides the format of the structures.
> 
> Ok, let's _just_ review that api please:
> 
> > diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-
> smbios-wmi.h
> > new file mode 100644
> > index 000000000000..0d0d09b04021
> > --- /dev/null
> > +++ b/include/uapi/linux/dell-smbios-wmi.h
> > @@ -0,0 +1,25 @@
> > +#ifndef _UAPI_DELL_SMBIOS_WMI_H_
> > +#define _UAPI_DELL_SMBIOS_WMI_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/wmi.h>
> > +
> > +struct wmi_calling_interface_buffer {
> > +	u16 class;
> > +	u16 select;
> > +	u32 input[4];
> > +	u32 output[4];
> > +	u32 argattrib;
> > +	u32 blength;
> > +	u8 *data;
> > +} __packed;
> 
> {sigh}
> 
> For structures that cross the user/kernel boundry, you _HAVE_ to use the
> correct types.  For some things, that is easy, u16 needs to be __u16,
> u32 needs to be __u32, but u8*?  Hah, good luck!  Remember what I
> mentioned above about 32/64 bit issues?
> 
> Why do you need a pointer here at all?  You are providing a huge chunk
> of memory to the ioctl, what's the use of a pointer?  How are you
> dereferenceing this pointer (remember, it's a userspace pointer, which
> you are not saying here, so odds are the kernel code is wrong...)

So the part that is probably not obvious here is that the size of this buffer
The BIOS will expect will vary from one machine to another.  The two sizes
that will be encountered are 4k and 32k.  The last 10 years it's been 4k,
we just jumped up to 32k.  Maybe some day it will be 64k, who knows.

This detail of the size is encoded in the MOF, and also available through
the dell-wmi-descriptor driver call I added to look up buffer size.
> 
> 
> > +struct wmi_smbios_ioctl {
> > +	u32 length;
> 
> __u32.
> 
> And why not __u64?  Is 32 bits always going to be ok?

If length stays around, I'll adjust this.

> 
> > +	struct wmi_calling_interface_buffer *buf;
> 
> Another pointer?  2 pointer dereferences in the same ioctl structure?
> Crazy, you are wanting to make your life harder than it has to be...
> 
Well so the way I look at it, if you have to support 4k and 32k, you
want userspace to at least claim that it allocated the right size.

> 
> > +};
> > +
> > +/* only offers on the single instance */
> > +#define DELL_WMI_SMBIOS_CMD		WMI_IOWR(0)
> 
> I don't understand the comment, please explain it better.
> 
> Please sit down and work out your api here, I don't think you have
> thought it through properly, given the number of pointers alone.
> 
> thanks,
> 
> greg k-h

So I did give this some thought, which is why I ended up with where 
I am.

1) Userspace reads buffer size
2) Userspace allocates a structure to pass buffer size and a pointer
3) Userpsace allocates another structure of what buffer size should be and 
fills in the first structure with the length of the pointer.
4) Kernel picks it up, and if it sees that userspace used the wrong length
complains.
5) If it's the right length, kernel unwinds and does stuff.


The other alternative is to just always do 32k to and from userspace
and internally in the kernel copy only the supported size.
the difference in output is just the size of the data section, which
above 4k isn't used by very many calls anyway.

That's obviously a much cleaner API, but what happens if one day this
does need to go to 64k?  I hope it doesn't, but I don't have a crystal ball.

Pali Rohár Oct. 5, 2017, 4:34 p.m. UTC | #9

On Thursday 05 October 2017 16:28:40 [e-mail] wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:[e-mail]]
> > Sent: Thursday, October 5, 2017 2:23 AM
> > To: Limonciello, Mario <[e-mail]>
> > Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> > LKML <[e-mail]>; [e-mail];
> > Andy Lutomirski <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; [e-mail]
> > Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> > userspace interface
> > 
> > On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> > > This userspace character device will be used to perform SMBIOS calls
> > > from any applications.
> > >
> > > It provides an ioctl that will allow passing the 32k WMI calling
> > > interface buffer between userspace and kernel space.
> > 
> > {sigh}  Did you really test this?  It feels like it wasn't, due to the
> > api you are using here.  Did you run it with a 32bit userspace and 64bit
> > kernel?  32bit kernel/userspace?  How well did your userspace developer
> > fall down crying when you tried that?  :)
> 
> I tested 64 bit kernel / 64 bit userspace.
> 
> > 
> > > This character device is intended to deprecate the dcdbas kernel module
> > > and the interface that it provides to userspace.
> > 
> > At least that driver has a well-documented api to userspace, you are
> > throwing all of that away here, are you _sure_ you want to do that?
> > Seems like you just made things much harder.
> 
> Being a well-documented API isn't the same as a "good" API.  Have you
> seen how exactly that driver works to userspace?  It's not pretty.
> 
> That BIOS <-> OS interface that we use with it will stop working too on new
> machines at some point soon too.  With no new interface available
> this will just mean no way to get this data from userspace.
> 
> > 
> > > It's important for the driver to provide a R/W ioctl to ensure that
> > > two competing userspace processes don't race to provide or read each
> > > others data.
> > 
> > The whole goal of this patch is to provide that ioctl, right?  So of
> > course it is "important" :)
> > 
> > > The API for interacting with this interface is defined in documentation
> > > as well as a uapi header provides the format of the structures.
> > 
> > Ok, let's _just_ review that api please:
> > 
> > > diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-
> > smbios-wmi.h
> > > new file mode 100644
> > > index 000000000000..0d0d09b04021
> > > --- /dev/null
> > > +++ b/include/uapi/linux/dell-smbios-wmi.h
> > > @@ -0,0 +1,25 @@
> > > +#ifndef _UAPI_DELL_SMBIOS_WMI_H_
> > > +#define _UAPI_DELL_SMBIOS_WMI_H_
> > > +
> > > +#include <linux/ioctl.h>
> > > +#include <linux/wmi.h>
> > > +
> > > +struct wmi_calling_interface_buffer {
> > > +	u16 class;
> > > +	u16 select;
> > > +	u32 input[4];
> > > +	u32 output[4];
> > > +	u32 argattrib;
> > > +	u32 blength;
> > > +	u8 *data;
> > > +} __packed;
> > 
> > {sigh}
> > 
> > For structures that cross the user/kernel boundry, you _HAVE_ to use the
> > correct types.  For some things, that is easy, u16 needs to be __u16,
> > u32 needs to be __u32, but u8*?  Hah, good luck!  Remember what I
> > mentioned above about 32/64 bit issues?
> > 
> > Why do you need a pointer here at all?  You are providing a huge chunk
> > of memory to the ioctl, what's the use of a pointer?  How are you
> > dereferenceing this pointer (remember, it's a userspace pointer, which
> > you are not saying here, so odds are the kernel code is wrong...)
> 
> So the part that is probably not obvious here is that the size of this buffer
> The BIOS will expect will vary from one machine to another.  The two sizes
> that will be encountered are 4k and 32k.  The last 10 years it's been 4k,
> we just jumped up to 32k.  Maybe some day it will be 64k, who knows.
> 
> This detail of the size is encoded in the MOF, and also available through
> the dell-wmi-descriptor driver call I added to look up buffer size.
> > 
> > 
> > > +struct wmi_smbios_ioctl {
> > > +	u32 length;
> > 
> > __u32.
> > 
> > And why not __u64?  Is 32 bits always going to be ok?
> 
> If length stays around, I'll adjust this.
> 
> > 
> > > +	struct wmi_calling_interface_buffer *buf;
> > 
> > Another pointer?  2 pointer dereferences in the same ioctl structure?
> > Crazy, you are wanting to make your life harder than it has to be...
> > 
> Well so the way I look at it, if you have to support 4k and 32k, you
> want userspace to at least claim that it allocated the right size.
> 
> > 
> > > +};
> > > +
> > > +/* only offers on the single instance */
> > > +#define DELL_WMI_SMBIOS_CMD		WMI_IOWR(0)
> > 
> > I don't understand the comment, please explain it better.
> > 
> > Please sit down and work out your api here, I don't think you have
> > thought it through properly, given the number of pointers alone.
> > 
> > thanks,
> > 
> > greg k-h
> 
> So I did give this some thought, which is why I ended up with where 
> I am.
> 
> 1) Userspace reads buffer size
> 2) Userspace allocates a structure to pass buffer size and a pointer
> 3) Userpsace allocates another structure of what buffer size should be and 
> fills in the first structure with the length of the pointer.
> 4) Kernel picks it up, and if it sees that userspace used the wrong length
> complains.
> 5) If it's the right length, kernel unwinds and does stuff.
> 
> 
> The other alternative is to just always do 32k to and from userspace
> and internally in the kernel copy only the supported size.
> the difference in output is just the size of the data section, which
> above 4k isn't used by very many calls anyway.
> 
> That's obviously a much cleaner API, but what happens if one day this
> does need to go to 64k?  I hope it doesn't, but I don't have a crystal ball.

I hope that buffer itself is zero padded when passed to firmware. If
this is true, should not API/ABI looks like that userspace pass buffer
length + buffer itself and then kernel copy userspace buffer to kernel
buffer which would have right size (with possibility to zero pad it)?
And in case kernel buffer is smaller, userspace would get error.

Limonciello, Mario Oct. 5, 2017, 4:37 p.m. UTC | #10

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 2:33 AM
> To: Limonciello, Mario <[e-mail]>
> Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> LKML <[e-mail]>; [e-mail];
> Andy Lutomirski <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> > +static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd,
> > +	unsigned long arg)
> > +{
> > +	void __user *p = (void __user *) arg;
> > +	struct wmi_smbios_ioctl *input;
> > +	struct wmi_smbios_priv *priv;
> > +	struct wmi_device *wdev;
> > +	size_t ioctl_size;
> > +	int ret = 0;
> > +
> > +	switch (cmd) {
> > +	/* we only operate on first instance */
> > +	case DELL_WMI_SMBIOS_CMD:
> > +		wdev = get_first_wmi_device();
> > +		if (!wdev) {
> > +			pr_err("No WMI devices bound\n");
> 
> dev_err(), you are a driver, never use "raw" pr_ calls.
>
This particular error was if a device wasn't found.  It's a codepath I don't
really know can happen from the ioctl, but wanted to be sure.  I'll just remove
this error message.
 
> > +			return -ENODEV;
> > +		}
> > +		ioctl_size = sizeof(struct wmi_smbios_ioctl);
> > +		priv = dev_get_drvdata(&wdev->dev);
> > +		input = kmalloc(ioctl_size, GFP_KERNEL);
> > +		if (!input)
> > +			return -ENOMEM;
> > +		mutex_lock(&wmi_mutex);
> > +		if (!access_ok(VERIFY_READ, p, ioctl_size)) {
> 
> Hm, any time I see an access_ok() call, I get scared.  You should almost
> never need to make that call if you are using the correct kernel apis.
> 
> > +			pr_err("Unsafe userspace pointer passed\n");
> 
> dev_err().
>

Yep got it.
 
> > +			return -EFAULT;
> 
> Memory leak!

Doh, thanks.

> 
> 
> > +		}
> > +		if (copy_from_user(input, p, ioctl_size)) {
> > +			ret = -EFAULT;
> 
> So, why did you call access_ok() followed by copy_from_user()?
> copy_from/to() handle all of that for you automatically.
> 

I wasn't aware access_ok was handled in copy_from/to.  I'll remove this.

> > +			goto fail_smbios_cmd;
> > +		}
> > +		if (input->length != priv->buffer_size) {
> > +			pr_err("Got buffer size %d expected %d\n",
> > +				input->length, priv->buffer_size);
> 
> length is user provided, it can be whatever anyone sets it to.  I don't
> understand this error.
> 
> > +			ret = -EINVAL;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		if (!access_ok(VERIFY_WRITE, input->buf, priv->buffer_size)) {
> > +			pr_err("Unsafe userspace pointer passed\n");
> 
> Again, don't need this.
> 
> > +			ret = -EFAULT;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		if (copy_from_user(priv->buf, input->buf, priv->buffer_size)) {
> 
> Wait, input->buf is a user pointer?  Ick, see my previous email about
> your crazy api here being a mess.  This should not be needed.
> 
> And as you "know" the buffer size already, why do you have userspace
> specify it?  What good is it?

I mentioned this in the other mail, but the thing is buffer size is not consistent
from machine to machine.

It's convenient that this change happened now because otherwise there would
have been assumptions about it always being 4k that needed to be thrown away.

> 
> > +			ret = -EFAULT;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		ret = run_smbios_call(wdev);
> 
> No other checking of the values in the structure?  You just "trust"
> userspace to get it all right?  Hah!
> 
> > +		if (ret != 0)
> > +			goto fail_smbios_cmd;
> 
> You didn't run this through checkpatch :(
> 
I did..

$ ~/src/linux/scripts/checkpatch.pl 0013-platform-x86-dell-smbios-wmi-
introduce-userspace-int.patch 
total: 0 errors, 0 warnings, 241 lines checked

0013-platform-x86-dell-smbios-wmi-introduce-userspace-int.patch has no obvious 
style problems and is ready for submission.

> 
> > +		if (copy_to_user(input->buf, priv->buf, priv->buffer_size))
> > +			ret = -EFAULT;
> > +fail_smbios_cmd:
> > +		kfree(input);
> > +		mutex_unlock(&wmi_mutex);
> > +		break;
> > +	default:
> > +		pr_err("unsupported ioctl: %d.\n", cmd);
> > +		ret = -ENOIOCTLCMD;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static ssize_t buffer_size_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct wmi_smbios_priv *priv = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d\n", priv->buffer_size);
> > +}
> > +static DEVICE_ATTR_RO(buffer_size);
> > +
> > +static struct attribute *smbios_wmi_attrs[] = {
> > +	&dev_attr_buffer_size.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group smbios_wmi_attribute_group = {
> > +	.attrs = smbios_wmi_attrs,
> > +};
> > +
> >  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> >  {
> >  	struct wmi_smbios_priv *priv;
> > @@ -127,6 +209,11 @@ static int dell_smbios_wmi_probe(struct wmi_device
> *wdev)
> >  	if (!priv->buf)
> >  		return -ENOMEM;
> >
> > +	ret = sysfs_create_group(&wdev->dev.kobj,
> > +				 &smbios_wmi_attribute_group);
> 
> Hint, if a driver ever makes a call to sysfs_*(), something is wrong, it
> should never be needed.
> 
> Also, you just raced with userspace and lost :(
> 
> There is a way to fix all of this, in a simple way, I'll leave that as
> an exercise for the reader, I've reviewed enough of this code for
> today...
> 

Thanks, I see what I should do instead, I'll make changes.

> > +static const struct file_operations dell_smbios_wmi_fops = {
> > +	.owner		= THIS_MODULE,
> 
> And who uses that field?  Hint, no one is, which is another issue that I
> forgot to review in your previous patch where you use this structure.
> What is protecting this module from being unloaded while the ioctl call
> is running?  (hint, nothing...)
> 

Thanks, I'll add locking.

Greg KH Oct. 5, 2017, 4:40 p.m. UTC | #11

On Thu, Oct 05, 2017 at 04:28:40PM +0000, [e-mail] wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:[e-mail]]
> > Sent: Thursday, October 5, 2017 2:23 AM
> > To: Limonciello, Mario <[e-mail]>
> > Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> > LKML <[e-mail]>; [e-mail];
> > Andy Lutomirski <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; [e-mail]
> > Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> > userspace interface
> > 
> > On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> > > This userspace character device will be used to perform SMBIOS calls
> > > from any applications.
> > >
> > > It provides an ioctl that will allow passing the 32k WMI calling
> > > interface buffer between userspace and kernel space.
> > 
> > {sigh}  Did you really test this?  It feels like it wasn't, due to the
> > api you are using here.  Did you run it with a 32bit userspace and 64bit
> > kernel?  32bit kernel/userspace?  How well did your userspace developer
> > fall down crying when you tried that?  :)
> 
> I tested 64 bit kernel / 64 bit userspace.

It showed :(

> > > This character device is intended to deprecate the dcdbas kernel module
> > > and the interface that it provides to userspace.
> > 
> > At least that driver has a well-documented api to userspace, you are
> > throwing all of that away here, are you _sure_ you want to do that?
> > Seems like you just made things much harder.
> 
> Being a well-documented API isn't the same as a "good" API.  Have you
> seen how exactly that driver works to userspace?  It's not pretty.
> 
> That BIOS <-> OS interface that we use with it will stop working too on new
> machines at some point soon too.  With no new interface available
> this will just mean no way to get this data from userspace.

Sure it will stop, if you change the BIOS to use a different interface.
I don't understand the comparison here, you will have to do _something_,
right?  Giving a "raw pipe" to userspace doesn't feel like a good
solution given the issues involved (see the other emails in this
thread...)

> > > It's important for the driver to provide a R/W ioctl to ensure that
> > > two competing userspace processes don't race to provide or read each
> > > others data.
> > 
> > The whole goal of this patch is to provide that ioctl, right?  So of
> > course it is "important" :)
> > 
> > > The API for interacting with this interface is defined in documentation
> > > as well as a uapi header provides the format of the structures.
> > 
> > Ok, let's _just_ review that api please:
> > 
> > > diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-
> > smbios-wmi.h
> > > new file mode 100644
> > > index 000000000000..0d0d09b04021
> > > --- /dev/null
> > > +++ b/include/uapi/linux/dell-smbios-wmi.h
> > > @@ -0,0 +1,25 @@
> > > +#ifndef _UAPI_DELL_SMBIOS_WMI_H_
> > > +#define _UAPI_DELL_SMBIOS_WMI_H_
> > > +
> > > +#include <linux/ioctl.h>
> > > +#include <linux/wmi.h>
> > > +
> > > +struct wmi_calling_interface_buffer {
> > > +	u16 class;
> > > +	u16 select;
> > > +	u32 input[4];
> > > +	u32 output[4];
> > > +	u32 argattrib;
> > > +	u32 blength;
> > > +	u8 *data;
> > > +} __packed;
> > 
> > {sigh}
> > 
> > For structures that cross the user/kernel boundry, you _HAVE_ to use the
> > correct types.  For some things, that is easy, u16 needs to be __u16,
> > u32 needs to be __u32, but u8*?  Hah, good luck!  Remember what I
> > mentioned above about 32/64 bit issues?
> > 
> > Why do you need a pointer here at all?  You are providing a huge chunk
> > of memory to the ioctl, what's the use of a pointer?  How are you
> > dereferenceing this pointer (remember, it's a userspace pointer, which
> > you are not saying here, so odds are the kernel code is wrong...)
> 
> So the part that is probably not obvious here is that the size of this buffer
> The BIOS will expect will vary from one machine to another.  The two sizes
> that will be encountered are 4k and 32k.  The last 10 years it's been 4k,
> we just jumped up to 32k.  Maybe some day it will be 64k, who knows.
> 
> This detail of the size is encoded in the MOF, and also available through
> the dell-wmi-descriptor driver call I added to look up buffer size.

Fine, then make it a variable structure size.  Using a pointer is not
how to do it in a portable way (if you tried a 32bit userspace, boom...)

> > > +	struct wmi_calling_interface_buffer *buf;
> > 
> > Another pointer?  2 pointer dereferences in the same ioctl structure?
> > Crazy, you are wanting to make your life harder than it has to be...
> > 
> Well so the way I look at it, if you have to support 4k and 32k, you
> want userspace to at least claim that it allocated the right size.

Again, variable length structures are your friend.

> So I did give this some thought, which is why I ended up with where 
> I am.
> 
> 1) Userspace reads buffer size

From where?  That's a crazy thing in the first place you know, how does
the kernel "know" this in a way that userspace doesn't ahead of time?

> 2) Userspace allocates a structure to pass buffer size and a pointer

No pointers!

> 3) Userpsace allocates another structure of what buffer size should be and 
> fills in the first structure with the length of the pointer.

Ick ick ick.

> 4) Kernel picks it up, and if it sees that userspace used the wrong length
> complains.

length checking is good, no objection there.

> 5) If it's the right length, kernel unwinds and does stuff.

"unwinding" is hard to do right, on all of the needed combinations, try
it.  I'll be waiting :)

variable length structures are your friend, you can still put the length
it in, no objection there (you need it.)

hope this helps,

greg k-h

Limonciello, Mario Oct. 5, 2017, 4:48 p.m. UTC | #12

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 11:28 AM
> To: Pali Rohár <[e-mail]>
> Cc: Limonciello, Mario <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; platform-driver-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> On Thu, Oct 05, 2017 at 05:56:19PM +0200, Pali Rohár wrote:
> > On Thursday 05 October 2017 17:44:45 Greg KH wrote:
> > > On Thu, Oct 05, 2017 at 02:22:37PM +0000, [e-mail]
> wrote:
> > > > > -----Original Message-----
> > > > > From: Alan Cox [mailto:[e-mail]]
> > > > > Sent: Thursday, October 5, 2017 8:59 AM
> > > > > To: Limonciello, Mario <[e-mail]>
> > > > > Cc: [e-mail]; Andy Shevchenko
> <[e-mail]>;
> > > > > LKML <[e-mail]>; platform-driver-
> [e-mail];
> > > > > Andy Lutomirski <[e-mail]>; [e-mail];
> > > > > [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Greg
> > > > > KH <[e-mail]>
> > > > > Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> > > > > userspace interface
> > > > >
> > > > > On Wed,  4 Oct 2017 17:48:39 -0500
> > > > > Mario Limonciello <[e-mail]> wrote:
> > > > >
> > > > > > This userspace character device will be used to perform SMBIOS calls
> > > > > > from any applications.
> > > > > >
> > > > > > It provides an ioctl that will allow passing the 32k WMI calling
> > > > > > interface buffer between userspace and kernel space.
> > > > >
> > > > > What is your security model for firing 32K of random crap at the BIOS ?
> > > >
> > > > Adding new class and select methods requires a review with the security
> > > > team.  They will do STRIDE analysis and threat modeling.
> > > >
> > > > > Do you fuzz test the BIOS interface ?
> > > >
> > > > Yes there has been internal fuzz testing classes and selects used in the
> > > > ACPI interface in the past.  I can't comment on how regularly that is done.
> > > > I do think it's interesting is to use the interface in Linux for further fuzz
> > > > testing though.
> > >
> > > That should be simple, start firing random data at this memory location
> > > and see what happens.  Can you brick the box?  Change the
> > > manufactured-date?  Change the serial number?  Normally these types of
> > > BIOS interfaces allow all sorts of "fun" things like this, which is why
> > > we have the kernel "own" the interface, to protect yourself from
> > > breaking the box.
> >
> > There are also Dell calls to "permanently disable TPM" and similar which
> > are irreversible. So this can be really a problem.
> >
> > > > > How do we know that between now and the end of the universe every
> call is
> > > > > safe to execute as any random user without upsetting other users on the
> > > > > same PC ?
> > > >
> > > > Any random user shouldn't be executing the ioctl.
> > > > Only root should be executing any of these calls.
> > >
> > > "only root" isn't the best protection method, you should know better :)
> > >
> > > You are going to have to do some kind of parsing/whitelisting here,
> > > trust us...
> >
> > That is also why I in past suggested to create fully transparent
> > whitelisting filter for all WMI calls. And properly implement particular
> > filter in kernel...
> >
> > But that is of course hard as you need to know all details of internal
> > structures and data which may be sent via userspace. To prevent e.g.
> > action "permanently disable TPM" in kernel.
> 
> It's not "hard" as userspace would have to know these same things if it
> were to just have an clean "pipe" to the BIOS as it has to create and
> parse the commands to get the BIOS to do anything.
> 
> So I agree with you, whitelisting seems to be the only sane solution,
> unless people don't like their TPMs to be erased by root exploits?  :)
> 
> thanks,
> 
> greg k-h

Permanently disable TPM can't be run from OS through this interface.
Something like that requires a special manufacturing mode (which also
can't be activated through this interface).

There are some "write once" items that for the general purpose user that
won't brick a box, but should probably be blacklisted though.
I'll think this through some more.

Alan Cox Oct. 10, 2017, 7:40 p.m. UTC | #13

> There are some "write once" items that for the general purpose user that
> won't brick a box, but should probably be blacklisted though.
> I'll think this through some more.

I would strongly urge you to do whitelisting as it's good security. If
you can divide the calls into something like this it fits the Linxu
desktop model well:

Safe -> anyone can do it
Console -> only console owner
Superuser -> administrator level (things that can be irritating but
don't persist a reboot)

Beyond that it's trickier - if you are an enterprise business then you
don't for example want to allow someone with root on the system to
subvert BIOS level settings they can't access.

You need a minimum of CAP_SYS_RAWIO for anything that can (through bugs
or design) subvert secure boot type stuff. It's also probably an
appropriate check for 'anything goes' if you decide to have that
category. CAP_SYS_RAWIO implies total power over the machine although
with secureboot it's less clear.

Alan

Limonciello, Mario Oct. 10, 2017, 7:51 p.m. UTC | #14

Alan,

> -----Original Message-----
> From: Alan Cox [mailto:[e-mail]]
> Sent: Tuesday, October 10, 2017 2:41 PM
> To: Limonciello, Mario <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; platform-driver-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> > There are some "write once" items that for the general purpose user that
> > won't brick a box, but should probably be blacklisted though.
> > I'll think this through some more.
> 
> I would strongly urge you to do whitelisting as it's good security. If
> you can divide the calls into something like this it fits the Linxu
> desktop model well:
> 
> Safe -> anyone can do it
> Console -> only console owner
> Superuser -> administrator level (things that can be irritating but
> don't persist a reboot)

I've added filtering in my most recent submission (v6).

I don't feel that any single one of these calls or pieces of data should
be accessible without root.  Furthermore discussion on the merits of different
levels is stepping into bikeshedding territory. 
If someone wants to bubble up things that they feel are safe to read or write 
as user, that can be the job of a daemon or other service to decide not the kernel.

> 
> Beyond that it's trickier - if you are an enterprise business then you
> don't for example want to allow someone with root on the system to
> subvert BIOS level settings they can't access.

This level of split is actually comprehended already by the BIOS itself.  When you
enable an administrative password on the BIOS, you aren't able to access (read
or write) a variety of settings without a security key (which requires some handshaking).

> 
> You need a minimum of CAP_SYS_RAWIO for anything that can (through bugs
> or design) subvert secure boot type stuff. It's also probably an
> appropriate check for 'anything goes' if you decide to have that
> category. CAP_SYS_RAWIO implies total power over the machine although
> with secureboot it's less clear.
> 
Secure boot can't programmatically be disabled from within OS.

Patch

diff --git a/Documentation/ABI/testing/dell-smbios-wmi b/Documentation/ABI/testing/dell-smbios-wmi
new file mode 100644
index 000000000000..86ded18b41f7
--- /dev/null
+++ b/Documentation/ABI/testing/dell-smbios-wmi
@@ -0,0 +1,43 @@ 
+What:		/dev/wmi/dell-smbios
+Date:		November 2017
+KernelVersion:	4.15
+Contact:	"Mario Limonciello" <[e-mail]>
+Description:
+		Perform SMBIOS calls on supported Dell machines.
+		through the Dell ACPI-WMI interface.
+
+		IOCTL's and buffer formats are defined in:
+		<uapi/linux/dell-wmi-smbios.h>
+
+		1) To perform a call from userspace, you'll need to first
+		determine the size of the buffer for your machine.
+		Platforms that contain larger buffers can return larger
+		objects from the system firmware.
+		Commonly this size is either 4k or 32k.
+
+		To determine the size of the buffer, refer to:
+		sysfs-platform-dell-smbios-wmi
+
+		2) After you've determined the size of the calling interface
+		buffer, you can allocate a structure that represents the
+		ioctl struct documented above.
+
+		3) In the 'length' object store the size of the buffer you
+		determined above.
+
+		4) Then use this size to allocate an appropriately sized
+		calling interface buffer to assign to 'buf' object.
+
+		5) In this buf object, prepare as necessary for the SMBIOS
+		call you're interested in.  Typically SMBIOS buffers have
+		"class", "select", and "input" defined to values that coincide
+		with the data you are interested in.
+		Documenting class/select/input values is outside of the scope
+		of this documentation. Check with the libsmbios project for
+		further documentation on these values.
+
+		6) Run the call by using ioctl() as described in the header.
+
+		7) The output will be returned in the buf object.
+
+		8) Be sure to free up both of your allocated objects.
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
new file mode 100644
index 000000000000..6a0513703a3c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios-wmi
@@ -0,0 +1,10 @@ 
+What:		/sys/devices/platform/<platform>/buffer_size
+Date:		November 2017
+KernelVersion:	4.15
+Contact:	"Mario Limonciello" <[e-mail]>
+Description:
+		A read-only description of the size of a calling
+		interface buffer that can be passed to Dell
+		firmware.
+
+		Commonly this size is either 4k or 32k.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6db1d84999bc..0dd373cf7280 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3986,6 +3986,7 @@  M:	Mario Limonciello <[e-mail]>
 L:	[e-mail]
 S:	Maintained
 F:	drivers/platform/x86/dell-smbios-wmi.*
+F:	include/uapi/linux/dell-smbios-wmi.h
 
 DELL LAPTOP DRIVER
 M:	Matthew Garrett <[e-mail]>
diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
index 32e4e7dbf575..3156864e65e0 100644
--- a/drivers/platform/x86/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -15,6 +15,7 @@ 
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/wmi.h>
+#include <uapi/linux/dell-smbios-wmi.h>
 #include "dell-smbios-wmi.h"
 #include "dell-wmi-descriptor.h"
 static DEFINE_MUTEX(wmi_mutex);
@@ -107,6 +108,87 @@  void dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_wmi_call);
 
+static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd,
+	unsigned long arg)
+{
+	void __user *p = (void __user *) arg;
+	struct wmi_smbios_ioctl *input;
+	struct wmi_smbios_priv *priv;
+	struct wmi_device *wdev;
+	size_t ioctl_size;
+	int ret = 0;
+
+	switch (cmd) {
+	/* we only operate on first instance */
+	case DELL_WMI_SMBIOS_CMD:
+		wdev = get_first_wmi_device();
+		if (!wdev) {
+			pr_err("No WMI devices bound\n");
+			return -ENODEV;
+		}
+		ioctl_size = sizeof(struct wmi_smbios_ioctl);
+		priv = dev_get_drvdata(&wdev->dev);
+		input = kmalloc(ioctl_size, GFP_KERNEL);
+		if (!input)
+			return -ENOMEM;
+		mutex_lock(&wmi_mutex);
+		if (!access_ok(VERIFY_READ, p, ioctl_size)) {
+			pr_err("Unsafe userspace pointer passed\n");
+			return -EFAULT;
+		}
+		if (copy_from_user(input, p, ioctl_size)) {
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		if (input->length != priv->buffer_size) {
+			pr_err("Got buffer size %d expected %d\n",
+				input->length, priv->buffer_size);
+			ret = -EINVAL;
+			goto fail_smbios_cmd;
+		}
+		if (!access_ok(VERIFY_WRITE, input->buf, priv->buffer_size)) {
+			pr_err("Unsafe userspace pointer passed\n");
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		if (copy_from_user(priv->buf, input->buf, priv->buffer_size)) {
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		ret = run_smbios_call(wdev);
+		if (ret != 0)
+			goto fail_smbios_cmd;
+		if (copy_to_user(input->buf, priv->buf, priv->buffer_size))
+			ret = -EFAULT;
+fail_smbios_cmd:
+		kfree(input);
+		mutex_unlock(&wmi_mutex);
+		break;
+	default:
+		pr_err("unsupported ioctl: %d.\n", cmd);
+		ret = -ENOIOCTLCMD;
+	}
+	return ret;
+}
+
+static ssize_t buffer_size_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct wmi_smbios_priv *priv = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", priv->buffer_size);
+}
+static DEVICE_ATTR_RO(buffer_size);
+
+static struct attribute *smbios_wmi_attrs[] = {
+	&dev_attr_buffer_size.attr,
+	NULL
+};
+
+static const struct attribute_group smbios_wmi_attribute_group = {
+	.attrs = smbios_wmi_attrs,
+};
+
 static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 {
 	struct wmi_smbios_priv *priv;
@@ -127,6 +209,11 @@  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 	if (!priv->buf)
 		return -ENOMEM;
 
+	ret = sysfs_create_group(&wdev->dev.kobj,
+				 &smbios_wmi_attribute_group);
+	if (ret)
+		goto fail_create_group;
+
 	/* ID is used by dell-smbios to set priority of drivers */
 	wdev->dev.id = 1;
 	ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
@@ -140,6 +227,10 @@  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 	return 0;
 
 fail_register:
+	sysfs_remove_group(&wdev->dev.kobj,
+			   &smbios_wmi_attribute_group);
+
+fail_create_group:
 	free_pages((unsigned long)priv->buf, count);
 	return ret;
 }
@@ -151,6 +242,7 @@  static int dell_smbios_wmi_remove(struct wmi_device *wdev)
 
 	list_del(&priv->list);
 	dell_smbios_unregister_device(&wdev->dev);
+	sysfs_remove_group(&wdev->dev.kobj, &smbios_wmi_attribute_group);
 	count = get_order(priv->buffer_size);
 	free_pages((unsigned long)priv->buf, count);
 	return 0;
@@ -161,6 +253,11 @@  static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
 	{ },
 };
 
+static const struct file_operations dell_smbios_wmi_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= dell_smbios_wmi_ioctl,
+};
+
 static void __init parse_b1_table(const struct dmi_header *dm)
 {
 	struct misc_bios_flags_structure *flags =
@@ -189,6 +286,7 @@  static struct wmi_driver dell_smbios_wmi_driver = {
 	.probe = dell_smbios_wmi_probe,
 	.remove = dell_smbios_wmi_remove,
 	.id_table = dell_smbios_wmi_id_table,
+	.file_operations = &dell_smbios_wmi_fops,
 };
 
 static int __init init_dell_smbios_wmi(void)
diff --git a/drivers/platform/x86/dell-smbios-wmi.h b/drivers/platform/x86/dell-smbios-wmi.h
index 5dd91d8ff3d8..40a3666ea87a 100644
--- a/drivers/platform/x86/dell-smbios-wmi.h
+++ b/drivers/platform/x86/dell-smbios-wmi.h
@@ -13,16 +13,6 @@ 
 
 #include "dell-smbios.h"
 
-struct wmi_calling_interface_buffer {
-	u16 class;
-	u16 select;
-	u32 input[4];
-	u32 output[4];
-	u32 argattrib;
-	u32 blength;
-	u8 *data;
-} __packed;
-
 void dell_smbios_wmi_call(struct calling_interface_buffer *buffer);
 
 #endif
diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-smbios-wmi.h
new file mode 100644
index 000000000000..0d0d09b04021
--- /dev/null
+++ b/include/uapi/linux/dell-smbios-wmi.h
@@ -0,0 +1,25 @@ 
+#ifndef _UAPI_DELL_SMBIOS_WMI_H_
+#define _UAPI_DELL_SMBIOS_WMI_H_
+
+#include <linux/ioctl.h>
+#include <linux/wmi.h>
+
+struct wmi_calling_interface_buffer {
+	u16 class;
+	u16 select;
+	u32 input[4];
+	u32 output[4];
+	u32 argattrib;
+	u32 blength;
+	u8 *data;
+} __packed;
+
+struct wmi_smbios_ioctl {
+	u32 length;
+	struct wmi_calling_interface_buffer *buf;
+};
+
+/* only offers on the single instance */
+#define DELL_WMI_SMBIOS_CMD		WMI_IOWR(0)
+
+#endif /* _UAPI_DELL_WMI_SMBIOS_H_ */