On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> +{
> + return nonseekable_open(inode, file);
> +}
> +
> +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> +{
> + return 0;
> +}
Why even declare an open/release if you don't do anything with them?
Just leave them empty.
> +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + void __user *p = (void __user *) arg;
> + size_t size;
> + int ret = 0;
> +
> + if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> + return -ENOTTY;
> +
> + switch (cmd) {
> + case DELL_WMI_SMBIOS_CALL_CMD:
> + size = sizeof(struct wmi_calling_interface_buffer);
> + mutex_lock(&buffer_mutex);
> + if (copy_from_user(devfs_buffer, p, size)) {
> + ret = -EFAULT;
> + goto fail_smbios_cmd;
> + }
> + ret = run_wmi_smbios_call(devfs_buffer);
You _are_ checking that your structures are valid here, right? I didn't
see you really were, so I'll let you go audit your code paths for the
next set of patches.
But really, ugh, this seems horrible. It's a huge vague data blob going
both ways, this feels ripe for abuse and other bad things. Do you have
a working userspace implementation for all of this to publish at the
same time?
thanks,
greg k-h
