Date:   Thu, 5 Oct 2017 15:10:01 +0000
From:   <Mario.Limonciello@...l.com>
To:     <dvhart@...radead.org>
CC:     <greg@...ah.com>, <andy.shevchenko@...il.com>,
        <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>, <luto@...nel.org>,
        <quasisec@...gle.com>, <pali.rohar@...il.com>
Subject: RE: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
 device for userspace

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@...radead.org]
> Sent: Wednesday, October 4, 2017 7:03 PM
> To: Limonciello, Mario <Mario_Limonciello@...l.com>
> Cc: greg@...ah.com; andy.shevchenko@...il.com; linux-
> kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org; luto@...nel.org;
> quasisec@...gle.com; pali.rohar@...il.com
> Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Tue, Oct 03, 2017 at 03:49:04PM +0000, Mario.Limonciello@...l.com wrote:
> > > -----Original Message-----
> > > From: Darren Hart [mailto:dvhart@...radead.org]
> > > Sent: Tuesday, October 3, 2017 10:20 AM
> > > To: Greg KH <greg@...ah.com>
> > > Cc: Limonciello, Mario <Mario_Limonciello@...l.com>; Andy Shevchenko
> > > <andy.shevchenko@...il.com>; LKML <linux-kernel@...r.kernel.org>;
> > > platform-driver-x86@...r.kernel.org; Andy Lutomirski <luto@...nel.org>;
> > > quasisec@...gle.com; pali.rohar@...il.com
> > > Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce
> character
> > > device for userspace
> > >
> > > On Tue, Oct 03, 2017 at 11:26:11AM +0200, Greg KH wrote:
> > > > On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > > > > +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> > > > > +{
> > > > > +	return nonseekable_open(inode, file);
> > > > > +}
> > > > > +
> > > > > +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> > > > > +{
> > > > > +	return 0;
> > > > > +}
> > > >
> > > > Why even declare an open/release if you don't do anything with them?
> > > > Just leave them empty.
> > > >
> > > > > +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> > > > > +	unsigned long arg)
> > > > > +{
> > > > > +	void __user *p = (void __user *) arg;
> > > > > +	size_t size;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> > > > > +		return -ENOTTY;
> > > > > +
> > > > > +	switch (cmd) {
> > > > > +	case DELL_WMI_SMBIOS_CALL_CMD:
> > > > > +		size = sizeof(struct wmi_calling_interface_buffer);
> > > > > +		mutex_lock(&buffer_mutex);
> > > > > +		if (copy_from_user(devfs_buffer, p, size)) {
> > > > > +			ret = -EFAULT;
> > > > > +			goto fail_smbios_cmd;
> > > > > +		}
> > > > > +		ret = run_wmi_smbios_call(devfs_buffer);
> > > >
> > > > You _are_ checking that your structures are valid here, right?  I didn't
> > > > see you really were, so I'll let you go audit your code paths for the
> > > > next set of patches.
> > > >
> > > > But really, ugh, this seems horrible.  It's a huge vague data blob going
> > > > both ways, this feels ripe for abuse and other bad things.  Do you have
> > > > a working userspace implementation for all of this to publish at the
> > > > same time?
> > > >
> > >
> > > This is the general challenge with WMI support, as it was designed to
> > > provide userspace with a way to talk to the firmware.
> > >
> > > For the more general case going forward the intent is to perform the MOF
> > > parsing in the kernel, which will make it possible to do some automated
> > > buffer validation.
> > >
> > > This particular driver is an intermediate step improving on the
> > > mechanism used for existing devices (libsmbios -> dcdbas -> SMM) to use
> > > the safer WMI entry point (using an op region instead of passing a
> > > physical memory pointer to SMM).
> > >
> > > But, that said... Mario, in lieu of the MOF description of the buffer,
> > > we should be able to do some driver specific validation of this buffer
> > > here.
> > >
> >
> > As the WMI interface exists today even the MOF doesn't describe the
> > Content of the buffer either.  It's a buffer passed from userspace to BIOS and
> > back.  I described it as the struct, but I'm not really sure how that can
> > be further validated by the Linux kernel without a checksum.
> > This is no different than how dell-smbios and dcdbas operates.
> 
> The notation about equivalence with the dell-smbios and dcdbas
> implementation is a valid one. If all we do is move this to a WMI
> managed access to SMBIOS and move away from the SMI based one, this is
> still an improvement for difference in how memory is passed around as
> described above.
> 
> I don't think a checksum will help with the concerns Greg is raising. A
> userspace generated checksum only tells the kernel that what is in the
> buffer is what userpsace intended. Similarly for the buffer coming back
> from SMM.
> 
> Generally speaking, the kernel would want to validate data passing to
> and from userspace and firmware - which WMI was designed to circumvent.
> 
> Mario:
> Are there no specific bits that can be sanity checked? No reserved bits
> in the buffer format? No Command Codes with a known range of values?
> 
Looking through the spec, I think I should be able to let the kernel check if 
the particular class is supported on the system when the call is made.  
Assuming that works I'll add something in v5.

This might also be useful for something like dell-laptop to check if the classes
its using are supported (and soften the blow up when new WMI interfaces
are introduced).

> > New interfaces will have description that is parsable by MOF.
> 
> And to elaborate here. The above is about an improvement to the
> dell-smbios driver for existing platforms which do not have available
> MOF data. For platforms that do provide MOF data, we're working toward
> moving the MOF parser into the kernel (rather than leaving it in
> userspace as it was designed to be) which will afford the kernel more
> oversight of these messages.
> 
> This series is an incremental improvement until the above is ready - but
> it does lay out some of the ground work which we want to get agreement
> on, mostly from patch 4 of 8 in the series:
> 
> 1) The char dev IOCTL model for calling WMI methods
> 
> 2) The /dev/wmi/<driver> path for the char dev
> 
> 3) The function ops approach for WMI drivers to request the char dev
>    from the WMI bus
> 
> --
> Darren Hart
> VMware Open Source Technology Center

Powered by blists - more mailing lists