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

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Thursday, October 5, 2017 2:33 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: dvhart@xxxxxxxxxxxxx; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>;
> LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx;
> Andy Lutomirski <luto@xxxxxxxxxx>; quasisec@xxxxxxxxxx;
> pali.rohar@xxxxxxxxx; rjw@xxxxxxxxxxxxx; mjg59@xxxxxxxxxx; hch@xxxxxx
> Subject: Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce
> userspace interface
> 
> On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> > +static long dell_smbios_wmi_ioctl(struct file *filp, unsigned int cmd,
> > +	unsigned long arg)
> > +{
> > +	void __user *p = (void __user *) arg;
> > +	struct wmi_smbios_ioctl *input;
> > +	struct wmi_smbios_priv *priv;
> > +	struct wmi_device *wdev;
> > +	size_t ioctl_size;
> > +	int ret = 0;
> > +
> > +	switch (cmd) {
> > +	/* we only operate on first instance */
> > +	case DELL_WMI_SMBIOS_CMD:
> > +		wdev = get_first_wmi_device();
> > +		if (!wdev) {
> > +			pr_err("No WMI devices bound\n");
> 
> dev_err(), you are a driver, never use "raw" pr_ calls.
>
This particular error was if a device wasn't found.  It's a codepath I don't
really know can happen from the ioctl, but wanted to be sure.  I'll just remove
this error message.
 
> > +			return -ENODEV;
> > +		}
> > +		ioctl_size = sizeof(struct wmi_smbios_ioctl);
> > +		priv = dev_get_drvdata(&wdev->dev);
> > +		input = kmalloc(ioctl_size, GFP_KERNEL);
> > +		if (!input)
> > +			return -ENOMEM;
> > +		mutex_lock(&wmi_mutex);
> > +		if (!access_ok(VERIFY_READ, p, ioctl_size)) {
> 
> Hm, any time I see an access_ok() call, I get scared.  You should almost
> never need to make that call if you are using the correct kernel apis.
> 
> > +			pr_err("Unsafe userspace pointer passed\n");
> 
> dev_err().
>

Yep got it.
 
> > +			return -EFAULT;
> 
> Memory leak!

Doh, thanks.

> 
> 
> > +		}
> > +		if (copy_from_user(input, p, ioctl_size)) {
> > +			ret = -EFAULT;
> 
> So, why did you call access_ok() followed by copy_from_user()?
> copy_from/to() handle all of that for you automatically.
> 

I wasn't aware access_ok was handled in copy_from/to.  I'll remove this.

> > +			goto fail_smbios_cmd;
> > +		}
> > +		if (input->length != priv->buffer_size) {
> > +			pr_err("Got buffer size %d expected %d\n",
> > +				input->length, priv->buffer_size);
> 
> length is user provided, it can be whatever anyone sets it to.  I don't
> understand this error.
> 
> > +			ret = -EINVAL;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		if (!access_ok(VERIFY_WRITE, input->buf, priv->buffer_size)) {
> > +			pr_err("Unsafe userspace pointer passed\n");
> 
> Again, don't need this.
> 
> > +			ret = -EFAULT;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		if (copy_from_user(priv->buf, input->buf, priv->buffer_size)) {
> 
> Wait, input->buf is a user pointer?  Ick, see my previous email about
> your crazy api here being a mess.  This should not be needed.
> 
> And as you "know" the buffer size already, why do you have userspace
> specify it?  What good is it?

I mentioned this in the other mail, but the thing is buffer size is not consistent
from machine to machine.

It's convenient that this change happened now because otherwise there would
have been assumptions about it always being 4k that needed to be thrown away.

> 
> > +			ret = -EFAULT;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		ret = run_smbios_call(wdev);
> 
> No other checking of the values in the structure?  You just "trust"
> userspace to get it all right?  Hah!
> 
> > +		if (ret != 0)
> > +			goto fail_smbios_cmd;
> 
> You didn't run this through checkpatch :(
> 
I did..

$ ~/src/linux/scripts/checkpatch.pl 0013-platform-x86-dell-smbios-wmi-
introduce-userspace-int.patch 
total: 0 errors, 0 warnings, 241 lines checked

0013-platform-x86-dell-smbios-wmi-introduce-userspace-int.patch has no obvious 
style problems and is ready for submission.

> 
> > +		if (copy_to_user(input->buf, priv->buf, priv->buffer_size))
> > +			ret = -EFAULT;
> > +fail_smbios_cmd:
> > +		kfree(input);
> > +		mutex_unlock(&wmi_mutex);
> > +		break;
> > +	default:
> > +		pr_err("unsupported ioctl: %d.\n", cmd);
> > +		ret = -ENOIOCTLCMD;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static ssize_t buffer_size_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct wmi_smbios_priv *priv = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d\n", priv->buffer_size);
> > +}
> > +static DEVICE_ATTR_RO(buffer_size);
> > +
> > +static struct attribute *smbios_wmi_attrs[] = {
> > +	&dev_attr_buffer_size.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group smbios_wmi_attribute_group = {
> > +	.attrs = smbios_wmi_attrs,
> > +};
> > +
> >  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> >  {
> >  	struct wmi_smbios_priv *priv;
> > @@ -127,6 +209,11 @@ static int dell_smbios_wmi_probe(struct wmi_device
> *wdev)
> >  	if (!priv->buf)
> >  		return -ENOMEM;
> >
> > +	ret = sysfs_create_group(&wdev->dev.kobj,
> > +				 &smbios_wmi_attribute_group);
> 
> Hint, if a driver ever makes a call to sysfs_*(), something is wrong, it
> should never be needed.
> 
> Also, you just raced with userspace and lost :(
> 
> There is a way to fix all of this, in a simple way, I'll leave that as
> an exercise for the reader, I've reviewed enough of this code for
> today...
> 

Thanks, I see what I should do instead, I'll make changes.

> > +static const struct file_operations dell_smbios_wmi_fops = {
> > +	.owner		= THIS_MODULE,
> 
> And who uses that field?  Hint, no one is, which is another issue that I
> forgot to review in your previous patch where you use this structure.
> What is protecting this module from being unloaded while the ioctl call
> is running?  (hint, nothing...)
> 

Thanks, I'll add locking.