linux-kernel - Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Oct 2017 11:26:11 +0200
From:   Greg KH <greg@...ah.com>
To:     Mario Limonciello <mario.limonciello@...l.com>
Cc:     dvhart@...radead.org, Andy Shevchenko <andy.shevchenko@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        platform-driver-x86@...r.kernel.org,
        Andy Lutomirski <luto@...nel.org>, quasisec@...gle.com,
        pali.rohar@...il.com
Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce
 character device for userspace

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