* Re: [PATCH v2 4/4] net: apply __GFP_NO_AUTOINIT to AF_UNIX sk_buff allocations
From: Kees Cook @ 2019-05-16 16:53 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, kernel-hardening, Masahiro Yamada, James Morris,
Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Jann Horn, Mark Rutland, linux-mm, linux-security-module
In-Reply-To: <20190514143537.10435-5-glider@google.com>
On Tue, May 14, 2019 at 04:35:37PM +0200, Alexander Potapenko wrote:
> Add sock_alloc_send_pskb_noinit(), which is similar to
> sock_alloc_send_pskb(), but allocates with __GFP_NO_AUTOINIT.
> This helps reduce the slowdown on hackbench in the init_on_alloc mode
> from 6.84% to 3.45%.
Out of curiosity, why the creation of the new function over adding a
gfp flag argument to sock_alloc_send_pskb() and updating callers? (There
are only 6 callers, and this change already updates 2 of those.)
> Slowdown for the initialization features compared to init_on_free=0,
> init_on_alloc=0:
>
> hackbench, init_on_free=1: +7.71% sys time (st.err 0.45%)
> hackbench, init_on_alloc=1: +3.45% sys time (st.err 0.86%)
In the commit log it might be worth mentioning that this is only
changing the init_on_alloc case (in case it's not already obvious to
folks). Perhaps there needs to be a split of __GFP_NO_AUTOINIT into
__GFP_NO_AUTO_ALLOC_INIT and __GFP_NO_AUTO_FREE_INIT? Right now __GFP_NO_AUTOINIT is only checked for init_on_alloc:
static inline bool want_init_on_alloc(gfp_t flags)
{
if (static_branch_unlikely(&init_on_alloc))
return !(flags & __GFP_NO_AUTOINIT);
return flags & __GFP_ZERO;
}
...
static inline bool want_init_on_free(void)
{
return static_branch_unlikely(&init_on_free);
}
On a related note, it might be nice to add an exclusion list to
the kmem_cache_create() cases, since it seems likely that further
tuning will be needed there. For example, with the init_on_free-similar
PAX_MEMORY_SANITIZE changes in the last public release of PaX/grsecurity,
the following were excluded from wipe-on-free:
buffer_head
names_cache
mm_struct
vm_area_struct
anon_vma
anon_vma_chain
skbuff_head_cache
skbuff_fclone_cache
Adding these and others (with details on why they were selected),
might improve init_on_free performance further without trading too
much coverage.
Having a kernel param with a comma-separated list of cache names and
the logic to add __GFP_NO_AUTOINIT at creation time would be a nice
(and cheap!) debug feature to let folks tune things for their specific
workloads, if they choose to. (And it could maybe also know what "none"
meant, to actually remove the built-in exclusions, similar to what
PaX's "pax_sanitize_slab=full" does.)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-05-16 16:42 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Christoph Lameter, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <201905160907.92FAC880@keescook>
From: Kees Cook <keescook@chromium.org>
Date: Thu, May 16, 2019 at 6:20 PM
To: Alexander Potapenko
Cc: <akpm@linux-foundation.org>, <cl@linux.com>,
<kernel-hardening@lists.openwall.com>, Masahiro Yamada, James Morris,
Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov,
Sandeep Patil, Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland,
<linux-mm@kvack.org>, <linux-security-module@vger.kernel.org>
> On Tue, May 14, 2019 at 04:35:34PM +0200, Alexander Potapenko wrote:
> > 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%)
>
> I wonder if the patch series should be reorganized to introduce
> __GFP_NO_AUTOINIT first, so that when the commit with benchmarks appears,
> we get the "final" numbers...
>
> > 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%)
>
> I'm working on reproducing these benchmarks. I'd really like to narrow
> down the +24% number here. But it does
I suspect the slowdown of init_on_free is bigger than that of
PAX_SANITIZE_MEMORY, as we've set the goal to have fully zeroed memory
at alloc time.
If we want a mode that only wipes the user data upon free() but
doesn't eliminate all uninit memory, then we can make it faster.
> > The slowdown for init_on_free=0, init_on_alloc=0 compared to the
> > baseline is within the standard error.
>
> I think the use of static keys here is really great: this is available
> by default for anyone that wants to turn it on.
>
> I'm thinking, given the configuable nature of this, it'd be worth adding
> a little more detail at boot time. I think maybe a separate patch could
> be added to describe the kernel's memory auto-initialization features,
> and add something like this to mm_init():
>
> +void __init report_meminit(void)
> +{
> + const char *stack;
> +
> + if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
> + stack = "all";
> + else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
> + stack = "byref_all";
> + else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
> + stack = "byref";
> + else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_USER))
> + stack = "__user";
> + else
> + stack = "off";
> +
> + /* Report memory auto-initialization states for this boot. */
> + pr_info("mem auto-init: stack:%s, heap alloc:%s, heap free:%s\n",
> + stack, want_init_on_alloc(GFP_KERNEL) ? "on" : "off",
> + want_init_on_free() ? "on" : "off");
> +}
>
> To get a boot line like:
>
> mem auto-init: stack:off, heap alloc:off, heap free:on
For stack there's no binary on/off, as you can potentially build half
of the kernel with stack instrumentation and another half without it.
We could make the instrumentation insert a static global flag into
each translation unit, but this won't give us any interesting info.
> And one other thought I had was that in the init_on_free=1 case, there is
> a large pause at boot while memory is being cleared. I think it'd be handy
> to include a comment about that, just to keep people from being surprised:
>
> diff --git a/init/main.c b/init/main.c
> index cf0c3948ce0e..aea278392338 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -529,6 +529,8 @@ static void __init mm_init(void)
> * bigger than MAX_ORDER unless SPARSEMEM.
> */
> page_ext_init_flatmem();
> + if (want_init_on_free())
> + pr_info("Clearing system memory ...\n");
> mem_init();
> kmem_cache_init();
> pgtable_init();
>
> Beyond these thoughts, I think this series is in good shape.
>
> Andrew (or anyone else) do you have any concerns about this?
>
> --
> Kees Cook
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Kees Cook @ 2019-05-16 16:19 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, kernel-hardening, Masahiro Yamada, James Morris,
Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Jann Horn, Mark Rutland, linux-mm, linux-security-module
In-Reply-To: <20190514143537.10435-2-glider@google.com>
On Tue, May 14, 2019 at 04:35:34PM +0200, Alexander Potapenko wrote:
> 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%)
I wonder if the patch series should be reorganized to introduce
__GFP_NO_AUTOINIT first, so that when the commit with benchmarks appears,
we get the "final" numbers...
> 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%)
I'm working on reproducing these benchmarks. I'd really like to narrow
down the +24% number here. But it does
> The slowdown for init_on_free=0, init_on_alloc=0 compared to the
> baseline is within the standard error.
I think the use of static keys here is really great: this is available
by default for anyone that wants to turn it on.
I'm thinking, given the configuable nature of this, it'd be worth adding
a little more detail at boot time. I think maybe a separate patch could
be added to describe the kernel's memory auto-initialization features,
and add something like this to mm_init():
+void __init report_meminit(void)
+{
+ const char *stack;
+
+ if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
+ stack = "all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
+ stack = "byref_all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
+ stack = "byref";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_USER))
+ stack = "__user";
+ else
+ stack = "off";
+
+ /* Report memory auto-initialization states for this boot. */
+ pr_info("mem auto-init: stack:%s, heap alloc:%s, heap free:%s\n",
+ stack, want_init_on_alloc(GFP_KERNEL) ? "on" : "off",
+ want_init_on_free() ? "on" : "off");
+}
To get a boot line like:
mem auto-init: stack:off, heap alloc:off, heap free:on
And one other thought I had was that in the init_on_free=1 case, there is
a large pause at boot while memory is being cleared. I think it'd be handy
to include a comment about that, just to keep people from being surprised:
diff --git a/init/main.c b/init/main.c
index cf0c3948ce0e..aea278392338 100644
--- a/init/main.c
+++ b/init/main.c
@@ -529,6 +529,8 @@ static void __init mm_init(void)
* bigger than MAX_ORDER unless SPARSEMEM.
*/
page_ext_init_flatmem();
+ if (want_init_on_free())
+ pr_info("Clearing system memory ...\n");
mem_init();
kmem_cache_init();
pgtable_init();
Beyond these thoughts, I think this series is in good shape.
Andrew (or anyone else) do you have any concerns about this?
--
Kees Cook
^ permalink raw reply related
* [PATCH 4/4] ima: only audit failed appraisal verifications
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190516161257.6640-1-roberto.sassu@huawei.com>
This patch ensures that integrity_audit_msg() is called only when the
status is not INTEGRITY_PASS.
Fixes: 8606404fa555c ("ima: digital signature verification support")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/ima/ima_appraise.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a32ed5d7afd1..f5f4506bcb8e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -359,8 +359,9 @@ int ima_appraise_measurement(enum ima_hooks func,
status = INTEGRITY_PASS;
}
- integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
- op, cause, rc, 0);
+ if (status != INTEGRITY_PASS)
+ integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
+ filename, op, cause, rc, 0);
} else {
ima_cache_flags(iint, func);
}
--
2.17.1
^ permalink raw reply related
* [PATCH 3/4] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190516161257.6640-1-roberto.sassu@huawei.com>
Currently, ima_appraise_measurement() ignores the EVM status when
evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
status. The problem is that the EVM status is overwritten with the
appraisal status.
This patch mitigates the issue by selecting signature verification as the
only method allowed for appraisal when EVM is not initialized. Since the
new behavior might break user space, it must be turned on by adding
ima_appraise_req_evm to the kernel command line.
Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
Documentation/admin-guide/kernel-parameters.txt | 3 +++
security/integrity/ima/ima_appraise.c | 12 ++++++++++++
2 files changed, 15 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 52e6fbb042cc..80e1c233656b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1588,6 +1588,9 @@
Format: { "off" | "enforce" | "fix" | "log" }
default: "enforce"
+ ima_appraise_req_evm
+ [IMA] require EVM for appraisal with file digests.
+
ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead.
The builtin appraise policy appraises all files
owned by uid=0.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..a32ed5d7afd1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -33,6 +33,14 @@ static int __init default_appraise_setup(char *str)
__setup("ima_appraise=", default_appraise_setup);
+static bool ima_appraise_req_evm;
+static int __init appraise_req_evm_setup(char *str)
+{
+ ima_appraise_req_evm = true;
+ return 1;
+}
+__setup("ima_appraise_req_evm", appraise_req_evm_setup);
+
/*
* is_ima_appraise_enabled - return appraise status
*
@@ -245,7 +253,11 @@ int ima_appraise_measurement(enum ima_hooks func,
switch (status) {
case INTEGRITY_PASS:
case INTEGRITY_PASS_IMMUTABLE:
+ break;
case INTEGRITY_UNKNOWN:
+ if (ima_appraise_req_evm &&
+ xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+ goto out;
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
--
2.17.1
^ permalink raw reply related
* [PATCH 2/4] evm: reset status in evm_inode_post_setattr()
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190516161257.6640-1-roberto.sassu@huawei.com>
This patch adds a call to evm_reset_status() in evm_inode_post_setattr(),
before security.evm is updated. The same is done in the other
evm_inode_post_* functions.
Fixes: 523b74b16bcbb ("evm: reset EVM status when file attributes change")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/evm/evm_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b6d9f14bc234..b41c2d8a8834 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -512,8 +512,11 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
if (!evm_key_loaded())
return;
- if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
+ if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) {
+ evm_reset_status(dentry->d_inode);
+
evm_update_evmxattr(dentry, NULL, NULL, 0);
+ }
}
/*
--
2.17.1
^ permalink raw reply related
* [PATCH 1/4] evm: check hash algorithm passed to init_desc()
From: Roberto Sassu @ 2019-05-16 16:12 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-doc, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
This patch prevents memory access beyond the evm_tfm array by checking the
validity of the index (hash algorithm) passed to init_desc(). The hash
algorithm can be arbitrarily set if the security.ima xattr type is not
EVM_XATTR_HMAC.
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/evm/evm_crypto.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index e11564eb645b..82a38e801ee4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
tfm = &hmac_tfm;
algo = evm_hmac;
} else {
+ if (hash_algo >= HASH_ALGO__LAST)
+ return ERR_PTR(-EINVAL);
+
tfm = &evm_tfm[hash_algo];
algo = hash_algo_name[hash_algo];
}
--
2.17.1
^ permalink raw reply related
* Re: KMSAN: uninit-value in tomoyo_check_inet_address
From: Tetsuo Handa @ 2019-05-16 13:37 UTC (permalink / raw)
To: Alexander Potapenko
Cc: syzbot, James Morris, LKML, linux-security-module,
Serge E. Hallyn, syzkaller-bugs, takedakn
In-Reply-To: <CAG_fn=UHjDv=Jc_nEKKknVGrr9CC4rZ+a+hw_yAL-j4y4=89cg@mail.gmail.com>
On 2019/05/16 22:19, Alexander Potapenko wrote:
>> commit e6193f78bb689f3f424559bb45f4a091c8b314df
>> Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Date: Fri Apr 12 19:59:36 2019 +0900
>>
>> tomoyo: Check address length before reading address family
>>
>> KMSAN will complain if valid address length passed to bind()/connect()/
>> sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
>>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Signed-off-by: James Morris <jamorris@linux.microsoft.com>
> Apparently the fix didn't make it to 5.1, I'll cherry-pick it to KMSAN tree.
Together with three more patches listed bottom, please.
> OOC, how did you know about this bug?
I did a tree-wide check when writing a patch for
KMSAN: uninit-value in rds_bind
KMSAN: uninit-value in rds_connect
reports. Patches for LSM part just did not make it to 5.1.
commit b9ef5513c99bf9c8bfd9c9e8051b67f52b2dee1e
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri Apr 12 19:59:35 2019 +0900
smack: Check address length before reading address family
commit 619ae03e922b65a1a5d4269ceae1e9e13a058d6b
Author: Casey Schaufler <casey@schaufler-ca.com>
Date: Tue Apr 30 14:13:32 2019 -0700
Smack: Fix kbuild reported build error
commit c750e6929d3c76d13d1d0ba475989d6dd74785d5
Author: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: Fri Apr 12 19:59:34 2019 +0900
selinux: Check address length before reading address family
^ permalink raw reply
* Re: KMSAN: uninit-value in tomoyo_check_inet_address
From: Alexander Potapenko @ 2019-05-16 13:19 UTC (permalink / raw)
To: Tetsuo Handa
Cc: syzbot, James Morris, LKML, linux-security-module,
Serge E. Hallyn, syzkaller-bugs, takedakn
In-Reply-To: <2b0b9d18-6773-f2dc-ecb2-9f8782d0962a@i-love.sakura.ne.jp>
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: Thu, May 16, 2019 at 3:13 PM
To: syzbot
Cc: <glider@google.com>, <jmorris@namei.org>,
<linux-kernel@vger.kernel.org>,
<linux-security-module@vger.kernel.org>, <serge@hallyn.com>,
<syzkaller-bugs@googlegroups.com>, <takedakn@nttdata.co.jp>
> On 2019/05/16 21:58, syzbot wrote:
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit: 3b955a40 usb-fuzzer: main usb gadget fuzzer driver
> > git tree: kmsan
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1027e608a00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=602468164ccdc30a
> > dashboard link: https://syzkaller.appspot.com/bug?extid=1018d578c410f9f37261
> > compiler: clang version 9.0.0 (/home/glider/llvm/clang 06d00afa61eef8f7f501ebdb4e8612ea43ec2d78)
> >
> > Unfortunately, I don't have any reproducer for this crash yet.
>
> This should be already fixed in linux.git.
>
> #syz fix: tomoyo: Check address length before reading address family
>
> commit e6193f78bb689f3f424559bb45f4a091c8b314df
> Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Fri Apr 12 19:59:36 2019 +0900
>
> tomoyo: Check address length before reading address family
>
> KMSAN will complain if valid address length passed to bind()/connect()/
> sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: James Morris <jamorris@linux.microsoft.com>
Apparently the fix didn't make it to 5.1, I'll cherry-pick it to KMSAN tree.
OOC, how did you know about this bug?
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/2b0b9d18-6773-f2dc-ecb2-9f8782d0962a%40i-love.sakura.ne.jp.
> For more options, visit https://groups.google.com/d/optout.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: KMSAN: uninit-value in tomoyo_check_inet_address
From: Tetsuo Handa @ 2019-05-16 13:13 UTC (permalink / raw)
To: syzbot
Cc: glider, jmorris, linux-kernel, linux-security-module, serge,
syzkaller-bugs, takedakn
In-Reply-To: <00000000000032685a058900d170@google.com>
On 2019/05/16 21:58, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 3b955a40 usb-fuzzer: main usb gadget fuzzer driver
> git tree: kmsan
> console output: https://syzkaller.appspot.com/x/log.txt?x=1027e608a00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=602468164ccdc30a
> dashboard link: https://syzkaller.appspot.com/bug?extid=1018d578c410f9f37261
> compiler: clang version 9.0.0 (/home/glider/llvm/clang 06d00afa61eef8f7f501ebdb4e8612ea43ec2d78)
>
> Unfortunately, I don't have any reproducer for this crash yet.
This should be already fixed in linux.git.
#syz fix: tomoyo: Check address length before reading address family
commit e6193f78bb689f3f424559bb45f4a091c8b314df
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri Apr 12 19:59:36 2019 +0900
tomoyo: Check address length before reading address family
KMSAN will complain if valid address length passed to bind()/connect()/
sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: James Morris <jamorris@linux.microsoft.com>
^ permalink raw reply
* KMSAN: uninit-value in tomoyo_check_unix_address
From: syzbot @ 2019-05-16 12:58 UTC (permalink / raw)
To: glider, jmorris, linux-kernel, linux-security-module,
penguin-kernel, serge, syzkaller-bugs, takedakn
Hello,
syzbot found the following crash on:
HEAD commit: 3b955a40 usb-fuzzer: main usb gadget fuzzer driver
git tree: kmsan
console output: https://syzkaller.appspot.com/x/log.txt?x=15014d44a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=602468164ccdc30a
dashboard link: https://syzkaller.appspot.com/bug?extid=e9687dbb0b6b2057d0ed
compiler: clang version 9.0.0 (/home/glider/llvm/clang
06d00afa61eef8f7f501ebdb4e8612ea43ec2d78)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+e9687dbb0b6b2057d0ed@syzkaller.appspotmail.com
==================================================================
BUG: KMSAN: uninit-value in tomoyo_check_unix_address+0x36a/0xa30
security/tomoyo/network.c:597
CPU: 1 PID: 30810 Comm: syz-executor.4 Not tainted 5.1.0+ #3
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x191/0x1f0 lib/dump_stack.c:113
kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:619
__msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
tomoyo_check_unix_address+0x36a/0xa30 security/tomoyo/network.c:597
tomoyo_socket_connect_permission+0x315/0x400 security/tomoyo/network.c:705
tomoyo_socket_connect+0x8b/0xa0 security/tomoyo/tomoyo.c:456
security_socket_connect+0x127/0x200 security/security.c:1948
__sys_connect+0x536/0x820 net/socket.c:1804
__do_sys_connect net/socket.c:1819 [inline]
__se_sys_connect+0x8d/0xb0 net/socket.c:1816
__x64_sys_connect+0x4a/0x70 net/socket.c:1816
do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x458da9
Code: ad b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 7b b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f825eeb8c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000458da9
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
RBP: 000000000073bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f825eeb96d4
R13: 00000000004bf2cf R14: 00000000004d0660 R15: 00000000ffffffff
Local variable description: ----address@__sys_connect
Variable was created at:
sockfd_lookup_light net/socket.c:488 [inline]
__sys_connect+0x8c/0x820 net/socket.c:1796
__do_sys_connect net/socket.c:1819 [inline]
__se_sys_connect+0x8d/0xb0 net/socket.c:1816
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* KMSAN: uninit-value in tomoyo_check_inet_address
From: syzbot @ 2019-05-16 12:58 UTC (permalink / raw)
To: glider, jmorris, linux-kernel, linux-security-module,
penguin-kernel, serge, syzkaller-bugs, takedakn
Hello,
syzbot found the following crash on:
HEAD commit: 3b955a40 usb-fuzzer: main usb gadget fuzzer driver
git tree: kmsan
console output: https://syzkaller.appspot.com/x/log.txt?x=1027e608a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=602468164ccdc30a
dashboard link: https://syzkaller.appspot.com/bug?extid=1018d578c410f9f37261
compiler: clang version 9.0.0 (/home/glider/llvm/clang
06d00afa61eef8f7f501ebdb4e8612ea43ec2d78)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+1018d578c410f9f37261@syzkaller.appspotmail.com
==================================================================
BUG: KMSAN: uninit-value in tomoyo_check_inet_address+0x143/0xd10
security/tomoyo/network.c:508
CPU: 0 PID: 22966 Comm: syz-executor.2 Not tainted 5.1.0+ #3
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x191/0x1f0 lib/dump_stack.c:113
kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:619
__msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
tomoyo_check_inet_address+0x143/0xd10 security/tomoyo/network.c:508
tomoyo_socket_bind_permission+0x343/0x3b0 security/tomoyo/network.c:741
tomoyo_socket_bind+0x8b/0xa0 security/tomoyo/tomoyo.c:471
security_socket_bind+0x127/0x200 security/security.c:1943
__sys_bind+0x536/0x7c0 net/socket.c:1623
__do_sys_bind net/socket.c:1638 [inline]
__se_sys_bind+0x8d/0xb0 net/socket.c:1636
__x64_sys_bind+0x4a/0x70 net/socket.c:1636
do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x458da9
Code: ad b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 7b b8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f4aeb5bbc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000031
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000458da9
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000004
RBP: 000000000073bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f4aeb5bc6d4
R13: 00000000004bee1f R14: 00000000004cfe68 R15: 00000000ffffffff
Local variable description: ----address@__sys_bind
Variable was created at:
sockfd_lookup_light net/socket.c:488 [inline]
__sys_bind+0x8c/0x7c0 net/socket.c:1619
__do_sys_bind net/socket.c:1638 [inline]
__se_sys_bind+0x8d/0xb0 net/socket.c:1636
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* [PATCH AUTOSEL 5.0 14/34] securityfs: fix use-after-free on symlink traversal
From: Sasha Levin @ 2019-05-16 11:39 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Al Viro, Sasha Levin, linux-security-module
In-Reply-To: <20190516113932.8348-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit 46c874419652bbefdfed17420fd6e88d8a31d9ec ]
symlink body shouldn't be freed without an RCU delay. Switch securityfs
to ->destroy_inode() and use of call_rcu(); free both the inode and symlink
body in the callback.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/inode.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/security/inode.c b/security/inode.c
index b7772a9b315ee..421dd72b58767 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -27,17 +27,22 @@
static struct vfsmount *mount;
static int mount_count;
-static void securityfs_evict_inode(struct inode *inode)
+static void securityfs_i_callback(struct rcu_head *head)
{
- truncate_inode_pages_final(&inode->i_data);
- clear_inode(inode);
+ struct inode *inode = container_of(head, struct inode, i_rcu);
if (S_ISLNK(inode->i_mode))
kfree(inode->i_link);
+ free_inode_nonrcu(inode);
+}
+
+static void securityfs_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, securityfs_i_callback);
}
static const struct super_operations securityfs_super_operations = {
.statfs = simple_statfs,
- .evict_inode = securityfs_evict_inode,
+ .destroy_inode = securityfs_destroy_inode,
};
static int fill_super(struct super_block *sb, void *data, int silent)
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 15/34] apparmorfs: fix use-after-free on symlink traversal
From: Sasha Levin @ 2019-05-16 11:39 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Al Viro, Sasha Levin, linux-security-module
In-Reply-To: <20190516113932.8348-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit f51dcd0f621caac5380ce90fbbeafc32ce4517ae ]
symlink body shouldn't be freed without an RCU delay. Switch apparmorfs
to ->destroy_inode() and use of call_rcu(); free both the inode and symlink
body in the callback.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/apparmorfs.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 3f80a684c232a..665853dd517ca 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -123,17 +123,22 @@ static int aafs_show_path(struct seq_file *seq, struct dentry *dentry)
return 0;
}
-static void aafs_evict_inode(struct inode *inode)
+static void aafs_i_callback(struct rcu_head *head)
{
- truncate_inode_pages_final(&inode->i_data);
- clear_inode(inode);
+ struct inode *inode = container_of(head, struct inode, i_rcu);
if (S_ISLNK(inode->i_mode))
kfree(inode->i_link);
+ free_inode_nonrcu(inode);
+}
+
+static void aafs_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, aafs_i_callback);
}
static const struct super_operations aafs_super_ops = {
.statfs = simple_statfs,
- .evict_inode = aafs_evict_inode,
+ .destroy_inode = aafs_destroy_inode,
.show_path = aafs_show_path,
};
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 10/25] securityfs: fix use-after-free on symlink traversal
From: Sasha Levin @ 2019-05-16 11:40 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Al Viro, Sasha Levin, linux-security-module
In-Reply-To: <20190516114029.8682-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit 46c874419652bbefdfed17420fd6e88d8a31d9ec ]
symlink body shouldn't be freed without an RCU delay. Switch securityfs
to ->destroy_inode() and use of call_rcu(); free both the inode and symlink
body in the callback.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/inode.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/security/inode.c b/security/inode.c
index 8dd9ca8848e43..829f15672e01f 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -26,17 +26,22 @@
static struct vfsmount *mount;
static int mount_count;
-static void securityfs_evict_inode(struct inode *inode)
+static void securityfs_i_callback(struct rcu_head *head)
{
- truncate_inode_pages_final(&inode->i_data);
- clear_inode(inode);
+ struct inode *inode = container_of(head, struct inode, i_rcu);
if (S_ISLNK(inode->i_mode))
kfree(inode->i_link);
+ free_inode_nonrcu(inode);
+}
+
+static void securityfs_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, securityfs_i_callback);
}
static const struct super_operations securityfs_super_operations = {
.statfs = simple_statfs,
- .evict_inode = securityfs_evict_inode,
+ .destroy_inode = securityfs_destroy_inode,
};
static int fill_super(struct super_block *sb, void *data, int silent)
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 11/25] apparmorfs: fix use-after-free on symlink traversal
From: Sasha Levin @ 2019-05-16 11:40 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Al Viro, Sasha Levin, linux-security-module
In-Reply-To: <20190516114029.8682-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit f51dcd0f621caac5380ce90fbbeafc32ce4517ae ]
symlink body shouldn't be freed without an RCU delay. Switch apparmorfs
to ->destroy_inode() and use of call_rcu(); free both the inode and symlink
body in the callback.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/apparmorfs.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index e09fe4d7307cd..40e3a098f6fb5 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -123,17 +123,22 @@ static int aafs_show_path(struct seq_file *seq, struct dentry *dentry)
return 0;
}
-static void aafs_evict_inode(struct inode *inode)
+static void aafs_i_callback(struct rcu_head *head)
{
- truncate_inode_pages_final(&inode->i_data);
- clear_inode(inode);
+ struct inode *inode = container_of(head, struct inode, i_rcu);
if (S_ISLNK(inode->i_mode))
kfree(inode->i_link);
+ free_inode_nonrcu(inode);
+}
+
+static void aafs_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, aafs_i_callback);
}
static const struct super_operations aafs_super_ops = {
.statfs = simple_statfs,
- .evict_inode = aafs_evict_inode,
+ .destroy_inode = aafs_destroy_inode,
.show_path = aafs_show_path,
};
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 08/16] apparmorfs: fix use-after-free on symlink traversal
From: Sasha Levin @ 2019-05-16 11:40 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Al Viro, Sasha Levin, linux-security-module
In-Reply-To: <20190516114107.8963-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit f51dcd0f621caac5380ce90fbbeafc32ce4517ae ]
symlink body shouldn't be freed without an RCU delay. Switch apparmorfs
to ->destroy_inode() and use of call_rcu(); free both the inode and symlink
body in the callback.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/apparmorfs.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 0e03377bb83ea..dd746bd69a9b2 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -126,17 +126,22 @@ static int aafs_show_path(struct seq_file *seq, struct dentry *dentry)
return 0;
}
-static void aafs_evict_inode(struct inode *inode)
+static void aafs_i_callback(struct rcu_head *head)
{
- truncate_inode_pages_final(&inode->i_data);
- clear_inode(inode);
+ struct inode *inode = container_of(head, struct inode, i_rcu);
if (S_ISLNK(inode->i_mode))
kfree(inode->i_link);
+ free_inode_nonrcu(inode);
+}
+
+static void aafs_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, aafs_i_callback);
}
static const struct super_operations aafs_super_ops = {
.statfs = simple_statfs,
- .evict_inode = aafs_evict_inode,
+ .destroy_inode = aafs_destroy_inode,
.show_path = aafs_show_path,
};
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 07/16] securityfs: fix use-after-free on symlink traversal
From: Sasha Levin @ 2019-05-16 11:40 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Al Viro, Sasha Levin, linux-security-module
In-Reply-To: <20190516114107.8963-1-sashal@kernel.org>
From: Al Viro <viro@zeniv.linux.org.uk>
[ Upstream commit 46c874419652bbefdfed17420fd6e88d8a31d9ec ]
symlink body shouldn't be freed without an RCU delay. Switch securityfs
to ->destroy_inode() and use of call_rcu(); free both the inode and symlink
body in the callback.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/inode.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/security/inode.c b/security/inode.c
index 8dd9ca8848e43..829f15672e01f 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -26,17 +26,22 @@
static struct vfsmount *mount;
static int mount_count;
-static void securityfs_evict_inode(struct inode *inode)
+static void securityfs_i_callback(struct rcu_head *head)
{
- truncate_inode_pages_final(&inode->i_data);
- clear_inode(inode);
+ struct inode *inode = container_of(head, struct inode, i_rcu);
if (S_ISLNK(inode->i_mode))
kfree(inode->i_link);
+ free_inode_nonrcu(inode);
+}
+
+static void securityfs_destroy_inode(struct inode *inode)
+{
+ call_rcu(&inode->i_rcu, securityfs_i_callback);
}
static const struct super_operations securityfs_super_operations = {
.statfs = simple_statfs,
- .evict_inode = securityfs_evict_inode,
+ .destroy_inode = securityfs_destroy_inode,
};
static int fill_super(struct super_block *sb, void *data, int silent)
--
2.20.1
^ permalink raw reply related
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: James Morris @ 2019-05-16 7:24 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrX2ovRx3Rre+1_xC-q6CiybyLjQ-gmB4FZF_qCZ-Qd+4A@mail.gmail.com>
On Wed, 15 May 2019, Andy Lutomirski wrote:
> On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
> >
> > You could try user.sigstruct, which does not require any privs.
> >
>
> I don't think I understand your proposal. What file would this
> attribute be on? What would consume it?
It would be on the enclave file, so you keep the sigstruct bound to it,
rather than needing a separate file to manage. It would simplify any LSM
policy check.
It would be consumed by (I guess) the SGX_INIT_THE_ENCLAVE ioctl in your
example, instead of having a 2nd fd.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-05-16 5:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux, Jethro Beekman,
Xing, Cedric, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com>
On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:
> Hi, LSM and SELinux people-
>
> We're trying to figure out how SGX fits in with LSMs. For background,
> an SGX library is functionally a bit like a DSO, except that it's
> nominally resistant to attack from outside and the process of loading
> it is complicated. To load an enclave, a program can open
> /dev/sgx/enclave, do some ioctls to load the code and data segments
> into the enclave, call a special ioctl to "initialize" the enclave,
> and then call into the enclave (using special CPU instructions).
>
> One nastiness is that there is not actually a universally agreed upon,
> documented file format for enclaves. Windows has an undocumented
> format, and there are probably a few others out there. No one really
> wants to teach the kernel to parse enclave files.
>
> There are two issues with how this interacts with LSMs:
>
> 1) LSMs might want to be able to whitelist, blacklist, or otherwise
> restrict what enclaves can run at all. The current proposal that
> everyone seems to dislike the least is to have a .sigstruct file on
> disk that contains a hash and signature of the enclave in a
> CPU-defined format. To initialize an enclave, a program will pass an
> fd to this file, and a new LSM hook can be called to allow or disallow
> the operation. In a SELinux context, the idea is that policy could
> require the .sigstruct file to be labeled with a type like
> sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
> with such a label could run.
Similarly if we could take data for the enclave from fd and enforce
it with sgx_enclave_t label.
> Here's a very vague proposal that's kind of like what I've been
> thinking over the past few days. The SGX inode could track, for each
> page, a "safe-to-execute" bit. When you first open /dev/sgx/enclave,
> you get a blank enclave and all pages are safe-to-execute. When you
> do the ioctl to load context (which could be code, data, or anything
> else), the kernel will check whether the *source* VMA is executable
> and, if not, mark the page of the enclave being loaded as unsafe.
> Once the enclave is initialized, the driver will clear the
> safe-to-execute bit for any page that is successfully mapped writably.
With the fd based model for source I'd mark SECINFO.W pages as unsafe
to execute and then check unsafe bit before applying lets say EMODT
or EMODPR.
There is a problem here though. Usually the enclave itself is just a
loader that then loads the application from outside source and creates
the executable pages from the content.
A great example of this is Graphene that bootstraps unmodified Linux
applications to an enclave:
https://github.com/oscarlab/graphene
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-16 4:40 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, James Morris, Christopherson, Sean J,
Serge E. Hallyn, LSM List, Paul Moore, Stephen Smalley,
Eric Paris, selinux@vger.kernel.org, Jarkko Sakkinen,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E38CD@ORSMSX116.amr.corp.intel.com>
> On May 15, 2019, at 8:03 PM, Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Andy,
>
>> From: Andy Lutomirski [mailto:luto@kernel.org]
>>
>>> On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
>>>
>>> On Wed, 15 May 2019, Andy Lutomirski wrote:
>>>
>>>>> Why not just use an xattr, like security.sgx ?
>>>>
>>>> Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
>>>> install an enclave? I think that this decision should be left up the
>>>> administrator, and it should be easy to set up a loose policy where
>>>> anyone can load whatever enclave they want. That's what would happen
>>>> in my proposal if there was no LSM loaded or of the LSM policy didn't
>>>> restrict what .sigstruct files were acceptable.
>>>>
>>>
>>> You could try user.sigstruct, which does not require any privs.
>>>
>>
>> I don't think I understand your proposal. What file would this
>> attribute be on? What would consume it?
>>
>> I'm imagining that there's some enclave in a file
>> crypto_thingy.enclave. There's also a file crypto_thingy.sigstruct.
>> crypto_thingy.enclave has type lib_t or similar so that it's
>> executable. crypto_thingy.sigstruct has type sgx_sigstruct_t. The
>> enclave loader does, in effect:
>>
>> void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
>> int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
>> int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
>>
>> ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
>> enclave_offset, len, ...);
>> ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
>> enclave_offset2, len, ...);
>> etc.
>>
>> /* Here's where LSMs get to check that the sigstruct is acceptable.
>> The CPU will check that the sigstruct matches the enclave. */
>> ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);
>
> SIGSTRUCT isn't necessarily stored on disk so may not always have a fd. How about the following?
> void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
>
> The idea here is SIGSTRUCT will still be passed in memory so it works the same way when no LSM modules are loaded or basing its decision on the .sigstruct file. Otherwise, an LSM module can figure out the backing file (and offset within that file) by looking into the VMA covering ss_pointer.
I don’t love this approach. Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It would
also complicate the kernel implementation, and looking at the inode
backing the vma that backs a pointer is at least rather unusual.
Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.
>
>>
>> /* Actually map the thing */
>> mmap(enclave_fd RO section, PROT_READ, ...);
>> mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
>> mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
>>
>> /* This should fail unless EXECMOD is available, I think */
>> mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
>>
>> And the idea here is that, if the .enclave file isn't mapped
>> PROT_EXEC, then mmapping the RX section will also require EXECMEM or
>> EXECMOD.
>
> From security perspective, I think it reasonable to give EXECMEM and EXECMOD to /dev/sgx/enclave because the actual permissions are guarded by EPCM permissions, which are "inherited" from the source pages, whose permissions have passed LSM checks.
I disagree. If you deny a program EXECMOD, it’s not because you
distrust the program. It’s because you want to enforce good security
practices. (Or you’re Apple and want to disallow third-party JITs.)
A policy that accepts any sigstruct but requires that enclaves come
from disk and respect W^X seems entirely reasonable.
I think that blocking EXECMOD has likely served two very real security
purposes. It helps force application and library developers to write
and compile their code in a way that doesn’t rely on dangerous tricks
like putting executable trampolines on the stack. It also makes it
essentially impossible for an exploit to run actual downloaded machine
code — if there is no way to run code that isn’t appropriately
labeled, then attackers are more limited in what they can do.
I don’t think that SGX should become an exception to either of these.
Code should not have an excuse to use WX memory just because it’s in
an enclave. Similarly, an exploit should not be able to run an
attacker-supplied enclave as a way around a policy that would
otherwise prevent downloaded code from running.
—Andy
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-16 3:03 UTC (permalink / raw)
To: Andy Lutomirski, James Morris
Cc: Christopherson, Sean J, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, Jethro Beekman, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrX2ovRx3Rre+1_xC-q6CiybyLjQ-gmB4FZF_qCZ-Qd+4A@mail.gmail.com>
Hi Andy,
> From: Andy Lutomirski [mailto:luto@kernel.org]
>
> On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
> >
> > On Wed, 15 May 2019, Andy Lutomirski wrote:
> >
> > > > Why not just use an xattr, like security.sgx ?
> > >
> > > Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
> > > install an enclave? I think that this decision should be left up the
> > > administrator, and it should be easy to set up a loose policy where
> > > anyone can load whatever enclave they want. That's what would happen
> > > in my proposal if there was no LSM loaded or of the LSM policy didn't
> > > restrict what .sigstruct files were acceptable.
> > >
> >
> > You could try user.sigstruct, which does not require any privs.
> >
>
> I don't think I understand your proposal. What file would this
> attribute be on? What would consume it?
>
> I'm imagining that there's some enclave in a file
> crypto_thingy.enclave. There's also a file crypto_thingy.sigstruct.
> crypto_thingy.enclave has type lib_t or similar so that it's
> executable. crypto_thingy.sigstruct has type sgx_sigstruct_t. The
> enclave loader does, in effect:
>
> void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
> int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
> int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
>
> ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
> enclave_offset, len, ...);
> ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
> enclave_offset2, len, ...);
> etc.
>
> /* Here's where LSMs get to check that the sigstruct is acceptable.
> The CPU will check that the sigstruct matches the enclave. */
> ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);
SIGSTRUCT isn't necessarily stored on disk so may not always have a fd. How about the following?
void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
The idea here is SIGSTRUCT will still be passed in memory so it works the same way when no LSM modules are loaded or basing its decision on the .sigstruct file. Otherwise, an LSM module can figure out the backing file (and offset within that file) by looking into the VMA covering ss_pointer.
>
> /* Actually map the thing */
> mmap(enclave_fd RO section, PROT_READ, ...);
> mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
> mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
>
> /* This should fail unless EXECMOD is available, I think */
> mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
>
> And the idea here is that, if the .enclave file isn't mapped
> PROT_EXEC, then mmapping the RX section will also require EXECMEM or
> EXECMOD.
From security perspective, I think it reasonable to give EXECMEM and EXECMOD to /dev/sgx/enclave because the actual permissions are guarded by EPCM permissions, which are "inherited" from the source pages, whose permissions have passed LSM checks.
Alternatively, I think we could mark enclave VMAs somewhat differently, such as defining a new VM_SGX flag. The reason behind that is, enclave ranges differ from "regular" virtual ranges in terms of both functionality (i.e. #PF will have to be handled quite differently) and security, so I believe demand will come up to distinguish them eventually - e.g., LSM modules can then enforce different policies on them (by a new security_sgx_mprot() hook?).
-Cedric
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Haitao Huang @ 2019-05-16 1:19 UTC (permalink / raw)
To: Andy Lutomirski, Sean Christopherson
Cc: James Morris, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190515213858.GG5875@linux.intel.com>
On Wed, 15 May 2019 16:38:58 -0500, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:
>> 2) Just like any other DSO, there are potential issues with how
>> enclaves deal with writable vs executable memory. This takes two
>> forms. First, a task should probably require EXECMEM, EXECMOD, or
>> similar permission to run an enclave that can modify its own text.
>> Second, it would be nice if a task that did *not* have EXECMEM,
>> EXECMOD, or similar could still run the enclave if it had EXECUTE
>> permission on the file containing the enclave.
>>
>> Currently, this all works because DSOs are run by mmapping the file to
>> create multiple VMAs, some of which are executable, non-writable, and
>> non-CoWed, and some of which are writable but not executable. With
>> SGX, there's only really one inode per enclave (the anon_inode that
>> comes form /dev/sgx/enclave), and it can only be sensibly mapped
>> MAP_SHARED.
>
> I was wrong when I said /dev/sgx/enclave creates and returns an anon
> inode. I was thinking of the KVM model for creating VMs. SGX creates
> an enclave when /dev/sgx/enclave is opened and associates the enclave
> with the newly opened /dev/sgx/enclave fd.
>
> Regardless, the fundamental problem remains, mmap() of EPC works on a
> single inode.
If I read code in file_map_prot_check() correctly, only when you request
W+X at the same time that EXECMEM would be required for MAP_SHARED, right?
If so, I believe SGX enclaves would never need that.
^ permalink raw reply
* Re: [PATCH v2 2/4] lib: introduce test_meminit module
From: Kees Cook @ 2019-05-16 1:02 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, kernel-hardening, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Jann Horn, linux-mm,
linux-security-module
In-Reply-To: <20190514143537.10435-3-glider@google.com>
On Tue, May 14, 2019 at 04:35:35PM +0200, Alexander Potapenko wrote:
> Add tests for heap and pagealloc initialization.
> These can be used to check init_on_alloc and init_on_free implementations
> as well as other approaches to initialization.
This is nice! Easy way to test the results. It might be helpful to show
here what to expect when loading this module:
with either init_on_alloc=1 or init_on_free=1, I happily see:
test_meminit: all 10 tests in test_pages passed
test_meminit: all 40 tests in test_kvmalloc passed
test_meminit: all 20 tests in test_kmemcache passed
test_meminit: all 70 tests passed!
and without:
test_meminit: test_pages failed 10 out of 10 times
test_meminit: test_kvmalloc failed 40 out of 40 times
test_meminit: test_kmemcache failed 10 out of 20 times
test_meminit: failures: 60 out of 70
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Kees Cook <keescook@chromium.org>
note below...
> [...]
> diff --git a/lib/test_meminit.c b/lib/test_meminit.c
> new file mode 100644
> index 000000000000..67d759498030
> --- /dev/null
> +++ b/lib/test_meminit.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> [...]
> +module_init(test_meminit_init);
I get a warning at build about missing the license:
WARNING: modpost: missing MODULE_LICENSE() in lib/test_meminit.o
So, following the SPDX line, just add:
MODULE_LICENSE("GPL");
--
Kees Cook
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-15 23:13 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Sean Christopherson, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <alpine.LRH.2.21.1905160844130.29250@namei.org>
On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
>
> On Wed, 15 May 2019, Andy Lutomirski wrote:
>
> > > Why not just use an xattr, like security.sgx ?
> >
> > Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
> > install an enclave? I think that this decision should be left up the
> > administrator, and it should be easy to set up a loose policy where
> > anyone can load whatever enclave they want. That's what would happen
> > in my proposal if there was no LSM loaded or of the LSM policy didn't
> > restrict what .sigstruct files were acceptable.
> >
>
> You could try user.sigstruct, which does not require any privs.
>
I don't think I understand your proposal. What file would this
attribute be on? What would consume it?
I'm imagining that there's some enclave in a file
crypto_thingy.enclave. There's also a file crypto_thingy.sigstruct.
crypto_thingy.enclave has type lib_t or similar so that it's
executable. crypto_thingy.sigstruct has type sgx_sigstruct_t. The
enclave loader does, in effect:
void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
enclave_offset, len, ...);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
enclave_offset2, len, ...);
etc.
/* Here's where LSMs get to check that the sigstruct is acceptable.
The CPU will check that the sigstruct matches the enclave. */
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);
/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox