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]

Message-ID: <fa3a72ef-ba0a-ada9-48bf-bd7cef0a8174@amazon.com>
Date:   Mon, 25 May 2020 23:49:50 +0300
From:   "Paraschiv, Andra-Irina" <andraprs@...zon.com>
To:     Greg KH <gregkh@...uxfoundation.org>
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 22/05/2020 10:07, Greg KH wrote:
> 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?

This is a CPU pool that can be set by the root user and that includes 
CPUs set aside to be used for the enclave(s) setup; these CPUs are 
offlined. From this CPU pool, the kernel logic chooses the CPUs that are 
set for the created enclave(s).

The cpu-list format is matching the same that is documented here:

https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html

I've also thought of having a sysfs entry for the setup of this enclave 
CPU pool.

>
>> +/* 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.

I removed this and other file ops occurrences that do nothing for now.

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

This logic is completed in the next patch in the series.

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

Done, I removed it for now.

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

True, this is not needed. Removed the memset() call.

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

Updated to pr_err().

Thank you.

Andra



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.