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

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: <84b136e7368a982c80dc344b17eb992ea73a0896.camel@intel.com>
Date:   Wed, 16 Jun 2021 12:28:57 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>, linux-sgx@...r.kernel.org,
        x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, bp@...en8.de, seanjc@...gle.com,
        jarkko@...nel.org, tglx@...utronix.de, mingo@...hat.com,
        Yang Zhong <yang.zhong@...el.com>
Subject: Re: [PATCH] x86/sgx: Add missing xa_destroy() when virtual EPC is
 destroyed

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@...el.com>

Thank you!