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
