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: <8fba3d1db7914c4ca34750b7ff4cf9dc@ausx13mpc120.AMER.DELL.COM>
Date:   Tue, 3 Oct 2017 15:09:24 +0000
From:   <Mario.Limonciello@...l.com>
To:     <greg@...ah.com>
CC:     <dvhart@...radead.org>, <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: Greg KH [mailto:greg@...ah.com]
> Sent: Tuesday, October 3, 2017 4:26 AM
> To: Limonciello, Mario <Mario_Limonciello@...l.com>
> Cc: dvhart@...radead.org; 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 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.

Thanks, I'll modify that.

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

>From the Linux side there isn't much that can be done to verify the 
structure.  It doesn't contain a checksum, valid data that looks like a select
of 0 and class of 0 with a few input values looks the same as a few words
of the same length.

The validity of the structure is done by the system firmware.  If it's
invalid, appropriate return codes are set and consumers of it will be 
able to read that.

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

Yes, I've got some support into fwupd and fwupdate that use this interface
already.  I expect the fwupdate PR will be ready to go at this same time,
but I'm working through some issues with fwupd still.