linux-kernel - Re: [PATCH v2 07/18] nitro_enclaves: Init misc device providing the ioctl interface

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:   Fri, 22 May 2020 09:07:08 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Andra Paraschiv <andraprs@...zon.com>
Cc:     linux-kernel@...r.kernel.org,
        Anthony Liguori <aliguori@...zon.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Colm MacCarthaigh <colmmacc@...zon.com>,
        Bjoern Doebel <doebel@...zon.de>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Frank van der Linden <fllinden@...zon.com>,
        Alexander Graf <graf@...zon.de>,
        Martin Pohlack <mpohlack@...zon.de>,
        Matt Wilson <msw@...zon.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Balbir Singh <sblbir@...zon.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stewart Smith <trawets@...zon.com>,
        Uwe Dannowski <uwed@...zon.de>, kvm@...r.kernel.org,
        ne-devel-upstream@...zon.com
Subject: Re: [PATCH v2 07/18] nitro_enclaves: Init misc device providing the
 ioctl interface

On Fri, May 22, 2020 at 09:29:35AM +0300, Andra Paraschiv wrote:
> +static char *ne_cpus;
> +module_param(ne_cpus, charp, 0644);
> +MODULE_PARM_DESC(ne_cpus, "<cpu-list> - CPU pool used for Nitro Enclaves");

This is not the 1990's, don't use module parameters if you can help it.
Why is this needed, and where is it documented?

> +/* CPU pool used for Nitro Enclaves. */
> +struct ne_cpu_pool {
> +	/* Available CPUs in the pool. */
> +	cpumask_var_t avail;
> +	struct mutex mutex;
> +};
> +
> +static struct ne_cpu_pool ne_cpu_pool;
> +
> +static int ne_open(struct inode *node, struct file *file)
> +{
> +	return 0;
> +}

If open does nothing, just don't even provide it.

> +
> +static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}

Same for ioctl.

> +
> +static int ne_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}

Same for release.

> +
> +static const struct file_operations ne_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= noop_llseek,
> +	.unlocked_ioctl	= ne_ioctl,
> +	.open		= ne_open,
> +	.release	= ne_release,
> +};
> +
> +struct miscdevice ne_miscdevice = {
> +	.minor	= MISC_DYNAMIC_MINOR,
> +	.name	= NE_DEV_NAME,
> +	.fops	= &ne_fops,
> +	.mode	= 0660,
> +};
> +
> +static int __init ne_init(void)
> +{
> +	unsigned int cpu = 0;
> +	unsigned int cpu_sibling = 0;
> +	int rc = -EINVAL;
> +
> +	memset(&ne_cpu_pool, 0, sizeof(ne_cpu_pool));

Why did you just set a structure to 0 that was already initialized by
the system to 0?  Are you sure about this?

> +
> +	if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	mutex_init(&ne_cpu_pool.mutex);
> +
> +	rc = cpulist_parse(ne_cpus, ne_cpu_pool.avail);
> +	if (rc < 0) {
> +		pr_err_ratelimited(NE "Error in cpulist parse [rc=%d]\n", rc);

Again, drop all ratelimited stuff please.

thanks,

greg k-h