Re: WARNING in apparmor_secid_to_secctx

[Tetsuo Handa]

On 2019/02/01 19:09, Dmitry Vyukov wrote:
> Thanks for the explanations.
>
> Here is the change that I've come up with:
> https://github.com/google/syzkaller/commit/aa53be276dc84aa8b3825b3416542447ff82b41a

You are not going to apply this updated config to upstream kernels now, are you?
Removing CONFIG_DEFAULT_SECURITY="apparmor" from configs used by upstream 
kernels
will cause failing to enable AppArmor (unless security=apparmor is specified).

I guess you can apply this updated config to linux-next kernels given that
you replace

  CONFIG_LSM="yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"

with

  CONFIG_LSM="yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo"

so that AppArmor is enabled instead of SELinux.

>
> I've disabled CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER (it
> actually looked like omitting a user-space loader that I don't have is
> the right thing to do, but okay, it indeed does not with =y).
>
> For now I just enabled TOMOYO and SAFESETID.
> I see the problem with making both linux-next and upstream work. If we
> use a single config and lsm= cmdline argument, then on upstream all
> kernels will use the same module (they won't understand lsm=). But if
> we add security= then it will take precedence over lsm= on linux-next,
> so we won't get stacked modules.

Right.

>
> Let's go with (c) because I don't want an additional long-term maintenance 
> cost.

OK.

> If I understand it correctly later we will need to replace:
> security=selinux
> security=smack
> security=apparmor
>
> with:
> lsm=yama,safesetid,integrity,selinux,tomoyo
> lsm=yama,safesetid,integrity,smack,tomoyo
> lsm=yama,safesetid,integrity,tomoyo,apparmor

Yes. Thank you.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] LSM: Allow syzbot to ignore security= parameter.

[Tetsuo Handa]

On 2019/02/01 19:50, Dmitry Vyukov wrote:
> On Fri, Feb 1, 2019 at 11:44 AM Tetsuo Handa
>  wrote:
>>
>> On 2019/02/01 19:09, Dmitry Vyukov wrote:
>>> Thanks for the explanations.
>>>
>>> Here is the change that I've come up with:
>>> https://github.com/google/syzkaller/commit/aa53be276dc84aa8b3825b3416542447ff82b41a
>>
>> You are not going to apply this updated config to upstream kernels now, are 
>> you?
>> Removing CONFIG_DEFAULT_SECURITY="apparmor" from configs used by upstream 
>> kernels
>> will cause failing to enable AppArmor (unless security=apparmor is 
>> specified).
> 
> 
> We do use  security=apparmor, see:
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-apparmor.cmdline
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-selinux.cmdline
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-smack.cmdline
> 

Oh, security= parameter is explicitly specified on all targets?
Then, we can abuse CONFIG_DEBUG_AID_FOR_SYZBOT option. ;-)

LSM folks, may we use this patch for linux-next.git ?
CONFIG_DEBUG_AID_FOR_SYZBOT is a linux-next.git-only kernel config option used 
by syzbot.



>From c7d21f9c1c0b610ddea4233b89edf7d3140b8baf Mon Sep 17 00:00:00 2001
From: Tetsuo Handa 
Date: Fri, 1 Feb 2019 22:03:55 +0900
Subject: [PATCH linux-next] LSM: Allow syzbot to ignore security= parameter.

LSM is going to get infrastructure managed security blob support in Linux
5.1, and it becomes possible to run TOMOYO with SELinux/Smack/AppArmor.
But for compatibility reason, since security= parameter makes it
impossible to run TOMOYO with SELinux/Smack/AppArmor, syzbot can't
test that combination. Therefore, this patch allows syzbot to temporarily
ignore security= parameter. This patch is meant for linux-next.git only,
and will be removed after infrastructure managed security blob support
went to linux.git.

Signed-off-by: Tetsuo Handa 
---
 security/security.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/security.c b/security/security.c
index ef03643..0632feb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -346,12 +346,14 @@ int __init security_init(void)
 }
 
 /* Save user chosen LSM */
+#ifndef CONFIG_DEBUG_AID_FOR_SYZBOT
 static int __init choose_major_lsm(char *str)
 {
chosen_major_lsm = str;
return 1;
 }
 __setup("security=", choose_major_lsm);
+#endif
 
 /* Explicitly choose LSM initialization order. */
 static int __init choose_lsm_order(char *str)
-- 
1.8.3.1

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: WARNING in apparmor_secid_to_secctx

[Tetsuo Handa]

On 2019/01/30 23:45, Dmitry Vyukov wrote:
>> Dmitry, is it possible to update configs for linux-next.git , for
>> we want to test a big change in LSM which will go to Linux 5.1 ?
>>
>> TOMOYO security module (CONFIG_SECURITY_TOMOYO=y) can now coexist with
>> SELinux/Smack/AppArmor security modules, and SafeSetID security module
>> (CONFIG_SECURITY_SAFESETID=y) was added. Testing with these modules also
>> enabled might find something...
> 
> Hi,
> 
> syzbot configs/cmdline args are stored here:
> https://github.com/google/syzkaller/tree/master/dashboard/config
> 
> I've tried to update to the latest kernel, the diff is below.
> Few questions:
> 1. How are modules enabled now?
> We pass security=selinux of security=smack on command line. What do we
> need to pass now to enable several modules at the same time?

Removing security= parameter from kernel boot command line will do it.

  security/apparmor/lsm.c:.flags = LSM_FLAG_LEGACY_MAJOR | 
LSM_FLAG_EXCLUSIVE,
  security/selinux/hooks.c:   .flags = LSM_FLAG_LEGACY_MAJOR | 
LSM_FLAG_EXCLUSIVE,
  security/smack/smack_lsm.c: .flags = LSM_FLAG_LEGACY_MAJOR | 
LSM_FLAG_EXCLUSIVE,
  security/tomoyo/tomoyo.c:   .flags = LSM_FLAG_LEGACY_MAJOR,
  security/security.c:if ((major->flags & 
LSM_FLAG_LEGACY_MAJOR) &&

But this means that, if same kernel config/cmdline are used between
linux-next.git and linux.git (etc.), syzbot will need to choose from

 (a) stop sharing kernel cmdline between linux-next.git and linux.git (etc.)

or

 (b) stop sharing kernel config between SELinux, Smack and AppArmor

or

 (c) start testing after the LSM changes went to linux.git as Linux 5.1-rc1

. Is (a) or (b) possible? If this is a too much change, (c) will be OK.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: WARNING in apparmor_secid_to_secctx

[Tetsuo Handa]

On 2018/09/06 19:59, Dmitry Vyukov wrote:
> On Wed, Sep 5, 2018 at 7:37 PM, Casey Schaufler  
> wrote:
>> On 9/5/2018 4:08 AM, Dmitry Vyukov wrote:
>>> Thanks! I've re-enabled selinux on syzbot:
>>> https://github.com/google/syzkaller/commit/196410e4f5665d4d2bf6c818d06f1c8d03cfa8cc
>>> Now we will have instances with apparmor and with selinux.
>>
>> Any chance we could get a Smack instance as well?
> 
> Hi Casey,
> 
> Sure!
> Provided you want to fix bugs ;)
> I've setup an instance with smack enabled:
> https://github.com/google/syzkaller/commit/0bb7a7eb8e0958c6fbe2d69615b9fae4af88c8ee
> 

Dmitry, is it possible to update configs for linux-next.git , for
we want to test a big change in LSM which will go to Linux 5.1 ?

TOMOYO security module (CONFIG_SECURITY_TOMOYO=y) can now coexist with
SELinux/Smack/AppArmor security modules, and SafeSetID security module
(CONFIG_SECURITY_SAFESETID=y) was added. Testing with these modules also
enabled might find something...
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock

[Tetsuo Handa]

On 2018/09/25 2:16, Casey Schaufler wrote:
>> Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
>> might use security blobs for only a few objects. For example, AKARI uses
>> inode security blob for remembering whether source address/port of an
>> accept()ed socket was already checked, only during accept() operation and
>> first socket operation on the accept()ed socket. Thus, there is no need
>> to waste memory by assigning blobs for all inode objects.
> 
> The first question is why use an inode blob? Shouldn't you
> be using a socket blob for this socket based information?

Indeed. AKARI can as well use security_sk_free() using address of
"struct sock" as a key.

> 
> If you only want information part of the time you can declare
> a pointer sized blob and manage what hangs off that as you will.
> I personally think that the added complexity of conditional
> blob management is more pain than it's worth, but if you want
> a really big blob, but only on occasion, I could see doing it.

LKM based LSMs are too late for updating blob_sizes.* fields.
Even if they could, they after all have to somehow check whether
corresponding init hook was called. That's checking for NULL.

>>
 @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file)
  {
void *blob;
  
 +  call_void_hook(file_free_security, file);
 +
if (!lsm_file_cache)
return;
  
 -  call_void_hook(file_free_security, file);
 -
>>> Why does this make sense? If the lsm_file_cache isn't
>>> initialized you can't have allocated any file blobs,
>>> no module can have initialized a file blob, hence there
>>> can be nothing for the module to do.
>>>
>> For modules (not limited to LKM-based LSMs) which want to use
>> file blobs for only a few objects and avoid wasting memory by
>> allocating file blobs to all file objects.
>>
>> Infrastructure based blob management fits well for LSM modules
>> which want to assign blobs to all objects (like SELinux). But
>> forcing infrastructure based blob management can become a huge
>> waste of memory for LSM modules which want to assign blobs to
>> only a few objects. Unconditionally calling file_free_security
>> hook (as with other hooks) preserves a room for allowing the
>> latter type of LSM modules without using infrastructure based
>> blob management.
> 
> There is a hypothetical issue here, but that would require abuse
> of the infrastructure. Having a file_free_security hook that doesn't
> free a security blob allocated by file_alloc_security may coincidentaly
> be useful, but that's not the intent of the hook.
> 

The free hook might be used for freeing resources which were not allocated
by alloc hook. Yama is using task_free hook without task_alloc hook.
Someone might want to use file_free hook without file_alloc hook.

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock

[Tetsuo Handa]

On 2018/09/25 1:15, Casey Schaufler wrote:
    Since all free hooks are called when one of init hooks failed, each
    free hook needs to check whether init hook was called. An example is
    inode_free_security() in security/selinux/hooks.c (but not addressed in
    this patch).
>>>
>>> I *think* that selinux_inode_free_security() is safe in this
>>> case because the blob will be zeroed, hence isec->list will
>>> be NULL.
>>
>> That's not safe - look more closely at what list_empty_careful() tests, and 
>> then think about what happens when list_del_init() gets called on that 
>> isec->list.  selinux_inode_free_security() presumes that 
>> selinux_inode_alloc_security() has been called already.  If you are breaking 
>> that assumption, you have to fix it.
> 
> Yup. I misread the macro my first time around. Easy fix.

Oh, I didn't notice that it is doing !list_empty_careful() than 
list_empty_careful().
Unsafe indeed. But easy to fix.

> 
>> Is there a reason you can't make inode_alloc_security() return void since 
>> you moved the allocation to the framework? 
> 
> No reason with any of the existing modules, But I could see someone
> doing unnatural things during allocation that might result in a
> failure.

Currently upstreamed LSM modules and AKARI would be OK. But I can't guarantee it
for future / not-yet-upstreamed LSM modules.

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock

[Tetsuo Handa]

On 2018/09/23 11:43, Kees Cook wrote:
>>> I'm excited about getting this landed!
>>
>> Soon. Real soon. I hope. I would very much like for
>> someone from the SELinux camp to chime in, especially on
>> the selinux_is_enabled() removal.
> 
> Agreed.
> 

This patchset from Casey lands before the patchset from Kees, doesn't it?
OK, a few comments (if I didn't overlook something).

  lsm_early_cred()/lsm_early_task() are called from only __init functions.

  lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .

  lsm_early_inode() should be avoided because it is not appropriate to
  call panic() when lsm_early_inode() is called after __init phase.

  Since all free hooks are called when one of init hooks failed, each
  free hook needs to check whether init hook was called. An example is
  inode_free_security() in security/selinux/hooks.c (but not addressed in
  this patch).

  This patchset might fatally prevent LKM-based LSM modules, for LKM-based
  LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
  be updated upon loading LKM-based LSMs. If security_file_free() is called
  regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
  loaded using current behavior (apart from the fact that legitimate
  interface for appending to security_hook_heads is currently missing).
  How do you plan to handle LKM-based LSMs?

 include/linux/lsm_hooks.h  |6 ++
 security/security.c|   31 ++-
 security/smack/smack_lsm.c |8 +++-
 3 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7e8b32f..8014614 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2095,13 +2095,11 @@ static inline void __init yama_add_hooks(void) { }
 static inline void loadpin_add_hooks(void) { };
 #endif
 
-extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
 extern int lsm_inode_alloc(struct inode *inode);
 
 #ifdef CONFIG_SECURITY
-void lsm_early_cred(struct cred *cred);
-void lsm_early_inode(struct inode *inode);
-void lsm_early_task(struct task_struct *task);
+void __init lsm_early_cred(struct cred *cred);
+void __init lsm_early_task(struct task_struct *task);
 #endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/security.c b/security/security.c
index e7c85060..341e8df 100644
--- a/security/security.c
+++ b/security/security.c
@@ -267,7 +267,7 @@ int unregister_lsm_notifier(struct notifier_block *nb)
  *
  * Returns 0, or -ENOMEM if memory can't be allocated.
  */
-int lsm_cred_alloc(struct cred *cred, gfp_t gfp)
+static int lsm_cred_alloc(struct cred *cred, gfp_t gfp)
 {
if (blob_sizes.lbs_cred == 0) {
cred->security = NULL;
@@ -286,7 +286,7 @@ int lsm_cred_alloc(struct cred *cred, gfp_t gfp)
  *
  * Allocate the cred blob for all the modules if it's not already there
  */
-void lsm_early_cred(struct cred *cred)
+void __init lsm_early_cred(struct cred *cred)
 {
int rc;
 
@@ -344,7 +344,7 @@ void __init security_add_blobs(struct lsm_blob_sizes 
*needed)
  *
  * Returns 0, or -ENOMEM if memory can't be allocated.
  */
-int lsm_file_alloc(struct file *file)
+static int lsm_file_alloc(struct file *file)
 {
if (!lsm_file_cache) {
file->f_security = NULL;
@@ -379,25 +379,6 @@ int lsm_inode_alloc(struct inode *inode)
 }
 
 /**
- * lsm_early_inode - during initialization allocate a composite inode blob
- * @inode: the inode that needs a blob
- *
- * Allocate the inode blob for all the modules if it's not already there
- */
-void lsm_early_inode(struct inode *inode)
-{
-   int rc;
-
-   if (inode == NULL)
-   panic("%s: NULL inode.\n", __func__);
-   if (inode->i_security != NULL)
-   return;
-   rc = lsm_inode_alloc(inode);
-   if (rc)
-   panic("%s: Early inode alloc failed.\n", __func__);
-}
-
-/**
  * lsm_task_alloc - allocate a composite task blob
  * @task: the task that needs a blob
  *
@@ -466,7 +447,7 @@ int lsm_msg_msg_alloc(struct msg_msg *mp)
  *
  * Allocate the task blob for all the modules if it's not already there
  */
-void lsm_early_task(struct task_struct *task)
+void __init lsm_early_task(struct task_struct *task)
 {
int rc;
 
@@ -1202,11 +1183,11 @@ void security_file_free(struct file *file)
 {
void *blob;
 
+   call_void_hook(file_free_security, file);
+
if (!lsm_file_cache)
return;
 
-   call_void_hook(file_free_security, file);
-
blob = file->f_security;
file->f_security = NULL;
kmem_cache_free(lsm_file_cache, blob);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7843004..b0b4045 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -750,6 +750,13 @@ static int smack_set_mnt_opts(struct super_block *sb,
if (sp->smk_flags & SMK_SB_INITIALIZED)
return 0;
 
+

Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock

[Tetsuo Handa]

On 2018/09/24 2:09, Casey Schaufler wrote:
>>   Since all free hooks are called when one of init hooks failed, each
>>   free hook needs to check whether init hook was called. An example is
>>   inode_free_security() in security/selinux/hooks.c (but not addressed in
>>   this patch).
> 
> I *think* that selinux_inode_free_security() is safe in this
> case because the blob will be zeroed, hence isec->list will
> be NULL.
> 

OK.

>>   This patchset might fatally prevent LKM-based LSM modules, for LKM-based
>>   LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
>>   be updated upon loading LKM-based LSMs.
> 
> LKM based security modules will require dynamically sized blobs.
> These can be added to the scheme used here. Each blob would get a
> header identifying the modules for which it contains data. When an
> LKM is registered if has to declare it's blob space requirements
> and gets back the offsets. All alloc operations have to put their
> marks in the header. All LKM blob users have to check that the blob
> they are looking at has the required data.
> 
> module_cred(struct cred *cred) {
>   return cred->security + module_blob_sizes.lbs_cred;
> }
> 
> becomes
> 
> module_cred(struct cred *cred) {
>   if (blob_includes(module_id))
>   return cred->security + module_blob_sizes.lbs_cred;
>   return NULL;
> }
> 
> and the calling code needs to accept a NULL return.

Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
might use security blobs for only a few objects. For example, AKARI uses
inode security blob for remembering whether source address/port of an
accept()ed socket was already checked, only during accept() operation and
first socket operation on the accept()ed socket. Thus, there is no need
to waste memory by assigning blobs for all inode objects.

> Blobs can never get smaller because readjusting the offsets
> isn't going to work, so unloading an LKM security module isn't
> going to be as complete as you might like. There may be a way
> around this if you unload all the LKM modules, but that's a
> special case and there may be dragon lurking in the mist.

If LKM-based LSMs who want to use security blobs have to check for
NULL return, they might choose "not using infrastructure managed
security blobs" and "using locally hashed blobs associated with
object's address" (like AKARI does).

> 
>>  If security_file_free() is called
>>   regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
>>   loaded using current behavior (apart from the fact that legitimate
>>   interface for appending to security_hook_heads is currently missing).
>>   How do you plan to handle LKM-based LSMs?
> 
> My position all along has been that I don't plan to handle LKM
> based LSMs, but that I won't do anything to prevent someone else
> from adding them later. I believe that I've done that. Several
> designs, including a separate list for dynamically loaded modules
> have been proposed. I think some of those would work.

Though AKARI is not using security_file_free(), some of LKM-based LSMs
might want to use it. If file_free_security hook is called unconditionally,
such LKM-based LSMs can be registered/unregistered, without worrying about
inability to shrink sizes for blobs.

>> @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file)
>>  {
>>  void *blob;
>>  
>> +call_void_hook(file_free_security, file);
>> +
>>  if (!lsm_file_cache)
>>  return;
>>  
>> -call_void_hook(file_free_security, file);
>> -
> 
> Why does this make sense? If the lsm_file_cache isn't
> initialized you can't have allocated any file blobs,
> no module can have initialized a file blob, hence there
> can be nothing for the module to do.
> 

For modules (not limited to LKM-based LSMs) which want to use
file blobs for only a few objects and avoid wasting memory by
allocating file blobs to all file objects.

Infrastructure based blob management fits well for LSM modules
which want to assign blobs to all objects (like SELinux). But
forcing infrastructure based blob management can become a huge
waste of memory for LSM modules which want to assign blobs to
only a few objects. Unconditionally calling file_free_security
hook (as with other hooks) preserves a room for allowing the
latter type of LSM modules without using infrastructure based
blob management.

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()

[Tetsuo Handa]

On 2018/09/13 12:02, Paul Moore wrote:
> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
>  wrote:
>> syzbot is hitting warning at str_read() [1] because len parameter can
>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
>> this case.
>>
>> [1] 
>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
>>
>> Signed-off-by: Tetsuo Handa 
>> Reported-by: syzbot 
>> ---
>>  security/selinux/ss/policydb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>> index e9394e7..f4eadd3 100644
>> --- a/security/selinux/ss/policydb.c
>> +++ b/security/selinux/ss/policydb.c
>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void 
>> *fp, u32 len)
>> if ((len == 0) || (len == (u32)-1))
>> return -EINVAL;
>>
>> -   str = kmalloc(len + 1, flags);
>> +   str = kmalloc(len + 1, flags | __GFP_NOWARN);
>> if (!str)
>> return -ENOMEM;
> 
> Thanks for the patch.
> 
> My eyes are starting to glaze over a bit chasing down all of the
> different kmalloc() code paths trying to ensure that this always does
> the right thing based on size of the allocation and the different slab
> allocators ... are we sure that this will always return NULL when (len
> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
> configurations?
> 

Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
ZERO_SIZE_PTR) due to (len == (u32)-1) check above.

The only concern would be whether you want allocation failure messages.
I assumed you don't need it because we are returning -ENOMEM to the caller.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()

[Tetsuo Handa]

syzbot is hitting warning at str_read() [1] because len parameter can
become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
this case.

[1] 
https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0

Signed-off-by: Tetsuo Handa 
Reported-by: syzbot 
---
 security/selinux/ss/policydb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index e9394e7..f4eadd3 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, 
u32 len)
if ((len == 0) || (len == (u32)-1))
return -EINVAL;
 
-   str = kmalloc(len + 1, flags);
+   str = kmalloc(len + 1, flags | __GFP_NOWARN);
if (!str)
return -ENOMEM;
 
-- 
1.8.3.1

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: BUG: Mount ignores mount options

[Tetsuo Handa]

On 2018/08/10 23:05, Eric W. Biederman wrote:
> 
> There is a serious problem with mount options today that fsopen does not
> address.  The problem is that mount options are ignored for block based
> filesystems, and any other type of filesystem that follows the same
> pattern.
> 
> The script below demonstrates this bug.  Showing this bug can cause the
> ext4 "acl" "quota" and "user_xattr" options to be silently ignored.
> 
> fsopen has my nack until it addresses this issue.
> 
> I don't know if we can fix this in the context of sys_mount.  But we if
> we are redoing the option parsing of how we mount filesystems this needs
> to be fixed before we start worrying about bug compatibility.
> 
> Hopefully this report is simple and clear enough that we can at least
> agree on the problem.
> 
> Eric

This might be related to a problem that syzbot is failing to reproduce a 
problem.

  https://groups.google.com/forum/#!msg/syzkaller-bugs/R03vI7RCVco/0PijCTrcCgAJ

  syzbot found a reproducer, and the reproducer was working until next-20180803.
  But the reproducer is failing to reproduce this problem in next-20180806 
despite
  there is no mm related change between next-20180803 and next-20180806.

  Therefore, I suspect that the reproducer is no longer working as intended. And
  there was parser change (David Howells' patch) between next-20180803 and 
next-20180806.

I'm waiting for response from David Howells...
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH 0/8] LSM: Security module stacking

[Tetsuo Handa]

Casey Schaufler wrote:
> 1/8: Add the smack subdirectory to /proc/.../attr
> 2/8: Move management of cred security blobs to the LSM infrastructure
> 3/8: Move management of file security blobs to the LSM infrastructure
> 4/8: Move management of task security blobs to the LSM infrastructure
> 5/8: Move management of the remaining security blobs to the LSM infrastructure
> 6/8: Change the configuration controls for security stacking
> 7/8: Allow multiple modules to provide mount options
> 8/8: Maintain and use compound secids instead of a single integer
> 
> Also available git://github.com/cschaufler/lsm_stacking.git#stacking-4.17

s/lsm_stacking/lsm-stacking/



Is there any objection regarding "LSM: Security module stacking" itself?
If no objections, can we use step-by-step changes? We are so easily
overlooking error handler paths because this series is trying to change
all-at-once.

For example, security_inode_alloc() will cause an oops if SELinux is not
the first module linked to the inode_alloc_security list and an error
occurred inside the first module, for security_inode_alloc() will call
selinux_inode_free_security() even if selinux_inode_alloc_security() was
not called while "struct inode_security_struct" is initialized only when
selinux_inode_alloc_security() is called. And I don't like fixing it in
this series or next series, for we will likely overlook somewhere else.


static int inode_alloc_security(struct inode *inode)
{
struct inode_security_struct *isec = selinux_inode(inode);
u32 sid = current_sid();

spin_lock_init(>lock);
INIT_LIST_HEAD(>list);
isec->inode = inode;
isec->sid = SECINITSID_UNLABELED;
isec->sclass = SECCLASS_FILE;
isec->task_sid = sid;
isec->initialized = LABEL_INVALID;

return 0;
}

static int selinux_inode_alloc_security(struct inode *inode)
{
return inode_alloc_security(inode);
}

static void inode_free_security(struct inode *inode)
{
struct inode_security_struct *isec = selinux_inode(inode);
struct superblock_security_struct *sbsec =
selinux_superblock(inode->i_sb);

/*
 * As not all inode security structures are in a list, we check for
 * empty list outside of the lock to make sure that we won't waste
 * time taking a lock doing nothing.
 *
 * The list_del_init() function can be safely called more than once.
 * It should not be possible for this function to be called with
 * concurrent list_add(), but for better safety against future changes
 * in the code, we use list_empty_careful() here.
 */
if (!list_empty_careful(>list)) {
spin_lock(>isec_lock);
list_del_init(>list);
spin_unlock(>isec_lock);
}
}

static void selinux_inode_free_security(struct inode *inode)
{
inode_free_security(inode);
}

int security_inode_alloc(struct inode *inode)
{
int rc = lsm_inode_alloc(inode);

if (unlikely(rc))
return rc;
rc = call_int_hook(inode_alloc_security, 0, inode);
if (unlikely(rc))
security_inode_free(inode);
return rc;
}

void security_inode_free(struct inode *inode)
{
integrity_inode_free(inode);
call_void_hook(inode_free_security, inode);
/*
 * The inode may still be referenced in a path walk and
 * a call to security_inode_permission() can be made
 * after inode_free_security() is called. Ideally, the VFS
 * wouldn't do this, but fixing that is a much harder
 * job. For now, simply free the i_security via RCU, and
 * leave the current inode->i_security pointer intact.
 * The inode will be freed after the RCU grace period too.
 */
if (inode->i_security)
call_rcu((struct rcu_head *)inode->i_security,
inode_free_by_rcu);
}


What I prefer is to split this series into patches which involve functional
changes and patches which do not involve functional changes. For example,
we can apply

-   const struct task_security_struct *tsec = cred->security;
+   const struct task_security_struct *tsec = selinux_cred(cred);

by using a stub

+#define selinux_cred(cred) ((cred)->security)

and apply

 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
 {
-   return call_int_hook(cred_alloc_blank, 0, cred, gfp);
+   int rc = lsm_cred_alloc(cred, gfp);
+
+   if (rc)
+   return rc;
+
+   rc = call_int_hook(cred_alloc_blank, 0, cred, gfp);
+   if (rc)
+   lsm_cred_free(cred);
+   return rc;
 }

by using a stub

+#define lsm_cred_alloc(cred, gfp) 0
+#define lsm_cred_free(cred, gfp) do { } while(0)

before making other changes. Such stubs will allow each 

Re: KASAN: slab-out-of-bounds Read in strcmp

[Tetsuo Handa]

Tetsuo Handa wrote:
> which will allow strcmp() to trigger out of bound read when "size" is
> larger than strlen(initial_sid_to_string[i]).

Oops. "smaller" than.

> 
> Thus, I guess the simplest fix is to use strncmp() instead of strcmp().

Can somebody test below patch? (My CentOS 7 environment does not support
enabling SELinux in linux.git . Userspace tool is too old to support?)
--
>From 3efab617f7c22360361a2bd89a0ccaf3bcd47951 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Date: Sun, 3 Dec 2017 22:12:17 +0900
Subject: [PATCH] selinux: Fix out of bounds read at
 security_context_to_sid_core()

Syzbot caught an out of bounds read at security_context_to_sid_core()
because security_context_to_sid_core() assumed that the value written to
/proc/pid/attr interface is terminated with either '\0' or '\n'.
When the value is not terminated with either '\0' or '\n' and
scontext_len < strlen(initial_sid_to_string[i]) is true, strcmp() will
trigger out of bounds read.

--
BUG: KASAN: slab-out-of-bounds in strcmp+0x96/0xb0 lib/string.c:328
Read of size 1 at addr 8801cd99d2c1 by task syzkaller242593/3087

CPU: 0 PID: 3087 Comm: syzkaller242593 Not tainted 4.15.0-rc1-next-20171201+ #57
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x194/0x257 lib/dump_stack.c:53
 print_address_description+0x73/0x250 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351 [inline]
 kasan_report+0x25b/0x340 mm/kasan/report.c:409
 __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:427
 strcmp+0x96/0xb0 lib/string.c:328
 security_context_to_sid_core+0x437/0x620 security/selinux/ss/services.c:1420
 security_context_to_sid+0x32/0x40 security/selinux/ss/services.c:1479
 selinux_setprocattr+0x51c/0xb50 security/selinux/hooks.c:5986
 security_setprocattr+0x85/0xc0 security/security.c:1264
 proc_pid_attr_write+0x1e6/0x280 fs/proc/base.c:2574
 __vfs_write+0xef/0x970 fs/read_write.c:480
 __kernel_write+0xfe/0x350 fs/read_write.c:501
 write_pipe_buf+0x175/0x220 fs/splice.c:797
 splice_from_pipe_feed fs/splice.c:502 [inline]
 __splice_from_pipe+0x328/0x730 fs/splice.c:626
 splice_from_pipe+0x1e9/0x330 fs/splice.c:661
 default_file_splice_write+0x40/0x90 fs/splice.c:809
 do_splice_from fs/splice.c:851 [inline]
 direct_splice_actor+0x125/0x180 fs/splice.c:1018
 splice_direct_to_actor+0x2c1/0x820 fs/splice.c:973
 do_splice_direct+0x2a7/0x3d0 fs/splice.c:1061
 do_sendfile+0x5d5/0xe90 fs/read_write.c:1413
 SYSC_sendfile64 fs/read_write.c:1468 [inline]
 SyS_sendfile64+0xbd/0x160 fs/read_write.c:1460
 entry_SYSCALL_64_fastpath+0x1f/0x96
--

Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Reported-by: syzbot <syzkal...@googlegroups.com>
---
 security/selinux/ss/services.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 33cfe5d..2b2ce3e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1417,7 +1417,9 @@ static int security_context_to_sid_core(const char 
*scontext, u32 scontext_len,
int i;
 
for (i = 1; i < SECINITSID_NUM; i++) {
-   if (!strcmp(initial_sid_to_string[i], scontext)) {
+   if (!strncmp(initial_sid_to_string[i], scontext,
+scontext_len) &&
+   !initial_sid_to_string[i][scontext_len]) {
*sid = i;
return 0;
}
-- 
1.8.3.1

Re: KASAN: slab-out-of-bounds Read in strcmp

[Tetsuo Handa]

Stephen Smalley wrote:
> > Thus, I guess the simplest fix is to use strncmp() instead of
> > strcmp().
> 
> Already fixed by
> https://www.spinics.net/lists/selinux/msg23589.html
> 

OK, I thought everyone was too busy.
I would appreciate if you responded to all.

Re: KASAN: slab-out-of-bounds Read in strcmp

[Tetsuo Handa]

James Morris wrote:
> On Sun, 3 Dec 2017, Tetsuo Handa wrote:
> 
> > Tetsuo Handa wrote:
> > > which will allow strcmp() to trigger out of bound read when "size" is
> > > larger than strlen(initial_sid_to_string[i]).
> > 
> > Oops. "smaller" than.
> > 
> > > 
> > > Thus, I guess the simplest fix is to use strncmp() instead of strcmp().
> > 
> > Can somebody test below patch? (My CentOS 7 environment does not support
> > enabling SELinux in linux.git . Userspace tool is too old to support?)
> 
> You mean enabling KASAN?  Yep, you need gcc 4.9.2 or better.  Recent 
> Fedora has it.

I was not able to find "SELinux:  Initializing." line for some reason, and
it turned out that I just forgot to run "make install". ;-)

I tested using debug printk() and init for built-in initramfs shown below.
It is strange that KASAN does not trigger upon strcmp()ing
initial_sid_to_string[1]. But anyway, my patch fixes this problem.

--
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5973,6 +5973,7 @@ static int selinux_setprocattr(const char *name, void 
*value, size_t size)
 PROCESS__SETCURRENT, NULL);
else
error = -EINVAL;
+   printk("setprocattr %s=%d size=%lu\n", name, error, size);
if (error)
return error;

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 33cfe5d..fbf0ade 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1417,6 +1417,7 @@ static int security_context_to_sid_core(const char 
*scontext, u32 scontext_len,
int i;

for (i = 1; i < SECINITSID_NUM; i++) {
+   printk("Comparing with %s\n", initial_sid_to_string[i]);
if (!strcmp(initial_sid_to_string[i], scontext)) {
*sid = i;
return 0;
--

--
#include 
#include 
#include 
#include 
#include 
#include 

int main(int argc, char *argv[])
{
int fd;
mount("/proc", "/proc", "proc", 0, NULL);
fd = open("/proc/self/attr/current", O_WRONLY);
write(fd, "n", 1);
close(fd);
return 0;
}
--

--
[7.894061] Write protecting the kernel read-only data: 71680k
[7.899889] Freeing unused kernel memory: 1744K
[7.923592] Freeing unused kernel memory: 1832K
[7.926960] setprocattr current=0 size=1
[7.928253] Comparing with kernel
[7.929350] Comparing with security
[7.930457] Comparing with unlabeled
[7.931581] Comparing with fs
[7.932538] Comparing with file
[7.933537] Comparing with file_labels
[7.934720] Comparing with init
[7.935719] Comparing with any_socket
[7.936866] Comparing with port
[7.937874] Comparing with netif
[7.938965] 
==
[7.941183] BUG: KASAN: slab-out-of-bounds in strcmp+0x96/0xb0
[7.942957] Read of size 1 at addr 8801145a5441 by task init/1
[7.944832] 
[7.945349] CPU: 2 PID: 1 Comm: init Not tainted 4.15.0-rc2+ #323
[7.947177] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/02/2015
[7.950331] Call Trace:
[7.951133]  dump_stack+0x12e/0x188
[7.95]  ? vprintk_default+0x28/0x30
[7.953431]  ? strcmp+0x96/0xb0
[7.954421]  print_address_description+0x73/0x260
[7.955860]  ? strcmp+0x96/0xb0
[7.956855]  kasan_report+0x22b/0x340
[7.957987]  __asan_report_load1_noabort+0x14/0x20
[7.959460]  strcmp+0x96/0xb0
[7.960408]  security_context_to_sid_core+0x312/0x450
[7.961945]  ? string_to_context_struct+0x940/0x940
[7.963434]  ? vprintk_func+0x5e/0xc0
[7.964564]  ? printk+0xaa/0xca
[7.965554]  ? show_regs_print_info+0x65/0x65
[7.966876]  ? proc_pid_attr_write+0x169/0x280
[7.968178]  security_context_to_sid+0x32/0x40
[7.969480]  selinux_setprocattr+0x2e1/0x8f0
[7.970734]  ? ptrace_parent_sid+0x400/0x400
[7.972034]  security_setprocattr+0x85/0xc0
[7.973326]  proc_pid_attr_write+0x1d8/0x280
[7.974638]  __vfs_write+0x10d/0x610
[7.975746]  ? comm_write+0x230/0x230
[7.976903]  ? kernel_read+0x120/0x120
[7.978064]  ? __might_sleep+0x95/0x190
[7.979266]  ? rcu_read_lock_sched_held+0xa3/0x120
[7.980722]  ? rcu_sync_lockdep_assert+0x70/0xb0
[7.982134]  ? __sb_start_write+0x211/0x2d0
[7.983413]  vfs_write+0x18d/0x510
[7.984477]  SyS_write+0xd4/0x1a0
[7.985518]  ? SyS_read+0x1a0/0x1a0
[7.986622]  ? trace_hardirqs_on_caller+0x442/0x5c0
[7.988107]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[7.989524]  entry_SYSCALL_64_fastpath+0x1f/0x96
[7.990928] RIP: 0033:0x40f7a0
[ 

Re: KASAN: slab-out-of-bounds Read in strcmp

[Tetsuo Handa]

Tetsuo Handa wrote:
> James Morris wrote:
> > On Sun, 3 Dec 2017, Tetsuo Handa wrote:
> > 
> > > Tetsuo Handa wrote:
> > > > which will allow strcmp() to trigger out of bound read when "size" is
> > > > larger than strlen(initial_sid_to_string[i]).
> > > 
> > > Oops. "smaller" than.
> > > 
> > > > 
> > > > Thus, I guess the simplest fix is to use strncmp() instead of strcmp().
> > > 
> > > Can somebody test below patch? (My CentOS 7 environment does not support
> > > enabling SELinux in linux.git . Userspace tool is too old to support?)
> > 
> > You mean enabling KASAN?  Yep, you need gcc 4.9.2 or better.  Recent 
> > Fedora has it.
> 
> I was not able to find "SELinux:  Initializing." line for some reason, and
> it turned out that I just forgot to run "make install". ;-)
> 
> I tested using debug printk() and init for built-in initramfs shown below.
> It is strange that KASAN does not trigger upon strcmp()ing
> initial_sid_to_string[1]. But anyway, my patch fixes this problem.

Sorry for noise. It was just initial_sid_to_string[10] which compared the 
second byte.
Thus, nothing is strange.

Re: KASAN: slab-out-of-bounds Read in strcmp

[Tetsuo Handa]

On 2017/12/02 3:52, syzbot wrote:
> ==
> BUG: KASAN: slab-out-of-bounds in strcmp+0x96/0xb0 lib/string.c:328
> Read of size 1 at addr 8801cd99d2c1 by task syzkaller242593/3087
> 
> CPU: 0 PID: 3087 Comm: syzkaller242593 Not tainted 4.15.0-rc1-next-20171201+ 
> #57
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS 
> Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:17 [inline]
>  dump_stack+0x194/0x257 lib/dump_stack.c:53
>  print_address_description+0x73/0x250 mm/kasan/report.c:252
>  kasan_report_error mm/kasan/report.c:351 [inline]
>  kasan_report+0x25b/0x340 mm/kasan/report.c:409
>  __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:427
>  strcmp+0x96/0xb0 lib/string.c:328

This seems to be out of bound read for "scontext" at

for (i = 1; i < SECINITSID_NUM; i++) {
if (!strcmp(initial_sid_to_string[i], scontext)) {
*sid = i;
return 0;
}
}

because "initial_sid_to_string[i]" is "const char *".

>  security_context_to_sid_core+0x437/0x620 security/selinux/ss/services.c:1420
>  security_context_to_sid+0x32/0x40 security/selinux/ss/services.c:1479
>  selinux_setprocattr+0x51c/0xb50 security/selinux/hooks.c:5986
>  security_setprocattr+0x85/0xc0 security/security.c:1264

If "value" does not terminate with '\0' or '\n', "value" and
"size" are as-is passed to "scontext" and "scontext_len" above

/* Obtain a SID for the context, if one was specified. */
if (size && str[0] && str[0] != '\n') {
if (str[size-1] == '\n') {
str[size-1] = 0;
size--;
}
error = security_context_to_sid(value, size, , GFP_KERNEL);

which will allow strcmp() to trigger out of bound read when "size" is
larger than strlen(initial_sid_to_string[i]).

Thus, I guess the simplest fix is to use strncmp() instead of strcmp().

>  proc_pid_attr_write+0x1e6/0x280 fs/proc/base.c:2574
>  __vfs_write+0xef/0x970 fs/read_write.c:480
>  __kernel_write+0xfe/0x350 fs/read_write.c:501
>  write_pipe_buf+0x175/0x220 fs/splice.c:797
>  splice_from_pipe_feed fs/splice.c:502 [inline]
>  __splice_from_pipe+0x328/0x730 fs/splice.c:626
>  splice_from_pipe+0x1e9/0x330 fs/splice.c:661
>  default_file_splice_write+0x40/0x90 fs/splice.c:809
>  do_splice_from fs/splice.c:851 [inline]
>  direct_splice_actor+0x125/0x180 fs/splice.c:1018
>  splice_direct_to_actor+0x2c1/0x820 fs/splice.c:973
>  do_splice_direct+0x2a7/0x3d0 fs/splice.c:1061
>  do_sendfile+0x5d5/0xe90 fs/read_write.c:1413
>  SYSC_sendfile64 fs/read_write.c:1468 [inline]
>  SyS_sendfile64+0xbd/0x160 fs/read_write.c:1460
>  entry_SYSCALL_64_fastpath+0x1f/0x96

Re: suspicious __GFP_NOMEMALLOC in selinux

[Tetsuo Handa]

On 2017/08/03 17:11, Michal Hocko wrote:
> [CC Mel]
> 
> On Wed 02-08-17 17:45:56, Paul Moore wrote:
>> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko  wrote:
>>> Hi,
>>> while doing something completely unrelated to selinux I've noticed a
>>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
>>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
>>> on its own allows to access memory reserves while the later flag tells
>>> we cannot use memory reserves at all. The primary usecase for
>>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
>>> need.
>>>
>>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
>>> ioctls") which doesn't explain this aspect so let me ask. Why is the
>>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
>>> What makes this path important to access memory reserves?
>>
>> [NOTE: added the SELinux list to the CC line, please include that list
>> when asking SELinux questions]
> 
> Sorry about that. Will keep it in mind for next posts
>  
>> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
>> to security/selinux/avc.c, and digging a bit, I'm guessing commit
>> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
>> avc cache alloc as non-critical") and the avc_alloc_node() function.
> 
> Thanks for the pointer. That makes much more sense now. Back in 2012 we
> really didn't have a good way to distinguish non sleeping and atomic
> with reserves allocations.
>  
>> I can't say that I'm an expert at the vm subsystem and the variety of
>> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
>> security/selinux/avc.c seems reasonable and in keeping with the idea
>> behind commit 6290c2c43973.
> 
> What do you think about the following? I haven't tested it but it should
> be rather straightforward.

Why not at least __GFP_NOWARN ? And why not also __GFP_NOMEMALLOC ?
http://lkml.kernel.org/r/201706302210.gca05089.mffotqvjsol...@i-love.sakura.ne.jp

Re: suspicious __GFP_NOMEMALLOC in selinux

[Tetsuo Handa]

Michal Hocko wrote:
> On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> > On 2017/08/03 17:11, Michal Hocko wrote:
> > > [CC Mel]
> > > 
> > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mho...@kernel.org> wrote:
> > >>> Hi,
> > >>> while doing something completely unrelated to selinux I've noticed a
> > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > >>> on its own allows to access memory reserves while the later flag tells
> > >>> we cannot use memory reserves at all. The primary usecase for
> > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > >>> need.
> > >>>
> > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > >>> What makes this path important to access memory reserves?
> > >>
> > >> [NOTE: added the SELinux list to the CC line, please include that list
> > >> when asking SELinux questions]
> > > 
> > > Sorry about that. Will keep it in mind for next posts
> > >  
> > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > > 
> > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > > really didn't have a good way to distinguish non sleeping and atomic
> > > with reserves allocations.
> > >  
> > >> I can't say that I'm an expert at the vm subsystem and the variety of
> > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> > >> security/selinux/avc.c seems reasonable and in keeping with the idea
> > >> behind commit 6290c2c43973.
> > > 
> > > What do you think about the following? I haven't tested it but it should
> > > be rather straightforward.
> > 
> > Why not at least __GFP_NOWARN ?
> 
> This would require an additional justification.

If allocation failure is not a problem, printing allocation failure messages
is nothing but noisy.

> 
> > And why not also __GFP_NOMEMALLOC ?
> 
> What would be the purpose of __GFP_NOMEMALLOC? In other words which
> context would set PF_NOMEMALLOC so that the flag would override it?
> 

When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
Since that function might be called from !in_interrupt() context, it is
possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
the OOM victim will dip into memory reserves even when allocation failure
is not a problem.

Thus, I think that majority of plain GFP_NOWAIT users want to use
GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

Re: [PATCH] selinux: return -ENOMEM if kzalloc() fails

[Tetsuo Handa]

Stephen Smalley wrote:
> On Fri, 2017-06-30 at 10:56 +0300, Dan Carpenter wrote:
> > We accidentally return success instead of -ENOMEM on this failure
> > path.
> > 
> > Fixes: 409dcf31538a ("selinux: Add a cache for quicker retreival of
> > PKey SIDs")
> > Signed-off-by: Dan Carpenter 
> 
> NAK, that's intentional.  See the comment just above the code in
> question.

If allocation failure is no problem, please consider using
GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN instead of
GFP_ATOMIC, for memory reserves is mainly targeted for OOM victims.

Re: [PATCH] selinux: fix double free in selinux_parse_opts_str()

[Tetsuo Handa]

Paul Moore wrote:
> On Fri, Mar 24, 2017 at 10:55 PM, Tetsuo Handa
> <penguin-ker...@i-love.sakura.ne.jp> wrote:
> > Paul Moore wrote:
> >> Hi,
> >>
> >> Thank you very much for this patch, but I think we need to look a bit
> >> harder at this problem as it appears that many callers assume that
> >> selinux_parse_opts_str() cleans up after itself.  Looking quickly I
> >> found what appear to be two problems, there are likely more ...
> >>
> >> * selinux_sb_remount()
> >> If selinux_parse_opts_str() fails here it doesn't appear we cleanup
> >> opts properly, although changing the jump target from
> >> "out_free_secdata" to "out_free_opts" would appear to correct this.
> >>
> >> * btrfs_mount()
> >> This function calls parse_security_options() which in turn calls
> >> security_sb_parse_opts_str(), but if parse_security_options() fails in
> >> this case the security_mnt_opts are not free'd.
> >>
> >> At this point I wonder if the quick fix is to set opts->mnt_opts to
> >> NULL after kfree()'ing it, or simply drop the kfree() call and call
> >> security_free_mnt_opts() in the out_err error handling code; the
> >> latter is a bit more work than needed, but I believe it should be safe
> >> in all conditions.
> >
> > I think the latter is better.
> > We might allow multiple LSM modules to parse mount options in future
> > (not limited to SELinux + Smack combination, small LSMs might want to
> > parse mount options). Then, calling a common function for releasing
> > memory allocated by individual module will become needed.
> 
> Hello,
> 
> I just wanted to check to see if you were going to do a follow up
> patch for this?  If not I'll put something together, but I didn't want
> to conflict with anything you were working on.

I have no plan on this. You can propose whatever you like.

Re: [PATCH] selinux: Use task_alloc hook rather than task_create hook

[Tetsuo Handa]

Paul Moore wrote:
> > Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
> > Acked-by: Stephen Smalley <s...@tycho.nsa.gov>
> > ---
> >  security/selinux/hooks.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> When are you planning to remove the task_create() hook?
> 
> I have no objection to this patch, and I plan to merge it, but merging
> it now would require rebasing the selinux/next and I try to keep from
> rebasing during the development cycle unless absolutely necessary.  I
> think this can wait until after the next merge window, what do you
> think?

Nothing to hurry. SELinux is the only user. We can wait as much as you want.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] selinux: Use task_alloc hook rather than task_create hook

[Tetsuo Handa]

Stephen Smalley wrote:
> On Tue, 2017-03-28 at 22:12 +0900, Tetsuo Handa wrote:
> > This patch is a preparation for getting rid of task_create hook
> > because
> > task_create hook
> 
> task_alloc hook?

Oops, copy error. Yes, I meant task_alloc hook.

> 
> >  which can do what task_create hook can do was revived.
> > 
> > Creating a new thread is unlikely prohibited by security policy, for
> > fork()/execve()/exit() is fundamental of how processes are managed in
> > Unix. If a program is known to create a new thread, it is likely that
> > permission to create a new thread is given to that program.
> > Therefore,
> > a situation where security_task_create() returns an error is likely
> > that
> > the program was exploited and lost control. Even if SELinux failed to
> > check permission to create a thread at security_task_create(),
> > SELinux
> > can later check it at security_task_alloc(). Since the new thread is
> > not
> > yet visible from the rest of the system, nobody can do bad things
> > using
> > the new thread. What we waste will be limited to some initialization
> > steps such as dup_task_struct(), copy_creds() and audit_alloc() in
> > copy_process(). We can tolerate these overhead for unlikely
> > situation.
> > 
> > Therefore, this patch changes SELinux to use task_alloc hook rather
> > than
> > task_create hook so that we can remove task_create hook.
> 
> Aside from the nit on the patch description above,
> 
> Acked-by: Stephen Smalley <s...@tycho.nsa.gov>

Thank you.

>From b43bd0fc0cc267b91f51ad118f6fabd13efb921e Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Date: Tue, 28 Mar 2017 22:09:38 +0900
Subject: [PATCH v2] selinux: Use task_alloc hook rather than task_create hook

This patch is a preparation for getting rid of task_create hook because
task_alloc hook which can do what task_create hook can do was revived.

Creating a new thread is unlikely prohibited by security policy, for
fork()/execve()/exit() is fundamental of how processes are managed in
Unix. If a program is known to create a new thread, it is likely that
permission to create a new thread is given to that program. Therefore,
a situation where security_task_create() returns an error is likely that
the program was exploited and lost control. Even if SELinux failed to
check permission to create a thread at security_task_create(), SELinux
can later check it at security_task_alloc(). Since the new thread is not
yet visible from the rest of the system, nobody can do bad things using
the new thread. What we waste will be limited to some initialization
steps such as dup_task_struct(), copy_creds() and audit_alloc() in
copy_process(). We can tolerate these overhead for unlikely situation.

Therefore, this patch changes SELinux to use task_alloc hook rather than
task_create hook so that we can remove task_create hook.

Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Acked-by: Stephen Smalley <s...@tycho.nsa.gov>
---
 security/selinux/hooks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..d850b7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3710,7 +3710,8 @@ static int selinux_file_open(struct file *file, const 
struct cred *cred)
 
 /* task security operations */
 
-static int selinux_task_create(unsigned long clone_flags)
+static int selinux_task_alloc(struct task_struct *task,
+ unsigned long clone_flags)
 {
u32 sid = current_sid();
 
@@ -6205,7 +6206,7 @@ static int selinux_key_getsecurity(struct key *key, char 
**_buffer)
 
LSM_HOOK_INIT(file_open, selinux_file_open),
 
-   LSM_HOOK_INIT(task_create, selinux_task_create),
+   LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
LSM_HOOK_INIT(cred_free, selinux_cred_free),
LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
-- 
1.8.3.1
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] selinux: Use task_alloc hook rather than task_create hook

[Tetsuo Handa]

This patch is a preparation for getting rid of task_create hook because
task_create hook which can do what task_create hook can do was revived.

Creating a new thread is unlikely prohibited by security policy, for
fork()/execve()/exit() is fundamental of how processes are managed in
Unix. If a program is known to create a new thread, it is likely that
permission to create a new thread is given to that program. Therefore,
a situation where security_task_create() returns an error is likely that
the program was exploited and lost control. Even if SELinux failed to
check permission to create a thread at security_task_create(), SELinux
can later check it at security_task_alloc(). Since the new thread is not
yet visible from the rest of the system, nobody can do bad things using
the new thread. What we waste will be limited to some initialization
steps such as dup_task_struct(), copy_creds() and audit_alloc() in
copy_process(). We can tolerate these overhead for unlikely situation.

Therefore, this patch changes SELinux to use task_alloc hook rather than
task_create hook so that we can remove task_create hook.

Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
---
 security/selinux/hooks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..d850b7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3710,7 +3710,8 @@ static int selinux_file_open(struct file *file, const 
struct cred *cred)
 
 /* task security operations */
 
-static int selinux_task_create(unsigned long clone_flags)
+static int selinux_task_alloc(struct task_struct *task,
+ unsigned long clone_flags)
 {
u32 sid = current_sid();
 
@@ -6205,7 +6206,7 @@ static int selinux_key_getsecurity(struct key *key, char 
**_buffer)
 
LSM_HOOK_INIT(file_open, selinux_file_open),
 
-   LSM_HOOK_INIT(task_create, selinux_task_create),
+   LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
LSM_HOOK_INIT(cred_free, selinux_cred_free),
LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
-- 
1.8.3.1

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] selinux: fix double free in selinux_parse_opts_str()

[Tetsuo Handa]

Paul Moore wrote:
> Hi,
> 
> Thank you very much for this patch, but I think we need to look a bit
> harder at this problem as it appears that many callers assume that
> selinux_parse_opts_str() cleans up after itself.  Looking quickly I
> found what appear to be two problems, there are likely more ...
> 
> * selinux_sb_remount()
> If selinux_parse_opts_str() fails here it doesn't appear we cleanup
> opts properly, although changing the jump target from
> "out_free_secdata" to "out_free_opts" would appear to correct this.
> 
> * btrfs_mount()
> This function calls parse_security_options() which in turn calls
> security_sb_parse_opts_str(), but if parse_security_options() fails in
> this case the security_mnt_opts are not free'd.
> 
> At this point I wonder if the quick fix is to set opts->mnt_opts to
> NULL after kfree()'ing it, or simply drop the kfree() call and call
> security_free_mnt_opts() in the out_err error handling code; the
> latter is a bit more work than needed, but I believe it should be safe
> in all conditions.

I think the latter is better.
We might allow multiple LSM modules to parse mount options in future
(not limited to SELinux + Smack combination, small LSMs might want to
parse mount options). Then, calling a common function for releasing
memory allocated by individual module will become needed.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] selinux: fix double free in selinux_parse_opts_str()

[Tetsuo Handa]

Combination of memory allocation failure injection and syzkaller fuzzer
found a double free bug.

--
BUG: Double free or freeing an invalid pointer
Unexpected shadow byte: 0xFB
CPU: 2 PID: 15269 Comm: syz-executor1 Not tainted 4.11.0-rc3+ #364
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x1b8/0x28d lib/dump_stack.c:52
 kasan_object_err+0x1c/0x70 mm/kasan/report.c:166
 kasan_report_double_free+0x5c/0x70 mm/kasan/report.c:193
 kasan_slab_free+0xab/0xc0 mm/kasan/kasan.c:584
 __cache_free mm/slab.c:3514 [inline]
 kfree+0xd7/0x250 mm/slab.c:3831
 security_free_mnt_opts include/linux/security.h:175 [inline]
 superblock_doinit+0x2a3/0x430 security/selinux/hooks.c:1165
 selinux_sb_kern_mount+0xb2/0x300 security/selinux/hooks.c:2783
 security_sb_kern_mount+0x7d/0xb0 security/security.c:331
 mount_fs+0x11b/0x2f0 fs/super.c:1233
 vfs_kern_mount.part.23+0xc6/0x4b0 fs/namespace.c:979
 vfs_kern_mount fs/namespace.c:3293 [inline]
 kern_mount_data+0x50/0xb0 fs/namespace.c:3293
 mq_init_ns+0x167/0x220 ipc/mqueue.c:1418
 create_ipc_ns ipc/namespace.c:57 [inline]
 copy_ipcs+0x39b/0x580 ipc/namespace.c:83
 create_new_namespaces+0x285/0x8c0 kernel/nsproxy.c:86
 unshare_nsproxy_namespaces+0xae/0x1e0 kernel/nsproxy.c:205
 SYSC_unshare kernel/fork.c:2319 [inline]
 SyS_unshare+0x664/0xf80 kernel/fork.c:2269
 entry_SYSCALL_64_fastpath+0x1f/0xc2
--

selinux_parse_opts_str() calls kfree(opts->mnt_opts) when kcalloc() for
opts->mnt_opts_flags failed. But it should not have called it because
security_free_mnt_opts() will call kfree(opts->mnt_opts).

Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Reported-by: Dmitry Vyukov <dvyu...@google.com>
fixes: e0007529893c1c06 ("LSM/SELinux: Interfaces to allow FS to control mount 
options")
Cc: Eric Paris <epa...@redhat.com>
Cc: Stephen Smalley <s...@tycho.nsa.gov>
Cc: Casey Schaufler <ca...@schaufler-ca.com>
Cc: James Morris <jmor...@namei.org>
---
 security/selinux/hooks.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..7f81d17 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1106,10 +1106,8 @@ static int selinux_parse_opts_str(char *options,
 
opts->mnt_opts_flags = kcalloc(NUM_SEL_MNT_OPTS, sizeof(int),
   GFP_KERNEL);
-   if (!opts->mnt_opts_flags) {
-   kfree(opts->mnt_opts);
+   if (!opts->mnt_opts_flags)
goto out_err;
-   }
 
if (fscontext) {
opts->mnt_opts[num_mnt_opts] = fscontext;
-- 
1.8.3.1

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: security, hugetlbfs: write to user memory in hugetlbfs_destroy_inode

[Tetsuo Handa]

Dmitry Vyukov wrote:
> On Thu, Mar 23, 2017 at 2:06 PM, Dmitry Vyukov  wrote:
> > Hello,
> >
> > I've got the following report while running syzkaller fuzzer on
> > 093b995e3b55a0ae0670226ddfcb05bfbf0099ae. Note the preceding injected
> > kmalloc failure in inode_alloc_security, most likely it's the root
> > cause.

I don't think inode_alloc_security() failure is the root cause.
I think this is a bug in hugetlbfs or mm part.

If inode_alloc_security() fails, inode->i_security remains NULL
which was initialized to NULL at security_inode_alloc(). Thus,
security_inode_alloc() is irrelevant to this problem.

inode_init_always() returned -ENOMEM due to fault injection and

if (unlikely(inode_init_always(sb, inode))) {
if (inode->i_sb->s_op->destroy_inode)
inode->i_sb->s_op->destroy_inode(inode);
else
kmem_cache_free(inode_cachep, inode);
return NULL;
}

hugetlbfs_destroy_inode() was called via inode->i_sb->s_op->destroy_inode()
when inode initialization failed

static void hugetlbfs_destroy_inode(struct inode *inode)
{
hugetlbfs_inc_free_inodes(HUGETLBFS_SB(inode->i_sb));
mpol_free_shared_policy(_I(inode)->policy);
call_rcu(>i_rcu, hugetlbfs_i_callback);
}

but mpol_shared_policy_init() is called only when new_inode() succeeds.

inode = new_inode(sb);
if (inode) {
(...snipped...)
info = HUGETLBFS_I(inode);
/*
 * The policy is initialized here even if we are creating a
 * private inode because initialization simply creates an
 * an empty rb tree and calls rwlock_init(), later when we
 * call mpol_free_shared_policy() it will just return because
 * the rb tree will still be empty.
 */
mpol_shared_policy_init(>policy, NULL);
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: security: double-free in superblock_doinit

[Tetsuo Handa]

Dmitry Vyukov wrote:
> Hello,
> 
> I've got the following double-free report in superblock_doinit while
> running syzkaller fuzzer.
> Note the preceding injected failure in kmalloc, most likely that the root 
> cause.

Thank you for reporting.

selinux_parse_opts_str() and smack_parse_opts_str() forgot to set
opts->mnt_opts to NULL after kfree() at

if (!opts->mnt_opts_flags) {
kfree(opts->mnt_opts);
goto out_err;
}

and caused double free at

if (opts->mnt_opts)
for (i = 0; i < opts->num_mnt_opts; i++)
kfree(opts->mnt_opts[i]);

in security_free_mnt_opts().
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [RFC 4/7] selinux: mark __ro_mostly_after_init for selinux_hooks/selinux_nf_ops

[Tetsuo Handa]

On 2017/02/19 19:04, Hoeun Ryu wrote:
>  It would be good that selinux hooks objects are marked as
> `__ro_mostly_after_init`. They can not be simply marked as `__ro_after_init'
> because they should be writable during selinux_disable procedure.
> `__ro_mostly_after_init` section is temporarily read-write during
> selinux_disable procedure via set_ro_mostly_after_init_rw/ro pair. Now that
> they can be read-only except during the procedure.
> 
> -static struct security_hook_list selinux_hooks[] = {
> +static struct security_hook_list selinux_hooks[] __ro_mostly_after_init = {

This won't work. This variable is array of "struct list_head".
You need to set same attribute to variables pointed by
"struct list_head"->next and "struct list_head"->prev .

>   LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>   LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>   LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

Casey Schaufler wrote:
> On 2/16/2017 3:00 AM, Tetsuo Handa wrote:
> > Casey Schaufler wrote:
> >> I can't say that I'm buying the value of the additional
> >> complexity here. Sure, you're protecting part of the data
> >> all the time, but you're exposing part all the time, too.
> > Will you explain it in detail? As far as I know, __ro_after_init
> > annotation makes the kernel to call set_memory_ro() after __init
> > functions are completed, by gathering such variables into a section
> > so that set_memory_ro() can change page flags for a memory region
> > where it is PAGE_ALIGNED and the size is multiple of PAGE_SIZE bytes.
> >
> > This "struct lsm_entry" is for gathering only LSM related variables which
> > would have been marked as __ro_after_init if 
> > CONFIG_SECURITY_WRITABLE_HOOKS=n
> > into a list of memory regions where they are PAGE_ALIGNED and the size is
> > multiple of PAGE_SIZE bytes, so that the kernel can call set_memory_ro() on
> > these memory regions even if CONFIG_SECURITY_SELINUX_DISABLE=y or allowing
> > LKM based LSMs.
> 
> All I'm saying is that it's simpler to mark the entire
> structure than it is to mark parts.

I'm still unable to understand.
"struct lsm_entry" != "the entire structure" ?

> 
> >> For now I think that the solution James proposes makes the
> >> most sense. In the hypothetical loadable modules case I
> >> don't see real value in hardening bits of the data while
> >> explicitly making the most important parts dynamic.
> > Best solution for environments where it is known to enforce only one 
> > specific
> > LSM module and no user choices and no LKM based LSMs (including antivirus or
> > alike) is to directly embed that LSM module into security/security.c .
> 
> There are too few beers on the table in front of me
> to start that discussion. :)
> 
> > CONFIG_SECURITY_WRITABLE_HOOKS=n depends on 
> > CONFIG_SECURITY_SELINUX_DISABLE=n
> > but many of distributor's kernels enable multiple LSM modules including
> > CONFIG_SECURITY_SELINUX_DISABLE=y. Also, people are using antivirus software
> > on distributor's kernels.
> 
> I have to limit my comment on that to pointing out
> that we can't make decisions based on code that has
> never been proposed for the mainline.

If I recall correctly, LSM framework was considered as "should be used by only
rule based access restriction mechanisms (so-called MAC)". Hooks for e.g.
antivirus modules should use different interfaces (e.g. fsnotify_open())
which do not offer ability to reject access requests synchronously.
I think that this technical barrier was removed by commit b1d9e6b0646d0e5e "LSM:
Switch to lists of hooks". But there still remains non-technical barrier.

We think that we should not merge similar modules which provide same 
functionality.
What about antivirus modules? They offer same functionality (intercept and judge
access requests) but interface details vary depending on userspace daemon which
performs actual scan. They cannot be unified, but we won't agree with merging
all product's all implementations. Do we change our mind and encourage antivirus
modules developers to try proposing for the mainline?

> 
> >  At least one antivirus software (which allows
> > anonymous download of LKM source code) is using LSM hooks since Linux 2.6.32
> > instead of rewriting syscall tables. We are already allowing multiple 
> > concurrent
> > LSM modules (up to one fully armored module which uses "struct 
> > cred"->security
> > field or exclusive hooks like security_xfrm_state_pol_flow_match(), plus
> > unlimited number of lightweight modules which do not use "struct 
> > cred"->security
> > nor exclusive hooks) as long as they are built into the kernel. There is no
> > reason to keep LKM based LSM modules from antivirus software or alike away.
> 
> We're not to the point where in-kernel modules are stacking fully.
> Not everyone is on board for that, but hope springs eternal. Part
> of the design criteria I'm working under is that it shouldn't
> preclude loadable modules, and I still think that's doable. The
> patch James proposed is completely compatible with this philosophy.
> You can argue that it requires a loadable module configuration be
> less "hardened", but the opponents of loadable modules say that is
> inherent to loadable modules.

The patch James proposed looks like a placebo for me, for many of distributor
kernels will have to choose CONFIG_SECURITY_WRITABLE_HOOKS=y. If we offer a way 
to
call set_memory_ro()/set_memory_rw(), such distributor kernels will be able to
behave like CONFIG_SECURITY_

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

James Morris wrote:
> On Tue, 14 Feb 2017, Tetsuo Handa wrote:
> 
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index 118f454..f6f90c4 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -31,6 +31,11 @@ config SECURITY
> > >  
> > > If you are unsure how to answer this question, answer N.
> > >  
> > > +config SECURITY_WRITABLE_HOOKS
> > > + depends on SECURITY
> > > + bool
> > > + default n
> > > +
> > 
> > This configuration option must not be set to N without big fat explanation
> > about implications of setting this option to N.
> 
> It's not visible in the config menu, it's only there to support SELinux 
> runtime disablement, otherwise it wouldn't even be an option.
> 
> > 
> > Honestly, I still don't like this option, regardless of whether SELinux
> > needs this option or not.
> > 
> 
> I agree, it would be better to just enable RO hardening without an option 
> to disable it.

Here is my version. printk() is only for testing purpose and will be removed
in the final version.

  (1) Use set_memory_ro() instead of __ro_after_init so that runtime disablement
  of SELinux and runtime enablement of dynamically loadable LSMs can use
  set_memory_rw().

  (2) Replace "struct security_hook_list"->head with "struct 
security_hook_list"->offset
  so that "struct security_hook_heads security_hook_heads;" can become a 
local variable
  in security/security.c .

  (3) (Not in this patch.) The "struct security_hook_list" variable in each LSM
  module is considered as "__init" and "const" because the content is 
copied to
  dynamically allocated (and protected via set_memory_ro()) memory pages.

  (4) (Not in this patch.) If we add EXPORT_SYMBOL_GPL(security_add_hooks), 
dynamically
  loadable LSMs are legally allowed.

 include/linux/lsm_hooks.h |   31 +++-
 security/security.c   |   89 ++
 security/selinux/hooks.c  |   12 +-
 3 files changed, 107 insertions(+), 25 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..00050a7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1865,9 +1865,16 @@ struct security_hook_heads {
  */
 struct security_hook_list {
struct list_headlist;
-   struct list_head*head;
union security_list_options hook;
-   char*lsm;
+   const char  *lsm;
+   unsigned intoffset;
+};
+
+struct lsm_entry {
+   struct list_head head;
+   unsigned int order;
+   unsigned int count;
+   struct security_hook_list hooks[0];
 };
 
 /*
@@ -1877,14 +1884,13 @@ struct security_hook_list {
  * text involved.
  */
 #define LSM_HOOK_INIT(HEAD, HOOK) \
-   { .head = _hook_heads.HEAD, .hook = { .HEAD = HOOK } }
+   { .offset = offsetof(struct security_hook_heads, HEAD), \
+ .hook = { .HEAD = HOOK } }
 
-extern struct security_hook_heads security_hook_heads;
 extern char *lsm_names;
 
-extern void security_add_hooks(struct security_hook_list *hooks, int count,
-   char *lsm);
-
+extern struct lsm_entry *security_add_hooks(const struct security_hook_list *
+   hooks, int count, const char *lsm);
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 /*
  * Assuring the safety of deleting a security module is up to
@@ -1898,15 +1904,8 @@ extern void security_add_hooks(struct security_hook_list 
*hooks, int count,
  * disabling their module is a good idea needs to be at least as
  * careful as the SELinux team.
  */
-static inline void security_delete_hooks(struct security_hook_list *hooks,
-   int count)
-{
-   int i;
-
-   for (i = 0; i < count; i++)
-   list_del_rcu([i].list);
-}
-#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
+extern void security_delete_hooks(struct lsm_entry *entry);
+#endif
 
 extern int __init security_module_enable(const char *module);
 extern void __init capability_add_hooks(void);
diff --git a/security/security.c b/security/security.c
index d0e07f2..dffe69b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -32,6 +32,7 @@
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX  10
 
+static bool lsm_initialized;
 char *lsm_names;
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -47,6 +48,38 @@ static void __init do_security_initcalls(void)
}
 }
 
+static struct security_hook_heads security_hook_heads;
+static LIST_HEAD(lsm_entry_list);
+#ifdef CONFIG_DEBUG_RODATA
+s

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

James Morris wrote:
> > Loadable kernel modules used by antivirus software temporarily modify 
> > syscall tables
> > ( 
> > http://stackoverflow.com/questions/13876369/system-call-interception-in-linux-kernel-module-kernel-3-5
> >  )
> > in order to register hooks for execve()/open()/close(). It is very 
> > frustrating for
> > many users if you enforce CONFIG_MODULES=n or forbid post-__init 
> > registration of hooks.
> 
> We don't cater to out of tree code.
> 
> Additionally, I'd also seriously question whether the security benefits of 
> kernel AV outweigh its security risks.

Are you aware that loadable kernel modules used by antivirus software are used 
for
relaying events to userspace daemons? Such modules don't do scanning inside 
kernel.
Why such modules are considered as security risks?

Are you aware that SELinux (or AppArmor or whatever so-called MAC) is not always
enabled in all enterprise systems? Are you aware that there are systems where
administrators cannot afford using rule based access restriction mechanisms?
In such systems, the security benefits of loadable security modules (provided by
AV or alike) can outweigh read only LSM hooks.

> diff --git a/security/Kconfig b/security/Kconfig
> index 118f454..f6f90c4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -31,6 +31,11 @@ config SECURITY
>  
> If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_WRITABLE_HOOKS
> + depends on SECURITY
> + bool
> + default n
> +

This configuration option must not be set to N without big fat explanation
about implications of setting this option to N.

Honestly, I still don't like this option, regardless of whether SELinux
needs this option or not.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.