* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-26 6:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christopherson, Sean J, Stephen Smalley, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrXXVMutX8eZk6nnkOAeS+Tj0sQd0FkW+wk6Rx8hQxCe6w@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Saturday, May 25, 2019 5:58 PM
>
> On Sat, May 25, 2019 at 3:40 PM Xing, Cedric <cedric.xing@intel.com> wrote:
> >
> > > From: Andy Lutomirski [mailto:luto@amacapital.net]
> > > Sent: Friday, May 24, 2019 4:42 PM
> > >
> > > > On May 24, 2019, at 3:41 PM, Sean Christopherson <sean.j.christopherson@intel.com>
> wrote:
> > > >
> > > >> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> > > >> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> > > >> <sean.j.christopherson@intel.com> wrote:
> > > >>>
> > > >>>> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > > >>>>> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> > > >>>>>
> > > >>>>> If "initial permissions" for enclaves are less restrictive
> > > >>>>> than shared objects, then it'd become a backdoor for
> > > >>>>> circumventing LSM when enclave whitelisting is *not* in place.
> > > >>>>> For example, an adversary may load a page, which would
> > > >>>>> otherwise never be executable, as an executable
> > > page in EPC.
> > > >>>>>
> > > >>>>> In the case a RWX page is needed, the calling process has to
> > > >>>>> have a RWX page serving as the source for EADD so
> > > >>>>> PROCESS__EXECMEM will have been checked. For SGX2, changing an
> > > >>>>> EPC page to RWX is subject to FILE__EXECMEM on
> > > >>>>> /dev/sgx/enclave, which I see as a security benefit because it
> > > >>>>> only affects the enclave but not the whole process hosting
> > > it.
> > > >>>>
> > > >>>> So the permission would be like FILE__EXECMOD on the source
> > > >>>> enclave page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > > >>>> MAP_SHARED, PROT_WRITE isn't going to work because that means
> > > >>>> you can modify the file.
> > > >>>
> > > >>> Was this in response to Cedric's comment, or to my comment?
> > > >>
> > > >> Yours. I think that requiring source pages to be actually mapped
> > > >> W is not such a great idea.
> > > >
> > > > I wasn't requiring source pages to be mapped W. At least I didn't
> > > > intend to require W. What I was trying to say is that SGX could
> > > > trigger an EXECMEM check if userspace attempted to EADD or EAUG an
> > > > enclave page with RWX permissions, e.g.:
> > > >
> > > > if ((SECINFO.PERMS & RWX) == RWX) {
> > > > ret = security_mmap_file(NULL, RWX, ???);
> > > > if (ret)
> > > > return ret;
> > > > }
> > > >
> > > > But that's a moot point if we add security_enclave_load() or whatever.
> > > >
> > > >>
> > > >>>
> > > >>>> I'm starting to think that looking at the source VMA permission
> > > >>>> bits or source PTE permission bits is putting a bit too much
> > > >>>> policy into the driver as opposed to the LSM. How about
> > > >>>> delegating the whole thing to an LSM hook? The EADD operation
> > > >>>> would invoke a new hook, something like:
> > > >>>>
> > > >>>> int security_enclave_load_bytes(void *source_addr, struct
> > > >>>> vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > > >>>> maxperm);
> > > >>>>
> > > >>>> Then you don't have to muck with mapping anything PROT_EXEC.
> > > >>>> Instead you load from a mapping of a file and the LSM applies
> > > >>>> whatever policy it feels appropriate. If the first pass gets
> > > >>>> something wrong, the application or library authors can take it
> > > >>>> up with the SELinux folks without breaking the whole ABI :)
> > > >>>>
> > > >>>> (I'm proposing passing in the source_vma because this hook
> > > >>>> would be called with mmap_sem held for read to avoid a TOCTOU
> > > >>>> race.)
> > > >>>>
> > > >>>> If we go this route, the only substantial change to the
> > > >>>> existing driver that's needed for an initial upstream merge is
> > > >>>> the maxperm mechanism and whatever hopefully minimal API
> > > >>>> changes are needed to allow users to conveniently set up the
> > > >>>> mappings. And we don't need to worry about how to hack around
> > > >>>> mprotect() calling into the LSM, because the LSM will actually
> > > >>>> be aware of SGX and can just do the right thing.
> > > >>>
> > > >>> This doesn't address restricting which processes can run which
> > > >>> enclaves, it only allows restricting the build flow. Or are you
> > > >>> suggesting this be done in addition to whitelisting sigstructs?
> > > >>
> > > >> In addition.
> > > >>
> > > >> But I named the function badly and gave it a bad signature, which
> > > >> confused you. Let's try again:
> > > >>
> > > >> int security_enclave_load_from_memory(const struct vm_area_struct
> > > >> *source, unsigned int maxperm);
> > > >
> > > > I prefer security_enclave_load(), "from_memory" seems redundant at best.
> > >
> > > Fine with me.
> >
> > If we think of EADD as a way of mmap()'ing an enclave file into memory, would this
> security_enclave_load() be the same as security_mmap_file(source_vma->vm_file, maxperm,
> MAP_PRIVATE), except that the target is now EPC instead of regular pages?
>
> Hmm, that's clever. Although it seems plausible that an LSM would want to allow RX or RWX
> of a given file page but only in the context of an approved enclave, so I think it should
> still be its own hook.
What do you mean by "in the context of an approved enclave"? EPC pages are *inaccessible* to any software until after EINIT. So it would never be a security concern to EADD a page with wrong permissions as long as the enclave would be denied eventually by LSM at EINIT.
But I acknowledge the difference between loading a page into regular memory vs. into EPC. So it's beneficial to have a separate hook, which if not hooked, would pass through to security_mmap_file() by default?
>
> >
> > >
> > > >
> > > >> Maybe some really fancy future LSM would also want loff_t
> > > >> source_offset, but it's probably not terribly useful. This same
> > > >> callback would be used for EAUG.
> >
> > EAUG always zeroes the EPC page before making it available to an enclave. So I don't
> think there's anything needed to done here.
>
> Duh. So security_enclave_load_zeros() for EAUG. See below.
>
> >
> > > >>
> > > >> Following up on your discussion with Cedric about sigstruct, the
> > > >> other callback would be something like:
> > > >>
> > > >> int security_enclave_init(struct file *sigstruct_file);
> >
> > I'd still insist in using a pointer rather than a file, for reasons that we've discussed
> before. For those who can't recall, the major reason is that most implementation would
> embed SIGSTRUCT into the same file as the enclave (or at least I don't want to prevent
> anyone from doing so), which could also be part of another file, such as a shared object
> or even the main executable itself. It could be difficult to obtain a fd in those cases.
> memfd won't work because it can't retain the same attributes of the original file
> containing the SIGSTRUCT.
> >
> > After all, what matters is the attributes associated with the backing file, which could
> be easily retrieve from vm_file of the covering VMA. So for the sake of flexibility, let's
> stay with what we've agreed before - a pointer to SIGSTRUCT.
>
> I'm okay with this, except for one nastiness: there's a big difference between a file that
> is just a sigstruct and a file that contains essentially arbitrary data plus a sigstruct
> at an arbitrary offset.
> We could do something tricky like saying that SIGSTRUCT can be in a file that's just a
> SIGSTRUCT or it can be in a special SIGSTRUCT ELF note in a file that isn't just a
> SIGSTRUCT, but that could be annoyingly restrictive.
Agreed. Approving a file implies approving all SIGSTRUCTs within that file. But I guess it wouldn't cause practical problems.
>
> If it's going to be in an arbitrary file, then I think the signature needs to be more like:
>
> int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t sigstruct_offset,
> const sgx_sigstruct *sigstruct);
>
> So that the LSM still has the opportunity to base its decision on the contents of the
> SIGSTRUCT. Actually, we need that change regardless.
Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just a pointer, because the VMA could be looked up using the pointer and the offset would then be (pointer - vma->vm_start)?
>
> >
> > > >>
> > > >> The main issue I see is that we also want to control the
> > > >> enclave's ability to have RWX pages or to change a W page to X.
> > > >> We might also
> > > >> want:
> > > >>
> > > >> int security_enclave_load_zeros(unsigned int maxperm);
> > > >
> > > > What's the use case for this? @maxperm will always be at least RW
> > > > in this case, otherwise the page is useless to the enclave, and if
> > > > the enclave can write the page, the fact that it started as zeros
> > > > is irrelevant.
> > >
> > > This is how EAUG could ask if RWX is okay. If an enclave is
> > > internally doing dynamic loading, the it will need a heap page with
> > > maxperm = RWX. (If it’s well designed, it will make it RW and then
> > > RX, either by changing SECINFO or by asking the host to mprotect() it, but it still
> needs the overall RWX mask.).
> >
> > Any new page EAUG'ed will start in RW (as dictated by SGX ISA). EACCEPTCOPY will then
> change it to RX. RWX is never needed for all practical purposes. This in fact could be
> gated by mprotect() and the attributes associated with /dev/sgx/enclave. In the case of
> SELinux, FILE__EXECMOD is the right attribute and mprotect() will take care of all the
> rest. I don't see why the driver need a role here.
>
> I find the SDM's discussion of EAUG, EACCEPT, and EACCEPTCOPY to be extremely confusing.
> My copy of the SDM has EACCEPT's SECINFO argument as "Read access permitted by Non
> Enclave". Is that an error?
I'm confused by those descriptions too. Guess I cannot comment if that's an error or not.
Anyway, per our internal documents, for EAUG, SECINFO has to be set to PT_REG|RW. For EACCEPT, SGX ISA compares supplied SECINFO with EPCM attributes and returns an error if they don't match. EACCEPTCOPY only works on pending pages (i.e. SECINFO.P must be set), and sets EPCM access permissions to whatever supplied in SECINFO.
> And is EACCEPTCOPY just EACCEPT + memcpy or is there some other fundamental difference?
> 38.5.7 doesn't even mention EACCEPTCOPY.
2 differences: 1) EACCEPT only *compares* but EACCEPTCOPY *sets* EPCM permissions; and 2) EACCEPTCOPY does EACCEPT+memcpy atomically.
>
> Anyway, all my confusion aside, I was talking about the page table, not the EPCM. I think
> the enclave should need permission to write its own content into a page that will ever
> become X, and the enclave's untrusted host library would do this by adding the page with
> MAXPERM=RWX and then mapping/mprotecting it as PROT_WRITE and then (later or
> simultaneously) PROT_EXEC.
I was talking about the same thing. A code page in EPC will start in RW (both EPCM and PTE) and end in RX (both EPCM and PTE). EACCEPTCOPY takes care of EPCM, while mprotect() could take care of PTE as long as /dev/sgx/enclave has FILE__EXECMOD.
I understand your intention to enclave pages to segments with different MAXPERMs. My concern is though the host process may not always have a priori knowledge on which ranges to be used as code vs. data. After all, only the weakest link matters in security so I think what a host process cares would be whether the enclave loads code dynamically, or expands its data segments only, or neither. And for that reason, I think it more "user friendly" to keep just one MAXPERM - i.e. the most permissive one. Then we could associate that with /dev/sgx/enclave so as to relieve the driver from keeping track of too many things.
>
> Since SGX2 doesn't seem to have a way to add an initialized page to EPC after an enclave
> starts, I guess that it's impossible to have the enclave do something like dlopen()
> without MAXPERM=RWX. So be it.
That's true. The reason behind it is SGX doesn’t trust anything from outside. So non-predetermined contents must be measured (e.g. EADD+EEXTEND), but it's hard to measure (or attest to) dynamically added contents so we decided to allow predetermined contents (i.e. all zeros in the case of EAUG) only.
> Maybe someone will find this annoying someday and SGX3 will add EAUG-but-don't-zero and
> EACCEPT-with-existing-contents.
From security perspective, accepting a page that is measured/hashed to XYZ is equivalent to overwriting that page with content hashed to XYZ. EACCEPTCOPY actually does the latter. The annoying part is due to the mismatch between SGX ISA and the s/w model adopted by LSM, but that has nothing to do with security.
>
> >
> > >
> > > Also, do real SGX1 enclave formats have BSS? If so, then either we
> > > need an ioctl or load zeros or user code is going to load from
> > > /dev/zero or just from the heap, but the LSM is going to play better
> > > with an ioctl, I suspect :)
> >
> > Yes, it does. But an enclave would either measure BSS, in which case the initial bytes
> have to be zero or MRENCLAVE will change; or zero BSS explicitly in its initialization
> code.
> >
> > But from LSM's perspective it makes no difference than EADD'ing a page with non-zero
> content. And security_enclave_load(NULL, RW) would take care of it in exactly in the same
> way.
>
> Sure, I suppose the same hook with NULL parameters would be equivalent.
>
> >
> > >
> > > >
> > > >> An enclave that's going to modify its own code will need memory
> > > >> with maxperm = RWX or WX.
> >
> > With SGX2/EDMM, RWX is *never* needed for all practical purposes.
> >
> > In theory, in terms of security, no page shall be made executable while it is still
> being prepared. So W and X shall always be mutually exclusive, regardless it's in EPC or
> regular memory.
> >
> > RWX is only needed in SGX1, as a workaround for certain usages, because EPCM permissions
> can never change at runtime.
>
> As above, I think I disagree. MAXPERM is intended as an upper bound on the permissions
> that a page can ever have, at least until it's EREMOVEd and re-added. Since there's no
> EAUG-but-don't-zero, EAUG with MAXPERM.W=0 is basically useless because the page can never
> contain anything other than zeros, so a dynamically allocated page that is ever executed
> has to have MAXPERM=RWX or MAXPERM=WX. And that will need special permissions, which I
> think is consistent with your recent emails on how this could all map to SELinux
> permissions.
I'm totally with you. What I was trying to say was that only W or X would be needed at any given time. That said, MAXPERM=RWX but PROCESS__EXECMEM will not be needed, while FILE__EXECMOD will be needed only on /dev/sgx/enclave. So the inherent risk is contained.
>
> >
> > > >>
> > > >> But this is a bit awkward if the LSM's decision depends on the
> > > >> sigstruct. We could get fancy and require that the sigstruct be
> > > >> supplied before any EADD operations so that the maxperm decisions
> > > >> can depend on the sigstruct.
> > > >>
> > > >> Am I making more sense now?
> > > >
> > > > Yep. Requiring .sigstruct at ECREATE would be trivial. If we
> > > > wanted flexibility we could do:
> > > >
> > > > int security_enclave_load(struct file *file, struct vm_area_struct *vma,
> > > > unsigned long prot);
> > > >
> > > > And for ultimate flexibility we could pass both .sigstruct and the
> > > > file pointer for /dev/sgx/enclave, but that seems a bit ridiculous.
> > >
> > > I agree.
> >
> > Loosely speaking, an enclave (including initial contents of all of its pages and their
> permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision resistant
> property of SHA-2). So only one is needed for a decision, and either one would lead to the
> same decision. So I don't see anything making any sense here.
> >
> > Theoretically speaking, if LSM can make a decision at EINIT by means of
> security_enclave_load(), then security_enclave_load() is never needed.
> >
> > In practice, I support keeping both because security_enclave_load() can only approve an
> enumerable set while security_enclave_load() can approve a non-enumerable set of enclaves.
> Moreover, in order to determine the validity of a MRENCLAVE (as in development of a policy
> or in creation of a white/black list), system admins will need the audit log produced by
> security_enclave_load().
>
> I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows up. Also,
> security_enclave_load() provides no protection against loading a mishmash of two different
> enclave files. I see
> security_enclave_init() as "verify this SIGSTRUCT against your policy on who may sign
> enclaves and/or grant EXECMOD depending on SIGSTRUCT"
> and security_enclave_load() as "implement your EXECMOD / EXECUTE / WRITE / whatever policy
> and possibly check enclave files for some label."
Sorry for the confusion. I was saying the same thing except that the decision of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your prototype of security_enclave_load(), I think we are on the same page. I made the above comment to object to the idea of "require that the sigstruct be supplied before any EADD operations so that the maxperm decisions can depend on the sigstruct".
>
> >
> > >
> > > >
> > > > Passing both would allow tying EXECMOD to /dev/sgx/enclave as
> > > > Cedric wanted (without having to play games and pass
> > > > /dev/sgx/enclave to security_enclave_load()), but I don't think
> > > > there's anything fundamentally broken with using .sigstruct for
> > > > EXECMOD. It requires more verbose labeling, but that's not a bad thing.
> > >
> > > The benefit of putting it on .sigstruct is that it can be per-enclave.
> > >
> > > As I understand it from Fedora packaging, the way this works on
> > > distros is generally that a package will include some files and
> > > their associated labels, and, if the package needs EXECMOD, then the
> > > files are labeled with EXECMOD and the author of the relevant code might get a dirty
> look.
> > >
> > > This could translate to the author of an exclave that needs RWX
> > > regions getting a dirty look without leaking this permission into other enclaves.
> > >
> > > (In my opinion, the dirty looks are actually the best security
> > > benefit of the entire concept of LSMs making RWX difficult. A
> > > sufficiently creative attacker can almost always bypass W^X
> > > restrictions once they’ve pwned you, but W^X makes it harder to pwn
> > > you in the first place, and SELinux makes it really obvious when
> > > packaging a program that doesn’t respect W^X. The upshot is that a
> > > lot of programs got fixed.)
> >
> > I'm lost here. Dynamically linked enclaves, if running on SGX2, would need RW->RX, i.e.
> FILE__EXECMOD on /dev/sgx/enclave. But they never need RWX, i.e. PROCESS__EXECMEM.
>
> Hmm. If we want to make this distinction, we need something a big richer than my proposed
> callbacks. A check of the actual mprotect() /
> mmap() permissions would also be needed. Specifically, allowing MAXPERM=RWX wouldn't
> imply that PROT_WRITE | PROT_EXEC is allowed.
If we keep only one MAXPERM, wouldn't this be the current behavior of mmap()/mprotect()?
To be a bit more clear, system admin sets MAXPERM upper bound in the form of FILE__{READ|WRITE|EXECUTE|EXECMOD} of /dev/sgx/enclave. Then for a process/enclave, if what it requires falls below what's allowed on /dev/sgx/enclave, then everything will just work. Otherwise, it fails in the form of -EPERM returned from mmap()/mprotect(). Please note that MAXPERM here applies to "runtime" permissions, while "initial" permissions are taken care of by security_enclave_{load|init}. "initial" permissions could be more permissive than "runtime" permissions, e.g., RX is still required for initial code pages even though system admins could disable dynamically loaded code pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the "initial" mapping would still have to be done by the driver (to bypass LSM), either via a new ioctl or as part of IOC_EINIT.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-26 0:57 UTC (permalink / raw)
To: Xing, Cedric
Cc: Christopherson, Sean J, Andy Lutomirski, Stephen Smalley,
Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E965F@ORSMSX116.amr.corp.intel.com>
On Sat, May 25, 2019 at 3:40 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@amacapital.net]
> > Sent: Friday, May 24, 2019 4:42 PM
> >
> > > On May 24, 2019, at 3:41 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > >
> > >> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> > >> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> > >> <sean.j.christopherson@intel.com> wrote:
> > >>>
> > >>>> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > >>>>> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> > >>>>>
> > >>>>> If "initial permissions" for enclaves are less restrictive than
> > >>>>> shared objects, then it'd become a backdoor for circumventing LSM
> > >>>>> when enclave whitelisting is *not* in place. For example, an
> > >>>>> adversary may load a page, which would otherwise never be executable, as an executable
> > page in EPC.
> > >>>>>
> > >>>>> In the case a RWX page is needed, the calling process has to have
> > >>>>> a RWX page serving as the source for EADD so PROCESS__EXECMEM will
> > >>>>> have been checked. For SGX2, changing an EPC page to RWX is
> > >>>>> subject to FILE__EXECMEM on /dev/sgx/enclave, which I see as a
> > >>>>> security benefit because it only affects the enclave but not the whole process hosting
> > it.
> > >>>>
> > >>>> So the permission would be like FILE__EXECMOD on the source enclave
> > >>>> page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > >>>> MAP_SHARED, PROT_WRITE isn't going to work because that means you
> > >>>> can modify the file.
> > >>>
> > >>> Was this in response to Cedric's comment, or to my comment?
> > >>
> > >> Yours. I think that requiring source pages to be actually mapped W
> > >> is not such a great idea.
> > >
> > > I wasn't requiring source pages to be mapped W. At least I didn't
> > > intend to require W. What I was trying to say is that SGX could
> > > trigger an EXECMEM check if userspace attempted to EADD or EAUG an
> > > enclave page with RWX permissions, e.g.:
> > >
> > > if ((SECINFO.PERMS & RWX) == RWX) {
> > > ret = security_mmap_file(NULL, RWX, ???);
> > > if (ret)
> > > return ret;
> > > }
> > >
> > > But that's a moot point if we add security_enclave_load() or whatever.
> > >
> > >>
> > >>>
> > >>>> I'm starting to think that looking at the source VMA permission
> > >>>> bits or source PTE permission bits is putting a bit too much policy
> > >>>> into the driver as opposed to the LSM. How about delegating the
> > >>>> whole thing to an LSM hook? The EADD operation would invoke a new
> > >>>> hook, something like:
> > >>>>
> > >>>> int security_enclave_load_bytes(void *source_addr, struct
> > >>>> vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > >>>> maxperm);
> > >>>>
> > >>>> Then you don't have to muck with mapping anything PROT_EXEC.
> > >>>> Instead you load from a mapping of a file and the LSM applies
> > >>>> whatever policy it feels appropriate. If the first pass gets
> > >>>> something wrong, the application or library authors can take it up
> > >>>> with the SELinux folks without breaking the whole ABI :)
> > >>>>
> > >>>> (I'm proposing passing in the source_vma because this hook would be
> > >>>> called with mmap_sem held for read to avoid a TOCTOU race.)
> > >>>>
> > >>>> If we go this route, the only substantial change to the existing
> > >>>> driver that's needed for an initial upstream merge is the maxperm
> > >>>> mechanism and whatever hopefully minimal API changes are needed to
> > >>>> allow users to conveniently set up the mappings. And we don't need
> > >>>> to worry about how to hack around mprotect() calling into the LSM,
> > >>>> because the LSM will actually be aware of SGX and can just do the
> > >>>> right thing.
> > >>>
> > >>> This doesn't address restricting which processes can run which
> > >>> enclaves, it only allows restricting the build flow. Or are you
> > >>> suggesting this be done in addition to whitelisting sigstructs?
> > >>
> > >> In addition.
> > >>
> > >> But I named the function badly and gave it a bad signature, which
> > >> confused you. Let's try again:
> > >>
> > >> int security_enclave_load_from_memory(const struct vm_area_struct
> > >> *source, unsigned int maxperm);
> > >
> > > I prefer security_enclave_load(), "from_memory" seems redundant at best.
> >
> > Fine with me.
>
> If we think of EADD as a way of mmap()'ing an enclave file into memory, would this security_enclave_load() be the same as security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that the target is now EPC instead of regular pages?
Hmm, that's clever. Although it seems plausible that an LSM would
want to allow RX or RWX of a given file page but only in the context
of an approved enclave, so I think it should still be its own hook.
>
> >
> > >
> > >> Maybe some really fancy future LSM would also want loff_t
> > >> source_offset, but it's probably not terribly useful. This same
> > >> callback would be used for EAUG.
>
> EAUG always zeroes the EPC page before making it available to an enclave. So I don't think there's anything needed to done here.
Duh. So security_enclave_load_zeros() for EAUG. See below.
>
> > >>
> > >> Following up on your discussion with Cedric about sigstruct, the
> > >> other callback would be something like:
> > >>
> > >> int security_enclave_init(struct file *sigstruct_file);
>
> I'd still insist in using a pointer rather than a file, for reasons that we've discussed before. For those who can't recall, the major reason is that most implementation would embed SIGSTRUCT into the same file as the enclave (or at least I don't want to prevent anyone from doing so), which could also be part of another file, such as a shared object or even the main executable itself. It could be difficult to obtain a fd in those cases. memfd won't work because it can't retain the same attributes of the original file containing the SIGSTRUCT.
>
> After all, what matters is the attributes associated with the backing file, which could be easily retrieve from vm_file of the covering VMA. So for the sake of flexibility, let's stay with what we've agreed before - a pointer to SIGSTRUCT.
I'm okay with this, except for one nastiness: there's a big difference
between a file that is just a sigstruct and a file that contains
essentially arbitrary data plus a sigstruct at an arbitrary offset.
We could do something tricky like saying that SIGSTRUCT can be in a
file that's just a SIGSTRUCT or it can be in a special SIGSTRUCT ELF
note in a file that isn't just a SIGSTRUCT, but that could be
annoyingly restrictive.
If it's going to be in an arbitrary file, then I think the signature
needs to be more like:
int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
sigstruct_offset, const sgx_sigstruct *sigstruct);
So that the LSM still has the opportunity to base its decision on the
contents of the SIGSTRUCT. Actually, we need that change regardless.
>
> > >>
> > >> The main issue I see is that we also want to control the enclave's
> > >> ability to have RWX pages or to change a W page to X. We might also
> > >> want:
> > >>
> > >> int security_enclave_load_zeros(unsigned int maxperm);
> > >
> > > What's the use case for this? @maxperm will always be at least RW in
> > > this case, otherwise the page is useless to the enclave, and if the
> > > enclave can write the page, the fact that it started as zeros is
> > > irrelevant.
> >
> > This is how EAUG could ask if RWX is okay. If an enclave is internally doing dynamic loading,
> > the it will need a heap page with maxperm = RWX. (If it’s well designed, it will make it RW
> > and then RX, either by changing SECINFO or by asking the host to mprotect() it, but it still
> > needs the overall RWX mask.).
>
> Any new page EAUG'ed will start in RW (as dictated by SGX ISA). EACCEPTCOPY will then change it to RX. RWX is never needed for all practical purposes. This in fact could be gated by mprotect() and the attributes associated with /dev/sgx/enclave. In the case of SELinux, FILE__EXECMOD is the right attribute and mprotect() will take care of all the rest. I don't see why the driver need a role here.
I find the SDM's discussion of EAUG, EACCEPT, and EACCEPTCOPY to be
extremely confusing. My copy of the SDM has EACCEPT's SECINFO
argument as "Read access permitted by Non Enclave". Is that an error?
And is EACCEPTCOPY just EACCEPT + memcpy or is there some other
fundamental difference? 38.5.7 doesn't even mention EACCEPTCOPY.
Anyway, all my confusion aside, I was talking about the page table,
not the EPCM. I think the enclave should need permission to write its
own content into a page that will ever become X, and the enclave's
untrusted host library would do this by adding the page with
MAXPERM=RWX and then mapping/mprotecting it as PROT_WRITE and then
(later or simultaneously) PROT_EXEC.
Since SGX2 doesn't seem to have a way to add an initialized page to
EPC after an enclave starts, I guess that it's impossible to have the
enclave do something like dlopen() without MAXPERM=RWX. So be it.
Maybe someone will find this annoying someday and SGX3 will add
EAUG-but-don't-zero and EACCEPT-with-existing-contents.
>
> >
> > Also, do real SGX1 enclave formats have BSS? If so, then either we need an ioctl or load zeros
> > or user code is going to load from /dev/zero or just from the heap, but the LSM is going to
> > play better with an ioctl, I suspect :)
>
> Yes, it does. But an enclave would either measure BSS, in which case the initial bytes have to be zero or MRENCLAVE will change; or zero BSS explicitly in its initialization code.
>
> But from LSM's perspective it makes no difference than EADD'ing a page with non-zero content. And security_enclave_load(NULL, RW) would take care of it in exactly in the same way.
Sure, I suppose the same hook with NULL parameters would be equivalent.
>
> >
> > >
> > >> An enclave that's going to modify its own code will need memory with
> > >> maxperm = RWX or WX.
>
> With SGX2/EDMM, RWX is *never* needed for all practical purposes.
>
> In theory, in terms of security, no page shall be made executable while it is still being prepared. So W and X shall always be mutually exclusive, regardless it's in EPC or regular memory.
>
> RWX is only needed in SGX1, as a workaround for certain usages, because EPCM permissions can never change at runtime.
As above, I think I disagree. MAXPERM is intended as an upper bound
on the permissions that a page can ever have, at least until it's
EREMOVEd and re-added. Since there's no EAUG-but-don't-zero, EAUG
with MAXPERM.W=0 is basically useless because the page can never
contain anything other than zeros, so a dynamically allocated page
that is ever executed has to have MAXPERM=RWX or MAXPERM=WX. And that
will need special permissions, which I think is consistent with your
recent emails on how this could all map to SELinux permissions.
>
> > >>
> > >> But this is a bit awkward if the LSM's decision depends on the
> > >> sigstruct. We could get fancy and require that the sigstruct be
> > >> supplied before any EADD operations so that the maxperm decisions can
> > >> depend on the sigstruct.
> > >>
> > >> Am I making more sense now?
> > >
> > > Yep. Requiring .sigstruct at ECREATE would be trivial. If we wanted
> > > flexibility we could do:
> > >
> > > int security_enclave_load(struct file *file, struct vm_area_struct *vma,
> > > unsigned long prot);
> > >
> > > And for ultimate flexibility we could pass both .sigstruct and the
> > > file pointer for /dev/sgx/enclave, but that seems a bit ridiculous.
> >
> > I agree.
>
> Loosely speaking, an enclave (including initial contents of all of its pages and their permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision resistant property of SHA-2). So only one is needed for a decision, and either one would lead to the same decision. So I don't see anything making any sense here.
>
> Theoretically speaking, if LSM can make a decision at EINIT by means of security_enclave_load(), then security_enclave_load() is never needed.
>
> In practice, I support keeping both because security_enclave_load() can only approve an enumerable set while security_enclave_load() can approve a non-enumerable set of enclaves. Moreover, in order to determine the validity of a MRENCLAVE (as in development of a policy or in creation of a white/black list), system admins will need the audit log produced by security_enclave_load().
I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT
shows up. Also, security_enclave_load() provides no protection
against loading a mishmash of two different enclave files. I see
security_enclave_init() as "verify this SIGSTRUCT against your policy
on who may sign enclaves and/or grant EXECMOD depending on SIGSTRUCT"
and security_enclave_load() as "implement your EXECMOD / EXECUTE /
WRITE / whatever policy and possibly check enclave files for some
label."
>
> >
> > >
> > > Passing both would allow tying EXECMOD to /dev/sgx/enclave as Cedric
> > > wanted (without having to play games and pass /dev/sgx/enclave to
> > > security_enclave_load()), but I don't think there's anything
> > > fundamentally broken with using .sigstruct for EXECMOD. It requires
> > > more verbose labeling, but that's not a bad thing.
> >
> > The benefit of putting it on .sigstruct is that it can be per-enclave.
> >
> > As I understand it from Fedora packaging, the way this works on distros is generally that a
> > package will include some files and their associated labels, and, if the package needs EXECMOD,
> > then the files are labeled with EXECMOD and the author of the relevant code might get a dirty
> > look.
> >
> > This could translate to the author of an exclave that needs RWX regions getting a dirty look
> > without leaking this permission into other enclaves.
> >
> > (In my opinion, the dirty looks are actually the best security benefit of the entire concept
> > of LSMs making RWX difficult. A sufficiently creative attacker can almost always bypass W^X
> > restrictions once they’ve pwned you, but W^X makes it harder to pwn you in the first place,
> > and SELinux makes it really obvious when packaging a program that doesn’t respect W^X. The
> > upshot is that a lot of programs got fixed.)
>
> I'm lost here. Dynamically linked enclaves, if running on SGX2, would need RW->RX, i.e. FILE__EXECMOD on /dev/sgx/enclave. But they never need RWX, i.e. PROCESS__EXECMEM.
Hmm. If we want to make this distinction, we need something a big
richer than my proposed callbacks. A check of the actual mprotect() /
mmap() permissions would also be needed. Specifically, allowing
MAXPERM=RWX wouldn't imply that PROT_WRITE | PROT_EXEC is allowed.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-25 22:40 UTC (permalink / raw)
To: Andy Lutomirski, Christopherson, Sean J
Cc: Andy Lutomirski, Stephen Smalley, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <683B5E3D-AFB6-4B45-8D39-B00847312209@amacapital.net>
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Friday, May 24, 2019 4:42 PM
>
> > On May 24, 2019, at 3:41 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >
> >> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> >> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> >> <sean.j.christopherson@intel.com> wrote:
> >>>
> >>>> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> >>>>> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> >>>>>
> >>>>> If "initial permissions" for enclaves are less restrictive than
> >>>>> shared objects, then it'd become a backdoor for circumventing LSM
> >>>>> when enclave whitelisting is *not* in place. For example, an
> >>>>> adversary may load a page, which would otherwise never be executable, as an executable
> page in EPC.
> >>>>>
> >>>>> In the case a RWX page is needed, the calling process has to have
> >>>>> a RWX page serving as the source for EADD so PROCESS__EXECMEM will
> >>>>> have been checked. For SGX2, changing an EPC page to RWX is
> >>>>> subject to FILE__EXECMEM on /dev/sgx/enclave, which I see as a
> >>>>> security benefit because it only affects the enclave but not the whole process hosting
> it.
> >>>>
> >>>> So the permission would be like FILE__EXECMOD on the source enclave
> >>>> page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> >>>> MAP_SHARED, PROT_WRITE isn't going to work because that means you
> >>>> can modify the file.
> >>>
> >>> Was this in response to Cedric's comment, or to my comment?
> >>
> >> Yours. I think that requiring source pages to be actually mapped W
> >> is not such a great idea.
> >
> > I wasn't requiring source pages to be mapped W. At least I didn't
> > intend to require W. What I was trying to say is that SGX could
> > trigger an EXECMEM check if userspace attempted to EADD or EAUG an
> > enclave page with RWX permissions, e.g.:
> >
> > if ((SECINFO.PERMS & RWX) == RWX) {
> > ret = security_mmap_file(NULL, RWX, ???);
> > if (ret)
> > return ret;
> > }
> >
> > But that's a moot point if we add security_enclave_load() or whatever.
> >
> >>
> >>>
> >>>> I'm starting to think that looking at the source VMA permission
> >>>> bits or source PTE permission bits is putting a bit too much policy
> >>>> into the driver as opposed to the LSM. How about delegating the
> >>>> whole thing to an LSM hook? The EADD operation would invoke a new
> >>>> hook, something like:
> >>>>
> >>>> int security_enclave_load_bytes(void *source_addr, struct
> >>>> vm_area_struct *source_vma, loff_t source_offset, unsigned int
> >>>> maxperm);
> >>>>
> >>>> Then you don't have to muck with mapping anything PROT_EXEC.
> >>>> Instead you load from a mapping of a file and the LSM applies
> >>>> whatever policy it feels appropriate. If the first pass gets
> >>>> something wrong, the application or library authors can take it up
> >>>> with the SELinux folks without breaking the whole ABI :)
> >>>>
> >>>> (I'm proposing passing in the source_vma because this hook would be
> >>>> called with mmap_sem held for read to avoid a TOCTOU race.)
> >>>>
> >>>> If we go this route, the only substantial change to the existing
> >>>> driver that's needed for an initial upstream merge is the maxperm
> >>>> mechanism and whatever hopefully minimal API changes are needed to
> >>>> allow users to conveniently set up the mappings. And we don't need
> >>>> to worry about how to hack around mprotect() calling into the LSM,
> >>>> because the LSM will actually be aware of SGX and can just do the
> >>>> right thing.
> >>>
> >>> This doesn't address restricting which processes can run which
> >>> enclaves, it only allows restricting the build flow. Or are you
> >>> suggesting this be done in addition to whitelisting sigstructs?
> >>
> >> In addition.
> >>
> >> But I named the function badly and gave it a bad signature, which
> >> confused you. Let's try again:
> >>
> >> int security_enclave_load_from_memory(const struct vm_area_struct
> >> *source, unsigned int maxperm);
> >
> > I prefer security_enclave_load(), "from_memory" seems redundant at best.
>
> Fine with me.
If we think of EADD as a way of mmap()'ing an enclave file into memory, would this security_enclave_load() be the same as security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that the target is now EPC instead of regular pages?
>
> >
> >> Maybe some really fancy future LSM would also want loff_t
> >> source_offset, but it's probably not terribly useful. This same
> >> callback would be used for EAUG.
EAUG always zeroes the EPC page before making it available to an enclave. So I don't think there's anything needed to done here.
> >>
> >> Following up on your discussion with Cedric about sigstruct, the
> >> other callback would be something like:
> >>
> >> int security_enclave_init(struct file *sigstruct_file);
I'd still insist in using a pointer rather than a file, for reasons that we've discussed before. For those who can't recall, the major reason is that most implementation would embed SIGSTRUCT into the same file as the enclave (or at least I don't want to prevent anyone from doing so), which could also be part of another file, such as a shared object or even the main executable itself. It could be difficult to obtain a fd in those cases. memfd won't work because it can't retain the same attributes of the original file containing the SIGSTRUCT.
After all, what matters is the attributes associated with the backing file, which could be easily retrieve from vm_file of the covering VMA. So for the sake of flexibility, let's stay with what we've agreed before - a pointer to SIGSTRUCT.
> >>
> >> The main issue I see is that we also want to control the enclave's
> >> ability to have RWX pages or to change a W page to X. We might also
> >> want:
> >>
> >> int security_enclave_load_zeros(unsigned int maxperm);
> >
> > What's the use case for this? @maxperm will always be at least RW in
> > this case, otherwise the page is useless to the enclave, and if the
> > enclave can write the page, the fact that it started as zeros is
> > irrelevant.
>
> This is how EAUG could ask if RWX is okay. If an enclave is internally doing dynamic loading,
> the it will need a heap page with maxperm = RWX. (If it’s well designed, it will make it RW
> and then RX, either by changing SECINFO or by asking the host to mprotect() it, but it still
> needs the overall RWX mask.).
Any new page EAUG'ed will start in RW (as dictated by SGX ISA). EACCEPTCOPY will then change it to RX. RWX is never needed for all practical purposes. This in fact could be gated by mprotect() and the attributes associated with /dev/sgx/enclave. In the case of SELinux, FILE__EXECMOD is the right attribute and mprotect() will take care of all the rest. I don't see why the driver need a role here.
>
> Also, do real SGX1 enclave formats have BSS? If so, then either we need an ioctl or load zeros
> or user code is going to load from /dev/zero or just from the heap, but the LSM is going to
> play better with an ioctl, I suspect :)
Yes, it does. But an enclave would either measure BSS, in which case the initial bytes have to be zero or MRENCLAVE will change; or zero BSS explicitly in its initialization code.
But from LSM's perspective it makes no difference than EADD'ing a page with non-zero content. And security_enclave_load(NULL, RW) would take care of it in exactly in the same way.
>
> >
> >> An enclave that's going to modify its own code will need memory with
> >> maxperm = RWX or WX.
With SGX2/EDMM, RWX is *never* needed for all practical purposes.
In theory, in terms of security, no page shall be made executable while it is still being prepared. So W and X shall always be mutually exclusive, regardless it's in EPC or regular memory.
RWX is only needed in SGX1, as a workaround for certain usages, because EPCM permissions can never change at runtime.
> >>
> >> But this is a bit awkward if the LSM's decision depends on the
> >> sigstruct. We could get fancy and require that the sigstruct be
> >> supplied before any EADD operations so that the maxperm decisions can
> >> depend on the sigstruct.
> >>
> >> Am I making more sense now?
> >
> > Yep. Requiring .sigstruct at ECREATE would be trivial. If we wanted
> > flexibility we could do:
> >
> > int security_enclave_load(struct file *file, struct vm_area_struct *vma,
> > unsigned long prot);
> >
> > And for ultimate flexibility we could pass both .sigstruct and the
> > file pointer for /dev/sgx/enclave, but that seems a bit ridiculous.
>
> I agree.
Loosely speaking, an enclave (including initial contents of all of its pages and their permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision resistant property of SHA-2). So only one is needed for a decision, and either one would lead to the same decision. So I don't see anything making any sense here.
Theoretically speaking, if LSM can make a decision at EINIT by means of security_enclave_load(), then security_enclave_load() is never needed.
In practice, I support keeping both because security_enclave_load() can only approve an enumerable set while security_enclave_load() can approve a non-enumerable set of enclaves. Moreover, in order to determine the validity of a MRENCLAVE (as in development of a policy or in creation of a white/black list), system admins will need the audit log produced by security_enclave_load().
>
> >
> > Passing both would allow tying EXECMOD to /dev/sgx/enclave as Cedric
> > wanted (without having to play games and pass /dev/sgx/enclave to
> > security_enclave_load()), but I don't think there's anything
> > fundamentally broken with using .sigstruct for EXECMOD. It requires
> > more verbose labeling, but that's not a bad thing.
>
> The benefit of putting it on .sigstruct is that it can be per-enclave.
>
> As I understand it from Fedora packaging, the way this works on distros is generally that a
> package will include some files and their associated labels, and, if the package needs EXECMOD,
> then the files are labeled with EXECMOD and the author of the relevant code might get a dirty
> look.
>
> This could translate to the author of an exclave that needs RWX regions getting a dirty look
> without leaking this permission into other enclaves.
>
> (In my opinion, the dirty looks are actually the best security benefit of the entire concept
> of LSMs making RWX difficult. A sufficiently creative attacker can almost always bypass W^X
> restrictions once they’ve pwned you, but W^X makes it harder to pwn you in the first place,
> and SELinux makes it really obvious when packaging a program that doesn’t respect W^X. The
> upshot is that a lot of programs got fixed.)
I'm lost here. Dynamically linked enclaves, if running on SGX2, would need RW->RX, i.e. FILE__EXECMOD on /dev/sgx/enclave. But they never need RWX, i.e. PROCESS__EXECMEM.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Dr. Greg @ 2019-05-25 17:31 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Xing, Cedric, Stephen Smalley, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524200333.GF365@linux.intel.com>
On Fri, May 24, 2019 at 01:03:33PM -0700, Sean Christopherson wrote:
Good morning, I hope the weekend is going well for everyone. Skunky
holiday weather out here in West-Central Minnesota.
> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > If we go this route, the only substantial change to the existing
> > driver that's needed for an initial upstream merge is the maxperm
> > mechanism and whatever hopefully minimal API changes are needed to
> > allow users to conveniently set up the mappings. And we don't need to
> > worry about how to hack around mprotect() calling into the LSM,
> > because the LSM will actually be aware of SGX and can just do the
> > right thing.
> This doesn't address restricting which processes can run which
> enclaves, it only allows restricting the build flow. Or are you
> suggesting this be done in addition to whitelisting sigstructs?
>
> What's the value prop beyond whitelisting sigstructs?
> Realistically, I doubt LSMs/users will want to take the performance
> hit of scanning the source bytes every time an enclave is loaded.
>
> We could add seomthing like security_enclave_mprotect() in lieu of
> abusing security_file_mprotect(), but passing the full source bytes
> seems a bit much.
It would seem that we hold the moniker of responsibility for this
conversation, since without our provocation regarding cryptographic
verification of enclave source, there would be a driver headed
upstream whose only constraint against W^X sourced executable code,
running with full confidentiality and integrity protections, would be
a character device with o666 permissions. Given that, a couple of
reflections to facilitate further conversation, if nothing else for
the benefit of Jonathan Corbet and his bystanders... :-)
As the conversations to date have indicated, imposing LSM controls on
enclave executable code is a bit problematic, in no small part since
it is the theological equivalent of driving a square peg into a round
hole. SGX, as a technology, was designed around the concept of
cryptographic verification of code provenance and origin.
The decision to take that off the table, for reasons of political
idealogy only, means that mainstream Linux will not be a platform that
can achieve the full hardware security capabilities and protections of
SGX, nor will mainstream Linux be able to enjoy full protections from
the technology itself.
We will be dealing with that, from a driver and runtime perspective,
but that is a conversation for another day.
The issue of SGX2 and Enclave Dynamic Memory Management (EDMM) has
come up and to date there doesn't appear to have been a serious
conversation regarding whether or not all of the LSM machinations in
the world will make any difference when this technology goes mainline.
The agenda driving mainlining of the driver is to support Graphene for
cloud based solutions and without EDMM, dynamic code loading support
is decidedly more problematic.
Dynamic enclave code loading isn't problematic from a security
perspective when the code is being loaded from the platform itself,
since presumably, the encompassing conversation will result in LSM
controls being applied to the necessary code paths. However, with the
ability to exploit SGX2 instructions, an enclave with adverserial
intent could simply setup a mutually attested security context and
pull whatever executable code it wants from the INTERNET at large,
using an encrypted and integrity protected communications channel.
That has at least been our interpretation and experience with the
ENCLU[EMODPE] and ENCLU[EACCEPTCOPY] instructions and the out-of-tree
driver. Given the use of an encrypted channel, and the fact that
these instructions are ring 3 enclave mode only, it would seem that
all of the LSM controls in the world won't have visibility or control
over code that is being loaded and executed using such a mechanism.
We could have arguably missed something that the new driver will do to
address this issue. To date the only discussion seems to have been
about controls over ENCLS[EAUG], which are arguably a bit blunt for
this purpose.
In the land of SGX, if one is intellectually honest from an
engineering perspective, the only solid security contract one has to
work with is the notion of cryptographic identity. Hence our concern
and patches that implemented an absolutely minimal footprint ring-0
control infrastructure over the contents of an enclave's SIGSTRUCT.
Which is where we have arguably circled back to after 3-4 months and
one kernel release cycle.
Wrapping an LSM hook around our policy mechanism would seem to
achieve, from a security perspective, about the same level of security
effect that more major and invasive modifications would achieve, given
Cedric's proposal to inherit page permissions from the source, which
is what our runtime already does.
As always, apologies for excessive verbosity beyond LKML sensibilities.
Best wishes for a pleasant remainder of the spring weekend to
everyone.
Dr. Greg
As always,
Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC.
4206 N. 19th Ave. Specializing in information infra-structure
Fargo, ND 58102 development.
PH: 701-281-1686
FAX: 701-281-3949 EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Heaven goes by favor. If it went by merit, you would stay out and your
dog would go in."
-- Mark Twain
^ permalink raw reply
* Re: [PATCH 3/7] keys: sparse: Fix kdoc mismatches
From: James Morris @ 2019-05-25 3:57 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856410370.10428.4854212353631906601.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> Fix some kdoc argument description mismatches reported by sparse and give
> keyring_restrict() a description.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 2/7] keys: sparse: Fix incorrect RCU accesses
From: James Morris @ 2019-05-25 3:57 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856409697.10428.15024260088836364169.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> Fix a pair of accesses that should be using RCU protection.
>
> rcu_dereference_protected() is needed to access task_struct::real_parent.
>
> current_cred() should be used to access current->cred.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: Stacking patches for 5.3
From: James Morris @ 2019-05-25 3:55 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, Linux Security Module list, Kees Cook, John Johansen
In-Reply-To: <e42136b0-d656-1aa9-c848-d6a8aaa48c92@schaufler-ca.com>
On Fri, 24 May 2019, Casey Schaufler wrote:
> On 5/24/2019 12:37 PM, Paul Moore wrote:
> > On Fri, May 24, 2019 at 3:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> I have a set of 59 patches for the nest round of LSM stacking.
> >> They are all really small. Kees has expressed a preference for
> >> small patches. I think that the collapsed set of about 10 would
> >> make more sense. Which would the community as a whole prefer?
> >> The patches are not much different from what has been presented
> >> already.
> > Is there an option for "not posted on the Friday before a holiday weekend"? ;)
>
> I'm waiting for James' next-general to base it on, so you needn't worry.
I won't be merging with Linus' tree unless there's a specific reason to,
so go ahead with the current branch.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 23:42 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Xing, Cedric, Stephen Smalley, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524224107.GJ365@linux.intel.com>
> On May 24, 2019, at 3:41 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>
>> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
>> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>>
>>>> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
>>>>> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>>>>>
>>>>> If "initial permissions" for enclaves are less restrictive than shared
>>>>> objects, then it'd become a backdoor for circumventing LSM when enclave
>>>>> whitelisting is *not* in place. For example, an adversary may load a page,
>>>>> which would otherwise never be executable, as an executable page in EPC.
>>>>>
>>>>> In the case a RWX page is needed, the calling process has to have a RWX
>>>>> page serving as the source for EADD so PROCESS__EXECMEM will have been
>>>>> checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM
>>>>> on /dev/sgx/enclave, which I see as a security benefit because it only
>>>>> affects the enclave but not the whole process hosting it.
>>>>
>>>> So the permission would be like FILE__EXECMOD on the source enclave
>>>> page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
>>>> MAP_SHARED, PROT_WRITE isn't going to work because that means you can
>>>> modify the file.
>>>
>>> Was this in response to Cedric's comment, or to my comment?
>>
>> Yours. I think that requiring source pages to be actually mapped W is
>> not such a great idea.
>
> I wasn't requiring source pages to be mapped W. At least I didn't intend
> to require W. What I was trying to say is that SGX could trigger an
> EXECMEM check if userspace attempted to EADD or EAUG an enclave page with
> RWX permissions, e.g.:
>
> if ((SECINFO.PERMS & RWX) == RWX) {
> ret = security_mmap_file(NULL, RWX, ???);
> if (ret)
> return ret;
> }
>
> But that's a moot point if we add security_enclave_load() or whatever.
>
>>
>>>
>>>> I'm starting to think that looking at the source VMA permission bits
>>>> or source PTE permission bits is putting a bit too much policy into
>>>> the driver as opposed to the LSM. How about delegating the whole
>>>> thing to an LSM hook? The EADD operation would invoke a new hook,
>>>> something like:
>>>>
>>>> int security_enclave_load_bytes(void *source_addr, struct
>>>> vm_area_struct *source_vma, loff_t source_offset, unsigned int
>>>> maxperm);
>>>>
>>>> Then you don't have to muck with mapping anything PROT_EXEC. Instead
>>>> you load from a mapping of a file and the LSM applies whatever policy
>>>> it feels appropriate. If the first pass gets something wrong, the
>>>> application or library authors can take it up with the SELinux folks
>>>> without breaking the whole ABI :)
>>>>
>>>> (I'm proposing passing in the source_vma because this hook would be
>>>> called with mmap_sem held for read to avoid a TOCTOU race.)
>>>>
>>>> If we go this route, the only substantial change to the existing
>>>> driver that's needed for an initial upstream merge is the maxperm
>>>> mechanism and whatever hopefully minimal API changes are needed to
>>>> allow users to conveniently set up the mappings. And we don't need to
>>>> worry about how to hack around mprotect() calling into the LSM,
>>>> because the LSM will actually be aware of SGX and can just do the
>>>> right thing.
>>>
>>> This doesn't address restricting which processes can run which enclaves,
>>> it only allows restricting the build flow. Or are you suggesting this
>>> be done in addition to whitelisting sigstructs?
>>
>> In addition.
>>
>> But I named the function badly and gave it a bad signature, which
>> confused you. Let's try again:
>>
>> int security_enclave_load_from_memory(const struct vm_area_struct
>> *source, unsigned int maxperm);
>
> I prefer security_enclave_load(), "from_memory" seems redundant at best.
Fine with me.
>
>> Maybe some really fancy future LSM would also want loff_t
>> source_offset, but it's probably not terribly useful. This same
>> callback would be used for EAUG.
>>
>> Following up on your discussion with Cedric about sigstruct, the other
>> callback would be something like:
>>
>> int security_enclave_init(struct file *sigstruct_file);
>>
>> The main issue I see is that we also want to control the enclave's
>> ability to have RWX pages or to change a W page to X. We might also
>> want:
>>
>> int security_enclave_load_zeros(unsigned int maxperm);
>
> What's the use case for this? @maxperm will always be at least RW in
> this case, otherwise the page is useless to the enclave, and if the
> enclave can write the page, the fact that it started as zeros is
> irrelevant.
This is how EAUG could ask if RWX is okay. If an enclave is internally doing dynamic loading, the it will need a heap page with maxperm = RWX. (If it’s well designed, it will make it RW and then RX, either by changing SECINFO or by asking the host to mprotect() it, but it still needs the overall RWX mask.).
Also, do real SGX1 enclave formats have BSS? If so, then either we need an ioctl or load zeros or user code is going to load from /dev/zero or just from the heap, but the LSM is going to play better with an ioctl, I suspect :)
>
>> An enclave that's going to modify its own code will need memory with
>> maxperm = RWX or WX.
>>
>> But this is a bit awkward if the LSM's decision depends on the
>> sigstruct. We could get fancy and require that the sigstruct be
>> supplied before any EADD operations so that the maxperm decisions can
>> depend on the sigstruct.
>>
>> Am I making more sense now?
>
> Yep. Requiring .sigstruct at ECREATE would be trivial. If we wanted
> flexibility we could do:
>
> int security_enclave_load(struct file *file, struct vm_area_struct *vma,
> unsigned long prot);
>
> And for ultimate flexibility we could pass both .sigstruct and the file
> pointer for /dev/sgx/enclave, but that seems a bit ridiculous.
I agree.
>
> Passing both would allow tying EXECMOD to /dev/sgx/enclave as Cedric
> wanted (without having to play games and pass /dev/sgx/enclave to
> security_enclave_load()), but I don't think there's anything fundamentally
> broken with using .sigstruct for EXECMOD. It requires more verbose
> labeling, but that's not a bad thing.
The benefit of putting it on .sigstruct is that it can be per-enclave.
As I understand it from Fedora packaging, the way this works on distros is generally that a package will include some files and their associated labels, and, if the package needs EXECMOD, then the files are labeled with EXECMOD and the author of the relevant code might get a dirty look.
This could translate to the author of an exclave that needs RWX regions getting a dirty look without leaking this permission into other enclaves.
(In my opinion, the dirty looks are actually the best security benefit of the entire concept of LSMs making RWX difficult. A sufficiently creative attacker can almost always bypass W^X restrictions once they’ve pwned you, but W^X makes it harder to pwn you in the first place, and SELinux makes it really obvious when packaging a program that doesn’t respect W^X. The upshot is that a lot of programs got fixed.)
^ permalink raw reply
* [PATCH 1/1 v2] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-05-24 23:04 UTC (permalink / raw)
To: linux-security-module, linux-kernel
Cc: agk, snitzer, dm-devel, jmorris, scottsh
In-Reply-To: <20190524230411.9238-1-jaskarankhurana@linux.microsoft.com>
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.
The hash is added as a key of type "user" and the description is passed to
the kernel so it can look it up and use it for verification.
Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
against the roothash signature file *if* specified, if signature file is
specified verification must succeed prior to creation of device mapper
block device.
Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
specified for all dm verity volumes and verification must succeed prior
to creation of device mapper block device.
Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
drivers/md/Kconfig | 23 +++++
drivers/md/Makefile | 2 +-
drivers/md/dm-verity-target.c | 34 +++++++-
drivers/md/dm-verity-verify-sig.c | 137 ++++++++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 31 +++++++
5 files changed, 222 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..da4115753f25 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -489,6 +489,29 @@ config DM_VERITY
If unsure, say N.
+config DM_VERITY_VERIFY_ROOTHASH_SIG
+ def_bool n
+ bool "Verity data device root hash signature verification support"
+ depends on DM_VERITY
+ select SYSTEM_DATA_VERIFICATION
+ help
+ The device mapper target created by DM-VERITY can be validated if the
+ pre-generated tree of cryptographic checksums passed has a pkcs#7
+ signature file that can validate the roothash of the tree.
+
+ If unsure, say N.
+
+config DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE
+ def_bool n
+ bool "Forces all dm verity data device root hash should be signed"
+ depends on DM_VERITY_VERIFY_ROOTHASH_SIG
+ help
+ The device mapper target created by DM-VERITY will succeed only if the
+ pre-generated tree of cryptographic checksums passed also has a pkcs#7
+ signature file that can validate the roothash of the tree.
+
+ If unsure, say N.
+
config DM_VERITY_FEC
bool "Verity forward error correction support"
depends on DM_VERITY
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..8a8c142bcfe1 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE) += dm-log-userspace.o
obj-$(CONFIG_DM_ZERO) += dm-zero.o
obj-$(CONFIG_DM_RAID) += dm-raid.o
obj-$(CONFIG_DM_THIN_PROVISIONING) += dm-thin-pool.o
-obj-$(CONFIG_DM_VERITY) += dm-verity.o
+obj-$(CONFIG_DM_VERITY) += dm-verity.o dm-verity-verify-sig.o
obj-$(CONFIG_DM_CACHE) += dm-cache.o
obj-$(CONFIG_DM_CACHE_SMQ) += dm-cache-smq.o
obj-$(CONFIG_DM_ERA) += dm-era.o
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..56276669ac20 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
#include "dm-verity.h"
#include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
#include <linux/module.h>
#include <linux/reboot.h>
@@ -34,7 +34,8 @@
#define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
#define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
return r;
}
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *verify_args)
{
int r;
unsigned argc;
@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
if (r)
return r;
continue;
+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
+ r = verity_verify_sig_parse_opt_args(as, v,
+ verify_args,
+ &argc, arg_name);
+ if (r)
+ return r;
+ continue;
+
}
ti->error = "Unrecognized verity feature request";
@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
{
struct dm_verity *v;
+ struct dm_verity_sig_opts verify_args = {0};
struct dm_arg_set as;
unsigned int num;
unsigned long long num_ll;
@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
int i;
sector_t hash_position;
char dummy;
+ char *root_hash_digest_to_validate = NULL;
v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
if (!v) {
@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
r = -EINVAL;
goto bad;
}
+ root_hash_digest_to_validate = argv[8];
if (strcmp(argv[9], "-")) {
v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
as.argc = argc;
as.argv = argv;
- r = verity_parse_opt_args(&as, v);
+ r = verity_parse_opt_args(&as, v, &verify_args);
if (r < 0)
goto bad;
}
+ /* Root hash signature is a optional parameter*/
+ r = verity_verify_root_hash(root_hash_digest_to_validate,
+ strlen(root_hash_digest_to_validate),
+ verify_args.sig,
+ verify_args.sig_size);
+ if (r < 0) {
+ ti->error = "Root hash verification failed";
+ goto bad;
+ }
v->hash_per_block_bits =
__fls((1 << v->hash_dev_block_bits) / v->digest_size);
@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->per_io_data_size = roundup(ti->per_io_data_size,
__alignof__(struct dm_verity_io));
+ verity_verify_sig_opts_cleanup(&verify_args);
+
return 0;
bad:
+
+ verity_verify_sig_opts_cleanup(&verify_args);
verity_dtr(ti);
return r;
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..ba87c9342d55
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ * This file is released under the GPLv2.
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return (!strcasecmp(arg_name,
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+EXPORT_SYMBOL_GPL(verity_verify_is_sig_opt_arg);
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+ struct dm_verity_sig_opts *sig_opts)
+{
+ struct key *key;
+ const struct user_key_payload *ukp;
+ int ret = 0;
+
+ key = request_key(&key_type_user,
+ key_desc, NULL);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
+
+ down_read(&key->sem);
+
+ ukp = user_key_payload_locked(key);
+ if (!ukp) {
+ ret = -EKEYREVOKED;
+ goto end;
+ }
+
+ sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+ if (!sig_opts->sig) {
+ ret = -ENOMEM;
+ goto end;
+ }
+ sig_opts->sig_size = ukp->datalen;
+
+ memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+ up_read(&key->sem);
+ key_put(key);
+
+ return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+ struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc,
+ const char *arg_name)
+{
+ struct dm_target *ti = v->ti;
+ int ret = 0;
+ const char *sig_key = NULL;
+
+ if (!*argc) {
+ ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+ return -EINVAL;
+ }
+
+ sig_key = dm_shift_arg(as);
+ (*argc)--;
+
+ if (!IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG))
+ return 0;
+
+ ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+ if (ret < 0)
+ ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(verity_verify_sig_parse_opt_args);
+
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ * using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ int ret;
+
+ if (!root_hash || root_hash_len == 0)
+ return -EINVAL;
+
+ if (!sig_data || sig_len == 0) {
+ if (IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE))
+ return -EINVAL;
+ else
+ return 0;
+ }
+
+ ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+ sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+ NULL, NULL);
+
+ return ret;
+}
+#else
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ return 0;
+}
+#endif
+EXPORT_SYMBOL_GPL(verity_verify_root_hash);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+ kfree(sig_opts->sig);
+ sig_opts->sig = NULL;
+ sig_opts->sig_size = 0;
+}
+EXPORT_SYMBOL_GPL(verity_verify_sig_opts_cleanup);
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..4cdb5eeb90d4
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ * This file is released under the GPLv2.
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+struct dm_verity_sig_opts {
+ unsigned int sig_size;
+ u8 *sig;
+};
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len);
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
--
2.17.1
^ permalink raw reply related
* [PATCH 0/1 v2] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-05-24 23:04 UTC (permalink / raw)
To: linux-security-module, linux-kernel
Cc: agk, snitzer, dm-devel, jmorris, scottsh
This patch set adds in-kernel pkcs7 signature checking for the roothash of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
Why we are doing validation in the Kernel?
The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so we
can trust it.
What about attacker mounting non dm-verity volumes to run executable
code?
This verification can be used to have a security architecture where a LSM
can enforce this verification for all the volumes and by doing this it can
ensure that all executable code runs from signed and trusted dm-verity
volumes.
Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.
How are these changes tested?
veritysetup part of cryptsetup library was modified to take a optional
root-hash-sig parameter.
Commandline used to test the changes:
veritysetup open <data_device> <name> <hash_device> <root_hash>
--root-hash-sig=<root_hash_pkcs7_detached_sig>
The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig
Changelog:
v2:
- Code review feedback to pass the signature binary blob as a key that can be
looked up in the kernel and be used to verify the roothash.
[Suggested by Milan Broz]
- Made the code related change suggested in review of v1.
[Suggested by Balbir Singh]
v1:
- Add kconfigs to control dm-verity root has signature verification and
use the signature if specified to verify the root hash.
Jaskaran Khurana (1):
Adds in-kernel pkcs7 sig checking the roothash of the dm-verity hash
tree
drivers/md/Kconfig | 23 +++++
drivers/md/Makefile | 2 +-
drivers/md/dm-verity-target.c | 34 +++++++-
drivers/md/dm-verity-verify-sig.c | 137 ++++++++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 31 +++++++
5 files changed, 222 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
--
2.17.1
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 22:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Xing, Cedric, Stephen Smalley, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrUyAAhnQ+RUeN1L41TKj-vcD2CNt-FJ9siO=Zo6gvH1Aw@mail.gmail.com>
On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> > > >
> > > > If "initial permissions" for enclaves are less restrictive than shared
> > > > objects, then it'd become a backdoor for circumventing LSM when enclave
> > > > whitelisting is *not* in place. For example, an adversary may load a page,
> > > > which would otherwise never be executable, as an executable page in EPC.
> > > >
> > > > In the case a RWX page is needed, the calling process has to have a RWX
> > > > page serving as the source for EADD so PROCESS__EXECMEM will have been
> > > > checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM
> > > > on /dev/sgx/enclave, which I see as a security benefit because it only
> > > > affects the enclave but not the whole process hosting it.
> > >
> > > So the permission would be like FILE__EXECMOD on the source enclave
> > > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > > MAP_SHARED, PROT_WRITE isn't going to work because that means you can
> > > modify the file.
> >
> > Was this in response to Cedric's comment, or to my comment?
>
> Yours. I think that requiring source pages to be actually mapped W is
> not such a great idea.
I wasn't requiring source pages to be mapped W. At least I didn't intend
to require W. What I was trying to say is that SGX could trigger an
EXECMEM check if userspace attempted to EADD or EAUG an enclave page with
RWX permissions, e.g.:
if ((SECINFO.PERMS & RWX) == RWX) {
ret = security_mmap_file(NULL, RWX, ???);
if (ret)
return ret;
}
But that's a moot point if we add security_enclave_load() or whatever.
>
> >
> > > I'm starting to think that looking at the source VMA permission bits
> > > or source PTE permission bits is putting a bit too much policy into
> > > the driver as opposed to the LSM. How about delegating the whole
> > > thing to an LSM hook? The EADD operation would invoke a new hook,
> > > something like:
> > >
> > > int security_enclave_load_bytes(void *source_addr, struct
> > > vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > > maxperm);
> > >
> > > Then you don't have to muck with mapping anything PROT_EXEC. Instead
> > > you load from a mapping of a file and the LSM applies whatever policy
> > > it feels appropriate. If the first pass gets something wrong, the
> > > application or library authors can take it up with the SELinux folks
> > > without breaking the whole ABI :)
> > >
> > > (I'm proposing passing in the source_vma because this hook would be
> > > called with mmap_sem held for read to avoid a TOCTOU race.)
> > >
> > > If we go this route, the only substantial change to the existing
> > > driver that's needed for an initial upstream merge is the maxperm
> > > mechanism and whatever hopefully minimal API changes are needed to
> > > allow users to conveniently set up the mappings. And we don't need to
> > > worry about how to hack around mprotect() calling into the LSM,
> > > because the LSM will actually be aware of SGX and can just do the
> > > right thing.
> >
> > This doesn't address restricting which processes can run which enclaves,
> > it only allows restricting the build flow. Or are you suggesting this
> > be done in addition to whitelisting sigstructs?
>
> In addition.
>
> But I named the function badly and gave it a bad signature, which
> confused you. Let's try again:
>
> int security_enclave_load_from_memory(const struct vm_area_struct
> *source, unsigned int maxperm);
I prefer security_enclave_load(), "from_memory" seems redundant at best.
> Maybe some really fancy future LSM would also want loff_t
> source_offset, but it's probably not terribly useful. This same
> callback would be used for EAUG.
>
> Following up on your discussion with Cedric about sigstruct, the other
> callback would be something like:
>
> int security_enclave_init(struct file *sigstruct_file);
>
> The main issue I see is that we also want to control the enclave's
> ability to have RWX pages or to change a W page to X. We might also
> want:
>
> int security_enclave_load_zeros(unsigned int maxperm);
What's the use case for this? @maxperm will always be at least RW in
this case, otherwise the page is useless to the enclave, and if the
enclave can write the page, the fact that it started as zeros is
irrelevant.
> An enclave that's going to modify its own code will need memory with
> maxperm = RWX or WX.
>
> But this is a bit awkward if the LSM's decision depends on the
> sigstruct. We could get fancy and require that the sigstruct be
> supplied before any EADD operations so that the maxperm decisions can
> depend on the sigstruct.
>
> Am I making more sense now?
Yep. Requiring .sigstruct at ECREATE would be trivial. If we wanted
flexibility we could do:
int security_enclave_load(struct file *file, struct vm_area_struct *vma,
unsigned long prot);
And for ultimate flexibility we could pass both .sigstruct and the file
pointer for /dev/sgx/enclave, but that seems a bit ridiculous.
Passing both would allow tying EXECMOD to /dev/sgx/enclave as Cedric
wanted (without having to play games and pass /dev/sgx/enclave to
security_enclave_load()), but I don't think there's anything fundamentally
broken with using .sigstruct for EXECMOD. It requires more verbose
labeling, but that's not a bad thing.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 21:27 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Xing, Cedric, Stephen Smalley, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524200333.GF365@linux.intel.com>
On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> > >
> > > If "initial permissions" for enclaves are less restrictive than shared
> > > objects, then it'd become a backdoor for circumventing LSM when enclave
> > > whitelisting is *not* in place. For example, an adversary may load a page,
> > > which would otherwise never be executable, as an executable page in EPC.
> > >
> > > In the case a RWX page is needed, the calling process has to have a RWX
> > > page serving as the source for EADD so PROCESS__EXECMEM will have been
> > > checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM
> > > on /dev/sgx/enclave, which I see as a security benefit because it only
> > > affects the enclave but not the whole process hosting it.
> >
> > So the permission would be like FILE__EXECMOD on the source enclave
> > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > MAP_SHARED, PROT_WRITE isn't going to work because that means you can
> > modify the file.
>
> Was this in response to Cedric's comment, or to my comment?
Yours. I think that requiring source pages to be actually mapped W is
not such a great idea.
>
> > I'm starting to think that looking at the source VMA permission bits
> > or source PTE permission bits is putting a bit too much policy into
> > the driver as opposed to the LSM. How about delegating the whole
> > thing to an LSM hook? The EADD operation would invoke a new hook,
> > something like:
> >
> > int security_enclave_load_bytes(void *source_addr, struct
> > vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > maxperm);
> >
> > Then you don't have to muck with mapping anything PROT_EXEC. Instead
> > you load from a mapping of a file and the LSM applies whatever policy
> > it feels appropriate. If the first pass gets something wrong, the
> > application or library authors can take it up with the SELinux folks
> > without breaking the whole ABI :)
> >
> > (I'm proposing passing in the source_vma because this hook would be
> > called with mmap_sem held for read to avoid a TOCTOU race.)
> >
> > If we go this route, the only substantial change to the existing
> > driver that's needed for an initial upstream merge is the maxperm
> > mechanism and whatever hopefully minimal API changes are needed to
> > allow users to conveniently set up the mappings. And we don't need to
> > worry about how to hack around mprotect() calling into the LSM,
> > because the LSM will actually be aware of SGX and can just do the
> > right thing.
>
> This doesn't address restricting which processes can run which enclaves,
> it only allows restricting the build flow. Or are you suggesting this
> be done in addition to whitelisting sigstructs?
In addition.
But I named the function badly and gave it a bad signature, which
confused you. Let's try again:
int security_enclave_load_from_memory(const struct vm_area_struct
*source, unsigned int maxperm);
Maybe some really fancy future LSM would also want loff_t
source_offset, but it's probably not terribly useful. This same
callback would be used for EAUG.
Following up on your discussion with Cedric about sigstruct, the other
callback would be something like:
int security_enclave_init(struct file *sigstruct_file);
The main issue I see is that we also want to control the enclave's
ability to have RWX pages or to change a W page to X. We might also
want:
int security_enclave_load_zeros(unsigned int maxperm);
An enclave that's going to modify its own code will need memory with
maxperm = RWX or WX.
But this is a bit awkward if the LSM's decision depends on the
sigstruct. We could get fancy and require that the sigstruct be
supplied before any EADD operations so that the maxperm decisions can
depend on the sigstruct.
Am I making more sense now?
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 21:11 UTC (permalink / raw)
To: Xing, Cedric
Cc: Stephen Smalley, Andy Lutomirski, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E8FC2@ORSMSX116.amr.corp.intel.com>
On Fri, May 24, 2019 at 01:42:13PM -0700, Xing, Cedric wrote:
> > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > owner@vger.kernel.org] On Behalf Of Sean Christopherson
> > Sent: Friday, May 24, 2019 12:14 PM
> >
> > My point is that enclaves have different properties than shared objects.
> >
> > Normal LSM behavior with regard to executing files is to label files
> > with e.g. FILE__EXECUTE. Because an enclave must be built to the exact
> > specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct
> > is effectively the same as requiring FILE__EXECUTE on the enclave itself.
> >
> > Addressing your scenario of loading an executable page in EPC, doing so
> > would require one of the following:
> >
> > - Ability to install a .sigstruct with FILE__EXECUTE
> >
> > - PROCESS__EXECMEM
> >
> > - FILE__EXECMOD and SGX2 support
>
> Now I got your point. It sounds a great idea to me!
>
> But instead of using .sigstruct file, I'd still recommend using file mapping
> (i.e. SIGSTRUCT needs to reside in executable memory). But then there'll be a
Why? Even in the Graphene case the final .sigstruct can be known ahead of
time. Userspace can always use memfd() if it's generating SIGSTRUCT on
the fly.
> hole - a process having FILE__EXECMOD on any file could use that file as a
> SIGSTRUCT. Probably we'll need a new type in SELinux to label
> enclave/sigstruct files.
>
> > I don't see a fundamental difference between having RWX in an enclave
> > and RWX in normal memory, either way the process can execute arbitrary
> > code, i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on
> > certain instructions, but that's easily sidestepped by having a
> > trampoline in the host (marked RX) and piping arbitrary code into the
> > enclave. Or using EEXIT to do a bit of ROP.
>
> I'm with you.
>
> With your proposal only FILE__EXECMOD is needed on /dev/sgx/enclave to launch
> Graphene enclaves or the like.
It wouldn't even need FILE__EXECMOD, assuming Graphene does all of its
libc rewriting before building the enclave, i.e. doesn't EADD RWX pages.
> > > > > No changes are required to LSMs, SGX1 has a single LSM touchpoint
> > > > > in
> > > > its
> > > > > mmap(), and I *think* the only required userspace change is to
> > > > > mmap() PROT_NONE when allocating the enclave's virtual address
> > range.
> > >
> > > I'm not sure I understand the motivation behind this proposal to
> > > decouple initial EPC permissions from source pages.
> >
> > Pulling permissions from source pages means userspace needs to fully map
> > the in normal memory, including marking pages executable. That exposes
> > the loader to having executable pages in its address space that it has
> > no intention of executing (outside of the enclave). And for Graphene,
> > it means having to actively avoid PROCESS__EXECMEM, e.g. by using a
> > dummy backing file to build the enclave instead of anon memory.
>
> Agreed.
>
> >
> > > I don't think it a big deal to fully mmap() enclave files, which have
> > > to be parsed by user mode anyway to determine various things including
> > > but not limited to the size of heap(s), size and number of
> > > TCSs/stacks/TLS areas, and the overall enclave size. So with PHDRs
> > > parsed, it's trivial to mmap() each segment with permissions from its
> > PHDR.
> > >
> > > > > As for Graphene, it doesn't need extra permissions to run its
> > > > > enclaves, it just needs a way to install .sigstruct, which is a
> > > > > generic permissions problem and not SGX specific.
> > > > >
> > > > >
> > > > > For SGX2 maybe:
> > > > >
> > > > > - No additional requirements to map an EAUG'd page as RW page.
> > Not
> > > > > aligned with standard MAP_SHARED behavior, but we really don't
> > want
> > > > > to require FILE__WRITE, and thus allow writes to .sigstruct.
> > > > >
> > > > > - Require FILE__EXECMOD on the .sigstruct to map previously
> > writable
> > > > > page as executable (which indirectly includes all EAUG'd
> > pages).
> > > > > Wiring this up will be a little funky, but we again we don't
> > want
> > > > > to require FILE__WRITE on .sigstruct.
> > > > >
> > >
> > > I'm lost. Why is EAUG tied to permissions on .sigstruct?
> >
> > Because for the purposes of LSM checks, .sigstruct is the enclave's
> > backing file, and mapping a previously writable enclave page as
> > exectuable is roughly equivalent to mapping a CoW'd page as exectuable.
>
> I think I've got your idea. You are trying to use permissions on .sigstruct
> to determine whether EAUG will be available to that specific enclave. Am I
> right?
Yep.
> I'd tie EAUG to the permissions of /dev/sgx/enclave instead. But why? There
> are couple of reasons. For one, a SIGSTRUCT identifies the behavior of the
> enclave, hence the SGX features needed by that enclave. So if an enclave
> requires EAUG, the .sigstruct has to allow EAUG or the enclave wouldn't work.
> That means the system admin wouldn't have a choice but to match up what's
> needed by the enclave. For two, whether to allow, say loading code
> dynamically into an enclave, depends on whether the host process can tolerate
> the inherent risk. And that decision is seldom made on individual enclaves
> but to the host process as a whole. And /dev/sgx/enclave serves that purpose.
I think I'd be ok either way? What I really care about is having line of
sight to a sane way to support for SGX2, and both seem sane. I.e. we can
hash this detail out when SGX2 goes in.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-24 20:58 UTC (permalink / raw)
To: Christopherson, Sean J, Andy Lutomirski
Cc: Stephen Smalley, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190524200333.GF365@linux.intel.com>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Sean Christopherson
> Sent: Friday, May 24, 2019 1:04 PM
>
> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> > >
> > > If "initial permissions" for enclaves are less restrictive than
> > > shared objects, then it'd become a backdoor for circumventing LSM
> > > when enclave whitelisting is *not* in place. For example, an
> > > adversary may load a page, which would otherwise never be executable,
> as an executable page in EPC.
> > >
> > > In the case a RWX page is needed, the calling process has to have a
> > > RWX page serving as the source for EADD so PROCESS__EXECMEM will
> > > have been checked. For SGX2, changing an EPC page to RWX is subject
> > > to FILE__EXECMEM on /dev/sgx/enclave, which I see as a security
> > > benefit because it only affects the enclave but not the whole
> process hosting it.
> >
> > So the permission would be like FILE__EXECMOD on the source enclave
> > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > MAP_SHARED, PROT_WRITE isn't going to work because that means you can
> > modify the file.
>
> Was this in response to Cedric's comment, or to my comment?
Creating RWX source page requires PROCESS_EXECMEM. But as I responded to Sean earlier, I think his proposal of "aggregating" all "initial" permission checks into a single SIGSTRUCT check is probably a better approach.
>
> > I'm starting to think that looking at the source VMA permission bits
> > or source PTE permission bits is putting a bit too much policy into
> > the driver as opposed to the LSM. How about delegating the whole
> > thing to an LSM hook? The EADD operation would invoke a new hook,
> > something like:
> >
> > int security_enclave_load_bytes(void *source_addr, struct
> > vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > maxperm);
This is exactly what I was thinking. But with Sean's proposal this is probably no longer necessary.
> >
> > Then you don't have to muck with mapping anything PROT_EXEC. Instead
> > you load from a mapping of a file and the LSM applies whatever policy
> > it feels appropriate. If the first pass gets something wrong, the
> > application or library authors can take it up with the SELinux folks
> > without breaking the whole ABI :)
> >
> > (I'm proposing passing in the source_vma because this hook would be
> > called with mmap_sem held for read to avoid a TOCTOU race.)
> >
> > If we go this route, the only substantial change to the existing
> > driver that's needed for an initial upstream merge is the maxperm
> > mechanism and whatever hopefully minimal API changes are needed to
> > allow users to conveniently set up the mappings. And we don't need to
> > worry about how to hack around mprotect() calling into the LSM,
> > because the LSM will actually be aware of SGX and can just do the
> > right thing.
>
> This doesn't address restricting which processes can run which enclaves,
> it only allows restricting the build flow. Or are you suggesting this
> be done in addition to whitelisting sigstructs?
In the context of SELinux, new types could be defined to be associated with SIGSTRUCT (or more precisely, files containing SIGSTRUCTs). Then the LSM hook (I'd propose security_sgx_initialize_enclave) could enforce whatever...
>
> What's the value prop beyond whitelisting sigstructs? Realistically, I
> doubt LSMs/users will want to take the performance hit of scanning the
> source bytes every time an enclave is loaded.
>
> We could add seomthing like security_enclave_mprotect() in lieu of
> abusing security_file_mprotect(), but passing the full source bytes
> seems a bit much.
I'd just use /dev/sgx/enclave to govern "runtime" permissions any EPC page can mmap()/mprotect() to. Then we won't need any code changes in LSM.
-Cedric
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-24 20:42 UTC (permalink / raw)
To: Christopherson, Sean J
Cc: Stephen Smalley, Andy Lutomirski, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524191344.GD365@linux.intel.com>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Sean Christopherson
> Sent: Friday, May 24, 2019 12:14 PM
>
> My point is that enclaves have different properties than shared objects.
>
> Normal LSM behavior with regard to executing files is to label files
> with e.g. FILE__EXECUTE. Because an enclave must be built to the exact
> specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct
> is effectively the same as requiring FILE__EXECUTE on the enclave itself.
>
> Addressing your scenario of loading an executable page in EPC, doing so
> would require one of the following:
>
> - Ability to install a .sigstruct with FILE__EXECUTE
>
> - PROCESS__EXECMEM
>
> - FILE__EXECMOD and SGX2 support
Now I got your point. It sounds a great idea to me!
But instead of using .sigstruct file, I'd still recommend using file mapping (i.e. SIGSTRUCT needs to reside in executable memory). But then there'll be a hole - a process having FILE__EXECMOD on any file could use that file as a SIGSTRUCT. Probably we'll need a new type in SELinux to label enclave/sigstruct files.
>
> > In the case a RWX page is needed, the calling process has to have a
> > RWX page serving as the source for EADD so PROCESS__EXECMEM will have
> been checked.
> > For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on
> > /dev/sgx/enclave, which I see as a security benefit because it only
> > affects the enclave but not the whole process hosting it.
>
> There is no FILE__EXECMEM check, only PROCESS__EXECMEM and FILE__EXECMOD.
> I assume you're referring to the latter?
Yes.
>
> I don't see a fundamental difference between having RWX in an enclave
> and RWX in normal memory, either way the process can execute arbitrary
> code, i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on
> certain instructions, but that's easily sidestepped by having a
> trampoline in the host (marked RX) and piping arbitrary code into the
> enclave. Or using EEXIT to do a bit of ROP.
I'm with you.
With your proposal only FILE__EXECMOD is needed on /dev/sgx/enclave to launch Graphene enclaves or the like.
>
> > > > No changes are required to LSMs, SGX1 has a single LSM touchpoint
> > > > in
> > > its
> > > > mmap(), and I *think* the only required userspace change is to
> > > > mmap() PROT_NONE when allocating the enclave's virtual address
> range.
> >
> > I'm not sure I understand the motivation behind this proposal to
> > decouple initial EPC permissions from source pages.
>
> Pulling permissions from source pages means userspace needs to fully map
> the in normal memory, including marking pages executable. That exposes
> the loader to having executable pages in its address space that it has
> no intention of executing (outside of the enclave). And for Graphene,
> it means having to actively avoid PROCESS__EXECMEM, e.g. by using a
> dummy backing file to build the enclave instead of anon memory.
Agreed.
>
> > I don't think it a big deal to fully mmap() enclave files, which have
> > to be parsed by user mode anyway to determine various things including
> > but not limited to the size of heap(s), size and number of
> > TCSs/stacks/TLS areas, and the overall enclave size. So with PHDRs
> > parsed, it's trivial to mmap() each segment with permissions from its
> PHDR.
> >
> > > > As for Graphene, it doesn't need extra permissions to run its
> > > > enclaves, it just needs a way to install .sigstruct, which is a
> > > > generic permissions problem and not SGX specific.
> > > >
> > > >
> > > > For SGX2 maybe:
> > > >
> > > > - No additional requirements to map an EAUG'd page as RW page.
> Not
> > > > aligned with standard MAP_SHARED behavior, but we really don't
> want
> > > > to require FILE__WRITE, and thus allow writes to .sigstruct.
> > > >
> > > > - Require FILE__EXECMOD on the .sigstruct to map previously
> writable
> > > > page as executable (which indirectly includes all EAUG'd
> pages).
> > > > Wiring this up will be a little funky, but we again we don't
> want
> > > > to require FILE__WRITE on .sigstruct.
> > > >
> >
> > I'm lost. Why is EAUG tied to permissions on .sigstruct?
>
> Because for the purposes of LSM checks, .sigstruct is the enclave's
> backing file, and mapping a previously writable enclave page as
> exectuable is roughly equivalent to mapping a CoW'd page as exectuable.
I think I've got your idea. You are trying to use permissions on .sigstruct to determine whether EAUG will be available to that specific enclave. Am I right?
I'd tie EAUG to the permissions of /dev/sgx/enclave instead. But why? There are couple of reasons. For one, a SIGSTRUCT identifies the behavior of the enclave, hence the SGX features needed by that enclave. So if an enclave requires EAUG, the .sigstruct has to allow EAUG or the enclave wouldn't work. That means the system admin wouldn't have a choice but to match up what's needed by the enclave. For two, whether to allow, say loading code dynamically into an enclave, depends on whether the host process can tolerate the inherent risk. And that decision is seldom made on individual enclaves but to the host process as a whole. And /dev/sgx/enclave serves that purpose.
-Cedric
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 20:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Xing, Cedric, Stephen Smalley, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrUw5sEr-MHPMU4CzEzkrejDs-JOThHB9Buhoxo5-rdpRw@mail.gmail.com>
On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> >
> > If "initial permissions" for enclaves are less restrictive than shared
> > objects, then it'd become a backdoor for circumventing LSM when enclave
> > whitelisting is *not* in place. For example, an adversary may load a page,
> > which would otherwise never be executable, as an executable page in EPC.
> >
> > In the case a RWX page is needed, the calling process has to have a RWX
> > page serving as the source for EADD so PROCESS__EXECMEM will have been
> > checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM
> > on /dev/sgx/enclave, which I see as a security benefit because it only
> > affects the enclave but not the whole process hosting it.
>
> So the permission would be like FILE__EXECMOD on the source enclave
> page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> MAP_SHARED, PROT_WRITE isn't going to work because that means you can
> modify the file.
Was this in response to Cedric's comment, or to my comment?
> I'm starting to think that looking at the source VMA permission bits
> or source PTE permission bits is putting a bit too much policy into
> the driver as opposed to the LSM. How about delegating the whole
> thing to an LSM hook? The EADD operation would invoke a new hook,
> something like:
>
> int security_enclave_load_bytes(void *source_addr, struct
> vm_area_struct *source_vma, loff_t source_offset, unsigned int
> maxperm);
>
> Then you don't have to muck with mapping anything PROT_EXEC. Instead
> you load from a mapping of a file and the LSM applies whatever policy
> it feels appropriate. If the first pass gets something wrong, the
> application or library authors can take it up with the SELinux folks
> without breaking the whole ABI :)
>
> (I'm proposing passing in the source_vma because this hook would be
> called with mmap_sem held for read to avoid a TOCTOU race.)
>
> If we go this route, the only substantial change to the existing
> driver that's needed for an initial upstream merge is the maxperm
> mechanism and whatever hopefully minimal API changes are needed to
> allow users to conveniently set up the mappings. And we don't need to
> worry about how to hack around mprotect() calling into the LSM,
> because the LSM will actually be aware of SGX and can just do the
> right thing.
This doesn't address restricting which processes can run which enclaves,
it only allows restricting the build flow. Or are you suggesting this
be done in addition to whitelisting sigstructs?
What's the value prop beyond whitelisting sigstructs? Realistically, I
doubt LSMs/users will want to take the performance hit of scanning the
source bytes every time an enclave is loaded.
We could add seomthing like security_enclave_mprotect() in lieu of abusing
security_file_mprotect(), but passing the full source bytes seems a bit
much.
^ permalink raw reply
* Re: Stacking patches for 5.3
From: Casey Schaufler @ 2019-05-24 19:55 UTC (permalink / raw)
To: Paul Moore
Cc: Linux Security Module list, Kees Cook, James Morris,
John Johansen
In-Reply-To: <CAHC9VhRY8UOS9G_CgBPe=tVYgLmL7KHC_=RRtHyemHrx0_RqjA@mail.gmail.com>
On 5/24/2019 12:37 PM, Paul Moore wrote:
> On Fri, May 24, 2019 at 3:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> I have a set of 59 patches for the nest round of LSM stacking.
>> They are all really small. Kees has expressed a preference for
>> small patches. I think that the collapsed set of about 10 would
>> make more sense. Which would the community as a whole prefer?
>> The patches are not much different from what has been presented
>> already.
> Is there an option for "not posted on the Friday before a holiday weekend"? ;)
I'm waiting for James' next-general to base it on, so you needn't worry.
> I honestly don't care too much about patch size, regardless of how it
> is packaged it's a lot of pretty critical stuff to review.
Consensus looks like small patches are the way to go. So be it.
^ permalink raw reply
* Re: [PATCH 1/7] keys: sparse: Fix key_fs[ug]id_changed()
From: James Morris @ 2019-05-24 19:38 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856409010.10428.11692778420533145488.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
> accesses of tsk->cred, which is marked __rcu.
>
> Fix this by passing the new cred struct to these functions from
> commit_creds() rather than the task pointer.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 19:37 UTC (permalink / raw)
To: Xing, Cedric
Cc: Christopherson, Sean J, Stephen Smalley, Andy Lutomirski,
Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E8E1D@ORSMSX116.amr.corp.intel.com>
On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > owner@vger.kernel.org] On Behalf Of Sean Christopherson
> > Sent: Friday, May 24, 2019 10:55 AM
> >
> > On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:
> > > Hmm, I've been thinking more about pulling permissions from the source
> > > page. Conceptually I'm not sure we need to meet the same requirements
> > as
> > > non-enclave DSOs while the enclave is being built, i.e. do we really
> > need
> > > to force userspace to fully map the enclave in normal memory?
> > >
> > > Consider the Graphene scenario where it's building an enclave on the
> > fly.
> > > Pulling permissions from the source VMAs means Graphene has to map the
> > > code pages of the enclave with X. This means Graphene will need
> > EXEDMOD
> > > (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this
> > makes
> > > perfect sense since there is no way to verify the end result of RW->RX.
> > >
> > > But for SGX, assuming enclaves are whitelisted by their sigstruct
> > (checked
> > > at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it
> > *is*
> > > possible to verify the resulting RX contents. E.g. for the purposes
> > of
> > > LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> > > require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
> > >
> > > Stephen, is my logic sound?
> > >
> > >
> > > If so...
> > >
> > > - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the
> > enclave.
> > >
> > > - Prevent userspace from mapping the enclave with permissions beyond
> > the
> > > original permissions of the enclave. This can be done by
> > populating
> > > VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as
> > Andy's
> > > proposals). E.g. pre-EINIT, mmap() and mprotect() can only
> > succeed
> > > with PROT_NONE.
> > >
> > > - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for
> > simplicity,
> > > or provide an alternate SGX_IOC_MPROTECT if we want to sidestep
> > the
> > > FILE__WRITE requirement.
> >
> > One more thought. EADD (and the equivalent SGX2 flow) could do
> > security_mmap_file() with a NULL file on the SECINFO permissions, which
> > would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.
>
> If "initial permissions" for enclaves are less restrictive than shared objects, then it'd become a backdoor for circumventing LSM when enclave whitelisting is *not* in place. For example, an adversary may load a page, which would otherwise never be executable, as an executable page in EPC.
>
> In the case a RWX page is needed, the calling process has to have a RWX page serving as the source for EADD so PROCESS__EXECMEM will have been checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on /dev/sgx/enclave, which I see as a security benefit because it only affects the enclave but not the whole process hosting it.
So the permission would be like FILE__EXECMOD on the source enclave
page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
MAP_SHARED, PROT_WRITE isn't going to work because that means you can
modify the file.
I'm starting to think that looking at the source VMA permission bits
or source PTE permission bits is putting a bit too much policy into
the driver as opposed to the LSM. How about delegating the whole
thing to an LSM hook? The EADD operation would invoke a new hook,
something like:
int security_enclave_load_bytes(void *source_addr, struct
vm_area_struct *source_vma, loff_t source_offset, unsigned int
maxperm);
Then you don't have to muck with mapping anything PROT_EXEC. Instead
you load from a mapping of a file and the LSM applies whatever policy
it feels appropriate. If the first pass gets something wrong, the
application or library authors can take it up with the SELinux folks
without breaking the whole ABI :)
(I'm proposing passing in the source_vma because this hook would be
called with mmap_sem held for read to avoid a TOCTOU race.)
If we go this route, the only substantial change to the existing
driver that's needed for an initial upstream merge is the maxperm
mechanism and whatever hopefully minimal API changes are needed to
allow users to conveniently set up the mappings. And we don't need to
worry about how to hack around mprotect() calling into the LSM,
because the LSM will actually be aware of SGX and can just do the
right thing.
--Andy
^ permalink raw reply
* Re: Stacking patches for 5.3
From: Paul Moore @ 2019-05-24 19:37 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linux Security Module list, Kees Cook, James Morris,
John Johansen
In-Reply-To: <1106985e-1620-75b2-d6bf-c2c95d9a0fbb@schaufler-ca.com>
On Fri, May 24, 2019 at 3:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> I have a set of 59 patches for the nest round of LSM stacking.
> They are all really small. Kees has expressed a preference for
> small patches. I think that the collapsed set of about 10 would
> make more sense. Which would the community as a whole prefer?
> The patches are not much different from what has been presented
> already.
Is there an option for "not posted on the Friday before a holiday weekend"? ;)
I honestly don't care too much about patch size, regardless of how it
is packaged it's a lot of pretty critical stuff to review.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: Stacking patches for 5.3
From: James Morris @ 2019-05-24 19:36 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linux Security Module list, Kees Cook, John Johansen, Paul Moore
In-Reply-To: <1106985e-1620-75b2-d6bf-c2c95d9a0fbb@schaufler-ca.com>
On Fri, 24 May 2019, Casey Schaufler wrote:
> I have a set of 59 patches for the nest round of LSM stacking.
> They are all really small. Kees has expressed a preference for
> small patches. I think that the collapsed set of about 10 would
> make more sense. Which would the community as a whole prefer?
> The patches are not much different from what has been presented
> already.
I agree with Kees, smaller patches are easier to review.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 19:30 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Stephen Smalley, Andy Lutomirski, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524191344.GD365@linux.intel.com>
On Fri, May 24, 2019 at 12:13 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, May 24, 2019 at 11:34:32AM -0700, Xing, Cedric wrote:
> > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > > owner@vger.kernel.org] On Behalf Of Sean Christopherson
> > > Sent: Friday, May 24, 2019 10:55 AM
> I don't see a fundamental difference between having RWX in an enclave and
> RWX in normal memory, either way the process can execute arbitrary code,
> i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on certain
> instructions, but that's easily sidestepped by having a trampoline in the
> host (marked RX) and piping arbitrary code into the enclave. Or using
> EEXIT to do a bit of ROP.
There's a difference, albeit a somewhat weak one, if sigstructs are
whitelisted. FILE__EXECMOD on
either /dev/sgx/enclave or on the sigstruct is not an entirely crazy
way to express this.
^ permalink raw reply
* Stacking patches for 5.3
From: Casey Schaufler @ 2019-05-24 19:26 UTC (permalink / raw)
To: Linux Security Module list
Cc: Kees Cook, James Morris, John Johansen, Paul Moore, casey
I have a set of 59 patches for the nest round of LSM stacking.
They are all really small. Kees has expressed a preference for
small patches. I think that the collapsed set of about 10 would
make more sense. Which would the community as a whole prefer?
The patches are not much different from what has been presented
already.
^ permalink raw reply
* Re: [PATCH V7 0/4] Add support for crypto agile logs
From: James Morris @ 2019-05-24 19:22 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Matthew Garrett, linux-integrity, peterhuewe, jgg, roberto.sassu,
linux-efi, linux-security-module, linux-kernel, tweek, bsz,
Linus Torvalds
In-Reply-To: <20190524103846.GA11695@linux.intel.com>
On Fri, 24 May 2019, Jarkko Sakkinen wrote:
> I'm referring to these:
>
> https://lore.kernel.org/linux-integrity/20190329115544.GA27351@linux.intel.com/
>
> I got response from you that those were applied and there is another
> response in that thread that they are being sent to Linus. That is why I
> haven't done anything since. Most of them are critical fixes to v5.1
> changes.
These are in Linus' tree.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a556810d8e06aa2da8bbe22da3d105eb5a0d0c7d
I initially queued them in the next-tpm branch, but forgot to drop them
from there after sending them to Linus as a v5.1 fix. Linus was not happy
to see them again in the v5.2 merge window.
Apologies for the confusion.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: Re:
From: Mimi Zohar @ 2019-05-24 19:15 UTC (permalink / raw)
To: prakhar srivastava
Cc: Roberto Sassu, linux-integrity, linux-security-module,
linux-kernel, Matthew Garrett, vgoyal
In-Reply-To: <CAEFn8q+8QUs8iCP+Q3VujTkTU+XYYPGY8O8+KT1=6vvc-khJFg@mail.gmail.com>
On Fri, 2019-05-24 at 12:00 -0700, prakhar srivastava wrote:
> On Fri, May 24, 2019 at 11:09 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > > >> As mentioned, the first patch description should include a shell
> > > >> command for verifying the digest in the kexec boot command line
> > > >> measurement list record against /proc/cmdline. This patch description
> > > >> should include a shell command showing how to verify the digest based
> > > >> on the new field. Should the new field in the ascii measurement list
> > > >> be displayed as a string, not hex?
> > > >
> > > > We should define a new type. If the type is DATA_FMT_STRING, spaces are
> > > > replaced with '_'.
> > >
> > > Or better. Leave it as hex, otherwise there would be a parsing problem
> > > if there are spaces in the data for a field.
> >
> > After making a few changes, the measurement list contains the
> > following kexec-cmdline data:
> >
> > 10 edc32d1e3a5ba7272280a395b6fb56a5ef7c78c3 ima-buf
> > sha256:4f43b7db850e
> > 88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b45275
> > kexec-cmdline
> > 726f6f
> > 743d2f6465762f7364613420726f2072642e6c756b732e757569643d6c756b73
> > 2d6637
> > 3633643737632d653236622d343431642d613734652d62363633636334643832
> > 656120
> > 696d615f706f6c6963793d7463627c61707072616973655f746362
> >
> > There's probably a better shell command, but the following works to
> > verify the digest locally against the /proc/cmdline:
> >
> > $ echo -n -e `cat /proc/cmdline | sed 's/^.*root=/root=/'` | sha256sum
> > 4f43b7db850e88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b4527f65 -
> >
> > If we leave the "buf" field as ascii-hex, what would the shell command
> > look like when verifying the digest based on the "buf" field?
> >
> > Mimi
> >
> To quickly test the sha256 i used the my /proc/cmdline
> ro quiet splash vt.handoff=1 ima_policy=tcb ima_appraise=fix
> ima_template_fmt=n-ng|d-ng|sig|buf ima_hash=sha256
>
> export $VAL=
> 726f2071756965742073706c6173682076742e68616e646f66663d3120
> 696d615f706f6c6963793d74636220696d615f61707072616973653d666
> 97820696d615f74656d706c6174655f666d743d6e2d6e677c642d6e677c
> 7369677c62756620696d615f686173683d736861323536
>
> echo -n -e $VAL | xxd -r -p | sha256sum
> 0d0b891bb730120d9593799cba1a7b3febf68f2bb81fb1304b0c963f95f6bc58 -
>
> I will run it through the code as well, but the shell command should work.
Yes, that works.
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements
| grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox