[PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Kai Huang]

xa_destroy() needs to be called to destroy virtual EPC's page arra
before calling kfree() to free the virtual EPC.  Currently it is not
calaled.  Add the missing xa_destroy() to fix.

Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests")
Tested-by: Yang Zhong <yang.zhong@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/cpu/sgx/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 6ad165a5c0cc..64511c4a5200 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 		list_splice_tail(&secs_pages, &zombie_secs_pages);
 	mutex_unlock(&zombie_secs_pages_lock);
 
+	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
 	return 0;
-- 
2.31.1

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Jarkko Sakkinen]

On Tue, Jun 15, 2021 at 10:16:39PM +1200, Kai Huang wrote:
> xa_destroy() needs to be called to destroy virtual EPC's page arra
                                                                    y

> before calling kfree() to free the virtual EPC.  Currently it is not
> calaled.  Add the missing xa_destroy() to fix.
  called

> Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests")
> Tested-by: Yang Zhong <yang.zhong@intel.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/virt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 6ad165a5c0cc..64511c4a5200 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  		list_splice_tail(&secs_pages, &zombie_secs_pages);
>  	mutex_unlock(&zombie_secs_pages_lock);
>  
> +	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
>  	return 0;
> -- 
> 2.31.1
> 
> 

/Jarkko

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Dave Hansen]

On 6/15/21 3:16 AM, Kai Huang wrote:
> xa_destroy() needs to be called to destroy virtual EPC's page arra
> before calling kfree() to free the virtual EPC.  Currently it is not
> calaled.  Add the missing xa_destroy() to fix.

Looks good Kai, thanks for fixing this.

Could you please take a good look through the sgx_release() and the vpec
equivalent and see if anything else stands out as possibly being missed?
 Also, is this the kind of thing that a simple open/add/close selftest
might have found?

Maybe we should beef up the selftests a bit.

Acked-by: Dave Hansen <dave.hansen@intel.com>

[tip: x86/urgent] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[tip-bot2 for Kai Huang]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     4692bc775d2180a937335ccba0edce557103d44a
Gitweb:        https://git.kernel.org/tip/4692bc775d2180a937335ccba0edce557103d44a
Author:        Kai Huang <kai.huang@intel.com>
AuthorDate:    Tue, 15 Jun 2021 22:16:39 +12:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Tue, 15 Jun 2021 18:03:45 +02:00

x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

xa_destroy() needs to be called to destroy a virtual EPC's page array
before calling kfree() to free the virtual EPC. Currently it is not
called so add the missing xa_destroy().

Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests")
Signed-off-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Dave Hansen <dave.hansen@intel.com>
Tested-by: Yang Zhong <yang.zhong@intel.com>
Link: https://lkml.kernel.org/r/20210615101639.291929-1-kai.huang@intel.com
---
 arch/x86/kernel/cpu/sgx/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 6ad165a..64511c4 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 		list_splice_tail(&secs_pages, &zombie_secs_pages);
 	mutex_unlock(&zombie_secs_pages_lock);
 
+	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
 	return 0;

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Kai Huang]

On Tue, 2021-06-15 at 08:39 -0700, Dave Hansen wrote:
> On 6/15/21 3:16 AM, Kai Huang wrote:
> > xa_destroy() needs to be called to destroy virtual EPC's page arra
> > before calling kfree() to free the virtual EPC.  Currently it is not
> > calaled.  Add the missing xa_destroy() to fix.
> 
> Looks good Kai, thanks for fixing this.
> 
> Could you please take a good look through the sgx_release() and the vpec
> equivalent and see if anything else stands out as possibly being missed?

I looked over.  One potential issue is both sgx_encl and sgx_vepc have 'struct mutex lock'
embedded,  but mutex_destroy() is not called when they are released.  However I am not
sure whether this is worth fixing, since mutex_destroy() is empty unless
CONFIG_DEBUG_MUTEXES is turned on (even with it turned on, mutex_destroy() doesn't do
anything like freeing resources so in practice there should have no problem).

Another thing is sgx_encl_release() doesn't explicitly call xa_erase() for each EPC page
in encl->page_array when looping it over to free all EPC pages, but I think it is OK since
xa_destroy() is called later which will destroy all xarray internal data structures.  But
I don't know internal implementation of xarray.

>  Also, is this the kind of thing that a simple open/add/close selftest
> might have found?

It might be useful but I don't think it can detect things like xa_destroy() being missing.

> 
> Maybe we should beef up the selftests a bit.
> 
> Acked-by: Dave Hansen <dave.hansen@intel.com>

Thank you!

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Kai Huang]

On Tue, 2021-06-15 at 16:20 +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 15, 2021 at 10:16:39PM +1200, Kai Huang wrote:
> > xa_destroy() needs to be called to destroy virtual EPC's page arra
>                                                                     y
> 
> > before calling kfree() to free the virtual EPC.  Currently it is not
> > calaled.  Add the missing xa_destroy() to fix.
>   called

Thanks Jarkko. I literally need to find some way to avoid such error in future :)

> 
> > Fixes: 540745ddbc70 ("x86/sgx: Introduce virtual EPC for use by KVM guests")
> > Tested-by: Yang Zhong <yang.zhong@intel.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/virt.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> > index 6ad165a5c0cc..64511c4a5200 100644
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -212,6 +212,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
> >  		list_splice_tail(&secs_pages, &zombie_secs_pages);
> >  	mutex_unlock(&zombie_secs_pages_lock);
> >  
> > +	xa_destroy(&vepc->page_array);
> >  	kfree(vepc);
> >  
> >  	return 0;
> > -- 
> > 2.31.1
> > 
> > 
> 
> /Jarkko

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Borislav Petkov]

On Wed, Jun 16, 2021 at 12:30:04PM +1200, Kai Huang wrote:
> Thanks Jarkko. I literally need to find some way to avoid such error in future :)

That way is called "integrate checkpatch.pl into your patch creation
workflow".

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Kai Huang]

On Thu, 2021-06-17 at 16:34 +0200, Borislav Petkov wrote:
> On Wed, Jun 16, 2021 at 12:30:04PM +1200, Kai Huang wrote:
> > Thanks Jarkko. I literally need to find some way to avoid such error in future :)
> 
> That way is called "integrate checkpatch.pl into your patch creation
> workflow".
> 

Thanks for suggestion. Yes I actually did the checkpatch.pl, but it didn't report typo in
commit message.  A little bit strange.

Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is destroyed

[Borislav Petkov]

On Fri, Jun 18, 2021 at 12:04:55PM +1200, Kai Huang wrote:
> Thanks for suggestion. Yes I actually did the checkpatch.pl, but it
> didn't report typo in commit message. A little bit strange.

Yah, and I know it does catch typos. It seems it does so only sometimes:

$ ./scripts/checkpatch.pl --strict /tmp/kai.01
total: 0 errors, 0 warnings, 0 checks, 7 lines checked

/tmp/kai.01 has no obvious style problems and is ready for submission.

Someday soon we'll have a better way to deal with this.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette