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]

Message-ID: <20200925000052.GA20333@linux.intel.com>
Date:   Thu, 24 Sep 2020 17:00:52 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Haitao Huang <haitao.huang@...ux.intel.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.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 Thu, Sep 24, 2020 at 04:09:25PM -0700, Dave Hansen wrote:
> On 9/24/20 4:05 PM, Sean Christopherson wrote:
> > The problem is that enforcing permissions via mprotect() needs to be done
> > unconditionally, otherwise we end up with weird behavior where the existence
> > of an LSM will change what is/isn't allowed, even if the LSM(s) has no SGX
> > policy whatsover.
> 
> Could we make this a bit less abstract, please?
> 
> Could someone point to code or another examples that demonstrates how
> the mere existence of an LSM will change what is/isn't allowed?
> 
> I can't seem to wrap my head around it as-is.

Without pre-declared permissions, loading and running an enclave would be:

	ptr = malloc(size);
        memcpy(ptr, evil_shenanigans, size);
	ioctl(sgx_fd, ENCLAVE_ADD_PAGE, ptr, size);
        ...
	ioctl(sgx_fd, ENCLAVE_INIT);

        enclave = mmap(sgx_fd, ... PROT_READ);
        mprotect(enclave, ..., PROT_READ | PROT_EXEC);

	EENTER;

With the existing security_file_mprotect(), an LSM will see:

        vma->vm_file ~= /dev/sgx/enclave
        prot = PROT_READ | PROT_EXEC

>From a policy perspective, the LSM can't do much because every enclave is
backed by /dev/sgx/enclave, and all enclaves need READ and EXEC perms, i.e.
a policy can only deny all enclaves or allow enclaves.

The solution we came up with is to require userspace to declare the intended
permissions at build time so that an LSM hook can be invoked when the source
VMA is availble, e.g. in this example, the LSM would see that the process is
loading executable code into an enclave from an anonymous VMA:

        ptr = malloc(size);
        memcpy(ptr, evil_shenanigans, size);
	ioctl(sgx_fd, ENCLAVE_ADD_PAGE, SGX_PROT_READ | SGX_PROT_EXEC, ptr, size);
        {
                ret = security_enclave_load(ptr, prot);
                if (ret)
                        return ret;

                enclave_page->declared_prot = prot;
        }
        ...
	ioctl(sgx_fd, ENCLAVE_INIT);

and then enforce the declared perms via ->mprotect()

        enclave = mmap(sgx_fd, ... PROT_READ);
        mprotect(enclave, ..., PROT_READ | PROT_EXEC);
        |
        -> sgx_mprotect(...)
           {
                   if (~enclave_page->declared_prot & prot)
                        return -EACCES;
           }

	EENTER;

So the above would be allowed, but this would fail (irrespective of LSM behavior):

        ptr = malloc(size);
        memcpy(ptr, evil_shenanigans, size);
	ioctl(sgx_fd, ENCLAVE_ADD_PAGE, SGX_PROT_READ, ptr, size);
        ...
	ioctl(sgx_fd, ENCLAVE_INIT);

        enclave = mmap(sgx_fd, ... PROT_READ);
        mprotect(enclave, ..., PROT_READ | PROT_EXEC);


My concern is that if we merge this

        ioctl(sgx_fd, ENCLAVE_ADD_PAGE, SGX_PROT_READ | SGX_PROT_EXEC, ptr, size);

without ->mprotect(), we can't actually enforce the declared protections.  And
if we drop the field altogether:

        ioctl(sgx_fd, ENCLAVE_ADD_PAGE, ptr, size);

then we can't implement security_enclave_load().