* RE: [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits
From: Xing, Cedric @ 2019-07-01 19:22 UTC (permalink / raw)
To: Andy Lutomirski, Christopherson, Sean J
Cc: Jarkko Sakkinen, linux-sgx@vger.kernel.org, LSM List,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <CALCETrXtVzCZW4mb994prdrzXxMP2T=xxU+fhy5k9N8AqjcqhA@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 11:00 AM
>
> On Wed, Jun 19, 2019 at 3:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > static int sgx_mmap(struct file *file, struct vm_area_struct *vma) {
> > struct sgx_encl *encl = file->private_data;
> > + unsigned long allowed_rwx;
> > int ret;
> >
> > + allowed_rwx = sgx_allowed_rwx(encl, vma);
> > + if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) &
> ~allowed_rwx)
> > + return -EACCES;
> > +
> > ret = sgx_encl_mm_add(encl, vma->vm_mm);
> > if (ret)
> > return ret;
> >
> > + if (!(allowed_rwx & VM_READ))
> > + vma->vm_flags &= ~VM_MAYREAD;
> > + if (!(allowed_rwx & VM_WRITE))
> > + vma->vm_flags &= ~VM_MAYWRITE;
> > + if (!(allowed_rwx & VM_EXEC))
> > + vma->vm_flags &= ~VM_MAYEXEC;
> > +
>
> I'm with Cedric here -- this is no good. The reason I think we
> need .may_mprotect or similar is exactly to avoid doing this.
>
> mmap() just needs to make the same type of VMA regardless of the pages
> in the range.
Instead of making decisions on its own, a more generic approach is for SGX subsystem/module to ask LSM for a decision, by calling security_file_mprotect() - as a new mapping could be considered as changing protection from PROT_NONE to (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)).
.may_mprotect() also solves part of the problem - i.e. VMAs will be created consistently but non-existent pages still cannot be mapped, which however is necessary for #PF driven EAUG in SGX2. Given that security_file_mprotect() is invoked by mprotect() syscall, it looks to me a more streamlined solution to call the same hook (security_file_mprotect) from both places (mmap and mprotect).
^ permalink raw reply
* Re: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Andy Lutomirski @ 2019-07-01 19:32 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Stephen Smalley, Christopherson, Sean J,
Jarkko Sakkinen, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
Roberts, William C, Schaufler, Casey, James Morris, Hansen, Dave,
Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D63B@ORSMSX116.amr.corp.intel.com>
On Mon, Jul 1, 2019 at 11:54 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Monday, July 01, 2019 10:54 AM
> >
> > On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com>
> > wrote:
> > >
> > > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > > Sent: Saturday, June 29, 2019 4:42 PM
> > > >
> > > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> > > > wrote:
> > > > >
> > > > > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > > > > >> From: Christopherson, Sean J
> > > > > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > > > > >>
> > > > > >> Intended use of each permission:
> > > > > >>
> > > > > >> - SGX_EXECDIRTY: dynamically load code within the enclave
> > itself
> > > > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > > > > >> Graphene
> > > > > >
> > > > > > Why does it matter whether a code page is measured or not?
> > > > >
> > > > > It won't be incorporated into an attestation?
> > > > >
> > > >
> > > > Also, if there is, in parallel, a policy that limits the set of
> > > > enclave SIGSTRUCTs that are accepted, requiring all code be measured
> > > > makes it harder to subvert by writing incompetent or maliciously
> > > > incompetent enclaves.
> > >
> > > As analyzed in my reply to one of Stephen's comments, no executable
> > page shall be "enclave only" as enclaves have access to host's memory,
> > so if an executable page in regular memory is considered posting a
> > threat to the process, it should be considered posting the same threat
> > inside an enclave as well.
>
> What I was trying to say was, an executable page, if considered a threat to the enclosing process, should always be considered a threat no matter it is in that process's memory or inside an enclave enclosed in that same process's address space.
>
> Therefore, for a page in regular memory, if it is denied executable, it is because it is considered a potential security threat to the enclosing process, so it shall not be used as the source for an executable enclave page, as the same threat exists regardless it is in regular memory or EPC. Does that make more sense?
It does make sense, but I'm not sure it's correct to assume that any
LSM policy will always allow execution on enclave source pages if it
would allow execution inside the enclave. As an example, here is a
policy that seems reasonable:
Task A cannot execute dynamic non-enclave code (no execmod, no
execmem, etc -- only approved unmodified file pages can be executed).
But task A can execute an enclave with MRENCLAVE == such-and-such, and
that enclave may be loaded from regular anonymous memory -- the
MRENCLAVE is considered enough verification.
>
> My patch doesn't require X on source pages either. I said "would", meaning X *would* be granted but doesn't have to be granted. You can see this in selinux_enclave_load() calling selinux_file_mprotect() in my code. The purpose is to determine if X *would* be granted to the source pages without actually granting X.
As above, I'm not convinced this assumption is valid.
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Andy Lutomirski @ 2019-07-01 19:36 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, linux-sgx@vger.kernel.org, LSM List,
selinux@vger.kernel.org, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D5FA@ORSMSX116.amr.corp.intel.com>
On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com> wrote:
> I intended to say the major reason I objected Sean's approach was its inability to support SGX2 smoothly - as #PF driven EAUG requires non-existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be dispatched so EAUG couldn't be issued in response to #PF.
I still think that, if the kernel wants to support #PF-driven EAUG, it
should be an opt-in thing. It would be something like
SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
the driver needs to learn how to track ranges of pages efficiently,
which is another reason to consider leaving all the fancy page / page
range tracking in the driver.
I don't think it's a good idea for a page fault on any non-EADDed page
in ELRANGE to automatically populate the page.
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Casey Schaufler @ 2019-07-01 19:53 UTC (permalink / raw)
To: Xing, Cedric, Stephen Smalley, casey
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Schaufler, Casey, jmorris@namei.org,
luto@kernel.org, jethro@fortanix.com, greg@enjellic.com,
sds@tycho.nsa.gov, jarkko.sakkinen@linux.intel.com,
Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D585@ORSMSX116.amr.corp.intel.com>
On 7/1/2019 10:57 AM, Xing, Cedric wrote:
>> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
>> owner@vger.kernel.org] On Behalf Of Casey Schaufler
>>
>> On 6/28/2019 6:37 PM, Stephen Smalley wrote:
>>> On Fri, Jun 28, 2019 at 1:22 PM Casey Schaufler <casey@schaufler-
>> ca.com> wrote:
>>>> On 6/27/2019 5:47 PM, Xing, Cedric wrote:
>>>>>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>>>>>> Sent: Thursday, June 27, 2019 4:37 PM
>>>>>>>> This code should not be mixed in with the LSM infrastructure.
>>>>>>>> It should all be contained in its own module, under
>> security/enclave.
>>>>>>> lsm_ema is *intended* to be part of the LSM infrastructure.
>>>>>> That's not going to fly, not for a minute.
>>>>> Why not, if there's a need for it?
>>>>>
>>>>> And what's the concern here if it becomes part of the LSM
>> infrastructure.
>>>> The LSM infrastructure provides a framework for hooks and allocation
>>>> of blobs. That's it. It's a layer for connecting system features like
>>>> VFS, IPC and the IP stack to the security modules. It does not
>>>> implement any policy of it's own. We are not going to implement SGX
>>>> or any other mechanism within the LSM infrastructure.
>>> I don't think you understand the purpose of this code. It isn't
>>> implementing SGX, nor is it needed by SGX.
>>> It is providing shared infrastructure for security modules, similar to
>>> lsm_audit.c, so that security modules can enforce W^X or similar
>>> memory protection guarantees for SGX enclave memory, which has unique
>>> properties that render the existing mmap and mprotect hooks
>>> insufficient. They can certainly implement it only for SELinux, but
>>> then any other security module that wants to provide such guarantees
>>> will have to replicate that code.
>> I am not objecting to the purpose of the code.
>> I *am* objecting to calling it part of the LSM infrastructure.
>> It needs to be it's own thing, off somewhere else.
>> It must not use the lsm_ prefix. That's namespace pollution.
>> The code must not be embedded in the LSM infrastructure code, that
>> breaks with how everything else works.
> If you understand the purpose,
The purpose is to support the SGX hardware, is it not?
If you don't have SGX hardware (e.g. MIPS, ARM, s390) you
don't need this code.
> then why are you objecting the lsm_ prefix as they are APIs to be used by all LSM modules?
We name interfaces based on what they provide, not who consumes them.
As your code provides enclave services, that is how they should be named.
> Or what should be the prefix in your mind?
I'm pretty sure that I've consistently suggested "enclave".
> Or what kind of APIs do you think can qualify the lsm_ prefix?
Code that implements the LSM infrastructure. There is one LSM
blob allocation interface, lsm_inode_alloc(), that is used in
early set-up that is exported. As I've mentioned more than once,
enclave/page management is not an LSM infrastructure function,
it's a memory management function.
> And I'd like to clarify that it doesn't break anything, but is just a bit different, in that security_enclave_load() and security_file_free() call into those APIs.
There should be nothing in security_enclave_load() except calls to the enclave_load()
hooks (e.g. selinux_enclave_load()). There should be nothing in security_file_free()
except file blob management calls to the file_free() hooks (e.g. apparmor_file_free()).
> But there's a need for them because otherwise code/data would have to be duplicated among LSMs
There's plenty of code duplication among the LSMs, because a lot
of what they do is the same thing. Someday there may be an effort
to address some of that, but I don't think it's on anybody's radar.
As for data duplication, there's a reason we use lots of pointers.
> and the logic would be harder to comprehend.
Keeping the layering clean is critical to comprehension.
There's a lot of VFS code that could have been implemented
within the LSM infrastructure, but I don't think that anyone
would argue that it should have been.
> So that's a trade-off.
I remain completely unconvinced that your proposal
represents a good way to implement you scheme.
> Then what's the practical drawback of doing that?
Name space pollution.
Layering violation.
Architecture specific implementation detail in a
general infrastructure.
> If no, why would we want to pay for the cost for not doing that?
Modularity and maintainability come directly to mind.
>> ... and the notion that you allocate data for one blob that gets freed
>> relative to another breaks the data management model.
> What do you mean here?
You're freeing the EMA data from security_file_free().
If selinux wants to free EMA data it has allocated in
selinux_enclave_load() in selinux_file_free() that's fine,
but the LSM infrastructure has no need to know about it.
EMA needs to manage its own data, just like VFS does.
The LSM infrastructure provides blob management so that
the security modules can extend data if they want to.
> EMA blobs are allocated/freed *not* relative to any other blobs.
In the code you proposed they are freed in security_file_free().
That is for file blob management.
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-01 19:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx@vger.kernel.org, LSM List, selinux@vger.kernel.org,
Schaufler, Casey, James Morris, Jethro Beekman,
Dr. Greg Wettstein, Stephen Smalley, Jarkko Sakkinen,
Christopherson, Sean J
In-Reply-To: <CALCETrUAme7ujK3j-6gg1=_FtVGY3ZM8WBg9_9Sn-keywd7Smg@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 12:36 PM
>
> On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> > I intended to say the major reason I objected Sean's approach was its
> inability to support SGX2 smoothly - as #PF driven EAUG requires non-
> existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be
> dispatched so EAUG couldn't be issued in response to #PF.
>
> I still think that, if the kernel wants to support #PF-driven EAUG, it
> should be an opt-in thing. It would be something like
> SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
> the driver needs to learn how to track ranges of pages efficiently,
> which is another reason to consider leaving all the fancy page / page
> range tracking in the driver.
>
> I don't think it's a good idea for a page fault on any non-EADDed page
> in ELRANGE to automatically populate the page.
I'm with you. The user code shall be explicit on which range to EAUG pages upon #PF. What I'm saying is, a range has to be mapped before the driver could receive #PFs (in the form of vm_ops->fault callbacks). But Sean's series doesn’t support that because no pages can be mapped before coming into existence.
For "page tracking", what information to track is LSM dependent, so it may run into problems if different LSMs want to track different things. And that's the major reason I think it should be done inside LSM.
Besides, the current page tracking structure in the driver is page oriented and its sole purpose is to serve #PFs. Page protection is better tracked using range oriented structures. Those 2 are orthogonal. It wouldn't reduce the complexity of the whole kernel by moving it into SGX driver.
^ permalink raw reply
* RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Xing, Cedric @ 2019-07-01 20:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <CALCETrU=Btr+o9jb-zbj2kw8571WGhuhA6ZdttxQ_5_3pzZwUw@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 12:33 PM
>
> It does make sense, but I'm not sure it's correct to assume that any LSM
> policy will always allow execution on enclave source pages if it would
> allow execution inside the enclave. As an example, here is a policy
> that seems reasonable:
>
> Task A cannot execute dynamic non-enclave code (no execmod, no execmem,
> etc -- only approved unmodified file pages can be executed).
> But task A can execute an enclave with MRENCLAVE == such-and-such, and
> that enclave may be loaded from regular anonymous memory -- the
> MRENCLAVE is considered enough verification.
You are right. That's a reasonable policy. But I still can't see the need for SGX_EXECUNMR, as MRENCLAVE is considered enough verification.
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-01 21:45 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Schaufler, Casey, jmorris@namei.org,
luto@kernel.org, jethro@fortanix.com, greg@enjellic.com,
sds@tycho.nsa.gov, jarkko.sakkinen@linux.intel.com,
Christopherson, Sean J
In-Reply-To: <f59529e4-6cc8-2405-d7db-2519727f9a80@schaufler-ca.com>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Casey Schaufler
> Sent: Monday, July 01, 2019 12:54 PM
> > If you understand the purpose,
>
> The purpose is to support the SGX hardware, is it not?
> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
> this code.
No, it is NOT to support SGX - i.e. SGX doesn't require this piece of code to work.
And as Dr. Greg pointed out, it can be used for other TEEs than SGX. It doesn't contain SGX h/w specifics. It is compiled out because there's no module calling it on other architectures today. But it doesn't conflict with any h/w and may be useful (for other TEEs) on other architectures in future.
>
> > then why are you objecting the lsm_ prefix as they are APIs to be used
> by all LSM modules?
>
> We name interfaces based on what they provide, not who consumes them.
> As your code provides enclave services, that is how they should be named.
>
> > Or what should be the prefix in your mind?
>
> I'm pretty sure that I've consistently suggested "enclave".
>
> > Or what kind of APIs do you think can qualify the lsm_ prefix?
>
> Code that implements the LSM infrastructure. There is one LSM blob
> allocation interface, lsm_inode_alloc(), that is used in early set-up
> that is exported. As I've mentioned more than once, enclave/page
> management is not an LSM infrastructure function, it's a memory
> management function.
It doesn't manage anything. The reason it appears in the infrastructure is because the decision of inserting an EMA depends on the decisions from *all* active LSMs. That is NOT new either, as you can see it in security_file_permission() and security_vm_enough_memory_mm(), both do something after all LSM modules make their decisions.
Would you please explain why you don't see those as problems but calling EMA functions in security_enclave_load() is a problem?
>
> > And I'd like to clarify that it doesn't break anything, but is just a
> bit different, in that security_enclave_load() and security_file_free()
> call into those APIs.
>
> There should be nothing in security_enclave_load() except calls to the
> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
> nothing in security_file_free() except file blob management calls to the
> file_free() hooks (e.g. apparmor_file_free()).
As above, there are examples in security/security.c where the hook does more than just calling registered hooks from LSMs.
>
> > But there's a need for them because otherwise code/data would have to
> > be duplicated among LSMs
>
> There's plenty of code duplication among the LSMs, because a lot of what
> they do is the same thing. Someday there may be an effort to address
> some of that, but I don't think it's on anybody's radar.
> As for data duplication, there's a reason we use lots of pointers.
As stated above, security_enclave_load() needs to do something extra after all LSMs make their decisions. How can pointers help here?
>
> > and the logic would be harder to comprehend.
>
> Keeping the layering clean is critical to comprehension.
> There's a lot of VFS code that could have been implemented within the
> LSM infrastructure, but I don't think that anyone would argue that it
> should have been.
>
> > So that's a trade-off.
>
> I remain completely unconvinced that your proposal represents a good way
> to implement you scheme.
>
> > Then what's the practical drawback of doing that?
>
> Name space pollution.
Alright, I can fix the names.
> Layering violation.
Not sure what you are referring to.
If you are referring to buffers allocated in one layer and freed in elsewhere, you have got the code wrong. Buffers allocated in security_enclave_load() is freed in security_file_free(). Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-enclave files, otherwise it could be done in file_alloc() to be more "paired" with file_free(). But I don't see it necessary.
> Architecture specific implementation detail in a general infrastructure.
Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction. It could be left on all the time or controlled by a different config macro. It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only so far) TEE that needs attention from LSM, but there could be more in future.
>
> > If no, why would we want to pay for the cost for not doing that?
>
> Modularity and maintainability come directly to mind.
Putting it elsewhere will incur more maintenance cost.
>
> >> ... and the notion that you allocate data for one blob that gets
> >> freed relative to another breaks the data management model.
> > What do you mean here?
>
> You're freeing the EMA data from security_file_free().
> If selinux wants to free EMA data it has allocated in
> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
> infrastructure has no need to know about it.
> EMA needs to manage its own data, just like VFS does.
> The LSM infrastructure provides blob management so that the security
> modules can extend data if they want to.
You've got the code wrong. selinux_enclave_load() doesn't allocate any memory. selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.
The LSM infrastructure doesn't know anything about what LSM modules do, nor does it manage any buffers allocated by any LSM modules.
EMA is currently managing its own data. What's needed is the trigger - to let EMA know when to update its states. The trigger could be placed in LSM infrastructure or inside individual LSMs. The reason to put it in the infrastructure, is that it depends on the decision of *all* LSMs whether to insert a new EMA. That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made by the infrastructure but not individual LSMs.
>
> > EMA blobs are allocated/freed *not* relative to any other blobs.
>
> In the code you proposed they are freed in security_file_free().
> That is for file blob management.
Yes. EMA contributes to the file blob. But it only frees memory allocated by the infrastructure itself, not anything from any LSM modules.
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Casey Schaufler @ 2019-07-01 23:11 UTC (permalink / raw)
To: Xing, Cedric, Stephen Smalley, casey
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Schaufler, Casey, jmorris@namei.org,
luto@kernel.org, jethro@fortanix.com, greg@enjellic.com,
sds@tycho.nsa.gov, jarkko.sakkinen@linux.intel.com,
Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D7F7@ORSMSX116.amr.corp.intel.com>
On 7/1/2019 2:45 PM, Xing, Cedric wrote:
>> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
>> owner@vger.kernel.org] On Behalf Of Casey Schaufler
>> Sent: Monday, July 01, 2019 12:54 PM
>>> If you understand the purpose,
>> The purpose is to support the SGX hardware, is it not?
>> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
>> this code.
> No, it is NOT to support SGX
Then what *is* it for?
> - i.e. SGX doesn't require this piece of code to work.
>
> And as Dr. Greg pointed out, it can be used for other TEEs than SGX.
That sure makes it sound like it's for SGX to me.
> It doesn't contain SGX h/w specifics.
I never said it did. But no one ever suggested doing anything
here before SGX, and your subject line:
"x86/sgx: Add SGX specific LSM hooks"
says it does.
> It is compiled out because there's no module calling it on other architectures today. But it doesn't conflict with any h/w and may be useful (for other TEEs) on other architectures in future.
>
>>> then why are you objecting the lsm_ prefix as they are APIs to be used
>> by all LSM modules?
>>
>> We name interfaces based on what they provide, not who consumes them.
>> As your code provides enclave services, that is how they should be named.
>>
>>> Or what should be the prefix in your mind?
>> I'm pretty sure that I've consistently suggested "enclave".
>>
>>> Or what kind of APIs do you think can qualify the lsm_ prefix?
>> Code that implements the LSM infrastructure. There is one LSM blob
>> allocation interface, lsm_inode_alloc(), that is used in early set-up
>> that is exported. As I've mentioned more than once, enclave/page
>> management is not an LSM infrastructure function, it's a memory
>> management function.
> It doesn't manage anything.
Sorry, "memory management" as in all that stuff around pages and
TLBs and who gets what pages, as opposed to keeping track of anything
on its own.
> The reason it appears in the infrastructure is because the decision of inserting an EMA depends on the decisions from *all* active LSMs.
You have not been listening. Most LSMs use VFS. We haven't rolled VFS
functions into the LSM infrastructure.
> That is NOT new either, as you can see it in security_file_permission() and security_vm_enough_memory_mm(), both do something after all LSM modules make their decisions.
Did you look to see what it is they're doing? If you had,
you would see that is nothing like what you're proposing.
> Would you please explain why you don't see those as problems but calling EMA functions in security_enclave_load() is a problem?
The enclave code should be calling security_enclave_load(),
not the other way around. That assumes you're using the naming
convention correctly.
security_vm_enough_memory_mm() was discussed at length and there
wasn't a clean way to get the logic right without putting the code
here. security_file_permission() has the fs_notify_perm call for
similar reasons. Neither is anything like what you're suggesting.
>>> And I'd like to clarify that it doesn't break anything, but is just a
>> bit different, in that security_enclave_load() and security_file_free()
>> call into those APIs.
>>
>> There should be nothing in security_enclave_load() except calls to the
>> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
>> nothing in security_file_free() except file blob management calls to the
>> file_free() hooks (e.g. apparmor_file_free()).
> As above, there are examples in security/security.c where the hook does more than just calling registered hooks from LSMs.
And as I've said, that doesn't matter. You're still going about
using the LSM infrastructure backwards.
>>> But there's a need for them because otherwise code/data would have to
>>> be duplicated among LSMs
>> There's plenty of code duplication among the LSMs, because a lot of what
>> they do is the same thing. Someday there may be an effort to address
>> some of that, but I don't think it's on anybody's radar.
>> As for data duplication, there's a reason we use lots of pointers.
> As stated above, security_enclave_load() needs to do something extra after all LSMs make their decisions. How can pointers help here?
I can explain it, but you clearly have no interest in doing
anything to make your code fit into the system. I have a lot
of other things to be doing.
>>> and the logic would be harder to comprehend.
>> Keeping the layering clean is critical to comprehension.
>> There's a lot of VFS code that could have been implemented within the
>> LSM infrastructure, but I don't think that anyone would argue that it
>> should have been.
>>
>>> So that's a trade-off.
>> I remain completely unconvinced that your proposal represents a good way
>> to implement you scheme.
>>
>>> Then what's the practical drawback of doing that?
>> Name space pollution.
> Alright, I can fix the names.
Good!
>> Layering violation.
> Not sure what you are referring to.
The only places where the blob freed by security_file_free()
may be allocated is security_file_alloc(). The security modules
are welcome to do anything they like in addition, provided
they clean up after themselves in their file_free() hooks.
If SELinux wants to support controls on enclave information,
and that requires additional data, SELinux should include
space in its file blob for that information, or a pointer to
the place where the enclave code is maintaining it.
That's the way audit works.
> If you are referring to buffers allocated in one layer and freed in elsewhere, you have got the code wrong. Buffers allocated in security_enclave_load() is freed in security_file_free().
It's up to the security module's file_free() to clean up anything that
wasn't allocated in security_file_free(). Interested security modules
should call enclave_load(), and put the information into their portion
of the security blob. The module specific code can call enclave_file_free(),
or whatever interface you want to provide, to clean up. That might take
place in file_free(), but it also might be elsewhere.
> Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-enclave files, otherwise it could be done in file_alloc() to be more "paired" with file_free(). But I don't see it necessary.
Try looking at maintaining what you've put into the LSM code as
a separate entity. It makes it simpler. Really.
>> Architecture specific implementation detail in a general infrastructure.
> Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction.
Then put it in the TEE system.
> It could be left on all the time or controlled by a different config macro.
True in any case.
> It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only so far) TEE that needs attention from LSM, but there could be more in future.
All the more reason to keep it separate. These things never get simpler
when they get more generalized.
>>> If no, why would we want to pay for the cost for not doing that?
>> Modularity and maintainability come directly to mind.
> Putting it elsewhere will incur more maintenance cost.
I don't believe that for a second. 40 years of C programming
have taught me that trying to do multiple things in one place
is always a bad idea.
>>>> ... and the notion that you allocate data for one blob that gets
>>>> freed relative to another breaks the data management model.
>>> What do you mean here?
>> You're freeing the EMA data from security_file_free().
>> If selinux wants to free EMA data it has allocated in
>> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
>> infrastructure has no need to know about it.
>> EMA needs to manage its own data, just like VFS does.
>> The LSM infrastructure provides blob management so that the security
>> modules can extend data if they want to.
> You've got the code wrong. selinux_enclave_load() doesn't allocate any memory. selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.
... and the LSM infrastructure doesn't care and
must not be made to care. It's all up to SELinux.
> The LSM infrastructure doesn't know anything about what LSM modules do, nor does it manage any buffers allocated by any LSM modules.
Right, which is why putting your lsm_ema_blob is wrong, and why
forcing into the file blob is wrong.
> EMA is currently managing its own data. What's needed is the trigger - to let EMA know when to update its states. The trigger could be placed in LSM infrastructure or inside individual LSMs.
Yes. The latter.
> The reason to put it in the infrastructure, is that it depends on the decision of *all* LSMs whether to insert a new EMA.
That's basic stacking behavior. "Bail on fail", which says that once
denial is detected, you're done.
> That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made by the infrastructure but not individual LSMs.
Do you really understand the painful reasons that case is required?
And if so, why you aren't taking steps to avoid them?
>>> EMA blobs are allocated/freed *not* relative to any other blobs.
>> In the code you proposed they are freed in security_file_free().
>> That is for file blob management.
> Yes. EMA contributes to the file blob. But it only frees memory allocated by the infrastructure itself, not anything from any LSM modules.
That's not the way it's supposed to be done. The module tells
the infrastructure what it needs, which may include space for
EMA data. The module asks EMA for the data it needs and stuffs
it somewhere, and the file blob is a fine choice. The module
cleans up in file_free, or at any time before that. If no module
uses EMA, nothing goes in the blob. If two modules use EMA each
is responsible for the data it uses, which may be the same or
may be different.
I've looked at your code. Making it work the way it should would
not be difficult and would likely simplify a bunch of it.
^ permalink raw reply
* Re: Fwd: [PATCH v4 15/23] LSM: Specify which LSM to display
From: James Morris @ 2019-07-02 0:49 UTC (permalink / raw)
To: Stephen Smalley
Cc: Stephen Smalley, Schaufler, Casey, LSM List, selinux, keescook,
John Johansen, penguin-kernel, Paul Moore, Casey Schaufler
In-Reply-To: <CAB9W1A29fCn=cH_Mx-g-P6M-5t+832ayhMmjy3PFZ-BOL3BuDQ@mail.gmail.com>
On Fri, 28 Jun 2019, Stephen Smalley wrote:
> > Balancing backward compatibility with new behavior is hard!
> > What would you suggest for audit logs? Should we put all LSM
> > data in every record? Is NFS a concern for anyone not using
> > SELinux?
>
> Yes to all on audit if stacking is going to be real. And yes, I think
> other security modules will care about NFS if they are serious.
Agreed.
There must better way to approach this, somehow...
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: Fwd: [PATCH v4 15/23] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-07-02 1:20 UTC (permalink / raw)
To: James Morris, Stephen Smalley
Cc: Stephen Smalley, Schaufler, Casey, LSM List, selinux, keescook,
John Johansen, penguin-kernel, Paul Moore, casey
In-Reply-To: <alpine.LRH.2.21.1907011745480.13468@namei.org>
On 7/1/2019 5:49 PM, James Morris wrote:
> On Fri, 28 Jun 2019, Stephen Smalley wrote:
>
>>> Balancing backward compatibility with new behavior is hard!
>>> What would you suggest for audit logs? Should we put all LSM
>>> data in every record? Is NFS a concern for anyone not using
>>> SELinux?
>> Yes to all on audit if stacking is going to be real. And yes, I think
>> other security modules will care about NFS if they are serious.
> Agreed.
>
> There must better way to approach this, somehow...
It not like I haven't proposed a number of mechanisms!
The "display" mechanism has the best backward compatibility
story, at the cost of being awkward/dangerous in the face of
sophisticated user space environments. A combined string
(smack='System",AppArmor='unconfined') sucks at compatibility,
but provides the best information.
Right now I'm looking at a way to prevent internal confusion.
I think that may be possible.
I'll point out that lib<lsm> has the option of verifying
the display before doing scary writes, but that's a lot of
work that no one is looking forward to.
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Andy Lutomirski @ 2019-07-02 2:29 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, linux-sgx@vger.kernel.org, LSM List,
selinux@vger.kernel.org, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D75B@ORSMSX116.amr.corp.intel.com>
On Mon, Jul 1, 2019 at 12:56 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Monday, July 01, 2019 12:36 PM
> >
> > On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com>
> > wrote:
> > > I intended to say the major reason I objected Sean's approach was its
> > inability to support SGX2 smoothly - as #PF driven EAUG requires non-
> > existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be
> > dispatched so EAUG couldn't be issued in response to #PF.
> >
> > I still think that, if the kernel wants to support #PF-driven EAUG, it
> > should be an opt-in thing. It would be something like
> > SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
> > the driver needs to learn how to track ranges of pages efficiently,
> > which is another reason to consider leaving all the fancy page / page
> > range tracking in the driver.
> >
> > I don't think it's a good idea for a page fault on any non-EADDed page
> > in ELRANGE to automatically populate the page.
>
> I'm with you. The user code shall be explicit on which range to EAUG pages upon #PF. What I'm saying is, a range has to be mapped before the driver could receive #PFs (in the form of vm_ops->fault callbacks). But Sean's series doesn’t support that because no pages can be mapped before coming into existence.
>
> For "page tracking", what information to track is LSM dependent, so it may run into problems if different LSMs want to track different things. And that's the major reason I think it should be done inside LSM.
>
> Besides, the current page tracking structure in the driver is page oriented and its sole purpose is to serve #PFs. Page protection is better tracked using range oriented structures. Those 2 are orthogonal. It wouldn't reduce the complexity of the whole kernel by moving it into SGX driver.
It seems to me that the driver is going to need to improve its data
structures to track ranges of pages regardless of any LSM issues. If
we're going to have an enclave with a huge ELRANGE and we're going to
mark some large subset of the full ELRANGE as allocate-on-demand, then
we are going to want to track that range in some efficient way. It
could be a single extent or a set of power-of-two-sized extents (i.e.
radix tree entries), or something else, but a list of pages, of which
some are marked not-yet-allocated, isn't going to cut it.
Once that happens, it seems natural to put whatever permission
tracking we need into the same data structure. That's why my proposal
had the driver getting coarse-grained info from LSM ("may execute
dirty page", for example) rather than asking the LSM to track the
whole state machine.
Does that make sense?
^ permalink raw reply
* Reminder: 1 open syzbot bug in "security/smack" subsystem
From: Eric Biggers @ 2019-07-02 5:11 UTC (permalink / raw)
To: linux-security-module, Casey Schaufler, James Morris,
Serge E. Hallyn
Cc: linux-kernel, syzkaller-bugs
[This email was generated by a script. Let me know if you have any suggestions
to make it better, or if you want it re-generated with the latest status.]
Of the currently open syzbot reports against the upstream kernel, I've manually
marked 1 of them as possibly being a bug in the "security/smack" subsystem.
If you believe this bug is no longer valid, please close the syzbot report by
sending a '#syz fix', '#syz dup', or '#syz invalid' command in reply to the
original thread, as explained at https://goo.gl/tpsmEJ#status
If you believe I misattributed this bug to the "security/smack" subsystem,
please let me know, and if possible forward the report to the correct people or
mailing list.
Here is the bug:
--------------------------------------------------------------------------------
Title: possible deadlock in ext4_evict_inode
Last occurred: 259 days ago
Reported: 298 days ago
Branches: Mainline
Dashboard link: https://syzkaller.appspot.com/bug?id=9eda6092f146cb23cb9109f675a2e2cb743ee48b
Original thread: https://lkml.kernel.org/lkml/00000000000091615e0575368e33@google.com/T/#u
This bug has a syzkaller reproducer only.
The original thread for this bug received 2 replies; the last was 298 days ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+0eefc1e06a77d327a056@syzkaller.appspotmail.com
If you send any email or patch for this bug, please consider replying to the
original thread. For the git send-email command to use, or tips on how to reply
if the thread isn't in your mailbox, see the "Reply instructions" at
https://lkml.kernel.org/r/00000000000091615e0575368e33@google.com
^ permalink raw reply
* Reminder: 2 open syzbot bugs in "security/tomoyo" subsystem
From: Eric Biggers @ 2019-07-02 5:14 UTC (permalink / raw)
To: linux-security-module, Kentaro Takeda, Tetsuo Handa, James Morris,
Serge E. Hallyn
Cc: linux-kernel, syzkaller-bugs
[This email was generated by a script. Let me know if you have any suggestions
to make it better, or if you want it re-generated with the latest status.]
Of the currently open syzbot reports against the upstream kernel, I've manually
marked 2 of them as possibly being bugs in the "security/tomoyo" subsystem.
I've listed these reports below, sorted by an algorithm that tries to list first
the reports most likely to be still valid, important, and actionable.
If you believe a bug is no longer valid, please close the syzbot report by
sending a '#syz fix', '#syz dup', or '#syz invalid' command in reply to the
original thread, as explained at https://goo.gl/tpsmEJ#status
If you believe I misattributed a bug to the "security/tomoyo" subsystem, please
let me know, and if possible forward the report to the correct people or mailing
list.
Here are the bugs:
--------------------------------------------------------------------------------
Title: KASAN: use-after-free Read in tomoyo_realpath_from_path
Last occurred: 6 days ago
Reported: 26 days ago
Branches: Mainline and others
Dashboard link: https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
Original thread: https://lkml.kernel.org/lkml/0000000000004f43fa058a97f4d3@google.com/T/#u
This bug has a syzkaller reproducer only.
The original thread for this bug has received 7 replies; the last was 9 days
ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com
If you send any email or patch for this bug, please reply to the original
thread, which had activity only 9 days ago. For the git send-email command to
use, or tips on how to reply if the thread isn't in your mailbox, see the "Reply
instructions" at https://lkml.kernel.org/r/0000000000004f43fa058a97f4d3@google.com
--------------------------------------------------------------------------------
Title: KASAN: invalid-free in tomoyo_realpath_from_path
Last occurred: 35 days ago
Reported: 34 days ago
Branches: net-next
Dashboard link: https://syzkaller.appspot.com/bug?id=e9e5a1d41c3fb5d0f79aeea0e4cd535f160a6702
Original thread: https://lkml.kernel.org/lkml/000000000000785e9d0589ec359a@google.com/T/#u
Unfortunately, this bug does not have a reproducer.
The original thread for this bug has received 1 reply, 34 days ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+9742b1c6c7aedf18beda@syzkaller.appspotmail.com
If you send any email or patch for this bug, please consider replying to the
original thread. For the git send-email command to use, or tips on how to reply
if the thread isn't in your mailbox, see the "Reply instructions" at
https://lkml.kernel.org/r/000000000000785e9d0589ec359a@google.com
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-02 6:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx@vger.kernel.org, LSM List, selinux@vger.kernel.org,
Schaufler, Casey, James Morris, Jethro Beekman,
Dr. Greg Wettstein, Stephen Smalley, Jarkko Sakkinen,
Christopherson, Sean J
In-Reply-To: <CALCETrVRDBN_AcKQaYbodtUVv5vW=s2bWP2OAucw=8OjhWo=ng@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 1, 2019 7:29 PM
>
> On Mon, Jul 1, 2019 at 12:56 PM Xing, Cedric <cedric.xing@intel.com> wrote:
> >
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Monday, July 01, 2019 12:36 PM
> > >
> > > On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com>
> > > wrote:
> > > > I intended to say the major reason I objected Sean's approach was its
> > > inability to support SGX2 smoothly - as #PF driven EAUG requires non-
> > > existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be
> > > dispatched so EAUG couldn't be issued in response to #PF.
> > >
> > > I still think that, if the kernel wants to support #PF-driven EAUG, it
> > > should be an opt-in thing. It would be something like
> > > SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
> > > the driver needs to learn how to track ranges of pages efficiently,
> > > which is another reason to consider leaving all the fancy page / page
> > > range tracking in the driver.
> > >
> > > I don't think it's a good idea for a page fault on any non-EADDed page
> > > in ELRANGE to automatically populate the page.
> >
> > I'm with you. The user code shall be explicit on which range to EAUG pages upon #PF.
> What I'm saying is, a range has to be mapped before the driver could receive #PFs (in the
> form of vm_ops->fault callbacks). But Sean's series doesn’t support that because no pages
> can be mapped before coming into existence.
> >
> > For "page tracking", what information to track is LSM dependent, so it may run into
> problems if different LSMs want to track different things. And that's the major reason I
> think it should be done inside LSM.
> >
> > Besides, the current page tracking structure in the driver is page oriented and its sole
> purpose is to serve #PFs. Page protection is better tracked using range oriented
> structures. Those 2 are orthogonal. It wouldn't reduce the complexity of the whole kernel
> by moving it into SGX driver.
>
> It seems to me that the driver is going to need to improve its data
> structures to track ranges of pages regardless of any LSM issues. If
> we're going to have an enclave with a huge ELRANGE and we're going to
> mark some large subset of the full ELRANGE as allocate-on-demand, then
> we are going to want to track that range in some efficient way. It
> could be a single extent or a set of power-of-two-sized extents (i.e.
> radix tree entries), or something else, but a list of pages, of which
> some are marked not-yet-allocated, isn't going to cut it.
>
> Once that happens, it seems natural to put whatever permission
> tracking we need into the same data structure. That's why my proposal
> had the driver getting coarse-grained info from LSM ("may execute
> dirty page", for example) rather than asking the LSM to track the
> whole state machine.
>
> Does that make sense?
The driver will eventually need some range oriented structures for managing EAUGs. But it doesn't necessarily have to be in the same structure as other per-page information. After all, they are touched by different components of the driver and indeed pretty orthogonal. Evils are always in the details. It may be counter-intuitive but per our prototype years ago, it would be simpler to just keep them separate.
IIUC, your idea is in fact keeping a FSM inside SGX driver and using return values from security_enclave_load() to argument it. That means LSM has to work quite "closely" with SGX driver (i.e. LSM needs to understand the FSM in SGX driver), which is quite different than all other existing hooks, which basically make binary decisions only. And I'm not sure how to chain LSMs if there are multiple active at the same time. Auditing is also a problem, as you can't generate audit log at the time a real mprotect() request is approved/denied. UAPI is also unpleasant as the enclave loader has to "predict" the maximal protection, which is not always available to the loader at load time, or significant changes to enclave build tools would be necessary. I think the FSM is really part of the policy and internal to LSM (or more particularly, SELinux, as different LSM modules may have different FSMs), so it still makes more sense to me to keep "LSM internals" internal to LSM.
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-02 7:42 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Schaufler, Casey, jmorris@namei.org,
luto@kernel.org, jethro@fortanix.com, greg@enjellic.com,
sds@tycho.nsa.gov, jarkko.sakkinen@linux.intel.com,
Christopherson, Sean J
In-Reply-To: <63c92ab6-dc8d-826b-b8bf-05ad262f06e4@schaufler-ca.com>
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Monday, July 1, 2019 4:12 PM
>
> On 7/1/2019 2:45 PM, Xing, Cedric wrote:
> >> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> >> owner@vger.kernel.org] On Behalf Of Casey Schaufler
> >> Sent: Monday, July 01, 2019 12:54 PM
> >>> If you understand the purpose,
> >> The purpose is to support the SGX hardware, is it not?
> >> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
> >> this code.
> > No, it is NOT to support SGX
>
> Then what *is* it for?
>
> > - i.e. SGX doesn't require this piece of code to work.
> >
> > And as Dr. Greg pointed out, it can be used for other TEEs than SGX.
>
> That sure makes it sound like it's for SGX to me.
I meant it is generic and potentially useful to more TEEs but so far useful to SGX, which is the first technology that uses this infrastructure. I hope that makes more sense.
>
> > It doesn't contain SGX h/w specifics.
>
> I never said it did. But no one ever suggested doing anything
> here before SGX, and your subject line:
>
> "x86/sgx: Add SGX specific LSM hooks"
>
> says it does.
Yes. And in the commit message I also stated that the need for such tracking structure is due to the unique lifespan of enclave pages. Hence this infrastructure will also be useful for other TEEs whose pages share the same lifespan.
>
> > It is compiled out because there's no module calling it on other architectures today.
> But it doesn't conflict with any h/w and may be useful (for other TEEs) on other
> architectures in future.
> >
> >>> then why are you objecting the lsm_ prefix as they are APIs to be used
> >> by all LSM modules?
> >>
> >> We name interfaces based on what they provide, not who consumes them.
> >> As your code provides enclave services, that is how they should be named.
> >>
> >>> Or what should be the prefix in your mind?
> >> I'm pretty sure that I've consistently suggested "enclave".
> >>
> >>> Or what kind of APIs do you think can qualify the lsm_ prefix?
> >> Code that implements the LSM infrastructure. There is one LSM blob
> >> allocation interface, lsm_inode_alloc(), that is used in early set-up
> >> that is exported. As I've mentioned more than once, enclave/page
> >> management is not an LSM infrastructure function, it's a memory
> >> management function.
> > It doesn't manage anything.
>
> Sorry, "memory management" as in all that stuff around pages and
> TLBs and who gets what pages, as opposed to keeping track of anything
> on its own.
>
> > The reason it appears in the infrastructure is because the decision of inserting an EMA
> depends on the decisions from *all* active LSMs.
>
> You have not been listening. Most LSMs use VFS. We haven't rolled VFS
> functions into the LSM infrastructure.
>
> > That is NOT new either, as you can see it in security_file_permission() and
> security_vm_enough_memory_mm(), both do something after all LSM modules make their
> decisions.
>
> Did you look to see what it is they're doing? If you had,
> you would see that is nothing like what you're proposing.
I feel like we are talking about different things. I said those hooks did more than just calling registered hooks. And security_enclave_load() is similar to them, also for a similar reason - something needs to be done after *all* LSM modules make their decisions.
I'm not sure what you are talking about.
>
>
> > Would you please explain why you don't see those as problems but calling EMA functions
> in security_enclave_load() is a problem?
>
> The enclave code should be calling security_enclave_load(),
> not the other way around. That assumes you're using the naming
> convention correctly.
Yes. The enclave code (SGX driver) calls security_enclave_load/init. Never the other way around.
Again, EMA code is similar to auditing code. It is supposed to be called by LSM modules.
>
> security_vm_enough_memory_mm() was discussed at length and there
> wasn't a clean way to get the logic right without putting the code
> here. security_file_permission() has the fs_notify_perm call for
> similar reasons. Neither is anything like what you're suggesting.
Guess we are discussing "at length" right now on how to get the logic right. I'm not sure why "neither is anything like what I'm suggesting".
>
>
> >>> And I'd like to clarify that it doesn't break anything, but is just a
> >> bit different, in that security_enclave_load() and security_file_free()
> >> call into those APIs.
> >>
> >> There should be nothing in security_enclave_load() except calls to the
> >> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
> >> nothing in security_file_free() except file blob management calls to the
> >> file_free() hooks (e.g. apparmor_file_free()).
> > As above, there are examples in security/security.c where the hook does more than just
> calling registered hooks from LSMs.
>
> And as I've said, that doesn't matter. You're still going about
> using the LSM infrastructure backwards.
>
> >>> But there's a need for them because otherwise code/data would have to
> >>> be duplicated among LSMs
> >> There's plenty of code duplication among the LSMs, because a lot of what
> >> they do is the same thing. Someday there may be an effort to address
> >> some of that, but I don't think it's on anybody's radar.
> >> As for data duplication, there's a reason we use lots of pointers.
> > As stated above, security_enclave_load() needs to do something extra after all LSMs make
> their decisions. How can pointers help here?
>
> I can explain it, but you clearly have no interest in doing
> anything to make your code fit into the system. I have a lot
> of other things to be doing.
I'm so interested in getting things fit. Just that what I see fit doesn't seem fit from your perspective.
>
> >>> and the logic would be harder to comprehend.
> >> Keeping the layering clean is critical to comprehension.
> >> There's a lot of VFS code that could have been implemented within the
> >> LSM infrastructure, but I don't think that anyone would argue that it
> >> should have been.
> >>
> >>> So that's a trade-off.
> >> I remain completely unconvinced that your proposal represents a good way
> >> to implement you scheme.
> >>
> >>> Then what's the practical drawback of doing that?
> >> Name space pollution.
> > Alright, I can fix the names.
>
> Good!
>
>
> >> Layering violation.
> > Not sure what you are referring to.
>
> The only places where the blob freed by security_file_free()
> may be allocated is security_file_alloc(). The security modules
> are welcome to do anything they like in addition, provided
> they clean up after themselves in their file_free() hooks.
Exactly!
Like I said, allocation could happen in security_file_alloc() but I did it in security_enclave_load() to avoid unnecessary allocation for non-enclaves.
I know "security" looks close to "selinux" but I beg your attention in the function names. Whatever allocated inside SELinux *never* gets freed by the infrastructure, except those implicit allocations due to EMA splits.
>
> If SELinux wants to support controls on enclave information,
> and that requires additional data, SELinux should include
> space in its file blob for that information, or a pointer to
> the place where the enclave code is maintaining it.
>
> That's the way audit works.
I have to repeat myself. This was what v1 does.
The drawback is, there could be multiple LSMs active at the same time. And an EMA is inserted iff *all* LSMs have approved it. Thus the actual insertion is now done at the end of security_enclave_load(). That makes the logic clear and less error prone, and saves code that'd be duplicated into multiple LSMs otherwise. And that's why I cited security_file_permission()/security_vm_enough_memory() as precedence to security_enclave_load().
>
> > If you are referring to buffers allocated in one layer and freed in elsewhere, you have
> got the code wrong. Buffers allocated in security_enclave_load() is freed in
> security_file_free().
>
> It's up to the security module's file_free() to clean up anything that
> wasn't allocated in security_file_free(). Interested security modules
> should call enclave_load(), and put the information into their portion
> of the security blob. The module specific code can call enclave_file_free(),
> or whatever interface you want to provide, to clean up. That might take
> place in file_free(), but it also might be elsewhere.
Would you please take a closer look at my code? Whatever I added to SELinux did *not* allocate anything! Or are you talking about something else?
>
>
> > Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The
> purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-
> enclave files, otherwise it could be done in file_alloc() to be more "paired" with
> file_free(). But I don't see it necessary.
>
> Try looking at maintaining what you've put into the LSM code as
> a separate entity. It makes it simpler. Really.
It is already a separate entity. It has its own header and own C file.
>
> >> Architecture specific implementation detail in a general infrastructure.
> > Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction.
>
> Then put it in the TEE system.
It's LSM's abstraction of TEE - i.e., it tracks what matters to LSM only. TEE doesn't care. It just provides information and asks for a decision at return. That's how LSM works.
>
> > It could be left on all the time or controlled by a different config macro.
>
> True in any case.
>
> > It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only
> so far) TEE that needs attention from LSM, but there could be more in future.
>
> All the more reason to keep it separate. These things never get simpler
> when they get more generalized.
I have a hard time understanding what you mean by "separate".
>
> >>> If no, why would we want to pay for the cost for not doing that?
> >> Modularity and maintainability come directly to mind.
> > Putting it elsewhere will incur more maintenance cost.
>
> I don't believe that for a second. 40 years of C programming
> have taught me that trying to do multiple things in one place
> is always a bad idea.
Agreed. I'm doing only one thing.
>
>
> >>>> ... and the notion that you allocate data for one blob that gets
> >>>> freed relative to another breaks the data management model.
> >>> What do you mean here?
> >> You're freeing the EMA data from security_file_free().
> >> If selinux wants to free EMA data it has allocated in
> >> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
> >> infrastructure has no need to know about it.
> >> EMA needs to manage its own data, just like VFS does.
> >> The LSM infrastructure provides blob management so that the security
> >> modules can extend data if they want to.
> > You've got the code wrong. selinux_enclave_load() doesn't allocate any memory.
> selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.
>
> ... and the LSM infrastructure doesn't care and
> must not be made to care. It's all up to SELinux.
It doesn't care the decisions. But it assists in maintaining information on which decisions (from multiple LSMs) are based.
>
> > The LSM infrastructure doesn't know anything about what LSM modules do, nor does it
> manage any buffers allocated by any LSM modules.
>
> Right, which is why putting your lsm_ema_blob is wrong, and why
> forcing into the file blob is wrong.
lsm_ema_blob is NOT part of file blob.
>
> > EMA is currently managing its own data. What's needed is the trigger - to let EMA know
> when to update its states. The trigger could be placed in LSM infrastructure or inside
> individual LSMs.
>
> Yes. The latter.
>
> > The reason to put it in the infrastructure, is that it depends on the decision of *all*
> LSMs whether to insert a new EMA.
>
> That's basic stacking behavior. "Bail on fail", which says that once
> denial is detected, you're done.
Who does the insertion on success, if not the LSM infrastructure? This is again similar to security_file_permission/security_vm_enough_memory. The last step is done by the infrastructure on success.
>
> > That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made
> by the infrastructure but not individual LSMs.
>
> Do you really understand the painful reasons that case is required?
> And if so, why you aren't taking steps to avoid them?
I think I've run into the same painful reasons.
Honestly, I tried not to do anything more than just a call_int_hooks. But I realized that'd make thing much more complicated and error prone in multiple-active-LSM cases.
So I think I've run into the same painful reasons. And I don't see any actionable suggestions from you so far.
>
>
> >>> EMA blobs are allocated/freed *not* relative to any other blobs.
> >> In the code you proposed they are freed in security_file_free().
> >> That is for file blob management.
> > Yes. EMA contributes to the file blob. But it only frees memory allocated by the
> infrastructure itself, not anything from any LSM modules.
>
> That's not the way it's supposed to be done. The module tells
> the infrastructure what it needs, which may include space for
> EMA data. The module asks EMA for the data it needs and stuffs
> it somewhere, and the file blob is a fine choice. The module
> cleans up in file_free, or at any time before that. If no module
> uses EMA, nothing goes in the blob. If two modules use EMA each
> is responsible for the data it uses, which may be the same or
> may be different.
>
> I've looked at your code. Making it work the way it should would
> not be difficult and would likely simplify a bunch of it.
Guess this discussion will never end if we don't get into code. Guess it'd be more productive to talk over phone then come back to this thread with a conclusion. Will that be ok with you?
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Casey Schaufler @ 2019-07-02 15:44 UTC (permalink / raw)
To: Xing, Cedric, Stephen Smalley, casey
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Schaufler, Casey, jmorris@namei.org,
luto@kernel.org, jethro@fortanix.com, greg@enjellic.com,
sds@tycho.nsa.gov, jarkko.sakkinen@linux.intel.com,
Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551DBF7@ORSMSX116.amr.corp.intel.com>
On 7/2/2019 12:42 AM, Xing, Cedric wrote:
> ...
> Guess this discussion will never end if we don't get into code. Guess it'd be more productive to talk over phone then come back to this thread with a conclusion. Will that be ok with you?
I don't think that a phone call is going to help. Talking
code issues tends to muddle them in my brain. If you can give
me a few days I will propose a rough version of how I think
your code should be integrated into the LSM environment. I'm
spending more time trying (unsuccessfully :( ) to discribe
the issues in English than it will probably take in C.
^ permalink raw reply
* [GIT PULL] SELinux patches for v5.3
From: Paul Moore @ 2019-07-02 17:28 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Hi Linus,
Like the audit PR this is a little early due to some upcoming vacation
plans and uncertain network access while I'm away. Also like the
audit PR, the list of patches here is pretty minor, the highlights
include:
- Explicitly use __le variables to make sure "sparse" can verify
proper byte endian handling.
- Remove some BUG_ON()s that are no longer needed.
- Allow zero-byte writes to the "keycreate" procfs attribute without
requiring key:create to make it easier for userspace to reset the
keycreate label.
- Consistently log the "invalid_context" field as an untrusted string
in the AUDIT_SELINUX_ERR audit records.
Please pull this once the merge window opens,
-Paul
--
The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:
Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20190702
for you to fetch changes up to ea74a685ad819aeed316a9bae3d2a5bf762da82d:
selinux: format all invalid context as untrusted
(2019-07-01 16:29:05 -0400)
----------------------------------------------------------------
selinux/stable-5.3 PR 20190702
----------------------------------------------------------------
Nicholas Mc Guire (1):
selinux: provide __le variables explicitly
Ondrej Mosnacek (2):
selinux: remove some no-op BUG_ONs
selinux: fix empty write to keycreate file
Richard Guy Briggs (1):
selinux: format all invalid context as untrusted
security/selinux/hooks.c | 11 ++++++-----
security/selinux/ss/ebitmap.c | 10 ++++++----
security/selinux/ss/services.c | 33 +++++++++++++++++++--------------
3 files changed, 31 insertions(+), 23 deletions(-)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH] apparmor: Replace two seq_printf() calls by seq_puts() in aa_label_seq_xprint()
From: Markus Elfring @ 2019-07-02 18:33 UTC (permalink / raw)
To: linux-security-module, James Morris, John Johansen,
Serge E. Hallyn
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 20:27:32 +0200
Two strings which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/apparmor/label.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index 59f1cc2557a7..20acc1f3112e 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1747,13 +1747,13 @@ void aa_label_seq_xprint(struct seq_file *f, struct aa_ns *ns,
AA_DEBUG("label print error");
return;
}
- seq_printf(f, "%s", str);
+ seq_puts(f, str);
kfree(str);
} else if (display_mode(ns, label, flags))
seq_printf(f, "%s (%s)", label->hname,
label_modename(ns, label, flags));
else
- seq_printf(f, "%s", label->hname);
+ seq_puts(f, label->hname);
}
void aa_label_xprintk(struct aa_ns *ns, struct aa_label *label, int flags,
--
2.22.0
^ permalink raw reply related
* [PATCH] ima: Replace two seq_printf() calls by seq_puts() in ima_show_template_data_ascii()
From: Markus Elfring @ 2019-07-02 19:00 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Dmitry Kasatkin,
James Morris, Mimi Zohar, Serge E. Hallyn
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 20:52:21 +0200
Two strings which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/integrity/ima/ima_template_lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 9fe0ef7f91e2..05636e9b19b1 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -74,7 +74,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
case DATA_FMT_DIGEST_WITH_ALGO:
buf_ptr = strnchr(field_data->data, buflen, ':');
if (buf_ptr != field_data->data)
- seq_printf(m, "%s", field_data->data);
+ seq_puts(m, field_data->data);
/* skip ':' and '\0' */
buf_ptr += 2;
@@ -87,7 +87,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
ima_print_digest(m, buf_ptr, buflen);
break;
case DATA_FMT_STRING:
- seq_printf(m, "%s", buf_ptr);
+ seq_puts(m, buf_ptr);
break;
default:
break;
--
2.22.0
^ permalink raw reply related
* [PATCH] ima: Replace two seq_printf() calls by seq_puts() in ima_show_template_data_ascii()
From: Markus Elfring @ 2019-07-02 19:00 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Dmitry Kasatkin,
James Morris, Mimi Zohar, Serge E. Hallyn
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 20:52:21 +0200
Two strings which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/integrity/ima/ima_template_lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 9fe0ef7f91e2..05636e9b19b1 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -74,7 +74,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
case DATA_FMT_DIGEST_WITH_ALGO:
buf_ptr = strnchr(field_data->data, buflen, ':');
if (buf_ptr != field_data->data)
- seq_printf(m, "%s", field_data->data);
+ seq_puts(m, field_data->data);
/* skip ':' and '\0' */
buf_ptr += 2;
@@ -87,7 +87,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
ima_print_digest(m, buf_ptr, buflen);
break;
case DATA_FMT_STRING:
- seq_printf(m, "%s", buf_ptr);
+ seq_puts(m, buf_ptr);
break;
default:
break;
--
2.22.0
^ permalink raw reply related
* [PATCH] ima: Replace two seq_printf() calls by seq_puts() in ima_show_template_data_ascii()
From: Markus Elfring @ 2019-07-02 19:00 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Dmitry Kasatkin,
James Morris, Mimi Zohar, Serge E. Hallyn
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 20:52:21 +0200
Two strings which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/integrity/ima/ima_template_lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 9fe0ef7f91e2..05636e9b19b1 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -74,7 +74,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
case DATA_FMT_DIGEST_WITH_ALGO:
buf_ptr = strnchr(field_data->data, buflen, ':');
if (buf_ptr != field_data->data)
- seq_printf(m, "%s", field_data->data);
+ seq_puts(m, field_data->data);
/* skip ':' and '\0' */
buf_ptr += 2;
@@ -87,7 +87,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
ima_print_digest(m, buf_ptr, buflen);
break;
case DATA_FMT_STRING:
- seq_printf(m, "%s", buf_ptr);
+ seq_puts(m, buf_ptr);
break;
default:
break;
--
2.22.0
^ permalink raw reply related
* [PATCH] ima: Replace two seq_printf() calls by seq_puts() in ima_show_template_data_ascii()
From: Markus Elfring @ 2019-07-02 19:00 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Dmitry Kasatkin,
James Morris, Mimi Zohar, Serge E. Hallyn
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 20:52:21 +0200
Two strings which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/integrity/ima/ima_template_lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 9fe0ef7f91e2..05636e9b19b1 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -74,7 +74,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
case DATA_FMT_DIGEST_WITH_ALGO:
buf_ptr = strnchr(field_data->data, buflen, ':');
if (buf_ptr != field_data->data)
- seq_printf(m, "%s", field_data->data);
+ seq_puts(m, field_data->data);
/* skip ':' and '\0' */
buf_ptr += 2;
@@ -87,7 +87,7 @@ static void ima_show_template_data_ascii(struct seq_file *m,
ima_print_digest(m, buf_ptr, buflen);
break;
case DATA_FMT_STRING:
- seq_printf(m, "%s", buf_ptr);
+ seq_puts(m, buf_ptr);
break;
default:
break;
--
2.22.0
^ permalink raw reply related
* RE: [RFC PATCH 0/1] security: add SECURE_KEEP_FSUID to preserve fsuid/fsgid across execve
From: Lubashev, Igor @ 2019-07-03 0:58 UTC (permalink / raw)
To: James Morris
Cc: Serge Hallyn, linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <alpine.LRH.2.21.1906142049480.3646@namei.org>
> From: James Morris <jmorris@namei.org> on Friday, June 14, 2019 11:54 PM:
> On Sat, 15 Jun 2019, Lubashev, Igor wrote:
>
> > Unfortunately, perf is using uid==0 and euid==0 as a "capability bits".
> >
> >
> > In tools/perf/util/evsel.c:
> > static bool perf_event_can_profile_kernel(void)
> > {
> > return geteuid() == 0 || perf_event_paranoid() == -1;
> > }
> >
> > In tools/perf/util/symbol.c:
> > static bool symbol__read_kptr_restrict(void)
> > {
> > ...
> > value = ((geteuid() != 0) || (getuid() != 0)) ?
> > (atoi(line) != 0) :
> > (atoi(line) == 2);
> > ...
> > }
>
> These are bugs. They should be checking for CAP_SYS_ADMIN.
Thanks for the suggestion.
Actually, the former one should be checking CAP_SYS_ADMIN, while the latter -- CAP_SYSLOG (see lib/vsprintf.c).
Just posted a patch to perf (http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664552.html).
Thank you,
- Igor
^ permalink raw reply
* Re: [PATCH v10 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Andrew Morton @ 2019-07-02 22:59 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Christoph Lameter, Kees Cook, Michal Hocko, James Morris,
Masahiro Yamada, Michal Hocko, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Qian Cai, linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190628093131.199499-2-glider@google.com>
On Fri, 28 Jun 2019 11:31:30 +0200 Alexander Potapenko <glider@google.com> wrote:
> The new options are needed to prevent possible information leaks and
> make control-flow bugs that depend on uninitialized values more
> deterministic.
>
> This is expected to be on-by-default on Android and Chrome OS. And it
> gives the opportunity for anyone else to use it under distros too via
> the boot args. (The init_on_free feature is regularly requested by
> folks where memory forensics is included in their threat models.)
>
> init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
> objects with zeroes. Initialization is done at allocation time at the
> places where checks for __GFP_ZERO are performed.
>
> init_on_free=1 makes the kernel initialize freed pages and heap objects
> with zeroes upon their deletion. This helps to ensure sensitive data
> doesn't leak via use-after-free accesses.
>
> Both init_on_alloc=1 and init_on_free=1 guarantee that the allocator
> returns zeroed memory. The two exceptions are slab caches with
> constructors and SLAB_TYPESAFE_BY_RCU flag. Those are never
> zero-initialized to preserve their semantics.
>
> Both init_on_alloc and init_on_free default to zero, but those defaults
> can be overridden with CONFIG_INIT_ON_ALLOC_DEFAULT_ON and
> CONFIG_INIT_ON_FREE_DEFAULT_ON.
>
> If either SLUB poisoning or page poisoning is enabled, those options
> take precedence over init_on_alloc and init_on_free: initialization is
> only applied to unpoisoned allocations.
>
> Slowdown for the new features compared to init_on_free=0,
> init_on_alloc=0:
>
> hackbench, init_on_free=1: +7.62% sys time (st.err 0.74%)
> hackbench, init_on_alloc=1: +7.75% sys time (st.err 2.14%)
>
> Linux build with -j12, init_on_free=1: +8.38% wall time (st.err 0.39%)
> Linux build with -j12, init_on_free=1: +24.42% sys time (st.err 0.52%)
> Linux build with -j12, init_on_alloc=1: -0.13% wall time (st.err 0.42%)
> Linux build with -j12, init_on_alloc=1: +0.57% sys time (st.err 0.40%)
>
> The slowdown for init_on_free=0, init_on_alloc=0 compared to the
> baseline is within the standard error.
>
> The new features are also going to pave the way for hardware memory
> tagging (e.g. arm64's MTE), which will require both on_alloc and on_free
> hooks to set the tags for heap objects. With MTE, tagging will have the
> same cost as memory initialization.
>
> Although init_on_free is rather costly, there are paranoid use-cases where
> in-memory data lifetime is desired to be minimized. There are various
> arguments for/against the realism of the associated threat models, but
> given that we'll need the infrastructure for MTE anyway, and there are
> people who want wipe-on-free behavior no matter what the performance cost,
> it seems reasonable to include it in this series.
>
> ...
>
> v10:
> - added Acked-by: tags
> - converted pr_warn() to pr_info()
There are unchangelogged alterations between v9 and v10. The
replacement of IS_ENABLED(CONFIG_PAGE_POISONING)) with
page_poisoning_enabled().
--- a/mm/page_alloc.c~mm-security-introduce-init_on_alloc=1-and-init_on_free=1-boot-options-v10
+++ a/mm/page_alloc.c
@@ -157,8 +157,8 @@ static int __init early_init_on_alloc(ch
if (!buf)
return -EINVAL;
ret = kstrtobool(buf, &bool_result);
- if (bool_result && IS_ENABLED(CONFIG_PAGE_POISONING))
- pr_warn("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_alloc\n");
+ if (bool_result && page_poisoning_enabled())
+ pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_alloc\n");
if (bool_result)
static_branch_enable(&init_on_alloc);
else
@@ -175,8 +175,8 @@ static int __init early_init_on_free(cha
if (!buf)
return -EINVAL;
ret = kstrtobool(buf, &bool_result);
- if (bool_result && IS_ENABLED(CONFIG_PAGE_POISONING))
- pr_warn("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_free\n");
+ if (bool_result && page_poisoning_enabled())
+ pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_free\n");
if (bool_result)
static_branch_enable(&init_on_free);
else
--- a/mm/slub.c~mm-security-introduce-init_on_alloc=1-and-init_on_free=1-boot-options-v10
+++ a/mm/slub.c
@@ -1281,9 +1281,8 @@ check_slabs:
out:
if ((static_branch_unlikely(&init_on_alloc) ||
static_branch_unlikely(&init_on_free)) &&
- (slub_debug & SLAB_POISON)) {
- pr_warn("mem auto-init: SLAB_POISON will take precedence over init_on_alloc/init_on_free\n");
- }
+ (slub_debug & SLAB_POISON))
+ pr_info("mem auto-init: SLAB_POISON will take precedence over init_on_alloc/init_on_free\n");
return 1;
}
_
^ permalink raw reply
* [PATCH -next] integrity: Remove set but not used variable 'acl'
From: YueHaibing @ 2019-07-03 2:55 UTC (permalink / raw)
To: James Morris, Serge E. Hallyn, David Howells, Kairui Song,
Thomas Gleixner, Mimi Zohar, Thiago Jung Bauermann, Nayna Jain,
Eric Biggers
Cc: YueHaibing, linux-security-module, linux-kernel, kernel-janitors
Fixes gcc '-Wunused-but-set-variable' warning:
security/integrity/digsig.c: In function 'integrity_init_keyring':
security/integrity/digsig.c:99:18: warning:
variable 'acl' set but not used [-Wunused-but-set-variable]
It seems 'acl' is needed in __integrity_init_keyring
Fixes: 6100ac53909d ("keys: Replace uid/gid/perm permissions checking with an ACL")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
security/integrity/digsig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 1f2c43f62047..f9f3c8ffe786 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -114,7 +114,7 @@ int __init integrity_init_keyring(const unsigned int id)
acl = &internal_writable_keyring_acl;
out:
- return __integrity_init_keyring(id, &internal_keyring_acl, restriction);
+ return __integrity_init_keyring(id, acl, restriction);
}
static int __init integrity_add_key(const unsigned int id, const void *data,
^ permalink raw reply related
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