RE: [PATCH v5 14/14] platform/x86: dell-smbios-wmi: introduce userspace interface

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Saturday, October 7, 2017 7:37 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: dvhart@xxxxxxxxxxxxx; andy.shevchenko@xxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; luto@xxxxxxxxxx;
> quasisec@xxxxxxxxxx; pali.rohar@xxxxxxxxx; rjw@xxxxxxxxxxxxx;
> mjg59@xxxxxxxxxx; hch@xxxxxx
> 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@xxxxxxxx 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.