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

[Greg KH]

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.

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

> +			return -EFAULT;

Memory leak!


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

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

> +			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 :(


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

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

I need more coffee...

greg k-h