* [PATCH bpf-next v2 1/7] bpf: Refactor trampoline update code
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman
In-Reply-To: <20200304015528.29661-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
As we need to introduce a third type of attachment for trampolines, the
flattened signature of arch_prepare_bpf_trampoline gets even more
complicated.
Refactor the prog and count argument to arch_prepare_bpf_trampoline to
use bpf_tramp_progs to simplify the addition and accounting for new
attachment types.
Signed-off-by: KP Singh <kpsingh@google.com>
---
arch/x86/net/bpf_jit_comp.c | 31 ++++++++++---------
include/linux/bpf.h | 13 ++++++--
kernel/bpf/bpf_struct_ops.c | 12 ++++++-
kernel/bpf/trampoline.c | 62 +++++++++++++++++++++----------------
4 files changed, 73 insertions(+), 45 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 9ba08e9abc09..15c7d28bc05c 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1362,12 +1362,12 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
}
static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
- struct bpf_prog **progs, int prog_cnt, int stack_size)
+ struct bpf_tramp_progs *tp, int stack_size)
{
u8 *prog = *pprog;
int cnt = 0, i;
- for (i = 0; i < prog_cnt; i++) {
+ for (i = 0; i < tp->nr_progs; i++) {
if (emit_call(&prog, __bpf_prog_enter, prog))
return -EINVAL;
/* remember prog start time returned by __bpf_prog_enter */
@@ -1376,17 +1376,17 @@ static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
/* arg1: lea rdi, [rbp - stack_size] */
EMIT4(0x48, 0x8D, 0x7D, -stack_size);
/* arg2: progs[i]->insnsi for interpreter */
- if (!progs[i]->jited)
+ if (!tp->progs[i]->jited)
emit_mov_imm64(&prog, BPF_REG_2,
- (long) progs[i]->insnsi >> 32,
- (u32) (long) progs[i]->insnsi);
+ (long) tp->progs[i]->insnsi >> 32,
+ (u32) (long) tp->progs[i]->insnsi);
/* call JITed bpf program or interpreter */
- if (emit_call(&prog, progs[i]->bpf_func, prog))
+ if (emit_call(&prog, tp->progs[i]->bpf_func, prog))
return -EINVAL;
/* arg1: mov rdi, progs[i] */
- emit_mov_imm64(&prog, BPF_REG_1, (long) progs[i] >> 32,
- (u32) (long) progs[i]);
+ emit_mov_imm64(&prog, BPF_REG_1, (long) tp->progs[i] >> 32,
+ (u32) (long) tp->progs[i]);
/* arg2: mov rsi, rbx <- start time in nsec */
emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
if (emit_call(&prog, __bpf_prog_exit, prog))
@@ -1458,12 +1458,13 @@ static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
*/
int arch_prepare_bpf_trampoline(void *image, void *image_end,
const struct btf_func_model *m, u32 flags,
- struct bpf_prog **fentry_progs, int fentry_cnt,
- struct bpf_prog **fexit_progs, int fexit_cnt,
+ struct bpf_tramp_progs *tprogs,
void *orig_call)
{
int cnt = 0, nr_args = m->nr_args;
int stack_size = nr_args * 8;
+ struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY];
+ struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT];
u8 *prog;
/* x86-64 supports up to 6 arguments. 7+ can be added in the future */
@@ -1492,12 +1493,12 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
save_regs(m, &prog, nr_args, stack_size);
- if (fentry_cnt)
- if (invoke_bpf(m, &prog, fentry_progs, fentry_cnt, stack_size))
+ if (fentry->nr_progs)
+ if (invoke_bpf(m, &prog, fentry, stack_size))
return -EINVAL;
if (flags & BPF_TRAMP_F_CALL_ORIG) {
- if (fentry_cnt)
+ if (fentry->nr_progs)
restore_regs(m, &prog, nr_args, stack_size);
/* call original function */
@@ -1507,8 +1508,8 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
}
- if (fexit_cnt)
- if (invoke_bpf(m, &prog, fexit_progs, fexit_cnt, stack_size))
+ if (fexit->nr_progs)
+ if (invoke_bpf(m, &prog, fexit, stack_size))
return -EINVAL;
if (flags & BPF_TRAMP_F_RESTORE_REGS)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f13c78c6f29d..98ec10b23dbb 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -433,6 +433,16 @@ struct btf_func_model {
*/
#define BPF_TRAMP_F_SKIP_FRAME BIT(2)
+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
+ * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2
+ */
+#define BPF_MAX_TRAMP_PROGS 40
+
+struct bpf_tramp_progs {
+ struct bpf_prog *progs[BPF_MAX_TRAMP_PROGS];
+ int nr_progs;
+};
+
/* Different use cases for BPF trampoline:
* 1. replace nop at the function entry (kprobe equivalent)
* flags = BPF_TRAMP_F_RESTORE_REGS
@@ -455,8 +465,7 @@ struct btf_func_model {
*/
int arch_prepare_bpf_trampoline(void *image, void *image_end,
const struct btf_func_model *m, u32 flags,
- struct bpf_prog **fentry_progs, int fentry_cnt,
- struct bpf_prog **fexit_progs, int fexit_cnt,
+ struct bpf_tramp_progs *tprogs,
void *orig_call);
/* these two functions are called from generated trampoline */
u64 notrace __bpf_prog_enter(void);
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index c498f0fffb40..9f7e0328a644 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -320,6 +320,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
struct bpf_struct_ops_value *uvalue, *kvalue;
const struct btf_member *member;
const struct btf_type *t = st_ops->type;
+ struct bpf_tramp_progs *tprogs = NULL;
void *udata, *kdata;
int prog_fd, err = 0;
void *image;
@@ -425,10 +426,18 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
goto reset_unlock;
}
+ tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
+ if (!tprogs) {
+ err = -ENOMEM;
+ goto reset_unlock;
+ }
+
+ tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
+ tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
err = arch_prepare_bpf_trampoline(image,
st_map->image + PAGE_SIZE,
&st_ops->func_models[i], 0,
- &prog, 1, NULL, 0, NULL);
+ tprogs, NULL);
if (err < 0)
goto reset_unlock;
@@ -469,6 +478,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
memset(uvalue, 0, map->value_size);
memset(kvalue, 0, map->value_size);
unlock:
+ kfree(tprogs);
mutex_unlock(&st_map->lock);
return err;
}
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 704fa787fec0..cfe96d4cd89f 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -190,40 +190,49 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
return ret;
}
-/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
- * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2
- */
-#define BPF_MAX_TRAMP_PROGS 40
+static struct bpf_tramp_progs *
+bpf_trampoline_update_progs(struct bpf_trampoline *tr, int *total)
+{
+ struct bpf_tramp_progs *tprogs;
+ struct bpf_prog **progs;
+ struct bpf_prog_aux *aux;
+ int kind;
+
+ *total = 0;
+ tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
+ if (!tprogs)
+ return ERR_PTR(-ENOMEM);
+
+ for (kind = 0; kind < BPF_TRAMP_MAX; kind++) {
+ tprogs[kind].nr_progs = tr->progs_cnt[kind];
+ *total += tr->progs_cnt[kind];
+ progs = tprogs[kind].progs;
+
+ hlist_for_each_entry(aux, &tr->progs_hlist[kind], tramp_hlist)
+ *progs++ = aux->prog;
+ }
+ return tprogs;
+}
static int bpf_trampoline_update(struct bpf_trampoline *tr)
{
void *old_image = tr->image + ((tr->selector + 1) & 1) * BPF_IMAGE_SIZE/2;
void *new_image = tr->image + (tr->selector & 1) * BPF_IMAGE_SIZE/2;
- struct bpf_prog *progs_to_run[BPF_MAX_TRAMP_PROGS];
- int fentry_cnt = tr->progs_cnt[BPF_TRAMP_FENTRY];
- int fexit_cnt = tr->progs_cnt[BPF_TRAMP_FEXIT];
- struct bpf_prog **progs, **fentry, **fexit;
+ struct bpf_tramp_progs *tprogs;
u32 flags = BPF_TRAMP_F_RESTORE_REGS;
- struct bpf_prog_aux *aux;
- int err;
+ int err, total;
+
+ tprogs = bpf_trampoline_update_progs(tr, &total);
+ if (IS_ERR(tprogs))
+ return PTR_ERR(tprogs);
- if (fentry_cnt + fexit_cnt == 0) {
+ if (total == 0) {
err = unregister_fentry(tr, old_image);
tr->selector = 0;
goto out;
}
- /* populate fentry progs */
- fentry = progs = progs_to_run;
- hlist_for_each_entry(aux, &tr->progs_hlist[BPF_TRAMP_FENTRY], tramp_hlist)
- *progs++ = aux->prog;
-
- /* populate fexit progs */
- fexit = progs;
- hlist_for_each_entry(aux, &tr->progs_hlist[BPF_TRAMP_FEXIT], tramp_hlist)
- *progs++ = aux->prog;
-
- if (fexit_cnt)
+ if (tprogs[BPF_TRAMP_FEXIT].nr_progs)
flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
/* Though the second half of trampoline page is unused a task could be
@@ -232,12 +241,11 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
* preempted task. Hence wait for tasks to voluntarily schedule or go
* to userspace.
*/
+
synchronize_rcu_tasks();
err = arch_prepare_bpf_trampoline(new_image, new_image + BPF_IMAGE_SIZE / 2,
- &tr->func.model, flags,
- fentry, fentry_cnt,
- fexit, fexit_cnt,
+ &tr->func.model, flags, tprogs,
tr->func.addr);
if (err < 0)
goto out;
@@ -252,6 +260,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
goto out;
tr->selector++;
out:
+ kfree(tprogs);
return err;
}
@@ -409,8 +418,7 @@ void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
int __weak
arch_prepare_bpf_trampoline(void *image, void *image_end,
const struct btf_func_model *m, u32 flags,
- struct bpf_prog **fentry_progs, int fentry_cnt,
- struct bpf_prog **fexit_progs, int fexit_cnt,
+ struct bpf_tramp_progs *tprogs,
void *orig_call)
{
return -ENOTSUPP;
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v2 7/7] bpf: Add selftests for BPF_MODIFY_RETURN
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Paul Turner,
Jann Horn, Florent Revest, Brendan Jackman
In-Reply-To: <20200304015528.29661-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
Test for two scenarios:
* When the fmod_ret program returns 0, the original function should
be called along with fentry and fexit programs.
* When the fmod_ret program returns a non-zero value, the original
function should not be called, no side effect should be observed and
fentry and fexit programs should be called.
The result from the kernel function call and whether a side-effect is
observed is returned via the retval attr of the BPF_PROG_TEST_RUN (bpf)
syscall.
Signed-off-by: KP Singh <kpsingh@google.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
net/bpf/test_run.c | 22 ++++++-
.../selftests/bpf/prog_tests/modify_return.c | 65 +++++++++++++++++++
.../selftests/bpf/progs/modify_return.c | 49 ++++++++++++++
3 files changed, 135 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/modify_return.c
create mode 100644 tools/testing/selftests/bpf/progs/modify_return.c
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 3600f098e7c6..4c921f5154e0 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -10,6 +10,7 @@
#include <net/bpf_sk_storage.h>
#include <net/sock.h>
#include <net/tcp.h>
+#include <linux/error-injection.h>
#define CREATE_TRACE_POINTS
#include <trace/events/bpf_test_run.h>
@@ -143,6 +144,14 @@ int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
return a + (long)b + c + d + (long)e + f;
}
+int noinline bpf_modify_return_test(int a, int *b)
+{
+ *b += 1;
+ return a + *b;
+}
+
+ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
+
static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
u32 headroom, u32 tailroom)
{
@@ -168,7 +177,9 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
const union bpf_attr *kattr,
union bpf_attr __user *uattr)
{
- int err = -EFAULT;
+ u16 side_effect = 0, ret = 0;
+ int b = 2, err = -EFAULT;
+ u32 retval = 0;
switch (prog->expected_attach_type) {
case BPF_TRACE_FENTRY:
@@ -181,10 +192,19 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog,
bpf_fentry_test6(16, (void *)17, 18, 19, (void *)20, 21) != 111)
goto out;
break;
+ case BPF_MODIFY_RETURN:
+ ret = bpf_modify_return_test(1, &b);
+ if (b != 2)
+ side_effect = 1;
+ break;
default:
goto out;
}
+ retval = ((u32)side_effect << 16) | ret;
+ if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
+ goto out;
+
err = 0;
out:
trace_bpf_test_finish(&err);
diff --git a/tools/testing/selftests/bpf/prog_tests/modify_return.c b/tools/testing/selftests/bpf/prog_tests/modify_return.c
new file mode 100644
index 000000000000..97fec70c600b
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/modify_return.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include <test_progs.h>
+#include "modify_return.skel.h"
+
+#define LOWER(x) ((x) & 0xffff)
+#define UPPER(x) ((x) >> 16)
+
+
+static void run_test(__u32 input_retval, __u16 want_side_effect, __s16 want_ret)
+{
+ struct modify_return *skel = NULL;
+ int err, prog_fd;
+ __u32 duration = 0, retval;
+ __u16 side_effect;
+ __s16 ret;
+
+ skel = modify_return__open_and_load();
+ if (CHECK(!skel, "skel_load", "modify_return skeleton failed\n"))
+ goto cleanup;
+
+ err = modify_return__attach(skel);
+ if (CHECK(err, "modify_return", "attach failed: %d\n", err))
+ goto cleanup;
+
+ skel->bss->input_retval = input_retval;
+ prog_fd = bpf_program__fd(skel->progs.fmod_ret_test);
+ err = bpf_prog_test_run(prog_fd, 1, NULL, 0, NULL, 0,
+ &retval, &duration);
+
+ CHECK(err, "test_run", "err %d errno %d\n", err, errno);
+
+ side_effect = UPPER(retval);
+ ret = LOWER(retval);
+
+ CHECK(ret != want_ret, "test_run",
+ "unexpected ret: %d, expected: %d\n", ret, want_ret);
+ CHECK(side_effect != want_side_effect, "modify_return",
+ "unexpected side_effect: %d\n", side_effect);
+
+ CHECK(skel->bss->fentry_result != 1, "modify_return",
+ "fentry failed\n");
+ CHECK(skel->bss->fexit_result != 1, "modify_return",
+ "fexit failed\n");
+ CHECK(skel->bss->fmod_ret_result != 1, "modify_return",
+ "fmod_ret failed\n");
+
+cleanup:
+ modify_return__destroy(skel);
+}
+
+void test_modify_return(void)
+{
+ run_test(0 /* input_retval */,
+ 1 /* want_side_effect */,
+ 4 /* want_ret */);
+ run_test(-EINVAL /* input_retval */,
+ 0 /* want_side_effect */,
+ -EINVAL /* want_ret */);
+}
+
diff --git a/tools/testing/selftests/bpf/progs/modify_return.c b/tools/testing/selftests/bpf/progs/modify_return.c
new file mode 100644
index 000000000000..8b7466a15c6b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/modify_return.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+static int sequence = 0;
+__s32 input_retval = 0;
+
+__u64 fentry_result = 0;
+SEC("fentry/bpf_modify_return_test")
+int BPF_PROG(fentry_test, int a, __u64 b)
+{
+ sequence++;
+ fentry_result = (sequence == 1);
+ return 0;
+}
+
+__u64 fmod_ret_result = 0;
+SEC("fmod_ret/bpf_modify_return_test")
+int BPF_PROG(fmod_ret_test, int a, int *b, int ret)
+{
+ sequence++;
+ /* This is the first fmod_ret program, the ret passed should be 0 */
+ fmod_ret_result = (sequence == 2 && ret == 0);
+ return input_retval;
+}
+
+__u64 fexit_result = 0;
+SEC("fexit/bpf_modify_return_test")
+int BPF_PROG(fexit_test, int a, __u64 b, int ret)
+{
+ sequence++;
+ /* If the input_reval is non-zero a successful modification should have
+ * occurred.
+ */
+ if (input_retval)
+ fexit_result = (sequence == 3 && ret == input_retval);
+ else
+ fexit_result = (sequence == 3 && ret == 4);
+
+ return 0;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v2 6/7] bpf: Add test ops for BPF_PROG_TYPE_TRACING
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman
In-Reply-To: <20200304015528.29661-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
The current fexit and fentry tests rely on a different program to
exercise the functions they attach to. Instead of doing this, implement
the test operations for tracing which will also be used for
BPF_MODIFY_RETURN in a subsequent patch.
Also, clean up the fexit test to use the generated skeleton.
Signed-off-by: KP Singh <kpsingh@google.com>
---
include/linux/bpf.h | 10 +++
kernel/trace/bpf_trace.c | 1 +
net/bpf/test_run.c | 37 +++++++---
.../selftests/bpf/prog_tests/fentry_fexit.c | 12 +---
.../selftests/bpf/prog_tests/fentry_test.c | 14 ++--
.../selftests/bpf/prog_tests/fexit_test.c | 69 ++++++-------------
6 files changed, 67 insertions(+), 76 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3cfdc216a2f4..c00919025532 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1156,6 +1156,9 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
+int bpf_prog_test_run_tracing(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr);
int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
const union bpf_attr *kattr,
union bpf_attr __user *uattr);
@@ -1313,6 +1316,13 @@ static inline int bpf_prog_test_run_skb(struct bpf_prog *prog,
return -ENOTSUPP;
}
+static inline int bpf_prog_test_run_tracing(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr)
+{
+ return -ENOTSUPP;
+}
+
static inline int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
const union bpf_attr *kattr,
union bpf_attr __user *uattr)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 07764c761073..363e0a2c75cf 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1266,6 +1266,7 @@ const struct bpf_verifier_ops tracing_verifier_ops = {
};
const struct bpf_prog_ops tracing_prog_ops = {
+ .test_run = bpf_prog_test_run_tracing,
};
static bool raw_tp_writable_prog_is_valid_access(int off, int size,
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 1cd7a1c2f8b2..3600f098e7c6 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -160,18 +160,37 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
kfree(data);
return ERR_PTR(-EFAULT);
}
- if (bpf_fentry_test1(1) != 2 ||
- bpf_fentry_test2(2, 3) != 5 ||
- bpf_fentry_test3(4, 5, 6) != 15 ||
- bpf_fentry_test4((void *)7, 8, 9, 10) != 34 ||
- bpf_fentry_test5(11, (void *)12, 13, 14, 15) != 65 ||
- bpf_fentry_test6(16, (void *)17, 18, 19, (void *)20, 21) != 111) {
- kfree(data);
- return ERR_PTR(-EFAULT);
- }
+
return data;
}
+int bpf_prog_test_run_tracing(struct bpf_prog *prog,
+ const union bpf_attr *kattr,
+ union bpf_attr __user *uattr)
+{
+ int err = -EFAULT;
+
+ switch (prog->expected_attach_type) {
+ case BPF_TRACE_FENTRY:
+ case BPF_TRACE_FEXIT:
+ if (bpf_fentry_test1(1) != 2 ||
+ bpf_fentry_test2(2, 3) != 5 ||
+ bpf_fentry_test3(4, 5, 6) != 15 ||
+ bpf_fentry_test4((void *)7, 8, 9, 10) != 34 ||
+ bpf_fentry_test5(11, (void *)12, 13, 14, 15) != 65 ||
+ bpf_fentry_test6(16, (void *)17, 18, 19, (void *)20, 21) != 111)
+ goto out;
+ break;
+ default:
+ goto out;
+ }
+
+ err = 0;
+out:
+ trace_bpf_test_finish(&err);
+ return err;
+}
+
static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
{
void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c b/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c
index 235ac4f67f5b..83493bd5745c 100644
--- a/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c
+++ b/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c
@@ -1,22 +1,17 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2019 Facebook */
#include <test_progs.h>
-#include "test_pkt_access.skel.h"
#include "fentry_test.skel.h"
#include "fexit_test.skel.h"
void test_fentry_fexit(void)
{
- struct test_pkt_access *pkt_skel = NULL;
struct fentry_test *fentry_skel = NULL;
struct fexit_test *fexit_skel = NULL;
__u64 *fentry_res, *fexit_res;
__u32 duration = 0, retval;
- int err, pkt_fd, i;
+ int err, prog_fd, i;
- pkt_skel = test_pkt_access__open_and_load();
- if (CHECK(!pkt_skel, "pkt_skel_load", "pkt_access skeleton failed\n"))
- return;
fentry_skel = fentry_test__open_and_load();
if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
goto close_prog;
@@ -31,8 +26,8 @@ void test_fentry_fexit(void)
if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
goto close_prog;
- pkt_fd = bpf_program__fd(pkt_skel->progs.test_pkt_access);
- err = bpf_prog_test_run(pkt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+ prog_fd = bpf_program__fd(fexit_skel->progs.test1);
+ err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
NULL, NULL, &retval, &duration);
CHECK(err || retval, "ipv6",
"err %d errno %d retval %d duration %d\n",
@@ -49,7 +44,6 @@ void test_fentry_fexit(void)
}
close_prog:
- test_pkt_access__destroy(pkt_skel);
fentry_test__destroy(fentry_skel);
fexit_test__destroy(fexit_skel);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_test.c b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
index 5cc06021f27d..04ebbf1cb390 100644
--- a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
@@ -1,20 +1,15 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2019 Facebook */
#include <test_progs.h>
-#include "test_pkt_access.skel.h"
#include "fentry_test.skel.h"
void test_fentry_test(void)
{
- struct test_pkt_access *pkt_skel = NULL;
struct fentry_test *fentry_skel = NULL;
- int err, pkt_fd, i;
+ int err, prog_fd, i;
__u32 duration = 0, retval;
__u64 *result;
- pkt_skel = test_pkt_access__open_and_load();
- if (CHECK(!pkt_skel, "pkt_skel_load", "pkt_access skeleton failed\n"))
- return;
fentry_skel = fentry_test__open_and_load();
if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
goto cleanup;
@@ -23,10 +18,10 @@ void test_fentry_test(void)
if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
goto cleanup;
- pkt_fd = bpf_program__fd(pkt_skel->progs.test_pkt_access);
- err = bpf_prog_test_run(pkt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+ prog_fd = bpf_program__fd(fentry_skel->progs.test1);
+ err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
NULL, NULL, &retval, &duration);
- CHECK(err || retval, "ipv6",
+ CHECK(err || retval, "test_run",
"err %d errno %d retval %d duration %d\n",
err, errno, retval, duration);
@@ -39,5 +34,4 @@ void test_fentry_test(void)
cleanup:
fentry_test__destroy(fentry_skel);
- test_pkt_access__destroy(pkt_skel);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_test.c b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
index d2c3655dd7a3..78d7a2765c27 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c
@@ -1,64 +1,37 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2019 Facebook */
#include <test_progs.h>
+#include "fexit_test.skel.h"
void test_fexit_test(void)
{
- struct bpf_prog_load_attr attr = {
- .file = "./fexit_test.o",
- };
-
- char prog_name[] = "fexit/bpf_fentry_testX";
- struct bpf_object *obj = NULL, *pkt_obj;
- int err, pkt_fd, kfree_skb_fd, i;
- struct bpf_link *link[6] = {};
- struct bpf_program *prog[6];
+ struct fexit_test *fexit_skel = NULL;
+ int err, prog_fd, i;
__u32 duration = 0, retval;
- struct bpf_map *data_map;
- const int zero = 0;
- u64 result[6];
+ __u64 *result;
- err = bpf_prog_load("./test_pkt_access.o", BPF_PROG_TYPE_SCHED_CLS,
- &pkt_obj, &pkt_fd);
- if (CHECK(err, "prog_load sched cls", "err %d errno %d\n", err, errno))
- return;
- err = bpf_prog_load_xattr(&attr, &obj, &kfree_skb_fd);
- if (CHECK(err, "prog_load fail", "err %d errno %d\n", err, errno))
- goto close_prog;
+ fexit_skel = fexit_test__open_and_load();
+ if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
+ goto cleanup;
- for (i = 0; i < 6; i++) {
- prog_name[sizeof(prog_name) - 2] = '1' + i;
- prog[i] = bpf_object__find_program_by_title(obj, prog_name);
- if (CHECK(!prog[i], "find_prog", "prog %s not found\n", prog_name))
- goto close_prog;
- link[i] = bpf_program__attach_trace(prog[i]);
- if (CHECK(IS_ERR(link[i]), "attach_trace", "failed to link\n"))
- goto close_prog;
- }
- data_map = bpf_object__find_map_by_name(obj, "fexit_te.bss");
- if (CHECK(!data_map, "find_data_map", "data map not found\n"))
- goto close_prog;
+ err = fexit_test__attach(fexit_skel);
+ if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err))
+ goto cleanup;
- err = bpf_prog_test_run(pkt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+ prog_fd = bpf_program__fd(fexit_skel->progs.test1);
+ err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
NULL, NULL, &retval, &duration);
- CHECK(err || retval, "ipv6",
+ CHECK(err || retval, "test_run",
"err %d errno %d retval %d duration %d\n",
err, errno, retval, duration);
- err = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, &result);
- if (CHECK(err, "get_result",
- "failed to get output data: %d\n", err))
- goto close_prog;
-
- for (i = 0; i < 6; i++)
- if (CHECK(result[i] != 1, "result", "bpf_fentry_test%d failed err %ld\n",
- i + 1, result[i]))
- goto close_prog;
+ result = (__u64 *)fexit_skel->bss;
+ for (i = 0; i < 6; i++) {
+ if (CHECK(result[i] != 1, "result",
+ "fexit_test%d failed err %lld\n", i + 1, result[i]))
+ goto cleanup;
+ }
-close_prog:
- for (i = 0; i < 6; i++)
- if (!IS_ERR_OR_NULL(link[i]))
- bpf_link__destroy(link[i]);
- bpf_object__close(obj);
- bpf_object__close(pkt_obj);
+cleanup:
+ fexit_test__destroy(fexit_skel);
}
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v2 4/7] bpf: Attachment verification for BPF_MODIFY_RETURN
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman
In-Reply-To: <20200304015528.29661-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
- Allow BPF_MODIFY_RETURN attachment only to functions that are:
* Whitelisted by for error injection i.e. by checking
within_error_injection_list. Similar disucssions happened for the
bpf_overrie_return helper.
* security hooks, this is expected to be cleaned up with the LSM
changes after the KRSI patches introduce the LSM_HOOK macro:
https://lore.kernel.org/bpf/20200220175250.10795-1-kpsingh@chromium.org/
- The attachment is currently limited to functions that return an int.
This can be extended later other types (e.g. PTR).
Signed-off-by: KP Singh <kpsingh@google.com>
---
kernel/bpf/btf.c | 28 ++++++++++++++++++++--------
kernel/bpf/verifier.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 30841fb8b3c0..50080add2ab9 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3710,14 +3710,26 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
nr_args--;
}
- if ((prog->expected_attach_type == BPF_TRACE_FEXIT ||
- prog->expected_attach_type == BPF_MODIFY_RETURN) &&
- arg == nr_args) {
- if (!t)
- /* Default prog with 5 args. 6th arg is retval. */
- return true;
- /* function return type */
- t = btf_type_by_id(btf, t->type);
+ if (arg == nr_args) {
+ if (prog->expected_attach_type == BPF_TRACE_FEXIT) {
+ if (!t)
+ return true;
+ t = btf_type_by_id(btf, t->type);
+ } else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
+ /* For now the BPF_MODIFY_RETURN can only be attached to
+ * functions that return an int.
+ */
+ if (!t)
+ return false;
+
+ t = btf_type_skip_modifiers(btf, t->type, NULL);
+ if (!btf_type_is_int(t)) {
+ bpf_log(log,
+ "ret type %s not allowed for fmod_ret\n",
+ btf_kind_str[BTF_INFO_KIND(t->info)]);
+ return false;
+ }
+ }
} else if (arg >= nr_args) {
bpf_log(log, "func '%s' doesn't have %d-th argument\n",
tname, arg + 1);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2460c8e6b5be..ae32517d4ccd 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19,6 +19,7 @@
#include <linux/sort.h>
#include <linux/perf_event.h>
#include <linux/ctype.h>
+#include <linux/error-injection.h>
#include "disasm.h"
@@ -9800,6 +9801,33 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
return 0;
}
+#define SECURITY_PREFIX "security_"
+
+static int check_attach_modify_return(struct bpf_verifier_env *env)
+{
+ struct bpf_prog *prog = env->prog;
+ unsigned long addr = (unsigned long) prog->aux->trampoline->func.addr;
+
+ if (within_error_injection_list(addr))
+ return 0;
+
+ /* This is expected to be cleaned up in the future with the KRSI effort
+ * introducing the LSM_HOOK macro for cleaning up lsm_hooks.h.
+ */
+ if (!strncmp(SECURITY_PREFIX, prog->aux->attach_func_name,
+ sizeof(SECURITY_PREFIX) - 1)) {
+
+ if (!capable(CAP_MAC_ADMIN))
+ return -EPERM;
+
+ return 0;
+ }
+
+ verbose(env, "fmod_ret attach_btf_id %u (%s) is not modifiable\n",
+ prog->aux->attach_btf_id, prog->aux->attach_func_name);
+
+ return -EINVAL;
+}
static int check_attach_btf_id(struct bpf_verifier_env *env)
{
@@ -10000,6 +10028,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
}
tr->func.addr = (void *)addr;
prog->aux->trampoline = tr;
+
+ if (prog->expected_attach_type == BPF_MODIFY_RETURN)
+ ret = check_attach_modify_return(env);
out:
mutex_unlock(&tr->mutex);
if (ret)
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v2 3/7] bpf: Introduce BPF_MODIFY_RETURN
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman
In-Reply-To: <20200304015528.29661-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
When multiple programs are attached, each program receives the return
value from the previous program on the stack and the last program
provides the return value to the attached function.
The fmod_ret bpf programs are run after the fentry programs and before
the fexit programs. The original function is only called if all the
fmod_ret programs return 0 to avoid any unintended side-effects. The
success value, i.e. 0 is not currently configurable but can be made so
where user-space can specify it at load time.
For example:
int func_to_be_attached(int a, int b)
{ <--- do_fentry
do_fmod_ret:
<update ret by calling fmod_ret>
if (ret != 0)
goto do_fexit;
original_function:
<side_effects_happen_here>
} <--- do_fexit
The fmod_ret program attached to this function can be defined as:
SEC("fmod_ret/func_to_be_attached")
int BPF_PROG(func_name, int a, int b, int ret)
{
// This will skip the original function logic.
return 1;
}
The first fmod_ret program is passed 0 in its return argument.
Signed-off-by: KP Singh <kpsingh@google.com>
---
arch/x86/net/bpf_jit_comp.c | 130 ++++++++++++++++++++++++++++++---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 1 +
kernel/bpf/btf.c | 3 +-
kernel/bpf/syscall.c | 1 +
kernel/bpf/trampoline.c | 5 +-
kernel/bpf/verifier.c | 1 +
tools/include/uapi/linux/bpf.h | 1 +
8 files changed, 130 insertions(+), 13 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index d6349e930b06..b1fd000feb89 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1362,7 +1362,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
}
static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
- struct bpf_prog *p, int stack_size)
+ struct bpf_prog *p, int stack_size, bool mod_ret)
{
u8 *prog = *pprog;
int cnt = 0;
@@ -1383,6 +1383,13 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
if (emit_call(&prog, p->bpf_func, prog))
return -EINVAL;
+ /* BPF_TRAMP_MODIFY_RETURN trampolines can modify the return
+ * of the previous call which is then passed on the stack to
+ * the next BPF program.
+ */
+ if (mod_ret)
+ emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
+
/* arg1: mov rdi, progs[i] */
emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32,
(u32) (long) p);
@@ -1442,6 +1449,23 @@ static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
return 0;
}
+static int emit_mod_ret_check_imm8(u8 **pprog, int value)
+{
+ u8 *prog = *pprog;
+ int cnt = 0;
+
+ if (!is_imm8(value))
+ return -EINVAL;
+
+ if (value == 0)
+ EMIT2(0x85, add_2reg(0xC0, BPF_REG_0, BPF_REG_0));
+ else
+ EMIT3(0x83, add_1reg(0xF8, BPF_REG_0), value);
+
+ *pprog = prog;
+ return 0;
+}
+
static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
struct bpf_tramp_progs *tp, int stack_size)
{
@@ -1449,9 +1473,49 @@ static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
u8 *prog = *pprog;
for (i = 0; i < tp->nr_progs; i++) {
- if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size))
+ if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size, false))
+ return -EINVAL;
+ }
+ *pprog = prog;
+ return 0;
+}
+
+static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
+ struct bpf_tramp_progs *tp, int stack_size,
+ u8 **branches)
+{
+ u8 *prog = *pprog;
+ int i;
+
+ /* The first fmod_ret program will receive a garbage return value.
+ * Set this to 0 to avoid confusing the program.
+ */
+ emit_mov_imm32(&prog, false, BPF_REG_0, 0);
+ emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
+ for (i = 0; i < tp->nr_progs; i++) {
+ if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size, true))
return -EINVAL;
+
+ /* Generate a branch:
+ *
+ * if (ret != 0)
+ * goto do_fexit;
+ *
+ * If needed this can be extended to any integer value which can
+ * be passed by user-space when the program is loaded.
+ */
+ if (emit_mod_ret_check_imm8(&prog, 0))
+ return -EINVAL;
+
+ /* Save the location of the branch and Generate 6 nops
+ * (4 bytes for an offset and 2 bytes for the jump) These nops
+ * are replaced with a conditional jump once do_fexit (i.e. the
+ * start of the fexit invocation) is finalized.
+ */
+ branches[i] = prog;
+ emit_nops(&prog, 4 + 2);
}
+
*pprog = prog;
return 0;
}
@@ -1521,10 +1585,12 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
struct bpf_tramp_progs *tprogs,
void *orig_call)
{
- int cnt = 0, nr_args = m->nr_args;
+ int ret, i, cnt = 0, nr_args = m->nr_args;
int stack_size = nr_args * 8;
struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY];
struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT];
+ struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
+ u8 **branches = NULL;
u8 *prog;
/* x86-64 supports up to 6 arguments. 7+ can be added in the future */
@@ -1557,24 +1623,60 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
if (invoke_bpf(m, &prog, fentry, stack_size))
return -EINVAL;
+ if (fmod_ret->nr_progs) {
+ branches = kcalloc(fmod_ret->nr_progs, sizeof(u8 *),
+ GFP_KERNEL);
+ if (!branches)
+ return -ENOMEM;
+
+ if (invoke_bpf_mod_ret(m, &prog, fmod_ret, stack_size,
+ branches)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
+ }
+
if (flags & BPF_TRAMP_F_CALL_ORIG) {
- if (fentry->nr_progs)
+ if (fentry->nr_progs || fmod_ret->nr_progs)
restore_regs(m, &prog, nr_args, stack_size);
/* call original function */
- if (emit_call(&prog, orig_call, prog))
- return -EINVAL;
+ if (emit_call(&prog, orig_call, prog)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
/* remember return value in a stack for bpf prog to access */
emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
}
+ if (fmod_ret->nr_progs) {
+ /* From Intel 64 and IA-32 Architectures Optimization
+ * Reference Manual, 3.4.1.4 Code Alignment, Assembly/Compiler
+ * Coding Rule 11: All branch targets should be 16-byte
+ * aligned.
+ */
+ emit_align(&prog, 16);
+ /* Update the branches saved in invoke_bpf_mod_ret with the
+ * aligned address of do_fexit.
+ */
+ for (i = 0; i < fmod_ret->nr_progs; i++)
+ emit_cond_near_jump(&branches[i], prog, branches[i],
+ X86_JNE);
+ }
+
if (fexit->nr_progs)
- if (invoke_bpf(m, &prog, fexit, stack_size))
- return -EINVAL;
+ if (invoke_bpf(m, &prog, fexit, stack_size)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
if (flags & BPF_TRAMP_F_RESTORE_REGS)
restore_regs(m, &prog, nr_args, stack_size);
+ /* This needs to be done regardless. If there were fmod_ret programs,
+ * the return value is only updated on the stack and still needs to be
+ * restored to R0.
+ */
if (flags & BPF_TRAMP_F_CALL_ORIG)
/* restore original return value back into RAX */
emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
@@ -1586,9 +1688,15 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
EMIT4(0x48, 0x83, 0xC4, 8); /* add rsp, 8 */
EMIT1(0xC3); /* ret */
/* Make sure the trampoline generation logic doesn't overflow */
- if (WARN_ON_ONCE(prog > (u8 *)image_end - BPF_INSN_SAFETY))
- return -EFAULT;
- return prog - (u8 *)image;
+ if (WARN_ON_ONCE(prog > (u8 *)image_end - BPF_INSN_SAFETY)) {
+ ret = -EFAULT;
+ goto cleanup;
+ }
+ ret = prog - (u8 *)image;
+
+cleanup:
+ kfree(branches);
+ return ret;
}
static int emit_fallback_jump(u8 **pprog)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 98ec10b23dbb..3cfdc216a2f4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -473,6 +473,7 @@ void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
enum bpf_tramp_prog_type {
BPF_TRAMP_FENTRY,
+ BPF_TRAMP_MODIFY_RETURN,
BPF_TRAMP_FEXIT,
BPF_TRAMP_MAX,
BPF_TRAMP_REPLACE, /* more than MAX */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 180337fae97e..5cbb92dad544 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -210,6 +210,7 @@ enum bpf_attach_type {
BPF_TRACE_RAW_TP,
BPF_TRACE_FENTRY,
BPF_TRACE_FEXIT,
+ BPF_MODIFY_RETURN,
__MAX_BPF_ATTACH_TYPE
};
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 787140095e58..30841fb8b3c0 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3710,7 +3710,8 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
nr_args--;
}
- if (prog->expected_attach_type == BPF_TRACE_FEXIT &&
+ if ((prog->expected_attach_type == BPF_TRACE_FEXIT ||
+ prog->expected_attach_type == BPF_MODIFY_RETURN) &&
arg == nr_args) {
if (!t)
/* Default prog with 5 args. 6th arg is retval. */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 13de65363ba2..7ce0815793dd 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2324,6 +2324,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
prog->expected_attach_type != BPF_TRACE_FEXIT &&
+ prog->expected_attach_type != BPF_MODIFY_RETURN &&
prog->type != BPF_PROG_TYPE_EXT) {
err = -EINVAL;
goto out_put_prog;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index cfe96d4cd89f..812fa50dc2fa 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -232,7 +232,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
goto out;
}
- if (tprogs[BPF_TRAMP_FEXIT].nr_progs)
+ if (tprogs[BPF_TRAMP_FEXIT].nr_progs ||
+ tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs)
flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
/* Though the second half of trampoline page is unused a task could be
@@ -269,6 +270,8 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(enum bpf_attach_type t)
switch (t) {
case BPF_TRACE_FENTRY:
return BPF_TRAMP_FENTRY;
+ case BPF_MODIFY_RETURN:
+ return BPF_TRAMP_MODIFY_RETURN;
case BPF_TRACE_FEXIT:
return BPF_TRAMP_FEXIT;
default:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 289383edfc8c..2460c8e6b5be 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9950,6 +9950,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
if (!prog_extension)
return -EINVAL;
/* fallthrough */
+ case BPF_MODIFY_RETURN:
case BPF_TRACE_FENTRY:
case BPF_TRACE_FEXIT:
if (!btf_type_is_func(t)) {
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 7c689f4552dd..7b0c307153e9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -210,6 +210,7 @@ enum bpf_attach_type {
BPF_TRACE_RAW_TP,
BPF_TRACE_FENTRY,
BPF_TRACE_FEXIT,
+ BPF_MODIFY_RETURN,
__MAX_BPF_ATTACH_TYPE
};
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v2 2/7] bpf: JIT helpers for fmod_ret progs
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman
In-Reply-To: <20200304015528.29661-1-kpsingh@chromium.org>
From: KP Singh <kpsingh@google.com>
* Split the invoke_bpf program to prepare for special handling of
fmod_ret programs introduced in a subsequent patch.
* Move the definition of emit_cond_near_jump and emit_nops as they are
needed for fmod_ret.
* Refactor branch target alignment into its own function
align16_branch_target.
Signed-off-by: KP Singh <kpsingh@google.com>
---
arch/x86/net/bpf_jit_comp.c | 148 +++++++++++++++++++++---------------
1 file changed, 85 insertions(+), 63 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 15c7d28bc05c..d6349e930b06 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1361,35 +1361,95 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
-(stack_size - i * 8));
}
+static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
+ struct bpf_prog *p, int stack_size)
+{
+ u8 *prog = *pprog;
+ int cnt = 0;
+
+ if (emit_call(&prog, __bpf_prog_enter, prog))
+ return -EINVAL;
+ /* remember prog start time returned by __bpf_prog_enter */
+ emit_mov_reg(&prog, true, BPF_REG_6, BPF_REG_0);
+
+ /* arg1: lea rdi, [rbp - stack_size] */
+ EMIT4(0x48, 0x8D, 0x7D, -stack_size);
+ /* arg2: progs[i]->insnsi for interpreter */
+ if (!p->jited)
+ emit_mov_imm64(&prog, BPF_REG_2,
+ (long) p->insnsi >> 32,
+ (u32) (long) p->insnsi);
+ /* call JITed bpf program or interpreter */
+ if (emit_call(&prog, p->bpf_func, prog))
+ return -EINVAL;
+
+ /* arg1: mov rdi, progs[i] */
+ emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32,
+ (u32) (long) p);
+ /* arg2: mov rsi, rbx <- start time in nsec */
+ emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
+ if (emit_call(&prog, __bpf_prog_exit, prog))
+ return -EINVAL;
+
+ *pprog = prog;
+ return 0;
+}
+
+static void emit_nops(u8 **pprog, unsigned int len)
+{
+ unsigned int i, noplen;
+ u8 *prog = *pprog;
+ int cnt = 0;
+
+ while (len > 0) {
+ noplen = len;
+
+ if (noplen > ASM_NOP_MAX)
+ noplen = ASM_NOP_MAX;
+
+ for (i = 0; i < noplen; i++)
+ EMIT1(ideal_nops[noplen][i]);
+ len -= noplen;
+ }
+
+ *pprog = prog;
+}
+
+static void emit_align(u8 **pprog, u32 align)
+{
+ u8 *target, *prog = *pprog;
+
+ target = PTR_ALIGN(prog, align);
+ if (target != prog)
+ emit_nops(&prog, target - prog);
+
+ *pprog = prog;
+}
+
+static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
+{
+ u8 *prog = *pprog;
+ int cnt = 0;
+ s64 offset;
+
+ offset = func - (ip + 2 + 4);
+ if (!is_simm32(offset)) {
+ pr_err("Target %p is out of range\n", func);
+ return -EINVAL;
+ }
+ EMIT2_off32(0x0F, jmp_cond + 0x10, offset);
+ *pprog = prog;
+ return 0;
+}
+
static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
struct bpf_tramp_progs *tp, int stack_size)
{
+ int i;
u8 *prog = *pprog;
- int cnt = 0, i;
for (i = 0; i < tp->nr_progs; i++) {
- if (emit_call(&prog, __bpf_prog_enter, prog))
- return -EINVAL;
- /* remember prog start time returned by __bpf_prog_enter */
- emit_mov_reg(&prog, true, BPF_REG_6, BPF_REG_0);
-
- /* arg1: lea rdi, [rbp - stack_size] */
- EMIT4(0x48, 0x8D, 0x7D, -stack_size);
- /* arg2: progs[i]->insnsi for interpreter */
- if (!tp->progs[i]->jited)
- emit_mov_imm64(&prog, BPF_REG_2,
- (long) tp->progs[i]->insnsi >> 32,
- (u32) (long) tp->progs[i]->insnsi);
- /* call JITed bpf program or interpreter */
- if (emit_call(&prog, tp->progs[i]->bpf_func, prog))
- return -EINVAL;
-
- /* arg1: mov rdi, progs[i] */
- emit_mov_imm64(&prog, BPF_REG_1, (long) tp->progs[i] >> 32,
- (u32) (long) tp->progs[i]);
- /* arg2: mov rsi, rbx <- start time in nsec */
- emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
- if (emit_call(&prog, __bpf_prog_exit, prog))
+ if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size))
return -EINVAL;
}
*pprog = prog;
@@ -1531,42 +1591,6 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
return prog - (u8 *)image;
}
-static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
-{
- u8 *prog = *pprog;
- int cnt = 0;
- s64 offset;
-
- offset = func - (ip + 2 + 4);
- if (!is_simm32(offset)) {
- pr_err("Target %p is out of range\n", func);
- return -EINVAL;
- }
- EMIT2_off32(0x0F, jmp_cond + 0x10, offset);
- *pprog = prog;
- return 0;
-}
-
-static void emit_nops(u8 **pprog, unsigned int len)
-{
- unsigned int i, noplen;
- u8 *prog = *pprog;
- int cnt = 0;
-
- while (len > 0) {
- noplen = len;
-
- if (noplen > ASM_NOP_MAX)
- noplen = ASM_NOP_MAX;
-
- for (i = 0; i < noplen; i++)
- EMIT1(ideal_nops[noplen][i]);
- len -= noplen;
- }
-
- *pprog = prog;
-}
-
static int emit_fallback_jump(u8 **pprog)
{
u8 *prog = *pprog;
@@ -1589,7 +1613,7 @@ static int emit_fallback_jump(u8 **pprog)
static int emit_bpf_dispatcher(u8 **pprog, int a, int b, s64 *progs)
{
- u8 *jg_reloc, *jg_target, *prog = *pprog;
+ u8 *jg_reloc, *prog = *pprog;
int pivot, err, jg_bytes = 1, cnt = 0;
s64 jg_offset;
@@ -1644,9 +1668,7 @@ static int emit_bpf_dispatcher(u8 **pprog, int a, int b, s64 *progs)
* Coding Rule 11: All branch targets should be 16-byte
* aligned.
*/
- jg_target = PTR_ALIGN(prog, 16);
- if (jg_target != prog)
- emit_nops(&prog, jg_target - prog);
+ emit_align(&prog, 16);
jg_offset = prog - jg_reloc;
emit_code(jg_reloc - jg_bytes, jg_offset, jg_bytes);
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v2 0/7] Introduce BPF_MODIFY_RET tracing progs
From: KP Singh @ 2020-03-04 1:55 UTC (permalink / raw)
To: linux-security-module, linux-kernel, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn,
Florent Revest, Brendan Jackman
From: KP Singh <kpsingh@google.com>
v1 -> v2:
* Adressed Andrii's feedback.
* Fixed a bug that Alexei noticed about nop generation.
* Rebase.
This was brought up in the KRSI v4 discussion and found to be useful
both for security and tracing programs.
https://lore.kernel.org/bpf/20200225193108.GB22391@chromium.org/
The modify_return programs are allowed for security hooks (with an
extra CAP_MAC_ADMIN check) and functions whitelisted for error
injection (ALLOW_ERROR_INJECTION).
The "security_" check is expected to be cleaned up with the KRSI patch
series.
Here is an example of how a fmod_ret program behaves:
int func_to_be_attached(int a, int b)
{ <--- do_fentry
do_fmod_ret:
<update ret by calling fmod_ret>
if (ret != 0)
goto do_fexit;
original_function:
<side_effects_happen_here>
} <--- do_fexit
ALLOW_ERROR_INJECTION(func_to_be_attached, ERRNO)
The fmod_ret program attached to this function can be defined as:
SEC("fmod_ret/func_to_be_attached")
int BPF_PROG(func_name, int a, int b, int ret)
{
// This will skip the original function logic.
return -1;
}
KP Singh (7):
bpf: Refactor trampoline update code
bpf: JIT helpers for fmod_ret progs
bpf: Introduce BPF_MODIFY_RETURN
bpf: Attachment verification for BPF_MODIFY_RETURN
tools/libbpf: Add support for BPF_MODIFY_RETURN
bpf: Add test ops for BPF_PROG_TYPE_TRACING
bpf: Add selftests for BPF_MODIFY_RETURN
arch/x86/net/bpf_jit_comp.c | 279 +++++++++++++-----
include/linux/bpf.h | 24 +-
include/uapi/linux/bpf.h | 1 +
kernel/bpf/bpf_struct_ops.c | 12 +-
kernel/bpf/btf.c | 27 +-
kernel/bpf/syscall.c | 1 +
kernel/bpf/trampoline.c | 65 ++--
kernel/bpf/verifier.c | 32 ++
kernel/trace/bpf_trace.c | 1 +
net/bpf/test_run.c | 57 +++-
tools/include/uapi/linux/bpf.h | 1 +
tools/lib/bpf/libbpf.c | 4 +
.../selftests/bpf/prog_tests/fentry_fexit.c | 12 +-
.../selftests/bpf/prog_tests/fentry_test.c | 14 +-
.../selftests/bpf/prog_tests/fexit_test.c | 69 ++---
.../selftests/bpf/prog_tests/modify_return.c | 65 ++++
.../selftests/bpf/progs/modify_return.c | 49 +++
17 files changed, 526 insertions(+), 187 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/modify_return.c
create mode 100644 tools/testing/selftests/bpf/progs/modify_return.c
--
2.20.1
^ permalink raw reply
* [PATCH v28 15/22] x86/sgx: Add provisioning
From: Jarkko Sakkinen @ 2020-03-03 23:36 UTC (permalink / raw)
To: linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk, Jarkko Sakkinen,
linux-security-module
In-Reply-To: <20200303233609.713348-1-jarkko.sakkinen@linux.intel.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=a, Size: 5616 bytes --]
In order to provide a mechanism for devilering provisoning rights:
1. Add a new device file /dev/sgx/provision that works as a token for
allowing an enclave to have the provisioning privileges.
2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
following data structure:
struct sgx_enclave_set_attribute {
__u64 addr;
__u64 attribute_fd;
};
A daemon could sit on top of /dev/sgx/provision and send a file
descriptor of this file to a process that needs to be able to provision
enclaves.
The way this API is used is straight-forward. Lets assume that dev_fd is
a handle to /dev/sgx/enclave and prov_fd is a handle to
/dev/sgx/provision. You would allow SGX_IOC_ENCLAVE_CREATE to
initialize an enclave with the PROVISIONKEY attribute by
params.addr = <enclave address>;
params.token_fd = prov_fd;
ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
Cc: linux-security-module@vger.kernel.org
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
arch/x86/include/uapi/asm/sgx.h | 11 ++++++++
arch/x86/kernel/cpu/sgx/driver.c | 14 ++++++++++
arch/x86/kernel/cpu/sgx/driver.h | 2 ++
arch/x86/kernel/cpu/sgx/ioctl.c | 47 ++++++++++++++++++++++++++++++++
4 files changed, 74 insertions(+)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 5edb08ab8fd0..57d0d30c79b3 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -25,6 +25,8 @@ enum sgx_page_flags {
_IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
#define SGX_IOC_ENCLAVE_INIT \
_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
+ _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
/**
* struct sgx_enclave_create - parameter structure for the
@@ -63,4 +65,13 @@ struct sgx_enclave_init {
__u64 sigstruct;
};
+/**
+ * struct sgx_enclave_set_attribute - parameter structure for the
+ * %SGX_IOC_ENCLAVE_SET_ATTRIBUTE ioctl
+ * @attribute_fd: file handle of the attribute file in the securityfs
+ */
+struct sgx_enclave_set_attribute {
+ __u64 attribute_fd;
+};
+
#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index b4aa7b9f8376..d90114cec1c3 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -150,6 +150,13 @@ static struct miscdevice sgx_dev_enclave = {
.fops = &sgx_encl_fops,
};
+static struct miscdevice sgx_dev_provision = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "provision",
+ .nodename = "sgx/provision",
+ .fops = &sgx_provision_fops,
+};
+
int __init sgx_drv_init(void)
{
unsigned int eax, ebx, ecx, edx;
@@ -190,5 +197,12 @@ int __init sgx_drv_init(void)
return ret;
}
+ ret = misc_register(&sgx_dev_provision);
+ if (ret) {
+ pr_err("Creating /dev/sgx/provision failed with %d.\n", ret);
+ misc_deregister(&sgx_dev_enclave);
+ return ret;
+ }
+
return 0;
}
diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
index e4063923115b..72747d01c046 100644
--- a/arch/x86/kernel/cpu/sgx/driver.h
+++ b/arch/x86/kernel/cpu/sgx/driver.h
@@ -23,6 +23,8 @@ extern u64 sgx_attributes_reserved_mask;
extern u64 sgx_xfrm_reserved_mask;
extern u32 sgx_xsave_size_tbl[64];
+extern const struct file_operations sgx_provision_fops;
+
long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
int sgx_drv_init(void);
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 27de304efe39..251b7a989606 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -658,6 +658,50 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
return ret;
}
+/**
+ * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
+ * @filep: open file to /dev/sgx
+ * @arg: userspace pointer to a struct sgx_enclave_set_attribute instance
+ *
+ * Mark the enclave as being allowed to access a restricted attribute bit.
+ * The requested attribute is specified via the attribute_fd field in the
+ * provided struct sgx_enclave_set_attribute. The attribute_fd must be a
+ * handle to an SGX attribute file, e.g. “/dev/sgx/provision".
+ *
+ * Failure to explicitly request access to a restricted attribute will cause
+ * sgx_ioc_enclave_init() to fail. Currently, the only restricted attribute
+ * is access to the PROVISION_KEY.
+ *
+ * Note, access to the EINITTOKEN_KEY is disallowed entirely.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static long sgx_ioc_enclave_set_attribute(struct sgx_encl *encl,
+ void __user *arg)
+{
+ struct sgx_enclave_set_attribute params;
+ struct file *attribute_file;
+ int ret;
+
+ if (copy_from_user(¶ms, arg, sizeof(params)))
+ return -EFAULT;
+
+ attribute_file = fget(params.attribute_fd);
+ if (!attribute_file)
+ return -EINVAL;
+
+ if (attribute_file->f_op != &sgx_provision_fops) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
+ ret = 0;
+
+out:
+ fput(attribute_file);
+ return ret;
+}
long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
{
@@ -681,6 +725,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
case SGX_IOC_ENCLAVE_INIT:
ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
break;
+ case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
+ ret = sgx_ioc_enclave_set_attribute(encl, (void __user *)arg);
+ break;
default:
ret = -ENOIOCTLCMD;
break;
--
2.25.0
^ permalink raw reply related
* [PATCH v28 11/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-03-03 23:35 UTC (permalink / raw)
To: linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk, Jarkko Sakkinen,
linux-security-module, Suresh Siddha, Haitao Huang,
Jethro Beekman, Chunyang Hui
In-Reply-To: <20200303233609.713348-1-jarkko.sakkinen@linux.intel.com>
Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
can be used by applications to set aside private regions of code and
data. The code outside the SGX hosted software entity is disallowed to
access the memory inside the enclave enforced by the CPU. We call these
entities as enclaves.
This commit implements a driver that provides an ioctl API to construct
and run enclaves. Enclaves are constructed from pages residing in
reserved physical memory areas. The contents of these pages can only be
accessed when they are mapped as part of an enclave, by a hardware
thread running inside the enclave.
The starting state of an enclave consists of a fixed measured set of
pages that are copied to the EPC during the construction process by
using ENCLS leaf functions and Software Enclave Control Structure (SECS)
that defines the enclave properties.
Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
the EPC and EINIT check a given signed measurement and moves the enclave
into a state ready for execution.
An initialized enclave can only be accessed through special Thread Control
Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER. This leaf
function converts a thread into enclave mode and continues the execution in
the offset defined by the TCS provided to EENTER. An enclave is exited
through syscall, exception, interrupts or by explicitly calling another
ENCLU leaf EEXIT.
The permissions, which enclave page is added will set the limit for maximum
permissions that can be set for mmap() and mprotect(). This will
effectively allow to build different security schemes between producers and
consumers of enclaves. Later on we can increase granularity with LSM hooks
for page addition (i.e. for producers) and mapping of the enclave (i.e. for
consumers)
Cc: linux-security-module@vger.kernel.org
Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Co-developed-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Tested-by: Haitao Huang <haitao.huang@linux.intel.com>
Tested-by: Jethro Beekman <jethro@fortanix.com>
Tested-by: Chunyang Hui <sanqian.hcy@antfin.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
arch/x86/include/uapi/asm/sgx.h | 66 ++
arch/x86/kernel/cpu/sgx/Makefile | 3 +
arch/x86/kernel/cpu/sgx/driver.c | 194 +++++
arch/x86/kernel/cpu/sgx/driver.h | 30 +
arch/x86/kernel/cpu/sgx/encl.c | 336 +++++++++
arch/x86/kernel/cpu/sgx/encl.h | 87 +++
arch/x86/kernel/cpu/sgx/encls.h | 5 +-
arch/x86/kernel/cpu/sgx/ioctl.c | 692 ++++++++++++++++++
arch/x86/kernel/cpu/sgx/main.c | 12 +-
arch/x86/kernel/cpu/sgx/reclaim.c | 1 +
11 files changed, 1423 insertions(+), 4 deletions(-)
create mode 100644 arch/x86/include/uapi/asm/sgx.h
create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 2e91370dc159..1c54dd2704db 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -321,6 +321,7 @@ Code Seq# Include File Comments
<mailto:tlewis@mindspring.com>
0xA3 90-9F linux/dtlk.h
0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
+0xA4 00-1F uapi/asm/sgx.h Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
0xAA 00-3F linux/uapi/linux/userfaultfd.h
0xAB 00-1F linux/nbd.h
0xAC 00-1F linux/raw.h
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
new file mode 100644
index 000000000000..5edb08ab8fd0
--- /dev/null
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
+/*
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+#ifndef _UAPI_ASM_X86_SGX_H
+#define _UAPI_ASM_X86_SGX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/**
+ * enum sgx_epage_flags - page control flags
+ * %SGX_PAGE_MEASURE: Measure the page contents with a sequence of
+ * ENCLS[EEXTEND] operations.
+ */
+enum sgx_page_flags {
+ SGX_PAGE_MEASURE = 0x01,
+};
+
+#define SGX_MAGIC 0xA4
+
+#define SGX_IOC_ENCLAVE_CREATE \
+ _IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
+#define SGX_IOC_ENCLAVE_ADD_PAGES \
+ _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
+#define SGX_IOC_ENCLAVE_INIT \
+ _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+
+/**
+ * struct sgx_enclave_create - parameter structure for the
+ * %SGX_IOC_ENCLAVE_CREATE ioctl
+ * @src: address for the SECS page data
+ */
+struct sgx_enclave_create {
+ __u64 src;
+};
+
+/**
+ * struct sgx_enclave_add_pages - parameter structure for the
+ * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
+ * @src: start address for the page data
+ * @offset: starting page offset
+ * @length: length of the data (multiple of the page size)
+ * @secinfo: address for the SECINFO data
+ * @flags: page control flags
+ * @count: number of bytes added (multiple of the page size)
+ */
+struct sgx_enclave_add_pages {
+ __u64 src;
+ __u64 offset;
+ __u64 length;
+ __u64 secinfo;
+ __u64 flags;
+ __u64 count;
+};
+
+/**
+ * struct sgx_enclave_init - parameter structure for the
+ * %SGX_IOC_ENCLAVE_INIT ioctl
+ * @sigstruct: address for the SIGSTRUCT data
+ */
+struct sgx_enclave_init {
+ __u64 sigstruct;
+};
+
+#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile
index 2dec75916a5e..f8d32da3a67a 100644
--- a/arch/x86/kernel/cpu/sgx/Makefile
+++ b/arch/x86/kernel/cpu/sgx/Makefile
@@ -1,3 +1,6 @@
obj-y += \
+ driver.o \
+ encl.o \
+ ioctl.o \
main.o \
reclaim.o
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
new file mode 100644
index 000000000000..b4aa7b9f8376
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include <linux/acpi.h>
+#include <linux/miscdevice.h>
+#include <linux/mman.h>
+#include <linux/security.h>
+#include <linux/suspend.h>
+#include <asm/traps.h>
+#include "driver.h"
+#include "encl.h"
+
+MODULE_DESCRIPTION("Intel SGX Enclave Driver");
+MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
+MODULE_LICENSE("Dual BSD/GPL");
+
+u64 sgx_encl_size_max_32;
+u64 sgx_encl_size_max_64;
+u32 sgx_misc_reserved_mask;
+u64 sgx_attributes_reserved_mask;
+u64 sgx_xfrm_reserved_mask = ~0x3;
+u32 sgx_xsave_size_tbl[64];
+
+static int sgx_open(struct inode *inode, struct file *file)
+{
+ struct sgx_encl *encl;
+ int ret;
+
+ encl = kzalloc(sizeof(*encl), GFP_KERNEL);
+ if (!encl)
+ return -ENOMEM;
+
+ atomic_set(&encl->flags, 0);
+ kref_init(&encl->refcount);
+ INIT_RADIX_TREE(&encl->page_tree, GFP_KERNEL);
+ mutex_init(&encl->lock);
+ INIT_LIST_HEAD(&encl->mm_list);
+ spin_lock_init(&encl->mm_lock);
+
+ ret = init_srcu_struct(&encl->srcu);
+ if (ret) {
+ kfree(encl);
+ return ret;
+ }
+
+ file->private_data = encl;
+
+ return 0;
+}
+
+static int sgx_release(struct inode *inode, struct file *file)
+{
+ struct sgx_encl *encl = file->private_data;
+ struct sgx_encl_mm *encl_mm;
+
+ for ( ; ; ) {
+ spin_lock(&encl->mm_lock);
+
+ if (list_empty(&encl->mm_list)) {
+ encl_mm = NULL;
+ } else {
+ encl_mm = list_first_entry(&encl->mm_list,
+ struct sgx_encl_mm, list);
+ list_del_rcu(&encl_mm->list);
+ }
+
+ spin_unlock(&encl->mm_lock);
+
+ /* The list is empty, ready to go. */
+ if (!encl_mm)
+ break;
+
+ synchronize_srcu(&encl->srcu);
+ mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
+ kfree(encl_mm);
+ };
+
+ mutex_lock(&encl->lock);
+ atomic_or(SGX_ENCL_DEAD, &encl->flags);
+ mutex_unlock(&encl->lock);
+
+ kref_put(&encl->refcount, sgx_encl_release);
+ return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ return sgx_ioctl(filep, cmd, arg);
+}
+#endif
+
+static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
+{
+ struct sgx_encl *encl = file->private_data;
+ int ret;
+
+ ret = sgx_encl_may_map(encl, vma->vm_start, vma->vm_end,
+ vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
+ if (ret)
+ return ret;
+
+ ret = sgx_encl_mm_add(encl, vma->vm_mm);
+ if (ret)
+ return ret;
+
+ vma->vm_ops = &sgx_vm_ops;
+ vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
+ vma->vm_private_data = encl;
+
+ return 0;
+}
+
+static unsigned long sgx_get_unmapped_area(struct file *file,
+ unsigned long addr,
+ unsigned long len,
+ unsigned long pgoff,
+ unsigned long flags)
+{
+ if (flags & MAP_PRIVATE)
+ return -EINVAL;
+
+ if (flags & MAP_FIXED)
+ return addr;
+
+ return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
+}
+
+static const struct file_operations sgx_encl_fops = {
+ .owner = THIS_MODULE,
+ .open = sgx_open,
+ .release = sgx_release,
+ .unlocked_ioctl = sgx_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = sgx_compat_ioctl,
+#endif
+ .mmap = sgx_mmap,
+ .get_unmapped_area = sgx_get_unmapped_area,
+};
+
+const struct file_operations sgx_provision_fops = {
+ .owner = THIS_MODULE,
+};
+
+static struct miscdevice sgx_dev_enclave = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "enclave",
+ .nodename = "sgx/enclave",
+ .fops = &sgx_encl_fops,
+};
+
+int __init sgx_drv_init(void)
+{
+ unsigned int eax, ebx, ecx, edx;
+ u64 attr_mask, xfrm_mask;
+ int ret;
+ int i;
+
+ if (!boot_cpu_has(X86_FEATURE_SGX_LC)) {
+ pr_info("The public key MSRs are not writable.\n");
+ return -ENODEV;
+ }
+
+ cpuid_count(SGX_CPUID, 0, &eax, &ebx, &ecx, &edx);
+ sgx_misc_reserved_mask = ~ebx | SGX_MISC_RESERVED_MASK;
+ sgx_encl_size_max_64 = 1ULL << ((edx >> 8) & 0xFF);
+ sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
+
+ cpuid_count(SGX_CPUID, 1, &eax, &ebx, &ecx, &edx);
+
+ attr_mask = (((u64)ebx) << 32) + (u64)eax;
+ sgx_attributes_reserved_mask = ~attr_mask | SGX_ATTR_RESERVED_MASK;
+
+ if (boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ xfrm_mask = (((u64)edx) << 32) + (u64)ecx;
+
+ for (i = 2; i < 64; i++) {
+ cpuid_count(0x0D, i, &eax, &ebx, &ecx, &edx);
+ if ((1 << i) & xfrm_mask)
+ sgx_xsave_size_tbl[i] = eax + ebx;
+ }
+
+ sgx_xfrm_reserved_mask = ~xfrm_mask;
+ }
+
+ ret = misc_register(&sgx_dev_enclave);
+ if (ret) {
+ pr_err("Creating /dev/sgx/enclave failed with %d.\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
new file mode 100644
index 000000000000..e4063923115b
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/driver.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+#ifndef __ARCH_SGX_DRIVER_H__
+#define __ARCH_SGX_DRIVER_H__
+
+#include <crypto/hash.h>
+#include <linux/kref.h>
+#include <linux/mmu_notifier.h>
+#include <linux/radix-tree.h>
+#include <linux/rwsem.h>
+#include <linux/sched.h>
+#include <linux/workqueue.h>
+#include <uapi/asm/sgx.h>
+#include "sgx.h"
+
+#define SGX_EINIT_SPIN_COUNT 20
+#define SGX_EINIT_SLEEP_COUNT 50
+#define SGX_EINIT_SLEEP_TIME 20
+
+extern u64 sgx_encl_size_max_32;
+extern u64 sgx_encl_size_max_64;
+extern u32 sgx_misc_reserved_mask;
+extern u64 sgx_attributes_reserved_mask;
+extern u64 sgx_xfrm_reserved_mask;
+extern u32 sgx_xsave_size_tbl[64];
+
+long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
+
+int sgx_drv_init(void);
+
+#endif /* __ARCH_X86_SGX_DRIVER_H__ */
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
new file mode 100644
index 000000000000..f349697c7508
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include <linux/lockdep.h>
+#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/shmem_fs.h>
+#include <linux/suspend.h>
+#include <linux/sched/mm.h>
+#include "arch.h"
+#include "encl.h"
+#include "sgx.h"
+
+static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
+ unsigned long addr)
+{
+ struct sgx_encl_page *entry;
+ unsigned int flags;
+
+ /* If process was forked, VMA is still there but vm_private_data is set
+ * to NULL.
+ */
+ if (!encl)
+ return ERR_PTR(-EFAULT);
+
+ flags = atomic_read(&encl->flags);
+
+ if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
+ return ERR_PTR(-EFAULT);
+
+ entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
+ if (!entry)
+ return ERR_PTR(-EFAULT);
+
+ /* Page is already resident in the EPC. */
+ if (entry->epc_page)
+ return entry;
+
+ return ERR_PTR(-EFAULT);
+}
+
+static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
+ struct mm_struct *mm)
+{
+ struct sgx_encl_mm *encl_mm =
+ container_of(mn, struct sgx_encl_mm, mmu_notifier);
+ struct sgx_encl_mm *tmp = NULL;
+
+ /*
+ * The enclave itself can remove encl_mm. Note, objects can't be moved
+ * off an RCU protected list, but deletion is ok.
+ */
+ spin_lock(&encl_mm->encl->mm_lock);
+ list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) {
+ if (tmp == encl_mm) {
+ list_del_rcu(&encl_mm->list);
+ break;
+ }
+ }
+ spin_unlock(&encl_mm->encl->mm_lock);
+
+ if (tmp == encl_mm) {
+ synchronize_srcu(&encl_mm->encl->srcu);
+ mmu_notifier_put(mn);
+ }
+}
+
+static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
+{
+ struct sgx_encl_mm *encl_mm =
+ container_of(mn, struct sgx_encl_mm, mmu_notifier);
+
+ kfree(encl_mm);
+}
+
+static const struct mmu_notifier_ops sgx_mmu_notifier_ops = {
+ .release = sgx_mmu_notifier_release,
+ .free_notifier = sgx_mmu_notifier_free,
+};
+
+static struct sgx_encl_mm *sgx_encl_find_mm(struct sgx_encl *encl,
+ struct mm_struct *mm)
+{
+ struct sgx_encl_mm *encl_mm = NULL;
+ struct sgx_encl_mm *tmp;
+ int idx;
+
+ idx = srcu_read_lock(&encl->srcu);
+
+ list_for_each_entry_rcu(tmp, &encl->mm_list, list) {
+ if (tmp->mm == mm) {
+ encl_mm = tmp;
+ break;
+ }
+ }
+
+ srcu_read_unlock(&encl->srcu, idx);
+
+ return encl_mm;
+}
+
+int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
+{
+ struct sgx_encl_mm *encl_mm;
+ int ret;
+
+ if (atomic_read(&encl->flags) & SGX_ENCL_DEAD)
+ return -EINVAL;
+
+ /*
+ * mm_structs are kept on mm_list until the mm or the enclave dies,
+ * i.e. once an mm is off the list, it's gone for good, therefore it's
+ * impossible to get a false positive on @mm due to a stale mm_list.
+ */
+ if (sgx_encl_find_mm(encl, mm))
+ return 0;
+
+ encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
+ if (!encl_mm)
+ return -ENOMEM;
+
+ encl_mm->encl = encl;
+ encl_mm->mm = mm;
+ encl_mm->mmu_notifier.ops = &sgx_mmu_notifier_ops;
+
+ ret = __mmu_notifier_register(&encl_mm->mmu_notifier, mm);
+ if (ret) {
+ kfree(encl_mm);
+ return ret;
+ }
+
+ spin_lock(&encl->mm_lock);
+ list_add_rcu(&encl_mm->list, &encl->mm_list);
+ spin_unlock(&encl->mm_lock);
+
+ synchronize_srcu(&encl->srcu);
+
+ return 0;
+}
+
+static void sgx_vma_open(struct vm_area_struct *vma)
+{
+ struct sgx_encl *encl = vma->vm_private_data;
+
+ if (!encl)
+ return;
+
+ if (sgx_encl_mm_add(encl, vma->vm_mm))
+ vma->vm_private_data = NULL;
+}
+
+static unsigned int sgx_vma_fault(struct vm_fault *vmf)
+{
+ unsigned long addr = (unsigned long)vmf->address;
+ struct vm_area_struct *vma = vmf->vma;
+ struct sgx_encl *encl = vma->vm_private_data;
+ struct sgx_encl_page *entry;
+ int ret = VM_FAULT_NOPAGE;
+ unsigned long pfn;
+
+ if (!encl)
+ return VM_FAULT_SIGBUS;
+
+ mutex_lock(&encl->lock);
+
+ entry = sgx_encl_load_page(encl, addr);
+ if (IS_ERR(entry)) {
+ if (unlikely(PTR_ERR(entry) != -EBUSY))
+ ret = VM_FAULT_SIGBUS;
+
+ goto out;
+ }
+
+ if (!follow_pfn(vma, addr, &pfn))
+ goto out;
+
+ ret = vmf_insert_pfn(vma, addr, PFN_DOWN(entry->epc_page->desc));
+ if (ret != VM_FAULT_NOPAGE) {
+ ret = VM_FAULT_SIGBUS;
+ goto out;
+ }
+
+out:
+ mutex_unlock(&encl->lock);
+ return ret;
+}
+
+/**
+ * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
+ * @encl: an enclave
+ * @start: lower bound of the address range, inclusive
+ * @end: upper bound of the address range, exclusive
+ * @vm_prot_bits: requested protections of the address range
+ *
+ * Iterate through the enclave pages contained within [@start, @end) to verify
+ * the permissions requested by @vm_prot_bits do not exceed that of any enclave
+ * page to be mapped. Page addresses that do not have an associated enclave
+ * page are interpreted to zero permissions.
+ *
+ * Return:
+ * 0 on success,
+ * -EACCES if VMA permissions exceed enclave page permissions
+ */
+int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
+ unsigned long end, unsigned long vm_prot_bits)
+{
+ unsigned long idx, idx_start, idx_end;
+ struct sgx_encl_page *page;
+
+ /*
+ * Disallow RIE tasks as their VMA permissions might conflict with the
+ * enclave page permissions.
+ */
+ if (!!(current->personality & READ_IMPLIES_EXEC))
+ return -EACCES;
+
+ /* PROT_NONE always succeeds. */
+ if (!vm_prot_bits)
+ return 0;
+
+ idx_start = PFN_DOWN(start);
+ idx_end = PFN_DOWN(end - 1);
+
+ for (idx = idx_start; idx <= idx_end; ++idx) {
+ mutex_lock(&encl->lock);
+ page = radix_tree_lookup(&encl->page_tree, idx);
+ mutex_unlock(&encl->lock);
+
+ if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
+ return -EACCES;
+ }
+
+ return 0;
+}
+
+static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
+ unsigned long end, unsigned long prot)
+{
+ return sgx_encl_may_map(vma->vm_private_data, start, end,
+ calc_vm_prot_bits(prot, 0));
+}
+
+const struct vm_operations_struct sgx_vm_ops = {
+ .open = sgx_vma_open,
+ .fault = sgx_vma_fault,
+ .may_mprotect = sgx_vma_mprotect,
+};
+
+/**
+ * sgx_encl_find - find an enclave
+ * @mm: mm struct of the current process
+ * @addr: address in the ELRANGE
+ * @vma: the resulting VMA
+ *
+ * Find an enclave identified by the given address. Give back a VMA that is
+ * part of the enclave and located in that address. The VMA is given back if it
+ * is a proper enclave VMA even if an &sgx_encl instance does not exist yet
+ * (enclave creation has not been performed).
+ *
+ * Return:
+ * 0 on success,
+ * -EINVAL if an enclave was not found,
+ * -ENOENT if the enclave has not been created yet
+ */
+int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
+ struct vm_area_struct **vma)
+{
+ struct vm_area_struct *result;
+ struct sgx_encl *encl;
+
+ result = find_vma(mm, addr);
+ if (!result || result->vm_ops != &sgx_vm_ops || addr < result->vm_start)
+ return -EINVAL;
+
+ encl = result->vm_private_data;
+ *vma = result;
+
+ return encl ? 0 : -ENOENT;
+}
+
+/**
+ * sgx_encl_destroy() - destroy enclave resources
+ * @encl: an &sgx_encl instance
+ */
+void sgx_encl_destroy(struct sgx_encl *encl)
+{
+ struct sgx_encl_page *entry;
+ struct radix_tree_iter iter;
+ void **slot;
+
+ atomic_or(SGX_ENCL_DEAD, &encl->flags);
+
+ radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
+ entry = *slot;
+
+ if (entry->epc_page) {
+ sgx_free_page(entry->epc_page);
+ encl->secs_child_cnt--;
+ entry->epc_page = NULL;
+ }
+
+ radix_tree_delete(&entry->encl->page_tree,
+ PFN_DOWN(entry->desc));
+ kfree(entry);
+ }
+
+ if (!encl->secs_child_cnt && encl->secs.epc_page) {
+ sgx_free_page(encl->secs.epc_page);
+ encl->secs.epc_page = NULL;
+ }
+}
+
+/**
+ * sgx_encl_release - Destroy an enclave instance
+ * @kref: address of a kref inside &sgx_encl
+ *
+ * Used together with kref_put(). Frees all the resources associated with the
+ * enclave and the instance itself.
+ */
+void sgx_encl_release(struct kref *ref)
+{
+ struct sgx_encl *encl = container_of(ref, struct sgx_encl, refcount);
+
+ sgx_encl_destroy(encl);
+
+ if (encl->backing)
+ fput(encl->backing);
+
+ WARN_ON_ONCE(!list_empty(&encl->mm_list));
+
+ /* Detect EPC page leak's. */
+ WARN_ON_ONCE(encl->secs_child_cnt);
+ WARN_ON_ONCE(encl->secs.epc_page);
+
+ kfree(encl);
+}
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
new file mode 100644
index 000000000000..1d1bc5d590ee
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/**
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+#ifndef _X86_ENCL_H
+#define _X86_ENCL_H
+
+#include <linux/cpumask.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/mm_types.h>
+#include <linux/mmu_notifier.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/radix-tree.h>
+#include <linux/srcu.h>
+#include <linux/workqueue.h>
+#include "sgx.h"
+
+/**
+ * enum sgx_encl_page_desc - defines bits for an enclave page's descriptor
+ * %SGX_ENCL_PAGE_ADDR_MASK: Holds the virtual address of the page.
+ *
+ * The page address for SECS is zero and is used by the subsystem to recognize
+ * the SECS page.
+ */
+enum sgx_encl_page_desc {
+ /* Bits 11:3 are available when the page is not swapped. */
+ SGX_ENCL_PAGE_ADDR_MASK = PAGE_MASK,
+};
+
+#define SGX_ENCL_PAGE_ADDR(page) \
+ ((page)->desc & SGX_ENCL_PAGE_ADDR_MASK)
+
+struct sgx_encl_page {
+ unsigned long desc;
+ unsigned long vm_max_prot_bits;
+ struct sgx_epc_page *epc_page;
+ struct sgx_encl *encl;
+};
+
+enum sgx_encl_flags {
+ SGX_ENCL_CREATED = BIT(0),
+ SGX_ENCL_INITIALIZED = BIT(1),
+ SGX_ENCL_DEBUG = BIT(2),
+ SGX_ENCL_DEAD = BIT(3),
+ SGX_ENCL_IOCTL = BIT(4),
+};
+
+struct sgx_encl_mm {
+ struct sgx_encl *encl;
+ struct mm_struct *mm;
+ struct list_head list;
+ struct mmu_notifier mmu_notifier;
+};
+
+struct sgx_encl {
+ atomic_t flags;
+ u64 secs_attributes;
+ u64 allowed_attributes;
+ unsigned int page_cnt;
+ unsigned int secs_child_cnt;
+ struct mutex lock;
+ struct list_head mm_list;
+ spinlock_t mm_lock;
+ struct file *backing;
+ struct kref refcount;
+ struct srcu_struct srcu;
+ unsigned long base;
+ unsigned long size;
+ unsigned long ssaframesize;
+ struct radix_tree_root page_tree;
+ struct sgx_encl_page secs;
+ cpumask_t cpumask;
+};
+
+extern const struct vm_operations_struct sgx_vm_ops;
+
+int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
+ struct vm_area_struct **vma);
+void sgx_encl_destroy(struct sgx_encl *encl);
+void sgx_encl_release(struct kref *ref);
+int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
+int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
+ unsigned long end, unsigned long vm_prot_bits);
+
+#endif /* _X86_ENCL_H */
diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index f91da6a1bddc..072d42973c68 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -186,10 +186,9 @@ static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr)
return __encls_2(EADD, pginfo, addr);
}
-static inline int __einit(void *sigstruct, struct sgx_einittoken *einittoken,
- void *secs)
+static inline int __einit(void *sigstruct, void *token, void *secs)
{
- return __encls_ret_3(EINIT, sigstruct, secs, einittoken);
+ return __encls_ret_3(EINIT, sigstruct, secs, token);
}
static inline int __eremove(void *addr)
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
new file mode 100644
index 000000000000..27de304efe39
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -0,0 +1,692 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-19 Intel Corporation.
+
+#include <asm/mman.h>
+#include <linux/mman.h>
+#include <linux/delay.h>
+#include <linux/file.h>
+#include <linux/hashtable.h>
+#include <linux/highmem.h>
+#include <linux/ratelimit.h>
+#include <linux/sched/signal.h>
+#include <linux/shmem_fs.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include "driver.h"
+#include "encl.h"
+#include "encls.h"
+
+/* A per-cpu cache for the last known values of IA32_SGXLEPUBKEYHASHx MSRs. */
+static DEFINE_PER_CPU(u64 [4], sgx_lepubkeyhash_cache);
+
+static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm)
+{
+ u32 size_max = PAGE_SIZE;
+ u32 size;
+ int i;
+
+ for (i = 2; i < 64; i++) {
+ if (!((1 << i) & xfrm))
+ continue;
+
+ size = SGX_SSA_GPRS_SIZE + sgx_xsave_size_tbl[i];
+ if (miscselect & SGX_MISC_EXINFO)
+ size += SGX_SSA_MISC_EXINFO_SIZE;
+
+ if (size > size_max)
+ size_max = size;
+ }
+
+ return PFN_UP(size_max);
+}
+
+static int sgx_validate_secs(const struct sgx_secs *secs,
+ unsigned long ssaframesize)
+{
+ if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
+ return -EINVAL;
+
+ if (secs->base & (secs->size - 1))
+ return -EINVAL;
+
+ if (secs->miscselect & sgx_misc_reserved_mask ||
+ secs->attributes & sgx_attributes_reserved_mask ||
+ secs->xfrm & sgx_xfrm_reserved_mask)
+ return -EINVAL;
+
+ if (secs->attributes & SGX_ATTR_MODE64BIT) {
+ if (secs->size > sgx_encl_size_max_64)
+ return -EINVAL;
+ } else if (secs->size > sgx_encl_size_max_32)
+ return -EINVAL;
+
+ if (!(secs->xfrm & XFEATURE_MASK_FP) ||
+ !(secs->xfrm & XFEATURE_MASK_SSE) ||
+ (((secs->xfrm >> XFEATURE_BNDREGS) & 1) !=
+ ((secs->xfrm >> XFEATURE_BNDCSR) & 1)))
+ return -EINVAL;
+
+ if (!secs->ssa_frame_size || ssaframesize > secs->ssa_frame_size)
+ return -EINVAL;
+
+ if (memchr_inv(secs->reserved1, 0, sizeof(secs->reserved1)) ||
+ memchr_inv(secs->reserved2, 0, sizeof(secs->reserved2)) ||
+ memchr_inv(secs->reserved3, 0, sizeof(secs->reserved3)) ||
+ memchr_inv(secs->reserved4, 0, sizeof(secs->reserved4)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
+ unsigned long offset,
+ u64 secinfo_flags)
+{
+ struct sgx_encl_page *encl_page;
+ unsigned long prot;
+
+ encl_page = kzalloc(sizeof(*encl_page), GFP_KERNEL);
+ if (!encl_page)
+ return ERR_PTR(-ENOMEM);
+
+ encl_page->desc = encl->base + offset;
+ encl_page->encl = encl;
+
+ prot = _calc_vm_trans(secinfo_flags, SGX_SECINFO_R, PROT_READ) |
+ _calc_vm_trans(secinfo_flags, SGX_SECINFO_W, PROT_WRITE) |
+ _calc_vm_trans(secinfo_flags, SGX_SECINFO_X, PROT_EXEC);
+
+ /*
+ * TCS pages must always RW set for CPU access while the SECINFO
+ * permissions are *always* zero - the CPU ignores the user provided
+ * values and silently overwrites them with zero permissions.
+ */
+ if ((secinfo_flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
+ prot |= PROT_READ | PROT_WRITE;
+
+ /* Calculate maximum of the VM flags for the page. */
+ encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
+
+ return encl_page;
+}
+
+static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
+{
+ unsigned long encl_size = secs->size + PAGE_SIZE;
+ struct sgx_epc_page *secs_epc;
+ unsigned long ssaframesize;
+ struct sgx_pageinfo pginfo;
+ struct sgx_secinfo secinfo;
+ struct file *backing;
+ long ret;
+
+ if (atomic_read(&encl->flags) & SGX_ENCL_CREATED)
+ return -EINVAL;
+
+ ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
+ if (sgx_validate_secs(secs, ssaframesize)) {
+ pr_debug("invalid SECS\n");
+ return -EINVAL;
+ }
+
+ backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
+ VM_NORESERVE);
+ if (IS_ERR(backing))
+ return PTR_ERR(backing);
+
+ encl->backing = backing;
+
+ secs_epc = sgx_try_alloc_page();
+ if (IS_ERR(secs_epc)) {
+ ret = PTR_ERR(secs_epc);
+ goto err_out_backing;
+ }
+
+ encl->secs.epc_page = secs_epc;
+
+ pginfo.addr = 0;
+ pginfo.contents = (unsigned long)secs;
+ pginfo.metadata = (unsigned long)&secinfo;
+ pginfo.secs = 0;
+ memset(&secinfo, 0, sizeof(secinfo));
+
+ ret = __ecreate((void *)&pginfo, sgx_epc_addr(secs_epc));
+ if (ret) {
+ pr_debug("ECREATE returned %ld\n", ret);
+ goto err_out;
+ }
+
+ if (secs->attributes & SGX_ATTR_DEBUG)
+ atomic_or(SGX_ENCL_DEBUG, &encl->flags);
+
+ encl->secs.encl = encl;
+ encl->secs_attributes = secs->attributes;
+ encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
+ encl->base = secs->base;
+ encl->size = secs->size;
+ encl->ssaframesize = secs->ssa_frame_size;
+
+ /*
+ * Set SGX_ENCL_CREATED only after the enclave is fully prepped. This
+ * allows setting and checking enclave creation without having to take
+ * encl->lock.
+ */
+ atomic_or(SGX_ENCL_CREATED, &encl->flags);
+
+ return 0;
+
+err_out:
+ sgx_free_page(encl->secs.epc_page);
+ encl->secs.epc_page = NULL;
+
+err_out_backing:
+ fput(encl->backing);
+ encl->backing = NULL;
+
+ return ret;
+}
+
+/**
+ * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
+ * @filep: open file to /dev/sgx
+ * @arg: userspace pointer to a struct sgx_enclave_create instance
+ *
+ * Allocate kernel data structures for a new enclave and execute ECREATE after
+ * verifying the correctness of the provided SECS.
+ *
+ * Note, enforcement of restricted and disallowed attributes is deferred until
+ * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
+ * checked by sgx_ioc_enclave_create().
+ *
+ * Return:
+ * 0 on success,
+ * -errno otherwise
+ */
+static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_enclave_create ecreate;
+ struct page *secs_page;
+ struct sgx_secs *secs;
+ int ret;
+
+ if (copy_from_user(&ecreate, arg, sizeof(ecreate)))
+ return -EFAULT;
+
+ secs_page = alloc_page(GFP_KERNEL);
+ if (!secs_page)
+ return -ENOMEM;
+
+ secs = kmap(secs_page);
+ if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = sgx_encl_create(encl, secs);
+
+out:
+ kunmap(secs_page);
+ __free_page(secs_page);
+ return ret;
+}
+
+static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
+{
+ u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
+ u64 pt = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
+
+ if (pt != SGX_SECINFO_REG && pt != SGX_SECINFO_TCS)
+ return -EINVAL;
+
+ if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
+ return -EINVAL;
+
+ /*
+ * CPU will silently overwrite the permissions as zero, which means
+ * that we need to validate it ourselves.
+ */
+ if (pt == SGX_SECINFO_TCS && perm)
+ return -EINVAL;
+
+ if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
+ return -EINVAL;
+
+ if (memchr_inv(secinfo->reserved, 0, sizeof(secinfo->reserved)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int __sgx_encl_add_page(struct sgx_encl *encl,
+ struct sgx_encl_page *encl_page,
+ struct sgx_epc_page *epc_page,
+ struct sgx_secinfo *secinfo, unsigned long src)
+{
+ struct sgx_pageinfo pginfo;
+ struct vm_area_struct *vma;
+ struct page *src_page;
+ int ret;
+
+ /* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */
+ if (encl_page->vm_max_prot_bits & VM_EXEC) {
+ vma = find_vma(current->mm, src);
+ if (!vma)
+ return -EFAULT;
+
+ if (!(vma->vm_flags & VM_MAYEXEC))
+ return -EACCES;
+ }
+
+ ret = get_user_pages(src, 1, 0, &src_page, NULL);
+ if (ret < 1)
+ return ret;
+
+ pginfo.secs = (unsigned long)sgx_epc_addr(encl->secs.epc_page);
+ pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
+ pginfo.metadata = (unsigned long)secinfo;
+ pginfo.contents = (unsigned long)kmap_atomic(src_page);
+
+ ret = __eadd(&pginfo, sgx_epc_addr(epc_page));
+
+ kunmap_atomic((void *)pginfo.contents);
+ put_page(src_page);
+
+ return ret ? -EIO : 0;
+}
+
+static int __sgx_encl_extend(struct sgx_encl *encl,
+ struct sgx_epc_page *epc_page)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < 16; i++) {
+ ret = __eextend(sgx_epc_addr(encl->secs.epc_page),
+ sgx_epc_addr(epc_page) + (i * 0x100));
+ if (ret) {
+ if (encls_failed(ret))
+ ENCLS_WARN(ret, "EEXTEND");
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
+static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
+ unsigned long offset, unsigned long length,
+ struct sgx_secinfo *secinfo, unsigned long flags)
+{
+ struct sgx_encl_page *encl_page;
+ struct sgx_epc_page *epc_page;
+ int ret;
+
+ encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
+ if (IS_ERR(encl_page))
+ return PTR_ERR(encl_page);
+
+ epc_page = sgx_try_alloc_page();
+ if (IS_ERR(epc_page)) {
+ kfree(encl_page);
+ return PTR_ERR(epc_page);
+ }
+
+ if (atomic_read(&encl->flags) &
+ (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
+ ret = -EFAULT;
+ goto err_out_free;
+ }
+
+ down_read(¤t->mm->mmap_sem);
+ mutex_lock(&encl->lock);
+
+ /*
+ * Insert prior to EADD in case of OOM. EADD modifies MRENCLAVE, i.e.
+ * can't be gracefully unwound, while failure on EADD/EXTEND is limited
+ * to userspace errors (or kernel/hardware bugs).
+ */
+ ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
+ encl_page);
+ if (ret)
+ goto err_out_unlock;
+
+ ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
+ src);
+ if (ret)
+ goto err_out;
+
+ /*
+ * Complete the "add" before doing the "extend" so that the "add"
+ * isn't in a half-baked state in the extremely unlikely scenario the
+ * the enclave will be destroyed in response to EEXTEND failure.
+ */
+ encl_page->encl = encl;
+ encl_page->epc_page = epc_page;
+ encl->secs_child_cnt++;
+
+ if (flags & SGX_PAGE_MEASURE) {
+ ret = __sgx_encl_extend(encl, epc_page);
+ if (ret)
+ goto err_out;
+ }
+
+ mutex_unlock(&encl->lock);
+ up_read(¤t->mm->mmap_sem);
+ return ret;
+
+err_out:
+ radix_tree_delete(&encl_page->encl->page_tree,
+ PFN_DOWN(encl_page->desc));
+
+err_out_unlock:
+ mutex_unlock(&encl->lock);
+ up_read(¤t->mm->mmap_sem);
+
+err_out_free:
+ sgx_free_page(epc_page);
+ kfree(encl_page);
+
+ /*
+ * Destroy enclave on ENCLS failure as this means that EPC has been
+ * invalidated.
+ */
+ if (ret == -EIO)
+ sgx_encl_destroy(encl);
+
+ return ret;
+}
+
+/**
+ * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
+ * @encl: pointer to an enclave instance (via ioctl() file pointer)
+ * @arg: a user pointer to a struct sgx_enclave_add_pages instance
+ *
+ * Add one or more pages to an uninitialized enclave, and optionally extend the
+ * measurement with the contents of the page. The address range of pages must
+ * be contiguous. The SECINFO and measurement mask are applied to all pages.
+ *
+ * A SECINFO for a TCS is required to always contain zero permissions because
+ * CPU silently zeros them. Allowing anything else would cause a mismatch in
+ * the measurement.
+ *
+ * mmap()'s protection bits are capped by the page permissions. For each page
+ * address, the maximum protection bits are computed with the following
+ * heuristics:
+ *
+ * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO permissions.
+ * 2. A TCS page: PROT_R | PROT_W.
+ * 3. No page: PROT_NONE.
+ *
+ * mmap() is not allowed to surpass the minimum of the maximum protection bits
+ * within the given address range.
+ *
+ * As stated above, a non-existent page is interpreted as a page with no
+ * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek
+ * an address range for the enclave that can be then populated into SECS.
+ *
+ * If ENCLS opcode fails, that effectively means that EPC has been invalidated.
+ * When this happens the enclave is destroyed and -EIO is returned to the
+ * caller.
+ *
+ * Return:
+ * 0 on success,
+ * -EACCES if an executable source page is located in a noexec partition,
+ * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
+ * -errno otherwise
+ */
+static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_enclave_add_pages addp;
+ struct sgx_secinfo secinfo;
+ unsigned long c;
+ int ret;
+
+ if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
+ return -EINVAL;
+
+ if (copy_from_user(&addp, arg, sizeof(addp)))
+ return -EFAULT;
+
+ if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
+ !IS_ALIGNED(addp.src, PAGE_SIZE))
+ return -EINVAL;
+
+ if (!(access_ok(addp.src, PAGE_SIZE)))
+ return -EFAULT;
+
+ if (addp.length & (PAGE_SIZE - 1))
+ return -EINVAL;
+
+ if (addp.offset + addp.length - PAGE_SIZE >= encl->size)
+ return -EINVAL;
+
+ if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
+ sizeof(secinfo)))
+ return -EFAULT;
+
+ if (sgx_validate_secinfo(&secinfo))
+ return -EINVAL;
+
+ for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
+ if (signal_pending(current)) {
+ ret = -EINTR;
+ break;
+ }
+
+ if (need_resched())
+ cond_resched();
+
+ ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
+ addp.length - c, &secinfo, addp.flags);
+ if (ret)
+ break;
+ }
+
+ addp.count = c;
+
+ if (copy_to_user(arg, &addp, sizeof(addp)))
+ return -EFAULT;
+
+ return ret;
+}
+
+static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus,
+ void *hash)
+{
+ SHASH_DESC_ON_STACK(shash, tfm);
+
+ shash->tfm = tfm;
+
+ return crypto_shash_digest(shash, modulus, SGX_MODULUS_SIZE, hash);
+}
+
+static int sgx_get_key_hash(const void *modulus, void *hash)
+{
+ struct crypto_shash *tfm;
+ int ret;
+
+ tfm = crypto_alloc_shash("sha256", 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(tfm))
+ return PTR_ERR(tfm);
+
+ ret = __sgx_get_key_hash(tfm, modulus, hash);
+
+ crypto_free_shash(tfm);
+ return ret;
+}
+
+static void sgx_update_lepubkeyhash_msrs(u64 *lepubkeyhash, bool enforce)
+{
+ u64 *cache;
+ int i;
+
+ cache = per_cpu(sgx_lepubkeyhash_cache, smp_processor_id());
+ for (i = 0; i < 4; i++) {
+ if (enforce || (lepubkeyhash[i] != cache[i])) {
+ wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]);
+ cache[i] = lepubkeyhash[i];
+ }
+ }
+}
+
+static int sgx_einit(struct sgx_sigstruct *sigstruct, void *token,
+ struct sgx_epc_page *secs, u64 *lepubkeyhash)
+{
+ int ret;
+
+ preempt_disable();
+ sgx_update_lepubkeyhash_msrs(lepubkeyhash, false);
+ ret = __einit(sigstruct, token, sgx_epc_addr(secs));
+ if (ret == SGX_INVALID_EINITTOKEN) {
+ sgx_update_lepubkeyhash_msrs(lepubkeyhash, true);
+ ret = __einit(sigstruct, token, sgx_epc_addr(secs));
+ }
+ preempt_enable();
+ return ret;
+}
+
+static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
+ void *token)
+{
+ u64 mrsigner[4];
+ int ret;
+ int i;
+ int j;
+
+ /* Check that the required attributes have been authorized. */
+ if (encl->secs_attributes & ~encl->allowed_attributes)
+ return -EACCES;
+
+ ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
+ if (ret)
+ return ret;
+
+ mutex_lock(&encl->lock);
+
+ if (atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED) {
+ ret = -EFAULT;
+ goto err_out;
+ }
+
+ for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
+ for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
+ ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
+ mrsigner);
+ if (ret == SGX_UNMASKED_EVENT)
+ continue;
+ else
+ break;
+ }
+
+ if (ret != SGX_UNMASKED_EVENT)
+ break;
+
+ msleep_interruptible(SGX_EINIT_SLEEP_TIME);
+
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ goto err_out;
+ }
+ }
+
+ if (ret & ENCLS_FAULT_FLAG) {
+ if (encls_failed(ret))
+ ENCLS_WARN(ret, "EINIT");
+
+ sgx_encl_destroy(encl);
+ ret = -EFAULT;
+ } else if (ret) {
+ pr_debug("EINIT returned %d\n", ret);
+ ret = -EPERM;
+ } else {
+ atomic_or(SGX_ENCL_INITIALIZED, &encl->flags);
+ }
+
+err_out:
+ mutex_unlock(&encl->lock);
+ return ret;
+}
+
+/**
+ * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
+ *
+ * @filep: open file to /dev/sgx
+ * @arg: userspace pointer to a struct sgx_enclave_init instance
+ *
+ * Flush any outstanding enqueued EADD operations and perform EINIT. The
+ * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
+ * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
+ *
+ * Return:
+ * 0 on success,
+ * SGX error code on EINIT failure,
+ * -errno otherwise
+ */
+static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_sigstruct *sigstruct;
+ struct sgx_enclave_init einit;
+ struct page *initp_page;
+ void *token;
+ int ret;
+
+ if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
+ return -EINVAL;
+
+ if (copy_from_user(&einit, arg, sizeof(einit)))
+ return -EFAULT;
+
+ initp_page = alloc_page(GFP_KERNEL);
+ if (!initp_page)
+ return -ENOMEM;
+
+ sigstruct = kmap(initp_page);
+ token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2);
+ memset(token, 0, SGX_LAUNCH_TOKEN_SIZE);
+
+ if (copy_from_user(sigstruct, (void __user *)einit.sigstruct,
+ sizeof(*sigstruct))) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = sgx_encl_init(encl, sigstruct, token);
+
+out:
+ kunmap(initp_page);
+ __free_page(initp_page);
+ return ret;
+}
+
+
+long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+ struct sgx_encl *encl = filep->private_data;
+ int ret, encl_flags;
+
+ encl_flags = atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags);
+ if (encl_flags & SGX_ENCL_IOCTL)
+ return -EBUSY;
+
+ if (encl_flags & SGX_ENCL_DEAD)
+ return -EFAULT;
+
+ switch (cmd) {
+ case SGX_IOC_ENCLAVE_CREATE:
+ ret = sgx_ioc_enclave_create(encl, (void __user *)arg);
+ break;
+ case SGX_IOC_ENCLAVE_ADD_PAGES:
+ ret = sgx_ioc_enclave_add_pages(encl, (void __user *)arg);
+ break;
+ case SGX_IOC_ENCLAVE_INIT:
+ ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
+ break;
+ }
+
+ atomic_andnot(SGX_ENCL_IOCTL, &encl->flags);
+
+ return ret;
+}
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 60d82e7537c8..842f9abba1c0 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -8,6 +8,7 @@
#include <linux/ratelimit.h>
#include <linux/sched/signal.h>
#include <linux/slab.h>
+#include "driver.h"
#include "encls.h"
struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
@@ -193,6 +194,8 @@ static bool __init sgx_page_cache_init(void)
static void __init sgx_init(void)
{
+ int ret;
+
if (!boot_cpu_has(X86_FEATURE_SGX))
return;
@@ -202,10 +205,17 @@ static void __init sgx_init(void)
if (!sgx_page_reclaimer_init())
goto err_page_cache;
+ ret = sgx_drv_init();
+ if (ret)
+ goto err_kthread;
+
return;
+err_kthread:
+ kthread_stop(ksgxswapd_tsk);
+
err_page_cache:
sgx_page_cache_teardown();
}
-arch_initcall(sgx_init);
+device_initcall(sgx_init);
diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index 215371588a25..9e6d3e147aa2 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -10,6 +10,7 @@
#include <linux/sched/mm.h>
#include <linux/sched/signal.h>
#include "encls.h"
+#include "driver.h"
struct task_struct *ksgxswapd_tsk;
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] security/apparmor/label.c: Clean code by removing redundant instructions
From: John Johansen @ 2020-03-03 18:40 UTC (permalink / raw)
To: mateusznosek0, linux-kernel, linux-security-module; +Cc: serge, jmorris
In-Reply-To: <20200303183023.32004-1-mateusznosek0@gmail.com>
On 3/3/20 10:30 AM, mateusznosek0@gmail.com wrote:
> From: Mateusz Nosek <mateusznosek0@gmail.com>
>
> Previously 'label->proxy->label' value checking
> and conditional reassigning were done twice in the same function.
> The second one is redundant and can be removed.
>
> Signed-off-by: Mateusz Nosek <mateusznosek0@gmail.com>
Acked-by: John Johansen <john.johansen@canonical.com>
I will pull this into apparmor-next
> ---
> security/apparmor/label.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index ba3987242282..676eebcbfd68 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -311,8 +311,6 @@ int aa_vec_unique(struct aa_profile **vec, int n, int flags)
>
> static void label_destroy(struct aa_label *label)
> {
> - struct aa_label *tmp;
> -
> AA_BUG(!label);
>
> if (!label_isprofile(label)) {
> @@ -333,10 +331,6 @@ static void label_destroy(struct aa_label *label)
>
> aa_free_secid(label->secid);
>
> - tmp = rcu_dereference_protected(label->proxy->label, true);
> - if (tmp == label)
> - rcu_assign_pointer(label->proxy->label, NULL);
> -
> aa_put_proxy(label->proxy);
> label->proxy = (struct aa_proxy *) PROXY_POISON + 1;
> }
>
^ permalink raw reply
* [PATCH] security/apparmor/label.c: Clean code by removing redundant instructions
From: mateusznosek0 @ 2020-03-03 18:30 UTC (permalink / raw)
To: linux-kernel, linux-security-module
Cc: Mateusz Nosek, serge, jmorris, john.johansen
From: Mateusz Nosek <mateusznosek0@gmail.com>
Previously 'label->proxy->label' value checking
and conditional reassigning were done twice in the same function.
The second one is redundant and can be removed.
Signed-off-by: Mateusz Nosek <mateusznosek0@gmail.com>
---
security/apparmor/label.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index ba3987242282..676eebcbfd68 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -311,8 +311,6 @@ int aa_vec_unique(struct aa_profile **vec, int n, int flags)
static void label_destroy(struct aa_label *label)
{
- struct aa_label *tmp;
-
AA_BUG(!label);
if (!label_isprofile(label)) {
@@ -333,10 +331,6 @@ static void label_destroy(struct aa_label *label)
aa_free_secid(label->secid);
- tmp = rcu_dereference_protected(label->proxy->label, true);
- if (tmp == label)
- rcu_assign_pointer(label->proxy->label, NULL);
-
aa_put_proxy(label->proxy);
label->proxy = (struct aa_proxy *) PROXY_POISON + 1;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in ima_template_entry
From: Mimi Zohar @ 2020-03-03 4:03 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200210100418.22049-1-roberto.sassu@huawei.com>
On Mon, 2020-02-10 at 11:04 +0100, Roberto Sassu wrote:
> @@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry)
>
> int __init ima_init_digests(void)
> {
> + u16 digest_size;
> + u16 crypto_id;
> int i;
>
> if (!ima_tpm_chip)
> @@ -229,8 +226,17 @@ int __init ima_init_digests(void)
> if (!digests)
> return -ENOMEM;
>
> - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
> + digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
> + crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> +
> + /* for unmapped TPM algorithms digest is still a padded SHA1 */
> + if (crypto_id == HASH_ALGO__LAST)
> + digest_size = SHA1_DIGEST_SIZE;
> +
> + memset(digests[i].digest, 0xff, digest_size);
Shouldn't the memset here be of the actual digest size even for
unmapped TPM algorithms.
> + }
>
> return 0;
> }
^ permalink raw reply
* [PATCH AUTOSEL 5.5 41/66] efi: Only print errors about failing to get certs if EFI vars are found
From: Sasha Levin @ 2020-03-03 2:45 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Javier Martinez Canillas, Hans de Goede, Ard Biesheuvel,
Mimi Zohar, Sasha Levin, linux-security-module
In-Reply-To: <20200303024615.8889-1-sashal@kernel.org>
From: Javier Martinez Canillas <javierm@redhat.com>
[ Upstream commit 3be54d558c75562e42bc83d665df024bd79d399b ]
If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
But it just assumes that the variables will be present and prints an error
if the certs can't be loaded, even when is possible that the variables may
not exist. For example the MokListRT variable will only be present if shim
is used.
So only print an error message about failing to get the certs list from an
EFI variable if this is found. Otherwise these printed errors just pollute
the kernel log ring buffer with confusing messages like the following:
[ 5.427251] Couldn't get size: 0x800000000000000e
[ 5.427261] MODSIGN: Couldn't get UEFI db list
[ 5.428012] Couldn't get size: 0x800000000000000e
[ 5.428023] Couldn't get UEFI MokListRT
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++-------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 111898aad56e4..f0c908241966a 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -35,16 +35,18 @@ static __init bool uefi_check_ignore_db(void)
* Get a certificate list blob from the named EFI variable.
*/
static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
- unsigned long *size)
+ unsigned long *size, efi_status_t *status)
{
- efi_status_t status;
unsigned long lsize = 4;
unsigned long tmpdb[4];
void *db;
- status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
- if (status != EFI_BUFFER_TOO_SMALL) {
- pr_err("Couldn't get size: 0x%lx\n", status);
+ *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+ if (*status == EFI_NOT_FOUND)
+ return NULL;
+
+ if (*status != EFI_BUFFER_TOO_SMALL) {
+ pr_err("Couldn't get size: 0x%lx\n", *status);
return NULL;
}
@@ -52,10 +54,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
if (!db)
return NULL;
- status = efi.get_variable(name, guid, NULL, &lsize, db);
- if (status != EFI_SUCCESS) {
+ *status = efi.get_variable(name, guid, NULL, &lsize, db);
+ if (*status != EFI_SUCCESS) {
kfree(db);
- pr_err("Error reading db var: 0x%lx\n", status);
+ pr_err("Error reading db var: 0x%lx\n", *status);
return NULL;
}
@@ -74,6 +76,7 @@ static int __init load_uefi_certs(void)
efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
void *db = NULL, *dbx = NULL, *mok = NULL;
unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+ efi_status_t status;
int rc = 0;
if (!efi.get_variable)
@@ -83,9 +86,12 @@ static int __init load_uefi_certs(void)
* an error if we can't get them.
*/
if (!uefi_check_ignore_db()) {
- db = get_cert_list(L"db", &secure_var, &dbsize);
+ db = get_cert_list(L"db", &secure_var, &dbsize, &status);
if (!db) {
- pr_err("MODSIGN: Couldn't get UEFI db list\n");
+ if (status == EFI_NOT_FOUND)
+ pr_debug("MODSIGN: db variable wasn't found\n");
+ else
+ pr_err("MODSIGN: Couldn't get UEFI db list\n");
} else {
rc = parse_efi_signature_list("UEFI:db",
db, dbsize, get_handler_for_db);
@@ -96,9 +102,12 @@ static int __init load_uefi_certs(void)
}
}
- mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
+ mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
if (!mok) {
- pr_info("Couldn't get UEFI MokListRT\n");
+ if (status == EFI_NOT_FOUND)
+ pr_debug("MokListRT variable wasn't found\n");
+ else
+ pr_info("Couldn't get UEFI MokListRT\n");
} else {
rc = parse_efi_signature_list("UEFI:MokListRT",
mok, moksize, get_handler_for_db);
@@ -107,9 +116,12 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
- dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
+ dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
if (!dbx) {
- pr_info("Couldn't get UEFI dbx list\n");
+ if (status == EFI_NOT_FOUND)
+ pr_debug("dbx variable wasn't found\n");
+ else
+ pr_info("Couldn't get UEFI dbx list\n");
} else {
rc = parse_efi_signature_list("UEFI:dbx",
dbx, dbxsize,
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.4 36/58] efi: Only print errors about failing to get certs if EFI vars are found
From: Sasha Levin @ 2020-03-03 2:47 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Javier Martinez Canillas, Hans de Goede, Ard Biesheuvel,
Mimi Zohar, Sasha Levin, linux-security-module
In-Reply-To: <20200303024740.9511-1-sashal@kernel.org>
From: Javier Martinez Canillas <javierm@redhat.com>
[ Upstream commit 3be54d558c75562e42bc83d665df024bd79d399b ]
If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
But it just assumes that the variables will be present and prints an error
if the certs can't be loaded, even when is possible that the variables may
not exist. For example the MokListRT variable will only be present if shim
is used.
So only print an error message about failing to get the certs list from an
EFI variable if this is found. Otherwise these printed errors just pollute
the kernel log ring buffer with confusing messages like the following:
[ 5.427251] Couldn't get size: 0x800000000000000e
[ 5.427261] MODSIGN: Couldn't get UEFI db list
[ 5.428012] Couldn't get size: 0x800000000000000e
[ 5.428023] Couldn't get UEFI MokListRT
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++-------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 81b19c52832ba..020fc7a11ef0e 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -39,16 +39,18 @@ static __init bool uefi_check_ignore_db(void)
* Get a certificate list blob from the named EFI variable.
*/
static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
- unsigned long *size)
+ unsigned long *size, efi_status_t *status)
{
- efi_status_t status;
unsigned long lsize = 4;
unsigned long tmpdb[4];
void *db;
- status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
- if (status != EFI_BUFFER_TOO_SMALL) {
- pr_err("Couldn't get size: 0x%lx\n", status);
+ *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+ if (*status == EFI_NOT_FOUND)
+ return NULL;
+
+ if (*status != EFI_BUFFER_TOO_SMALL) {
+ pr_err("Couldn't get size: 0x%lx\n", *status);
return NULL;
}
@@ -56,10 +58,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
if (!db)
return NULL;
- status = efi.get_variable(name, guid, NULL, &lsize, db);
- if (status != EFI_SUCCESS) {
+ *status = efi.get_variable(name, guid, NULL, &lsize, db);
+ if (*status != EFI_SUCCESS) {
kfree(db);
- pr_err("Error reading db var: 0x%lx\n", status);
+ pr_err("Error reading db var: 0x%lx\n", *status);
return NULL;
}
@@ -144,6 +146,7 @@ static int __init load_uefi_certs(void)
efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
void *db = NULL, *dbx = NULL, *mok = NULL;
unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+ efi_status_t status;
int rc = 0;
if (!efi.get_variable)
@@ -153,9 +156,12 @@ static int __init load_uefi_certs(void)
* an error if we can't get them.
*/
if (!uefi_check_ignore_db()) {
- db = get_cert_list(L"db", &secure_var, &dbsize);
+ db = get_cert_list(L"db", &secure_var, &dbsize, &status);
if (!db) {
- pr_err("MODSIGN: Couldn't get UEFI db list\n");
+ if (status == EFI_NOT_FOUND)
+ pr_debug("MODSIGN: db variable wasn't found\n");
+ else
+ pr_err("MODSIGN: Couldn't get UEFI db list\n");
} else {
rc = parse_efi_signature_list("UEFI:db",
db, dbsize, get_handler_for_db);
@@ -166,9 +172,12 @@ static int __init load_uefi_certs(void)
}
}
- mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
+ mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
if (!mok) {
- pr_info("Couldn't get UEFI MokListRT\n");
+ if (status == EFI_NOT_FOUND)
+ pr_debug("MokListRT variable wasn't found\n");
+ else
+ pr_info("Couldn't get UEFI MokListRT\n");
} else {
rc = parse_efi_signature_list("UEFI:MokListRT",
mok, moksize, get_handler_for_db);
@@ -177,9 +186,12 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
- dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
+ dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
if (!dbx) {
- pr_info("Couldn't get UEFI dbx list\n");
+ if (status == EFI_NOT_FOUND)
+ pr_debug("dbx variable wasn't found\n");
+ else
+ pr_info("Couldn't get UEFI dbx list\n");
} else {
rc = parse_efi_signature_list("UEFI:dbx",
dbx, dbxsize,
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3 5/8] ima: Switch to dynamically allocated buffer for template digests
From: Mimi Zohar @ 2020-03-02 22:20 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200210100244.21728-1-roberto.sassu@huawei.com>
On Mon, 2020-02-10 at 11:02 +0100, Roberto Sassu wrote:
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2f380fb92a7a..f04bec2ab83f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -45,11 +45,15 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> #define IMA_TEMPLATE_IMA_NAME "ima"
> #define IMA_TEMPLATE_IMA_FMT "d|n"
>
> +#define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
> +
> /* current content of the policy */
> extern int ima_policy_flag;
>
> /* set during initialization */
> extern int ima_hash_algo;
> +extern int ima_sha1_idx __ro_after_init;
> +extern int ima_extra_slots __ro_after_init;
> extern int ima_appraise;
> extern struct tpm_chip *ima_tpm_chip;
>
> @@ -92,7 +96,7 @@ struct ima_template_desc {
>
> struct ima_template_entry {
> int pcr;
> - u8 digest[TPM_DIGEST_SIZE]; /* sha1 or md5 measurement hash */
> + struct tpm_digest *digests;
> struct ima_template_desc *template_desc; /* template descriptor */
> u32 template_data_len;
> struct ima_field_data template_data[0]; /* template related data */
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 51f562111864..ea6d834a5fab 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -27,6 +27,7 @@ void ima_free_template_entry(struct ima_template_entry *entry)
> for (i = 0; i < entry->template_desc->num_fields; i++)
> kfree(entry->template_data[i].data);
>
> + kfree(entry->digests);
> kfree(entry);
> }
>
> @@ -50,6 +51,13 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
> if (!*entry)
> return -ENOMEM;
>
> + (*entry)->digests = kcalloc(NR_BANKS(ima_tpm_chip) + ima_extra_slots,
> + sizeof(*(*entry)->digests), GFP_NOFS);
"sizeof(*(*entry)" should be simplified for readability. Defining a
local "entry" or "digests" variable might help. Similarly in
ima_restore_template_data().
> + if (!(*entry)->digests) {
> + result = -ENOMEM;
> + goto out;
> + }
> +
> (*entry)->template_desc = template_desc;
> for (i = 0; i < template_desc->num_fields; i++) {
> const struct ima_template_field *field =
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 2d356ae8e823..ea24d2f6b513 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -59,6 +59,10 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
> static struct crypto_shash *ima_shash_tfm;
> static struct crypto_ahash *ima_ahash_tfm;
>
> +int ima_sha1_idx __ro_after_init;
> +/* Additional number of slots to be reserved for SHA1 and IMA default algo */
> +int ima_extra_slots __ro_after_init = 1;
The reasoning behind needing an extra slot is clear from the patch
description, but not the reason for defining a variable. Let's
clarify this comment a bit by inserting "reserved, as needed, for
SHA1"
> +
> int __init ima_init_crypto(void)
> {
> long rc;
> @@ -502,7 +506,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
> }
>
> if (!rc)
> - rc = crypto_shash_final(shash, entry->digest);
> + rc = crypto_shash_final(shash,
> + entry->digests[ima_sha1_idx].digest);
There's no reason to split this line. It's less than 80 characters.
Mimi
>
> return rc;
> }
^ permalink raw reply
* Re: [PATCHv3] exec: Fix a deadlock in ptrace
From: Bernd Edlinger @ 2020-03-02 20:28 UTC (permalink / raw)
To: Jann Horn, Christian Brauner
Cc: Eric W. Biederman, James Morris, Jonathan Corbet, Alexander Viro,
Andrew Morton, Alexey Dobriyan, Thomas Gleixner, Oleg Nesterov,
Frederic Weisbecker, Andrei Vagin, Ingo Molnar,
Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, stable@vger.kernel.org, linux-security-module
In-Reply-To: <AM6PR03MB517034D7787B305832FEA885E4E70@AM6PR03MB5170.eurprd03.prod.outlook.com>
On 3/2/20 9:10 PM, Bernd Edlinger wrote:
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -44,7 +44,11 @@ struct linux_binprm {
> * exec has happened. Used to sanitize execution environment
> * and to set AT_SECURE auxv for glibc.
> */
> - secureexec:1;
> + secureexec:1,
> + /*
> + * Set by flush_old_exec, when the cred_change_mutex is taken.
Oops, missed to update this comment, should be "when the cred_guard_mutex is taken".
I'll send a new patch later.
Bernd.
> + */
> + called_flush_old_exec:1;
> #ifdef __alpha__
> unsigned int taso:1;
> #endif
^ permalink raw reply
* [PATCHv3] exec: Fix a deadlock in ptrace
From: Bernd Edlinger @ 2020-03-02 20:10 UTC (permalink / raw)
To: Jann Horn, Christian Brauner
Cc: Eric W. Biederman, James Morris, Jonathan Corbet, Alexander Viro,
Andrew Morton, Alexey Dobriyan, Thomas Gleixner, Oleg Nesterov,
Frederic Weisbecker, Andrei Vagin, Ingo Molnar,
Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, stable@vger.kernel.org, linux-security-module
In-Reply-To: <CAG48ez0zfutdReRCP38+F2O=LMU11FUQAG59YkaKZY8AJNxSGQ@mail.gmail.com>
This fixes a deadlock in the tracer when tracing a multi-threaded
application that calls execve while more than one thread are running.
I observed that when running strace on the gcc test suite, it always
blocks after a while, when expect calls execve, because other threads
have to be terminated. They send ptrace events, but the strace is no
longer able to respond, since it is blocked in vm_access.
The deadlock is always happening when strace needs to access the
tracees process mmap, while another thread in the tracee starts to
execve a child process, but that cannot continue until the
PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
strace D 0 30614 30584 0x00000000
Call Trace:
__schedule+0x3ce/0x6e0
schedule+0x5c/0xd0
schedule_preempt_disabled+0x15/0x20
__mutex_lock.isra.13+0x1ec/0x520
__mutex_lock_killable_slowpath+0x13/0x20
mutex_lock_killable+0x28/0x30
mm_access+0x27/0xa0
process_vm_rw_core.isra.3+0xff/0x550
process_vm_rw+0xdd/0xf0
__x64_sys_process_vm_readv+0x31/0x40
do_syscall_64+0x64/0x220
entry_SYSCALL_64_after_hwframe+0x44/0xa9
expect D 0 31933 30876 0x80004003
Call Trace:
__schedule+0x3ce/0x6e0
schedule+0x5c/0xd0
flush_old_exec+0xc4/0x770
load_elf_binary+0x35a/0x16c0
search_binary_handler+0x97/0x1d0
__do_execve_file.isra.40+0x5d4/0x8a0
__x64_sys_execve+0x49/0x60
do_syscall_64+0x64/0x220
entry_SYSCALL_64_after_hwframe+0x44/0xa9
The proposed solution is to take the cred_guard_mutex only
in a critical section at the beginning, and at the end of the
execve function, and let PTRACE_ATTACH fail with EAGAIN while
execve is not complete, but other functions like vm_access are
allowed to complete normally.
I also took the opportunity to improve the documentation
of prepare_creds, which is obviously out of sync.
Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
---
Documentation/security/credentials.rst | 19 +++++----
fs/exec.c | 28 ++++++++++++--
include/linux/binfmts.h | 6 ++-
include/linux/sched/signal.h | 1 +
init/init_task.c | 1 +
kernel/cred.c | 2 +-
kernel/fork.c | 1 +
kernel/ptrace.c | 4 ++
mm/process_vm_access.c | 2 +-
tools/testing/selftests/ptrace/Makefile | 4 +-
tools/testing/selftests/ptrace/vmaccess.c | 64 +++++++++++++++++++++++++++++++
11 files changed, 115 insertions(+), 17 deletions(-)
create mode 100644 tools/testing/selftests/ptrace/vmaccess.c
v2: adds a test case which passes when this patch is applied.
v3: fixes the issue without introducing a new mutex.
diff --git a/Documentation/security/credentials.rst b/Documentation/security/credentials.rst
index 282e79f..61d6704 100644
--- a/Documentation/security/credentials.rst
+++ b/Documentation/security/credentials.rst
@@ -437,9 +437,14 @@ new set of credentials by calling::
struct cred *prepare_creds(void);
-this locks current->cred_replace_mutex and then allocates and constructs a
-duplicate of the current process's credentials, returning with the mutex still
-held if successful. It returns NULL if not successful (out of memory).
+this allocates and constructs a duplicate of the current process's credentials.
+It returns NULL if not successful (out of memory).
+
+If called from __do_execve_file, the mutex current->signal->cred_guard_mutex
+is acquired before this function gets called, and released after setting
+current->signal->cred_locked_for_ptrace. The same mutex is acquired later,
+while the credentials and the process mmap are actually changed, and
+current->signal->cred_locked_for_ptrace is reset again.
The mutex prevents ``ptrace()`` from altering the ptrace state of a process
while security checks on credentials construction and changing is taking place
@@ -466,9 +471,8 @@ by calling::
This will alter various aspects of the credentials and the process, giving the
LSM a chance to do likewise, then it will use ``rcu_assign_pointer()`` to
-actually commit the new credentials to ``current->cred``, it will release
-``current->cred_replace_mutex`` to allow ``ptrace()`` to take place, and it
-will notify the scheduler and others of the changes.
+actually commit the new credentials to ``current->cred``, and it will notify
+the scheduler and others of the changes.
This function is guaranteed to return 0, so that it can be tail-called at the
end of such functions as ``sys_setresuid()``.
@@ -486,8 +490,7 @@ invoked::
void abort_creds(struct cred *new);
-This releases the lock on ``current->cred_replace_mutex`` that
-``prepare_creds()`` got and then releases the new credentials.
+This releases the new credentials.
A typical credentials alteration function would look something like this::
diff --git a/fs/exec.c b/fs/exec.c
index 74d88da..e466301 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1266,6 +1266,12 @@ int flush_old_exec(struct linux_binprm * bprm)
if (retval)
goto out;
+ retval = mutex_lock_killable(¤t->signal->cred_guard_mutex);
+ if (retval)
+ goto out;
+
+ bprm->called_flush_old_exec = 1;
+
/*
* Must be called _before_ exec_mmap() as bprm->mm is
* not visibile until then. This also enables the update
@@ -1398,28 +1404,41 @@ void finalize_exec(struct linux_binprm *bprm)
EXPORT_SYMBOL(finalize_exec);
/*
- * Prepare credentials and lock ->cred_guard_mutex.
+ * Prepare credentials and set ->cred_locked_for_ptrace.
* install_exec_creds() commits the new creds and drops the lock.
* Or, if exec fails before, free_bprm() should release ->cred and
* and unlock.
*/
static int prepare_bprm_creds(struct linux_binprm *bprm)
{
+ int ret;
+
if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
return -ERESTARTNOINTR;
+ ret = -EAGAIN;
+ if (unlikely(current->signal->cred_locked_for_ptrace))
+ goto out;
+
+ ret = -ENOMEM;
bprm->cred = prepare_exec_creds();
- if (likely(bprm->cred))
- return 0;
+ if (likely(bprm->cred)) {
+ current->signal->cred_locked_for_ptrace = true;
+ ret = 0;
+ }
+out:
mutex_unlock(¤t->signal->cred_guard_mutex);
- return -ENOMEM;
+ return ret;
}
static void free_bprm(struct linux_binprm *bprm)
{
free_arg_pages(bprm);
if (bprm->cred) {
+ if (!bprm->called_flush_old_exec)
+ mutex_lock(¤t->signal->cred_guard_mutex);
+ current->signal->cred_locked_for_ptrace = false;
mutex_unlock(¤t->signal->cred_guard_mutex);
abort_creds(bprm->cred);
}
@@ -1469,6 +1488,7 @@ void install_exec_creds(struct linux_binprm *bprm)
* credentials; any time after this it may be unlocked.
*/
security_bprm_committed_creds(bprm);
+ current->signal->cred_locked_for_ptrace = false;
mutex_unlock(¤t->signal->cred_guard_mutex);
}
EXPORT_SYMBOL(install_exec_creds);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index b40fc63..2e1318b 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -44,7 +44,11 @@ struct linux_binprm {
* exec has happened. Used to sanitize execution environment
* and to set AT_SECURE auxv for glibc.
*/
- secureexec:1;
+ secureexec:1,
+ /*
+ * Set by flush_old_exec, when the cred_change_mutex is taken.
+ */
+ called_flush_old_exec:1;
#ifdef __alpha__
unsigned int taso:1;
#endif
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 8805025..073a2b7 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -225,6 +225,7 @@ struct signal_struct {
struct mutex cred_guard_mutex; /* guard against foreign influences on
* credential calculations
* (notably. ptrace) */
+ bool cred_locked_for_ptrace; /* set while in execve */
} __randomize_layout;
/*
diff --git a/init/init_task.c b/init/init_task.c
index 9e5cbe5..ecefff28 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -26,6 +26,7 @@
.multiprocess = HLIST_HEAD_INIT,
.rlim = INIT_RLIMITS,
.cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex),
+ .cred_locked_for_ptrace = false,
#ifdef CONFIG_POSIX_TIMERS
.posix_timers = LIST_HEAD_INIT(init_signals.posix_timers),
.cputimer = {
diff --git a/kernel/cred.c b/kernel/cred.c
index 809a985..e4c78de 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -676,7 +676,7 @@ void __init cred_init(void)
*
* Returns the new credentials or NULL if out of memory.
*
- * Does not take, and does not return holding current->cred_replace_mutex.
+ * Does not take, and does not return holding ->cred_guard_mutex.
*/
struct cred *prepare_kernel_cred(struct task_struct *daemon)
{
diff --git a/kernel/fork.c b/kernel/fork.c
index 0808095..a2b2ec8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1594,6 +1594,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
sig->oom_score_adj_min = current->signal->oom_score_adj_min;
mutex_init(&sig->cred_guard_mutex);
+ sig->cred_locked_for_ptrace = false;
return 0;
}
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 43d6179..abf09ba 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -395,6 +395,10 @@ static int ptrace_attach(struct task_struct *task, long request,
if (mutex_lock_interruptible(&task->signal->cred_guard_mutex))
goto out;
+ retval = -EAGAIN;
+ if (task->signal->cred_locked_for_ptrace)
+ goto unlock_creds;
+
task_lock(task);
retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
task_unlock(task);
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index 357aa7b..b3e6eb5 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -204,7 +204,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
if (!mm || IS_ERR(mm)) {
rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
/*
- * Explicitly map EACCES to EPERM as EPERM is a more a
+ * Explicitly map EACCES to EPERM as EPERM is a more
* appropriate error code for process_vw_readv/writev
*/
if (rc == -EACCES)
diff --git a/tools/testing/selftests/ptrace/Makefile b/tools/testing/selftests/ptrace/Makefile
index c0b7f89..2f1f532 100644
--- a/tools/testing/selftests/ptrace/Makefile
+++ b/tools/testing/selftests/ptrace/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
-CFLAGS += -iquote../../../../include/uapi -Wall
+CFLAGS += -std=c99 -pthread -iquote../../../../include/uapi -Wall
-TEST_GEN_PROGS := get_syscall_info peeksiginfo
+TEST_GEN_PROGS := get_syscall_info peeksiginfo vmaccess
include ../lib.mk
diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c
new file mode 100644
index 0000000..63ff531
--- /dev/null
+++ b/tools/testing/selftests/ptrace/vmaccess.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020 Bernd Edlinger <bernd.edlinger@hotmail.de>
+ * All rights reserved.
+ *
+ * Check whether /proc/$pid/mem can be accessed without causing deadlocks
+ * when de_thread is blocked with ->cred_guard_mutex held.
+ */
+
+#include "../kselftest_harness.h"
+#include <stdio.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <signal.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+
+static void *thread(void *arg)
+{
+ ptrace(PTRACE_TRACEME, 0, 0L, 0L);
+ return NULL;
+}
+
+TEST(vmaccess)
+{
+ int f, pid = fork();
+ char mm[64];
+
+ if (!pid) {
+ pthread_t pt;
+ pthread_create(&pt, NULL, thread, NULL);
+ pthread_join(pt, NULL);
+ execlp("true", "true", NULL);
+ }
+
+ sleep(1);
+ sprintf(mm, "/proc/%d/mem", pid);
+ f = open(mm, O_RDONLY);
+ ASSERT_LE(0, f);
+ close(f);
+ f = kill(pid, SIGCONT);
+ ASSERT_EQ(0, f);
+}
+
+TEST(attach)
+{
+ int f, pid = fork();
+
+ if (!pid) {
+ pthread_t pt;
+ pthread_create(&pt, NULL, thread, NULL);
+ pthread_join(pt, NULL);
+ execlp("true", "true", NULL);
+ }
+
+ sleep(1);
+ f = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
+ ASSERT_EQ(EAGAIN, errno);
+ ASSERT_EQ(f, -1);
+ f = kill(pid, SIGCONT);
+ ASSERT_EQ(0, f);
+}
+
+TEST_HARNESS_MAIN
--
1.9.1
^ permalink raw reply related
* Re: [Intel-gfx] [PATCH v7 00/12] Introduce CAP_PERFMON to secure system performance monitoring and observability
From: James Morris @ 2020-03-02 19:44 UTC (permalink / raw)
To: Serge Hallyn
Cc: Alexey Budankov, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner, linux-man,
Andi Kleen, linux-parisc@vger.kernel.org,
linux-doc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list, Jiri Olsa,
linux-arm-kernel
In-Reply-To: <20200302001913.GA21145@sl>
On Sun, 1 Mar 2020, Serge Hallyn wrote:
> Thanks, this looks good to me, in keeping with the CAP_SYSLOG break.
>
> Acked-by: Serge E. Hallyn <serge@hallyn.com>
>
> for the set.
>
> James/Ingo/Peter, if noone has remaining objections, whose branch
> should these go in through?
>
> thanks,
I was assuming via the perf tree, but I am happy to take them.
> -serge
>
> On Tue, Feb 25, 2020 at 12:55:54PM +0300, Alexey Budankov wrote:
> >
> > Hi,
> >
> > Is there anything else I could do in order to move the changes forward
> > or is something still missing from this patch set?
> > Could you please share you mind?
> >
> > Thanks,
> > Alexey
> >
> > On 17.02.2020 11:02, Alexey Budankov wrote:
> > >
> > > Currently access to perf_events, i915_perf and other performance
> > > monitoring and observability subsystems of the kernel is open only for
> > > a privileged process [1] with CAP_SYS_ADMIN capability enabled in the
> > > process effective set [2].
> > >
> > > This patch set introduces CAP_PERFMON capability designed to secure
> > > system performance monitoring and observability operations so that
> > > CAP_PERFMON would assist CAP_SYS_ADMIN capability in its governing role
> > > for performance monitoring and observability subsystems of the kernel.
> > >
> > > CAP_PERFMON intends to harden system security and integrity during
> > > performance monitoring and observability operations by decreasing attack
> > > surface that is available to a CAP_SYS_ADMIN privileged process [2].
> > > Providing the access to performance monitoring and observability
> > > operations under CAP_PERFMON capability singly, without the rest of
> > > CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials
> > > and makes the operation more secure. Thus, CAP_PERFMON implements the
> > > principal of least privilege for performance monitoring and
> > > observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle of
> > > least privilege: A security design principle that states that a process
> > > or program be granted only those privileges (e.g., capabilities)
> > > necessary to accomplish its legitimate function, and only for the time
> > > that such privileges are actually required)
> > >
> > > CAP_PERFMON intends to meet the demand to secure system performance
> > > monitoring and observability operations for adoption in security
> > > sensitive, restricted, multiuser production environments (e.g. HPC
> > > clusters, cloud and virtual compute environments), where root or
> > > CAP_SYS_ADMIN credentials are not available to mass users of a system,
> > > and securely unblock accessibility of system performance monitoring and
> > > observability operations beyond root and CAP_SYS_ADMIN use cases.
> > >
> > > CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
> > > system performance monitoring and observability operations and balance
> > > amount of CAP_SYS_ADMIN credentials following the recommendations in
> > > the capabilities man page [2] for CAP_SYS_ADMIN: "Note: this capability
> > > is overloaded; see Notes to kernel developers, below." For backward
> > > compatibility reasons access to system performance monitoring and
> > > observability subsystems of the kernel remains open for CAP_SYS_ADMIN
> > > privileged processes but CAP_SYS_ADMIN capability usage for secure
> > > system performance monitoring and observability operations is
> > > discouraged with respect to the designed CAP_PERFMON capability.
> > >
> > > Possible alternative solution to this system security hardening,
> > > capabilities balancing task of making performance monitoring and
> > > observability operations more secure and accessible could be to use
> > > the existing CAP_SYS_PTRACE capability to govern system performance
> > > monitoring and observability subsystems. However CAP_SYS_PTRACE
> > > capability still provides users with more credentials than are
> > > required for secure performance monitoring and observability
> > > operations and this excess is avoided by the designed CAP_PERFMON.
> > >
> > > Although software running under CAP_PERFMON can not ensure avoidance of
> > > related hardware issues, the software can still mitigate those issues
> > > following the official hardware issues mitigation procedure [3]. The
> > > bugs in the software itself can be fixed following the standard kernel
> > > development process [4] to maintain and harden security of system
> > > performance monitoring and observability operations. Finally, the patch
> > > set is shaped in the way that simplifies backtracking procedure of
> > > possible induced issues [5] as much as possible.
> > >
> > > The patch set is for tip perf/core repository:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip perf/core
> > > sha1: fdb64822443ec9fb8c3a74b598a74790ae8d2e22
> > >
> > > ---
> > > Changes in v7:
> > > - updated and extended kernel.rst and perf-security.rst documentation
> > > files with the information about CAP_PERFMON capability and its use cases
> > > - documented the case of double audit logging of CAP_PERFMON and CAP_SYS_ADMIN
> > > capabilities on a SELinux enabled system
> > > Changes in v6:
> > > - avoided noaudit checks in perfmon_capable() to explicitly advertise
> > > CAP_PERFMON usage thru audit logs to secure system performance
> > > monitoring and observability
> > > Changes in v5:
> > > - renamed CAP_SYS_PERFMON to CAP_PERFMON
> > > - extended perfmon_capable() with noaudit checks
> > > Changes in v4:
> > > - converted perfmon_capable() into an inline function
> > > - made perf_events kprobes, uprobes, hw breakpoints and namespaces data
> > > available to CAP_SYS_PERFMON privileged processes
> > > - applied perfmon_capable() to drivers/perf and drivers/oprofile
> > > - extended __cmd_ftrace() with support of CAP_SYS_PERFMON
> > > Changes in v3:
> > > - implemented perfmon_capable() macros aggregating required capabilities
> > > checks
> > > Changes in v2:
> > > - made perf_events trace points available to CAP_SYS_PERFMON privileged
> > > processes
> > > - made perf_event_paranoid_check() treat CAP_SYS_PERFMON equally to
> > > CAP_SYS_ADMIN
> > > - applied CAP_SYS_PERFMON to i915_perf, bpf_trace, powerpc and parisc
> > > system performance monitoring and observability related subsystems
> > >
> > > ---
> > > Alexey Budankov (12):
> > > capabilities: introduce CAP_PERFMON to kernel and user space
> > > perf/core: open access to the core for CAP_PERFMON privileged process
> > > perf/core: open access to probes for CAP_PERFMON privileged process
> > > perf tool: extend Perf tool with CAP_PERFMON capability support
> > > drm/i915/perf: open access for CAP_PERFMON privileged process
> > > trace/bpf_trace: open access for CAP_PERFMON privileged process
> > > powerpc/perf: open access for CAP_PERFMON privileged process
> > > parisc/perf: open access for CAP_PERFMON privileged process
> > > drivers/perf: open access for CAP_PERFMON privileged process
> > > drivers/oprofile: open access for CAP_PERFMON privileged process
> > > doc/admin-guide: update perf-security.rst with CAP_PERFMON information
> > > doc/admin-guide: update kernel.rst with CAP_PERFMON information
> > >
> > > Documentation/admin-guide/perf-security.rst | 65 +++++++++++++--------
> > > Documentation/admin-guide/sysctl/kernel.rst | 16 +++--
> > > arch/parisc/kernel/perf.c | 2 +-
> > > arch/powerpc/perf/imc-pmu.c | 4 +-
> > > drivers/gpu/drm/i915/i915_perf.c | 13 ++---
> > > drivers/oprofile/event_buffer.c | 2 +-
> > > drivers/perf/arm_spe_pmu.c | 4 +-
> > > include/linux/capability.h | 4 ++
> > > include/linux/perf_event.h | 6 +-
> > > include/uapi/linux/capability.h | 8 ++-
> > > kernel/events/core.c | 6 +-
> > > kernel/trace/bpf_trace.c | 2 +-
> > > security/selinux/include/classmap.h | 4 +-
> > > tools/perf/builtin-ftrace.c | 5 +-
> > > tools/perf/design.txt | 3 +-
> > > tools/perf/util/cap.h | 4 ++
> > > tools/perf/util/evsel.c | 10 ++--
> > > tools/perf/util/util.c | 1 +
> > > 18 files changed, 98 insertions(+), 61 deletions(-)
> > >
> > > ---
> > > Validation (Intel Skylake, 8 cores, Fedora 29, 5.5.0-rc3+, x86_64):
> > >
> > > libcap library [6], [7], [8] and Perf tool can be used to apply
> > > CAP_PERFMON capability for secure system performance monitoring and
> > > observability beyond the scope permitted by the system wide
> > > perf_event_paranoid kernel setting [9] and below are the steps for
> > > evaluation:
> > >
> > > - patch, build and boot the kernel
> > > - patch, build Perf tool e.g. to /home/user/perf
> > > ...
> > > # git clone git://git.kernel.org/pub/scm/libs/libcap/libcap.git libcap
> > > # pushd libcap
> > > # patch libcap/include/uapi/linux/capabilities.h with [PATCH 1]
> > > # make
> > > # pushd progs
> > > # ./setcap "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> > > # ./setcap -v "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> > > /home/user/perf: OK
> > > # ./getcap /home/user/perf
> > > /home/user/perf = cap_sys_ptrace,cap_syslog,cap_perfmon+ep
> > > # echo 2 > /proc/sys/kernel/perf_event_paranoid
> > > # cat /proc/sys/kernel/perf_event_paranoid
> > > 2
> > > ...
> > > $ /home/user/perf top
> > > ... works as expected ...
> > > $ cat /proc/`pidof perf`/status
> > > Name: perf
> > > Umask: 0002
> > > State: S (sleeping)
> > > Tgid: 2958
> > > Ngid: 0
> > > Pid: 2958
> > > PPid: 9847
> > > TracerPid: 0
> > > Uid: 500 500 500 500
> > > Gid: 500 500 500 500
> > > FDSize: 256
> > > ...
> > > CapInh: 0000000000000000
> > > CapPrm: 0000004400080000
> > > CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> > > cap_perfmon,cap_sys_ptrace,cap_syslog
> > > CapBnd: 0000007fffffffff
> > > CapAmb: 0000000000000000
> > > NoNewPrivs: 0
> > > Seccomp: 0
> > > Speculation_Store_Bypass: thread vulnerable
> > > Cpus_allowed: ff
> > > Cpus_allowed_list: 0-7
> > > ...
> > >
> > > Usage of cap_perfmon effectively avoids unused credentials excess:
> > >
> > > - with cap_sys_admin:
> > > CapEff: 0000007fffffffff => 01111111 11111111 11111111 11111111 11111111
> > >
> > > - with cap_perfmon:
> > > CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> > > 38 34 19
> > > perfmon syslog sys_ptrace
> > >
> > > ---
> > > [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
> > > [2] http://man7.org/linux/man-pages/man7/capabilities.7.html
> > > [3] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
> > > [4] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
> > > [5] https://www.kernel.org/doc/html/latest/process/management-style.html#decisions
> > > [6] http://man7.org/linux/man-pages/man8/setcap.8.html
> > > [7] https://git.kernel.org/pub/scm/libs/libcap/libcap.git
> > > [8] https://sites.google.com/site/fullycapable/, posix_1003.1e-990310.pdf
> > > [9] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCHv2] exec: Fix a deadlock in ptrace
From: Jann Horn @ 2020-03-02 18:08 UTC (permalink / raw)
To: Christian Brauner
Cc: Bernd Edlinger, Eric W. Biederman, James Morris, Jonathan Corbet,
Alexander Viro, Andrew Morton, Alexey Dobriyan, Thomas Gleixner,
Oleg Nesterov, Frederic Weisbecker, Andrei Vagin, Ingo Molnar,
Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
stable@vger.kernel.org, linux-security-module
In-Reply-To: <5e5d45a3.1c69fb81.f99ac.0806@mx.google.com>
On Mon, Mar 2, 2020 at 6:43 PM <christian@brauner.io> wrote:
> On March 2, 2020 6:37:27 PM GMT+01:00, Jann Horn <jannh@google.com> wrote:
> >On Mon, Mar 2, 2020 at 6:01 PM Bernd Edlinger
> ><bernd.edlinger@hotmail.de> wrote:
> >> On 3/2/20 5:43 PM, Jann Horn wrote:
> >> > On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman
> ><ebiederm@xmission.com> wrote:
[...]
> >> >> I am 99% convinced that the fix is to move cred_guard_mutex down.
> >> >
> >> > "move cred_guard_mutex down" as in "take it once we've already set
> >up
> >> > the new process, past the point of no return"?
> >> >
> >> >> Then right after we take cred_guard_mutex do:
> >> >> if (ptraced) {
> >> >> use_original_creds();
> >> >> }
> >> >>
> >> >> And call it a day.
> >> >>
> >> >> The details suck but I am 99% certain that would solve everyones
> >> >> problems, and not be too bad to audit either.
> >> >
> >> > Ah, hmm, that sounds like it'll work fine at least when no LSMs are
> >involved.
> >> >
> >> > SELinux normally doesn't do the execution-degrading thing, it just
> >> > blocks the execution completely - see their
> >selinux_bprm_set_creds()
> >> > hook. So I think they'd still need to set some state on the task
> >that
> >> > says "we're currently in the middle of an execution where the
> >target
> >> > task will run in context X", and then check against that in the
> >> > ptrace_may_access hook. Or I suppose they could just kill the task
> >> > near the end of execve, although that'd be kinda ugly.
> >> >
> >>
> >> We have current->in_execve for that, right?
> >> I think when the cred_guard_mutex is taken only in the critical
> >section,
> >> then PTRACE_ATTACH could take the guard_mutex, and look at
> >current->in_execve,
> >> and just return -EAGAIN in that case, right, everybody happy :)
> >
> >It's probably going to mean that things like strace will just randomly
> >fail to attach to processes if they happen to be in the middle of
> >execve... but I guess that works?
>
> That sounds like an acceptable outcome.
> We can at least risk it and if we regress
> revert or come up with the more complex
> solution suggested in another mail here?
Yeah, sounds reasonable, I guess.
^ permalink raw reply
* Re: [PATCHv2] exec: Fix a deadlock in ptrace
From: christian @ 2020-03-02 17:42 UTC (permalink / raw)
To: Jann Horn, Bernd Edlinger
Cc: Eric W. Biederman, James Morris, Jonathan Corbet, Alexander Viro,
Andrew Morton, Alexey Dobriyan, Thomas Gleixner, Oleg Nesterov,
Frederic Weisbecker, Andrei Vagin, Ingo Molnar,
Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc
In-Reply-To: <CAG48ez1jj_J3PtENWvu8piFGsik6RvuyD38ie48TYr2k1Rbf3A@mail.gmail.com>
<linux-doc@vger.kernel.org>,"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,"linux-mm@kvack.org" <linux-mm@kvack.org>,"stable@vger.kernel.org" <stable@vger.kernel.org>,linux-security-module <linux-security-module@vger.kernel.org>
From: Christian Brauner <christian.brauner@ubuntu.com>
Message-ID: <9C3BF644-0F82-48C9-9116-8554204FB57D@ubuntu.com>
On March 2, 2020 6:37:27 PM GMT+01:00, Jann Horn <jannh@google.com> wrote:
>On Mon, Mar 2, 2020 at 6:01 PM Bernd Edlinger
><bernd.edlinger@hotmail.de> wrote:
>> On 3/2/20 5:43 PM, Jann Horn wrote:
>> > On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman
><ebiederm@xmission.com> wrote:
>> >>
>> >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> >>
>> >>> On 3/2/20 4:57 PM, Eric W. Biederman wrote:
>> >>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> >>>>
>> >>>>>
>> >>>>> I tried this with s/EACCESS/EACCES/.
>> >>>>>
>> >>>>> The test case in this patch is not fixed, but strace does not
>freeze,
>> >>>>> at least with my setup where it did freeze repeatable.
>> >>>>
>> >>>> Thanks, That is what I was aiming at.
>> >>>>
>> >>>> So we have one method we can pursue to fix this in practice.
>> >>>>
>> >>>>> That is
>> >>>>> obviously because it bypasses the cred_guard_mutex. But all
>other
>> >>>>> process that access this file still freeze, and cannot be
>> >>>>> interrupted except with kill -9.
>> >>>>>
>> >>>>> However that smells like a denial of service, that this
>> >>>>> simple test case which can be executed by guest, creates a
>/proc/$pid/mem
>> >>>>> that freezes any process, even root, when it looks at it.
>> >>>>> I mean: "ln -s README /proc/$pid/mem" would be a nice bomb.
>> >>>>
>> >>>> Yes. Your the test case in your patch a variant of the original
>> >>>> problem.
>> >>>>
>> >>>>
>> >>>> I have been staring at this trying to understand the
>fundamentals of the
>> >>>> original deeper problem.
>> >>>>
>> >>>> The current scope of cred_guard_mutex in exec is because being
>ptraced
>> >>>> causes suid exec to act differently. So we need to know early
>if we are
>> >>>> ptraced.
>> >>>>
>> >>>
>> >>> It has a second use, that it prevents two threads entering
>execve,
>> >>> which would probably result in disaster.
>> >>
>> >> Exec can fail with an error code up until de_thread. de_thread
>causes
>> >> exec to fail with the error code -EAGAIN for the second thread to
>get
>> >> into de_thread.
>> >>
>> >> So no. The cred_guard_mutex is not needed for that case at all.
>> >>
>> >>>> If that case did not exist we could reduce the scope of the
>> >>>> cred_guard_mutex in exec to where your patch puts the
>cred_change_mutex.
>> >>>>
>> >>>> I am starting to think reworking how we deal with ptrace and
>exec is the
>> >>>> way to solve this problem.
>> >>
>> >>
>> >> I am 99% convinced that the fix is to move cred_guard_mutex down.
>> >
>> > "move cred_guard_mutex down" as in "take it once we've already set
>up
>> > the new process, past the point of no return"?
>> >
>> >> Then right after we take cred_guard_mutex do:
>> >> if (ptraced) {
>> >> use_original_creds();
>> >> }
>> >>
>> >> And call it a day.
>> >>
>> >> The details suck but I am 99% certain that would solve everyones
>> >> problems, and not be too bad to audit either.
>> >
>> > Ah, hmm, that sounds like it'll work fine at least when no LSMs are
>involved.
>> >
>> > SELinux normally doesn't do the execution-degrading thing, it just
>> > blocks the execution completely - see their
>selinux_bprm_set_creds()
>> > hook. So I think they'd still need to set some state on the task
>that
>> > says "we're currently in the middle of an execution where the
>target
>> > task will run in context X", and then check against that in the
>> > ptrace_may_access hook. Or I suppose they could just kill the task
>> > near the end of execve, although that'd be kinda ugly.
>> >
>>
>> We have current->in_execve for that, right?
>> I think when the cred_guard_mutex is taken only in the critical
>section,
>> then PTRACE_ATTACH could take the guard_mutex, and look at
>current->in_execve,
>> and just return -EAGAIN in that case, right, everybody happy :)
>
>It's probably going to mean that things like strace will just randomly
>fail to attach to processes if they happen to be in the middle of
>execve... but I guess that works?
That sounds like an acceptable outcome.
We can at least risk it and if we regress
revert or come up with the more complex
solution suggested in another mail here?
^ permalink raw reply
* Re: [PATCHv2] exec: Fix a deadlock in ptrace
From: Jann Horn @ 2020-03-02 17:37 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Eric W. Biederman, James Morris, Christian Brauner,
Jonathan Corbet, Alexander Viro, Andrew Morton, Alexey Dobriyan,
Thomas Gleixner, Oleg Nesterov, Frederic Weisbecker, Andrei Vagin,
Ingo Molnar, Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, stable@vger.kernel.org, linux-security-module
In-Reply-To: <AM6PR03MB5170BD130F15CE1909F59B55E4E70@AM6PR03MB5170.eurprd03.prod.outlook.com>
On Mon, Mar 2, 2020 at 6:01 PM Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> On 3/2/20 5:43 PM, Jann Horn wrote:
> > On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >>
> >>> On 3/2/20 4:57 PM, Eric W. Biederman wrote:
> >>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >>>>
> >>>>>
> >>>>> I tried this with s/EACCESS/EACCES/.
> >>>>>
> >>>>> The test case in this patch is not fixed, but strace does not freeze,
> >>>>> at least with my setup where it did freeze repeatable.
> >>>>
> >>>> Thanks, That is what I was aiming at.
> >>>>
> >>>> So we have one method we can pursue to fix this in practice.
> >>>>
> >>>>> That is
> >>>>> obviously because it bypasses the cred_guard_mutex. But all other
> >>>>> process that access this file still freeze, and cannot be
> >>>>> interrupted except with kill -9.
> >>>>>
> >>>>> However that smells like a denial of service, that this
> >>>>> simple test case which can be executed by guest, creates a /proc/$pid/mem
> >>>>> that freezes any process, even root, when it looks at it.
> >>>>> I mean: "ln -s README /proc/$pid/mem" would be a nice bomb.
> >>>>
> >>>> Yes. Your the test case in your patch a variant of the original
> >>>> problem.
> >>>>
> >>>>
> >>>> I have been staring at this trying to understand the fundamentals of the
> >>>> original deeper problem.
> >>>>
> >>>> The current scope of cred_guard_mutex in exec is because being ptraced
> >>>> causes suid exec to act differently. So we need to know early if we are
> >>>> ptraced.
> >>>>
> >>>
> >>> It has a second use, that it prevents two threads entering execve,
> >>> which would probably result in disaster.
> >>
> >> Exec can fail with an error code up until de_thread. de_thread causes
> >> exec to fail with the error code -EAGAIN for the second thread to get
> >> into de_thread.
> >>
> >> So no. The cred_guard_mutex is not needed for that case at all.
> >>
> >>>> If that case did not exist we could reduce the scope of the
> >>>> cred_guard_mutex in exec to where your patch puts the cred_change_mutex.
> >>>>
> >>>> I am starting to think reworking how we deal with ptrace and exec is the
> >>>> way to solve this problem.
> >>
> >>
> >> I am 99% convinced that the fix is to move cred_guard_mutex down.
> >
> > "move cred_guard_mutex down" as in "take it once we've already set up
> > the new process, past the point of no return"?
> >
> >> Then right after we take cred_guard_mutex do:
> >> if (ptraced) {
> >> use_original_creds();
> >> }
> >>
> >> And call it a day.
> >>
> >> The details suck but I am 99% certain that would solve everyones
> >> problems, and not be too bad to audit either.
> >
> > Ah, hmm, that sounds like it'll work fine at least when no LSMs are involved.
> >
> > SELinux normally doesn't do the execution-degrading thing, it just
> > blocks the execution completely - see their selinux_bprm_set_creds()
> > hook. So I think they'd still need to set some state on the task that
> > says "we're currently in the middle of an execution where the target
> > task will run in context X", and then check against that in the
> > ptrace_may_access hook. Or I suppose they could just kill the task
> > near the end of execve, although that'd be kinda ugly.
> >
>
> We have current->in_execve for that, right?
> I think when the cred_guard_mutex is taken only in the critical section,
> then PTRACE_ATTACH could take the guard_mutex, and look at current->in_execve,
> and just return -EAGAIN in that case, right, everybody happy :)
It's probably going to mean that things like strace will just randomly
fail to attach to processes if they happen to be in the middle of
execve... but I guess that works?
^ permalink raw reply
* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-03-02 17:33 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, Dmitry Kasatkin
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <a5e0cdc4839e478d926b90bd5ba0857c@huawei.com>
On Mon, 2020-03-02 at 15:11 +0000, Roberto Sassu wrote:
> > Yes, preference is given to the IMA default algorithm, but it should
> > fall back to using SHA256 or SHA1, based on the TPM.
>
> Ok. The patch already does it even if the TPM version is not checked.
> For TPM 1.2, if the default algorithm is not SHA1 the patch will select
> the first PCR bank (SHA1).
>
> Should I send a new patch which explicitly checks the TPM version?
Checking the TPM version shouldn't be necessary. The code currently
sets bank_idx to the HASH_ALGO_SHA256. If instead of initializing
bank_idx to 0, initialize it to the nr_allocated_banks or -1. As long
as the bank_idx value is the same as the initialized value, set the
bank_idx to HASH_ALGO_SHA1.
The subsequent bank_idx would then be limited to testing for the
initialized value.
Mimi
^ permalink raw reply
* Re: [PATCHv2] exec: Fix a deadlock in ptrace
From: Bernd Edlinger @ 2020-03-02 17:01 UTC (permalink / raw)
To: Jann Horn, Eric W. Biederman, James Morris
Cc: Christian Brauner, Jonathan Corbet, Alexander Viro, Andrew Morton,
Alexey Dobriyan, Thomas Gleixner, Oleg Nesterov,
Frederic Weisbecker, Andrei Vagin, Ingo Molnar,
Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, stable@vger.kernel.org, linux-security-module
In-Reply-To: <CAG48ez0iXMD0mduKWHG6GZZoR+s2jXy776zwiRd+tFADCEiBEw@mail.gmail.com>
On 3/2/20 5:43 PM, Jann Horn wrote:
> On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>
>>> On 3/2/20 4:57 PM, Eric W. Biederman wrote:
>>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>>>
>>>>>
>>>>> I tried this with s/EACCESS/EACCES/.
>>>>>
>>>>> The test case in this patch is not fixed, but strace does not freeze,
>>>>> at least with my setup where it did freeze repeatable.
>>>>
>>>> Thanks, That is what I was aiming at.
>>>>
>>>> So we have one method we can pursue to fix this in practice.
>>>>
>>>>> That is
>>>>> obviously because it bypasses the cred_guard_mutex. But all other
>>>>> process that access this file still freeze, and cannot be
>>>>> interrupted except with kill -9.
>>>>>
>>>>> However that smells like a denial of service, that this
>>>>> simple test case which can be executed by guest, creates a /proc/$pid/mem
>>>>> that freezes any process, even root, when it looks at it.
>>>>> I mean: "ln -s README /proc/$pid/mem" would be a nice bomb.
>>>>
>>>> Yes. Your the test case in your patch a variant of the original
>>>> problem.
>>>>
>>>>
>>>> I have been staring at this trying to understand the fundamentals of the
>>>> original deeper problem.
>>>>
>>>> The current scope of cred_guard_mutex in exec is because being ptraced
>>>> causes suid exec to act differently. So we need to know early if we are
>>>> ptraced.
>>>>
>>>
>>> It has a second use, that it prevents two threads entering execve,
>>> which would probably result in disaster.
>>
>> Exec can fail with an error code up until de_thread. de_thread causes
>> exec to fail with the error code -EAGAIN for the second thread to get
>> into de_thread.
>>
>> So no. The cred_guard_mutex is not needed for that case at all.
>>
>>>> If that case did not exist we could reduce the scope of the
>>>> cred_guard_mutex in exec to where your patch puts the cred_change_mutex.
>>>>
>>>> I am starting to think reworking how we deal with ptrace and exec is the
>>>> way to solve this problem.
>>
>>
>> I am 99% convinced that the fix is to move cred_guard_mutex down.
>
> "move cred_guard_mutex down" as in "take it once we've already set up
> the new process, past the point of no return"?
>
>> Then right after we take cred_guard_mutex do:
>> if (ptraced) {
>> use_original_creds();
>> }
>>
>> And call it a day.
>>
>> The details suck but I am 99% certain that would solve everyones
>> problems, and not be too bad to audit either.
>
> Ah, hmm, that sounds like it'll work fine at least when no LSMs are involved.
>
> SELinux normally doesn't do the execution-degrading thing, it just
> blocks the execution completely - see their selinux_bprm_set_creds()
> hook. So I think they'd still need to set some state on the task that
> says "we're currently in the middle of an execution where the target
> task will run in context X", and then check against that in the
> ptrace_may_access hook. Or I suppose they could just kill the task
> near the end of execve, although that'd be kinda ugly.
>
We have current->in_execve for that, right?
I think when the cred_guard_mutex is taken only in the critical section,
then PTRACE_ATTACH could take the guard_mutex, and look at current->in_execve,
and just return -EAGAIN in that case, right, everybody happy :)
Bernd.
^ permalink raw reply
* Re: [PATCHv2] exec: Fix a deadlock in ptrace
From: Jann Horn @ 2020-03-02 16:43 UTC (permalink / raw)
To: Eric W. Biederman, James Morris
Cc: Bernd Edlinger, Christian Brauner, Jonathan Corbet,
Alexander Viro, Andrew Morton, Alexey Dobriyan, Thomas Gleixner,
Oleg Nesterov, Frederic Weisbecker, Andrei Vagin, Ingo Molnar,
Peter Zijlstra (Intel), Yuyang Du, David Hildenbrand,
Sebastian Andrzej Siewior, Anshuman Khandual, David Howells,
Kees Cook, Greg Kroah-Hartman, Shakeel Butt, Jason Gunthorpe,
Christian Kellner, Andrea Arcangeli, Aleksa Sarai,
Dmitry V. Levin, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, stable@vger.kernel.org, linux-security-module
In-Reply-To: <875zfmloir.fsf@x220.int.ebiederm.org>
On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>
> > On 3/2/20 4:57 PM, Eric W. Biederman wrote:
> >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >>
> >>>
> >>> I tried this with s/EACCESS/EACCES/.
> >>>
> >>> The test case in this patch is not fixed, but strace does not freeze,
> >>> at least with my setup where it did freeze repeatable.
> >>
> >> Thanks, That is what I was aiming at.
> >>
> >> So we have one method we can pursue to fix this in practice.
> >>
> >>> That is
> >>> obviously because it bypasses the cred_guard_mutex. But all other
> >>> process that access this file still freeze, and cannot be
> >>> interrupted except with kill -9.
> >>>
> >>> However that smells like a denial of service, that this
> >>> simple test case which can be executed by guest, creates a /proc/$pid/mem
> >>> that freezes any process, even root, when it looks at it.
> >>> I mean: "ln -s README /proc/$pid/mem" would be a nice bomb.
> >>
> >> Yes. Your the test case in your patch a variant of the original
> >> problem.
> >>
> >>
> >> I have been staring at this trying to understand the fundamentals of the
> >> original deeper problem.
> >>
> >> The current scope of cred_guard_mutex in exec is because being ptraced
> >> causes suid exec to act differently. So we need to know early if we are
> >> ptraced.
> >>
> >
> > It has a second use, that it prevents two threads entering execve,
> > which would probably result in disaster.
>
> Exec can fail with an error code up until de_thread. de_thread causes
> exec to fail with the error code -EAGAIN for the second thread to get
> into de_thread.
>
> So no. The cred_guard_mutex is not needed for that case at all.
>
> >> If that case did not exist we could reduce the scope of the
> >> cred_guard_mutex in exec to where your patch puts the cred_change_mutex.
> >>
> >> I am starting to think reworking how we deal with ptrace and exec is the
> >> way to solve this problem.
>
>
> I am 99% convinced that the fix is to move cred_guard_mutex down.
"move cred_guard_mutex down" as in "take it once we've already set up
the new process, past the point of no return"?
> Then right after we take cred_guard_mutex do:
> if (ptraced) {
> use_original_creds();
> }
>
> And call it a day.
>
> The details suck but I am 99% certain that would solve everyones
> problems, and not be too bad to audit either.
Ah, hmm, that sounds like it'll work fine at least when no LSMs are involved.
SELinux normally doesn't do the execution-degrading thing, it just
blocks the execution completely - see their selinux_bprm_set_creds()
hook. So I think they'd still need to set some state on the task that
says "we're currently in the middle of an execution where the target
task will run in context X", and then check against that in the
ptrace_may_access hook. Or I suppose they could just kill the task
near the end of execve, although that'd be kinda ugly.
^ permalink raw reply
* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-03-02 15:11 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com, Dmitry Kasatkin
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1583160394.8544.89.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, March 2, 2020 3:47 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin
> <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
>
> > > > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > My initial patch attempted to use any common TPM and kernel hash
> > > algorithm to calculate the boot_aggregate. The discussion with James
> > > was pretty clear, which you even stated in the Changelog. Either we
> > > use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for
> TPM
> > > 1.2 for the boot-aggregate.
> >
> > Ok, I didn't understand fully. I thought we should use the default IMA
> > algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
> > PCR bank for default algorithm.
>
> Yes, preference is given to the IMA default algorithm, but it should
> fall back to using SHA256 or SHA1, based on the TPM.
Ok. The patch already does it even if the TPM version is not checked.
For TPM 1.2, if the default algorithm is not SHA1 the patch will select
the first PCR bank (SHA1).
Should I send a new patch which explicitly checks the TPM version?
> > I additionally implemented the logic to
> > select the first PCR bank if the SHA256 PCR bank is not available but I can
> > remove it.
> >
> > SHA256 should be the minimum requirement for boot aggregate. The
> > advantage of using the default IMA algorithm is that it will be possible to
> > select stronger algorithms when they are supported by the TPM. We
> might
> > introduce a new option to specify only the algorithm for boot aggregate,
> > like James suggested to support embedded systems. Let me know which
> > option you prefer.
>
> I don't remember James saying that, but if the community really wants
> that support, then it should be upstreamed independently, as a
> separate patch. Let's first get the basics working.
Ok.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ 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