linux-kernel - RE: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5cbdbc85d3e14880a187dfb8f76b1745@ausx13mpc120.AMER.DELL.COM>
Date:   Tue, 3 Oct 2017 15:49:04 +0000
From:   <Mario.Limonciello@...l.com>
To:     <dvhart@...radead.org>, <greg@...ah.com>
CC:     <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: 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.

New interfaces will have description that is parsable by MOF.