On Thu, 2017-10-12 at 12:44 +0300, Jarkko Sakkinen wrote:
> Added sgx_file_sem to keep track of the device file. The LE proxy is
> started when it is first opened and closed after no thread is using it
> anymore.
>
> By doing this LE proxy does not need to be started and stopped for every
> token generated. This will also make sure that the ioctl API is
> accessible if and only if the kernel is able to launch enclaves.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
> This an update to the RFC v3 patch set. It will be included to v4.
This change causes deadlocks due to a semaphore underrun. The LE task calls
sgx_release but not sgx_open because sgx_le_task_init creates a new file via
anon_inode_getfile and doesn't explicitly call fop->open. Based on other usage
of anon_inode_getfile, it looks like fop->open is not expected to be called.
> drivers/platform/x86/intel_sgx/sgx.h | 2 +
> drivers/platform/x86/intel_sgx/sgx_le.c | 66 ++++++++++++++++++++--------
> ---
> drivers/platform/x86/intel_sgx/sgx_main.c | 31 +++++++++++++++
> 3 files changed, 76 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h
> b/drivers/platform/x86/intel_sgx/sgx.h
> index cf66bda37c1f..69b61c63b53d 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -255,6 +255,8 @@ extern struct sgx_le_ctx sgx_le_ctx;
>
> int sgx_le_init(struct sgx_le_ctx *ctx);
> void sgx_le_exit(struct sgx_le_ctx *ctx);
> +void sgx_le_stop(struct sgx_le_ctx *ctx);
> +int sgx_le_start(struct sgx_le_ctx *ctx);
>
> int sgx_le_get_token(struct sgx_le_ctx *ctx,
> const struct sgx_encl *encl,
> diff --git a/drivers/platform/x86/intel_sgx/sgx_le.c
> b/drivers/platform/x86/intel_sgx/sgx_le.c
> index c4ed8e1ea70b..7c78dc6bf512 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_le.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_le.c
> @@ -178,7 +178,7 @@ static int sgx_le_task_init(struct subprocess_info
> *subinfo, struct cred *new)
> return 0;
> }
>
> -static void sgx_le_stop(struct sgx_le_ctx *ctx)
> +static void __sgx_le_stop(struct sgx_le_ctx *ctx)
> {
> int i;
>
> @@ -198,7 +198,15 @@ static void sgx_le_stop(struct sgx_le_ctx *ctx)
> }
> }
>
> -static int sgx_le_start(struct sgx_le_ctx *ctx)
> +
> +void sgx_le_stop(struct sgx_le_ctx *ctx)
> +{
> + mutex_lock(&ctx->lock);
> + __sgx_le_stop(ctx);
> + mutex_unlock(&ctx->lock);
> +}
> +
> +static int __sgx_le_start(struct sgx_le_ctx *ctx)
> {
> struct subprocess_info *subinfo;
> int ret;
> @@ -217,13 +225,24 @@ static int sgx_le_start(struct sgx_le_ctx *ctx)
>
> ret = call_usermodehelper_exec(subinfo, UMH_WAIT_EXEC);
> if (ret) {
> - sgx_le_stop(ctx);
> + __sgx_le_stop(ctx);
> return ret;
> }
>
> return 0;
> }
>
> +int sgx_le_start(struct sgx_le_ctx *ctx)
> +{
> + int ret;
> +
> + mutex_lock(&ctx->lock);
> + ret = __sgx_le_start(ctx);
> + mutex_unlock(&ctx->lock);
> +
> + return ret;
> +}
> +
> int sgx_le_init(struct sgx_le_ctx *ctx)
> {
> struct crypto_shash *tfm;
> @@ -241,50 +260,51 @@ int sgx_le_init(struct sgx_le_ctx *ctx)
> void sgx_le_exit(struct sgx_le_ctx *ctx)
> {
> mutex_lock(&ctx->lock);
> - sgx_le_stop(ctx);
> crypto_free_shash(ctx->tfm);
> mutex_unlock(&ctx->lock);
> }
>
> -int sgx_le_get_token(struct sgx_le_ctx *ctx,
> - const struct sgx_encl *encl,
> - const struct sgx_sigstruct *sigstruct,
> - struct sgx_einittoken *token)
> +static int __sgx_le_get_token(struct sgx_le_ctx *ctx,
> + const struct sgx_encl *encl,
> + const struct sgx_sigstruct *sigstruct,
> + struct sgx_einittoken *token)
> {
> u8 mrsigner[32];
> ssize_t ret;
>
> - mutex_lock(&ctx->lock);
> -
> ret = sgx_get_key_hash(ctx->tfm, sigstruct->modulus, mrsigner);
> if (ret)
> - goto out_unlock;
> -
> - ret = sgx_le_start(ctx);
> - if (ret)
> - goto out_unlock;
> + return ret;
>
> ret = sgx_le_write(ctx->pipes[0], sigstruct->body.mrenclave, 32);
> if (ret)
> - goto out_stop;
> + return ret;
>
> ret = sgx_le_write(ctx->pipes[0], mrsigner, 32);
> if (ret)
> - goto out_stop;
> + return ret;
>
> ret = sgx_le_write(ctx->pipes[0], &encl->attributes,
> sizeof(uint64_t));
> if (ret)
> - goto out_stop;
> + return ret;
>
> ret = sgx_le_write(ctx->pipes[0], &encl->xfrm, sizeof(uint64_t));
> if (ret)
> - goto out_stop;
> + return ret;
>
> - ret = sgx_le_read(ctx->pipes[1], token, sizeof(*token));
> + return sgx_le_read(ctx->pipes[1], token, sizeof(*token));
> +}
>
> -out_stop:
> - sgx_le_stop(ctx);
> -out_unlock:
> +int sgx_le_get_token(struct sgx_le_ctx *ctx,
> + const struct sgx_encl *encl,
> + const struct sgx_sigstruct *sigstruct,
> + struct sgx_einittoken *token)
> +{
> + int ret;
> +
> + mutex_lock(&ctx->lock);
> + ret = __sgx_le_get_token(ctx, encl, sigstruct, token);
> mutex_unlock(&ctx->lock);
> +
> return ret;
> }
> diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c
> b/drivers/platform/x86/intel_sgx/sgx_main.c
> index 8c5fbe9ee870..c15a063bfc7e 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_main.c
> @@ -93,6 +93,35 @@ u32 sgx_xsave_size_tbl[64];
> bool sgx_locked_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;
> +}
> +
> +static int sgx_release(struct inode *inode, struct file *file)
> +{
> + up_read(&sgx_file_sem);
> +
> + if (down_write_trylock(&sgx_file_sem)) {
> + sgx_le_stop(&sgx_le_ctx);
> + up_write(&sgx_file_sem);
> + }
> +
> + return 0;
> +}
> +
> #ifdef CONFIG_COMPAT
> long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long
> arg)
> {
> @@ -147,6 +176,8 @@ static unsigned long sgx_get_unmapped_area(struct file
> *file,
>
> const struct file_operations sgx_fops = {
> .owner = THIS_MODULE,
> + .open = sgx_open,
> + .release = sgx_release,
> .unlocked_ioctl = sgx_ioctl,
> #ifdef CONFIG_COMPAT
> .compat_ioctl = sgx_compat_ioctl,
