[intel-sgx-kernel-dev] intel_sgx: del ctx from list if refcnt==0 when adding encl
diff mbox

Message ID 1490793956-555-1-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson March 29, 2017, 1:25 p.m. UTC
If a ctx whose refcount has gone to zero is encountered when adding
an encl to the appropriate ctx, then delete the ctx from the global
list prior to adding a new ctx.  It is possible for multiple encls
to call sgx_add_to_tgid_ctx while sgx_tgid_ctx_release is waiting to
acquire sgx_tgid_ctx_mutex.  If the existing ctx is not deleted from
the list, subsequent calls to sgx_add_to_tgid_ctx will see the dying
ctx instead of the new ctx and will add their own redundant instance
of ctx.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx_ioctl.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Sean Christopherson March 29, 2017, 8:20 p.m. UTC | #1
Christopherson, Sean J <sean.j.christopherson@intel.com> wrote:
> If a ctx whose refcount has gone to zero is encountered when adding
> an encl to the appropriate ctx, then delete the ctx from the global
> list prior to adding a new ctx.  It is possible for multiple encls
> to call sgx_add_to_tgid_ctx while sgx_tgid_ctx_release is waiting to
> acquire sgx_tgid_ctx_mutex.  If the existing ctx is not deleted from
> the list, subsequent calls to sgx_add_to_tgid_ctx will see the dying
> ctx instead of the new ctx and will add their own redundant instance
> of ctx.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Just realized the new context is added to the head and not the tail, so
deleting the dying context is not absolutely necessary (and the commit message
is not accurate).   Deleting the old context may be good for robustness, though.

> ---
>  drivers/platform/x86/intel_sgx_ioctl.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c
> b/drivers/platform/x86/intel_sgx_ioctl.c index e0e2f14..9ead1b9 100644
> --- a/drivers/platform/x86/intel_sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> @@ -99,11 +99,15 @@ static int sgx_add_to_tgid_ctx(struct sgx_encl *encl)
>  	mutex_lock(&sgx_tgid_ctx_mutex);
>  
>  	ctx = sgx_find_tgid_ctx(tgid);
> -	if (ctx && kref_get_unless_zero(&ctx->refcount)) {
> -		encl->tgid_ctx = ctx;
> -		mutex_unlock(&sgx_tgid_ctx_mutex);
> -		put_pid(tgid);
> -		return 0;
> +	if (ctx) {
> +		if (kref_get_unless_zero(&ctx->refcount)) {
> +			encl->tgid_ctx = ctx;
> +			mutex_unlock(&sgx_tgid_ctx_mutex);
> +			put_pid(tgid);
> +			return 0;
> +		}
> +		else
> +			list_del_init(&ctx->list);
>  	}
>  
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> -- 
> 2.7.4
>
Jarkko Sakkinen April 4, 2017, 6:03 p.m. UTC | #2
On Wed, Mar 29, 2017 at 06:25:56AM -0700, Sean Christopherson wrote:
> If a ctx whose refcount has gone to zero is encountered when adding
> an encl to the appropriate ctx, then delete the ctx from the global
> list prior to adding a new ctx.  It is possible for multiple encls
> to call sgx_add_to_tgid_ctx while sgx_tgid_ctx_release is waiting to
> acquire sgx_tgid_ctx_mutex.  If the existing ctx is not deleted from
> the list, subsequent calls to sgx_add_to_tgid_ctx will see the dying
> ctx instead of the new ctx and will add their own redundant instance
> of ctx.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Thanks, good catch.

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx_ioctl.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
> index e0e2f14..9ead1b9 100644
> --- a/drivers/platform/x86/intel_sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> @@ -99,11 +99,15 @@ static int sgx_add_to_tgid_ctx(struct sgx_encl *encl)
>  	mutex_lock(&sgx_tgid_ctx_mutex);
>  
>  	ctx = sgx_find_tgid_ctx(tgid);
> -	if (ctx && kref_get_unless_zero(&ctx->refcount)) {
> -		encl->tgid_ctx = ctx;
> -		mutex_unlock(&sgx_tgid_ctx_mutex);
> -		put_pid(tgid);
> -		return 0;
> +	if (ctx) {
> +		if (kref_get_unless_zero(&ctx->refcount)) {
> +			encl->tgid_ctx = ctx;
> +			mutex_unlock(&sgx_tgid_ctx_mutex);
> +			put_pid(tgid);
> +			return 0;
> +		}
> +		else
> +			list_del_init(&ctx->list);
>  	}
>  
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
index e0e2f14..9ead1b9 100644
--- a/drivers/platform/x86/intel_sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx_ioctl.c
@@ -99,11 +99,15 @@  static int sgx_add_to_tgid_ctx(struct sgx_encl *encl)
 	mutex_lock(&sgx_tgid_ctx_mutex);
 
 	ctx = sgx_find_tgid_ctx(tgid);
-	if (ctx && kref_get_unless_zero(&ctx->refcount)) {
-		encl->tgid_ctx = ctx;
-		mutex_unlock(&sgx_tgid_ctx_mutex);
-		put_pid(tgid);
-		return 0;
+	if (ctx) {
+		if (kref_get_unless_zero(&ctx->refcount)) {
+			encl->tgid_ctx = ctx;
+			mutex_unlock(&sgx_tgid_ctx_mutex);
+			put_pid(tgid);
+			return 0;
+		}
+		else
+			list_del_init(&ctx->list);
 	}
 
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);