linux-kernel - Re: [PATCH v3 1/7] x86/sgx: Provide indication of life-cycle of EPC pages

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YQHpGq0GyAsYpE+D@google.com>
Date:   Wed, 28 Jul 2021 23:32:42 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     "Luck, Tony" <tony.luck@...el.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/7] x86/sgx: Provide indication of life-cycle of EPC
 pages

On Wed, Jul 28, 2021, Dave Hansen wrote:
> On 7/28/21 3:57 PM, Luck, Tony wrote:
> >> Wouldn't it be safer to do something like:
> >>
> >> 	page->owner = owner ? owner : (void *)-1;
> >>
> >> -1 is non-NULL, but also invalid, which makes it harder for us to poke
> >> ourselves in the eye.
> > Does Linux have some #define INVALID_POINTER thing that
> > provides a guaranteed bad (e.g. non-canonical) value?
> > 
> > (void *)-1 seems hacky.
> 
> ERR_PTR(-SOMETHING) wouldn't be too bad.  I guess it could even be:
> 
> 	page->owner = ERR_PTR(SGX_EPC_PAGE_VA);
> 
> and then:
> 
> #define SGX_EPC_PAGE_VA 0xffff...something...greppable
> 
> I *thought* we had a file full of these magic values, but maybe I'm
> misremembering the uapi magic header.

Rather than use a magic const, just pass in the actual va_page.  The only reason
NULL is passed is that prior to virtual EPC, there were only enclave pages and
VA pages, and assiging a non-NULL pointer to sgx_epc_page.owner, which is a
struct sgx_encl_page, was gross.  Virtual EPC sets owner somewhat prematurely;
it's needed iff an EPC cgroup is added, to support OOM EPC killing (and a pointer
to va_page is also needed in this case).

sgx_epc_page.owner can even be converted to 'void *' without additional changes
since all consumers capture it in a local sgx_encl_page variable.


diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 001808e3901c..f9da8fe4dd6b 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -674,12 +674,12 @@ int sgx_encl_test_and_clear_young(struct mm_struct *mm,
  *   a VA page,
  *   -errno otherwise
  */
-struct sgx_epc_page *sgx_alloc_va_page(void)
+struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page)
 {
        struct sgx_epc_page *epc_page;
        int ret;

-       epc_page = sgx_alloc_epc_page(NULL, true);
+       epc_page = sgx_alloc_epc_page(va_page, true);
        if (IS_ERR(epc_page))
                return ERR_CAST(epc_page);

diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index fec43ca65065..3d12dbeae14a 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -111,7 +111,7 @@ void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
                                  struct sgx_encl_page *page);

-struct sgx_epc_page *sgx_alloc_va_page(void);
+struct sgx_epc_page *sgx_alloc_va_page(struct sgx_va_page *va_page);
 unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page);
 void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
 bool sgx_va_page_full(struct sgx_va_page *va_page);
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 83df20e3e633..655ce0bb069d 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -30,7 +30,7 @@ static struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
                if (!va_page)
                        return ERR_PTR(-ENOMEM);

-               va_page->epc_page = sgx_alloc_va_page();
+               va_page->epc_page = sgx_alloc_va_page(va_page);
                if (IS_ERR(va_page->epc_page)) {
                        err = ERR_CAST(va_page->epc_page);
                        kfree(va_page);
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 4628acec0009..4e1a410b8a62 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -29,7 +29,7 @@
 struct sgx_epc_page {
        unsigned int section;
        unsigned int flags;
-       struct sgx_encl_page *owner;
+       void *owner;
        struct list_head list;
 };