| 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
| ||
|
Message-ID: <20171117230705.GE25974@fury>
Date: Fri, 17 Nov 2017 15:07:05 -0800
From: Darren Hart <dvhart@...radead.org>
To: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc: intel-sgx-kernel-dev@...ts.01.org,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 10/11] intel_sgx: glue code for in-kernel LE
On Mon, Nov 13, 2017 at 09:45:27PM +0200, Jarkko Sakkinen wrote:
> Glue code for hosting in-kernel Launch Enclave (LE) by using the user
> space helper framework.
>
> Tokens for launching enclaves are generated with by the following
> protocol:
>
> 1. The driver sends a SIGSTRUCT blob to the LE hosting process
> to the input pipe.
> 2. The LE hosting process reads the SIGSTRUCT blob from the input
> pipe.
> 3. After generating a EINITTOKEN blob, the LE hosting process writes
> it to the output pipe.
> 4. The driver reads the EINITTOKEN blob from the output pipe.
>
> If IA32_SGXLEPUBKEYHASH* MSRs are writable and they don't have the
> public key hash of the LE they will be updated.
>
A few nits throughout to keep in mind:
* #includes in alphabetical order in general
* function local variables declared in order of decreasing line length
* don't insert newlines where coding_style doesn't compel you to
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> -
...--
> diff --git a/drivers/platform/x86/intel_sgx/sgx_le.c b/drivers/platform/x86/intel_sgx/sgx_le.c
> new file mode 100644
> index 000000000000..d49c58f09db6
> --- /dev/null
> +++ b/drivers/platform/x86/intel_sgx/sgx_le.c
> @@ -0,0 +1,313 @@
...
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/kmod.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> +#include <linux/pipe_fs_i.h>
> +#include <linux/sched/signal.h>
> +#include <linux/shmem_fs.h>
> +#include <linux/anon_inodes.h>
alphabetical order
...
> +static int sgx_le_create_pipe(struct sgx_le_ctx *ctx,
> + unsigned int fd)
> +{
> + struct file *files[2];
> + int ret;
> +
> + ret = create_pipe_files(files, 0);
> + if (ret)
> + goto out;
Fairly inconsistent in the use of the goto out: model and returning
inline where there is no cleanup to be done. Whatever you do, please be
consistent within the file.
If there is no cleanup to do, a local return is fine.
> +
> + ctx->pipes[fd] = files[fd ^ 1];
> + ret = replace_fd(fd, files[fd], 0);
> + fput(files[fd]);
> +
> +out:
> + return ret;
> +}
> +
> +static int sgx_le_read(struct file *file, void *data, unsigned int len)
> +{
> + ssize_t ret;
> + loff_t pos = 0;
Decreasing line length.
> + ret = kernel_read(file, data, len, &pos);
> +
> + if (ret != len && ret >= 0)
> + return -ENOMEM;
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;
You save a 4 lines with:
if (ret < 0)
return ret;
return ret == len ? 0 : -ENOMEM;
They're common, but ternary ops aren't the most legible things in the
world, so your call.
> +}
> +
> +static int sgx_le_write(struct file *file, const void *data,
> + unsigned int len)
> +{
> + ssize_t ret;
> + loff_t pos = 0;
Decreasing line length.
...
> +static int sgx_le_task_init(struct subprocess_info *subinfo, struct cred *new)
> +{
> + struct sgx_le_ctx *ctx =
> + (struct sgx_le_ctx *)subinfo->data;
No wrap
> + unsigned long len;
> + struct file *tmp_filp;
> + int ret;
> +
> + len = (unsigned long)&sgx_le_proxy_end - (unsigned long)&sgx_le_proxy;
> +
> + tmp_filp = shmem_file_setup("[sgx_le_proxy]", len, 0);
> + if (IS_ERR(tmp_filp)) {
> + ret = PTR_ERR(tmp_filp);
> + return ret;
> + }
> + ret = replace_fd(SGX_LE_PROXY_FD, tmp_filp, 0);
> + fput(tmp_filp);
> + if (ret < 0)
> + return ret;
> +
> + ret = sgx_le_write(tmp_filp, &sgx_le_proxy, len);
> + if (ret)
> + return ret;
> +
> + tmp_filp = anon_inode_getfile("[/dev/sgx]", &sgx_fops, NULL, O_RDWR);
> + if (IS_ERR(tmp_filp))
> + return PTR_ERR(tmp_filp);
> + ret = replace_fd(SGX_LE_DEV_FD, tmp_filp, 0);
> + fput(tmp_filp);
> + if (ret < 0)
> + return ret;
> +
> + ret = sgx_le_create_pipe(ctx, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = sgx_le_create_pipe(ctx, 1);
> + if (ret < 0)
> + return ret;
> +
No incremental cleanup here - appears to all be handled through
sgx_le_stop - do I have that right?
...
> diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
> index 636a583bad31..7931083651f5 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_main.c
> @@ -88,6 +88,37 @@ u64 sgx_encl_size_max_64;
> u64 sgx_xfrm_mask = 0x3;
> u32 sgx_misc_reserved;
> u32 sgx_xsave_size_tbl[64];
> +bool sgx_unlocked_msrs;
> +u64 sgx_le_pubkeyhash[4];
> +
> +static DECLARE_RWSEM(sgx_file_sem);
> +
> +static int sgx_open(struct inode *inode, struct file *file)
> +{
> + int ret;
> +
> + down_read(&sgx_file_sem);
> +
> + ret = sgx_le_start(&sgx_le_ctx);
> + if (ret) {
> + up_read(&sgx_file_sem);
> + return ret;
> + }
> +
> + return 0;
We can simplify the return logic:
if (ret)
up_read...
return ret;
--
Darren Hart
VMware Open Source Technology Center
Powered by blists - more mailing lists