linux-kernel - Re: [PATCH v38 10/24] mm: Add vm

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]

Date:   Wed, 23 Sep 2020 16:30:59 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        linux-sgx@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Jethro Beekman <jethro@...tanix.com>,
        Darren Kenny <darren.kenny@...cle.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        asapek@...gle.com, Borislav Petkov <bp@...en8.de>,
        "Xing, Cedric" <cedric.xing@...el.com>, chenalexchen@...gle.com,
        Conrad Parker <conradparker@...gle.com>, cyhanish@...gle.com,
        "Huang, Haitao" <haitao.huang@...el.com>,
        Josh Triplett <josh@...htriplett.org>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Svahn, Kai" <kai.svahn@...el.com>, Keith Moyer <kmoy@...gle.com>,
        Christian Ludloff <ludloff@...gle.com>,
        Neil Horman <nhorman@...hat.com>,
        Nathaniel McCallum <npmccallum@...hat.com>,
        Patrick Uiterwijk <puiterwijk@...hat.com>,
        David Rientjes <rientjes@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>, yaozhangx@...gle.com
Subject: Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()

On Tue, Sep 22, 2020 at 08:11:14AM -0700, Dave Hansen wrote:
> > Enclave permissions can be dynamically modified by using ENCLS[EMODPE]
> 
> I'm not sure this sentence matters.  I'm not sure why I care what the
> instruction is named that does this.  But, it _sounds_ here like an
> enclave can adjust its own permissions directly with ENCLS[EMODPE].

If there was no EMODPE, I would drop this patch from the patch set. It
is the only reason I keep it. There are no other hard reasons to have
this.

> Now I'm confused.  I actually don't think I have a strong understanding
> of how an enclave actually gets loaded, how mmap() and mprotect() are
> normally used and what this hook is intended to thwart.

Enclave gets loaded with the ioctl API so ATM we rely only on
DAC permissions.

In future you might want to have LSM hooks to improve granularity
in two points:

1. When pages are added using SGX_IOC_ENCLAVE ADD_PAGES.
2. When enclave is initialized using SGX_IOC_ENCLAVE_INIT

In both you might want to make a policy decision based on origin
and page permissions.

If we do not limit mmap(), enclave could later on upgrade its
permissions with EMODPE and do mmap().

No matter what kind of LSM hooks or whatever possible improvements
are done later on to access control, they won't work unless we have
this because the permissions could be different than the original.

With this change you can still do EMODPE inside an enclave, but you
don't gain anything with it because your max permissions are capped
during the build time.

I'm not sure what exactly from this is missing from the commit message
but if you get this explanation maybe you can help me out with that.

Thank you for the feedback.

/Jarkko