lkml.org 
[lkml]   [2017]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

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

\
 
 \ /
  Last update: 2017-10-05 09:33    [W:0.237 / U:8.876 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site