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

[Greg KH <greg@kroah.com>]

On Thu, Oct 05, 2017 at 04:28:40PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Thursday, October 5, 2017 2:23 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
> > 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