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  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]
Date:   Sat, 7 Oct 2017 13:13:55 +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>, <rjw@...ysocki.net>,
        <mjg59@...gle.com>, <hch@....de>
Subject: RE: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce
 userspace interface

> -----Original Message-----
> From: Greg KH [mailto:greg@...ah.com]
> Sent: Saturday, October 7, 2017 7:37 AM
> To: Limonciello, Mario <Mario_Limonciello@...l.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; rjw@...ysocki.net;
> mjg59@...gle.com; hch@....de
> Subject: Re: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> On Sat, Oct 07, 2017 at 12:15:18PM +0000, Mario.Limonciello@...l.com wrote:
> > > > +	struct wmi_smbios_priv *priv;
> > > > +	int ret = 0;
> > > > +	size_t size;
> > > > +
> > > > +	switch (cmd) {
> > > > +	case DELL_WMI_SMBIOS_CMD:
> > > > +		priv = dev_get_drvdata(&wdev->dev);
> > > > +		if (!priv)
> > > > +			return -ENODEV;
> > > > +		size = sizeof(struct wmi_smbios_buffer);
> > > > +		mutex_lock(&call_mutex);
> > > > +		if (copy_from_user(priv->buf, input, size)) {
> 
> Wait, how do you know that input is size big?

How can you check this?  I guess just read the first u64 for the data first (where length
is stored).  Or is there a better way?

> 
> > > > +			dev_dbg(&wdev->dev, "Copy %lu from user failed\n",
> > > > +				size);
> > > > +			ret = -EFAULT;
> > > > +			goto fail_smbios_cmd;
> > > > +		}
> > > > +		if (priv->buf->length < priv->buffer_size) {
> > > > +			dev_err(&wdev->dev,
> > > > +				"Buffer %lld too small, need at least %d\n",
> > > > +				priv->buf->length, priv->buffer_size);
> > > > +			ret = -EINVAL;
> > > > +			goto fail_smbios_cmd;
> > > > +		}
> > >
> > > No checking for too big of a length?  Any other fields you should check
> > > for validity?  Like too small?
> >
> > Too big is actually intentionally ignored.
> 
> That seems "odd"...
> 
> > I split the copy into two segments to check for this.
> > 1. First copy the size of the structure
> > (if userspace didn't allocate at least sizeof(struct wmi_smbios_buffer) that's a
> problem)
> > 2. Verify the size claimed is "at least" what we internally are looking for.
> > 3. Copy the rest of the size internally needed.  If userspace sent more it's just
> not copied.
> > 4. When sending it back I only send back up to the "at least" internal size.
> 
> That feels strange, are you sure this is correct?  Why the odd two step
> process here?
> 
> What if 'length' is set to an invalid value (too big or small), will you
> catch that correctly here?

Too small is definitely caught, too big won't cause problems due to above logic.

The remaining problem is if length doesn't really represent how much memory
Userspace allocated.  I'm not sure how you can actually check that.

> 
> > > > +		if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
> > > > +			dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
> > > > +				priv->buf->std.class, priv->buf->std.select,
> > > > +				priv->buf->std.input[0]);
> > > > +			ret = -EFAULT;
> > > > +			goto fail_smbios_cmd;
> > > > +		}
> > > > +		size = priv->buffer_size - sizeof(struct wmi_smbios_buffer);
> > >
> > > What if size just went too small and wrapped around?  :(
> > >
> > > Remember, "All input is evil".  Go print that out and put it on the wall
> > > when you are designing this user/kernel api.  You can trust no one, you
> > > have to validate _everything_.
> >
> > priv->buffer_size can't be set by userspace.
> 
> Who sets it?  Your structure naming here doesn't make it obvious which
> data is from the kernel and which from userspace, making this very hard
> to audit :(

It's set during the probe routine.  I'll rename it to something more obvious.
I'll also add some comments to the ioctl so it's more apparent.

Powered by blists - more mailing lists