From: Roberto Sassu <[email protected]>
Notes:
- This patch set addresses the kernel panic described below, and not the
more broad issue of accessing kernel objects whose pointer is passed
as parameter by LSM hooks
- Alternative approaches trying to limit return values at run-time either
in the security subsystem or in the eBPF JIT are not preferred by the
respective maintainers
- Although all eBPF selftests have been verified to pass, it still might
be cumbersome to have an eBPF program being accepted by the eBPF
verifier (e.g. ANDing negative numbers causes existing bounds to be lost)
- The patch to store whether a register state changed due to an ALU64 or an
ALU32 operation might not be correct/complete, a review by eBPF
maintainers would be needed
- This patch set requires "lsm: make security_socket_getpeersec_stream()
sockptr_t safe", in lsm/next
- The modification of the LSM infrastructure to define allowed return
values for the LSM hooks could be replaced with an eBPF-only fix, with
the drawback of having to update the information manually each time a
new hook is added; allowing zero or negative values by default could be
reasonable, but there are already exceptions of LSM hooks accepting 0 or
1 (ismaclabel)
- The patches to fix the LSM infrastructure documentation are separated
from this patch set and available here:
https://lore.kernel.org/linux-security-module/[email protected]/
BPF LSM defines attachment points to allows security modules (eBPF programs
with type LSM) to provide their implementation of the desired LSM hooks.
Unfortunately, BPF LSM does not restrict which values security modules can
return (for non-void LSM hooks). If they put arbitrary values instead of
those stated in include/linux/lsm_hooks.h, they could cause big troubles.
For example, this simple eBPF program:
SEC("lsm/inode_permission")
int BPF_PROG(test_int_hook, struct inode *inode, int mask)
{
return 1;
}
causes the following kernel panic:
[ 181.130807] BUG: kernel NULL pointer dereference, address: 0000000000000079
[ 181.131478] #PF: supervisor read access in kernel mode
[ 181.131942] #PF: error_code(0x0000) - not-present page
[ 181.132407] PGD 0 P4D 0
[ 181.132650] Oops: 0000 [#1] PREEMPT SMP NOPTI
[ 181.133054] CPU: 5 PID: 857 Comm: systemd-oomd Tainted: G OE 6.1.0-rc7+ #530
[ 181.133806] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 181.134601] RIP: 0010:do_sys_openat2+0x235/0x300
[...]
[ 181.136682] RSP: 0018:ffffc90001557ee0 EFLAGS: 00010203
[ 181.137154] RAX: 0000000000000001 RBX: ffffc90001557f20 RCX: ffff888112003380
[ 181.137790] RDX: 0000000000000000 RSI: ffffffff8280b026 RDI: ffffc90001557e28
[ 181.138432] RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000000
[ 181.139081] R10: ffffffff835097dc R11: 0000000000000000 R12: ffff888106118000
[ 181.139717] R13: 000000000000000c R14: 0000000000000000 R15: 0000000000000000
[ 181.140149] FS: 00007fa6ceb0bb40(0000) GS:ffff88846fb40000(0000) knlGS:0000000000000000
[ 181.140556] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 181.140865] CR2: 0000000000000079 CR3: 0000000135c50000 CR4: 0000000000350ee0
[ 181.141239] Call Trace:
[ 181.141373] <TASK>
[ 181.141495] do_sys_open+0x34/0x60
[ 181.141678] do_syscall_64+0x3b/0x90
[ 181.141875] entry_SYSCALL_64_after_hwframe+0x63/0xcd
Avoid this situation by statically analyzing the eBPF programs attaching to
LSM hooks, and ensure that their return values are compatible with the LSM
infrastructure conventions.
First, add a preliminary patch (patch 1) to fix a small code duplication
issue.
Extend the eBPF verifier to let BPF LSM determine whether it should check
estimated 64 bit values or the 32 bit ones (patch 2). Also, extend the LSM
infrastructure to record more precisely the allowed return values depending
on the documentation found in include/linux/lsm_hooks.h (patch 3). Add the
LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE, LSM_RET_GT_ONE flags to an LSM hook
if that hook allows respectively > 0, 0, 1, > 1 return values.
Then, extend BPF LSM to verify that return values, estimated by the
verifier by analyzing the eBPF program, fall in the allowed intervals found
from the return value flags of the LSM hook being attached to (patch 4).
Finally, add new tests to ensure that the verifier enforces return values
correctly (patch 5), and slightly modify existing tests to make them follow
the LSM infrastructure conventions (patches 6-7) and are accepted by the
verifier.
Changelog:
v1:
- Complete the documentation of return values in lsm_hooks.h
- Introduce return value flags in the LSM infrastructure
- Use those flags instead of the scattered logic (suggested by KP)
- Expose a single verification function to the verifier (suggested by KP)
- Add new patch to remove duplicated function definition
- Add new patch to let BPF LSM determine the appropriate register values
to use
Roberto Sassu (7):
bpf: Remove superfluous btf_id_set_contains() declaration
bpf: Mark ALU32 operations in bpf_reg_state structure
lsm: Redefine LSM_HOOK() macro to add return value flags as argument
bpf-lsm: Enforce return value limitations on security modules
selftests/bpf: Check if return values of LSM programs are allowed
selftests/bpf: Prevent positive ret values in test_lsm and
verify_pkcs7_sig
selftests/bpf: Change return value in test_libbpf_get_fd_by_id_opts.c
include/linux/bpf.h | 1 -
include/linux/bpf_lsm.h | 11 +-
include/linux/bpf_verifier.h | 1 +
include/linux/lsm_hook_defs.h | 780 ++++++++++--------
include/linux/lsm_hooks.h | 9 +-
kernel/bpf/bpf_lsm.c | 81 +-
kernel/bpf/verifier.c | 17 +-
security/bpf/hooks.c | 2 +-
security/security.c | 4 +-
tools/testing/selftests/bpf/progs/lsm.c | 4 +
.../bpf/progs/test_libbpf_get_fd_by_id_opts.c | 7 +-
.../bpf/progs/test_verify_pkcs7_sig.c | 11 +-
.../testing/selftests/bpf/verifier/lsm_ret.c | 148 ++++
13 files changed, 729 insertions(+), 347 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/lsm_ret.c
--
2.25.1
From: Roberto Sassu <[email protected]>
BPF LSM needs a reliable source of information to determine if the return
value given by eBPF programs is acceptable or not. At the moment, choosing
either the 64 bit or the 32 bit one does not seem to be an option
(selftests fail).
If we choose the 64 bit one, the following happens.
14: 61 10 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
15: 74 00 00 00 15 00 00 00 w0 >>= 21
16: 54 00 00 00 01 00 00 00 w0 &= 1
17: 04 00 00 00 ff ff ff ff w0 += -1
This is the last part of test_deny_namespace. After #16, the register
values are:
smin_value = 0x0, smax_value = 0x1,
s32_min_value = 0x0, s32_max_value = 0x1,
After #17, they become:
smin_value = 0x0, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0x0
where only the 32 bit values are correct.
If we choose the 32 bit ones, the following happens.
0000000000000000 <check_access>:
0: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
1: 79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8)
2: 67 00 00 00 3e 00 00 00 r0 <<= 62
3: c7 00 00 00 3f 00 00 00 r0 s>>= 63
This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
case, 64 bit register values should be used (for the 32 bit ones, there is
no precise information from the verifier).
As the examples above suggest that which register values to use depends on
the specific case, mark ALU32 operations in bpf_reg_state structure, so
that BPF LSM can choose the proper ones.
Signed-off-by: Roberto Sassu <[email protected]>
---
include/linux/bpf_verifier.h | 1 +
kernel/bpf/verifier.c | 10 +++++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 70d06a99f0b8..29c9cf6b0d01 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -181,6 +181,7 @@ struct bpf_reg_state {
enum bpf_reg_liveness live;
/* if (!precise && SCALAR_VALUE) min/max/tnum don't affect safety */
bool precise;
+ bool alu32;
};
enum bpf_stack_slot_type {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8c5f0adbbde3..edce85c425a2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10524,9 +10524,13 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
break;
}
+ dst_reg->alu32 = false;
+
/* ALU32 ops are zero extended into 64bit register */
- if (alu32)
+ if (alu32) {
zext_32_to_64(dst_reg);
+ dst_reg->alu32 = true;
+ }
reg_bounds_sync(dst_reg);
return 0;
}
@@ -10700,6 +10704,7 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
*dst_reg = *src_reg;
dst_reg->live |= REG_LIVE_WRITTEN;
dst_reg->subreg_def = DEF_NOT_SUBREG;
+ dst_reg->alu32 = false;
} else {
/* R1 = (u32) R2 */
if (is_pointer_value(env, insn->src_reg)) {
@@ -10716,6 +10721,7 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
dst_reg->id = 0;
dst_reg->live |= REG_LIVE_WRITTEN;
dst_reg->subreg_def = env->insn_idx + 1;
+ dst_reg->alu32 = true;
} else {
mark_reg_unknown(env, regs,
insn->dst_reg);
@@ -10733,9 +10739,11 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
if (BPF_CLASS(insn->code) == BPF_ALU64) {
__mark_reg_known(regs + insn->dst_reg,
insn->imm);
+ regs[insn->dst_reg].alu32 = false;
} else {
__mark_reg_known(regs + insn->dst_reg,
(u32)insn->imm);
+ regs[insn->dst_reg].alu32 = true;
}
}
--
2.25.1
From: Roberto Sassu <[email protected]>
Ensure that the eBPF verifier allows to load only LSM programs that return
an allowed value depending on the LSM hook they attach to.
Signed-off-by: Roberto Sassu <[email protected]>
---
.../testing/selftests/bpf/verifier/lsm_ret.c | 148 ++++++++++++++++++
1 file changed, 148 insertions(+)
create mode 100644 tools/testing/selftests/bpf/verifier/lsm_ret.c
diff --git a/tools/testing/selftests/bpf/verifier/lsm_ret.c b/tools/testing/selftests/bpf/verifier/lsm_ret.c
new file mode 100644
index 000000000000..c9c9cee8e406
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/lsm_ret.c
@@ -0,0 +1,148 @@
+{
+ "lsm return value: positive not allowed, return -EPERM",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, -EPERM),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "inode_permission",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: positive not allowed, return zero",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "inode_permission",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: positive not allowed, return one",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "inode_permission",
+ .expected_attach_type = BPF_LSM_MAC,
+ .errstr = "Invalid R0, cannot return 1",
+ .result = REJECT,
+},
+{
+ "lsm return value: zero/positive not allowed, return -EPERM",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, -EPERM),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "inode_init_security",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: zero/positive not allowed, return zero",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "inode_init_security",
+ .expected_attach_type = BPF_LSM_MAC,
+ .errstr = "Invalid R0, cannot return 0",
+ .result = REJECT,
+},
+{
+ "lsm return value: zero/positive not allowed, return one",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "inode_init_security",
+ .expected_attach_type = BPF_LSM_MAC,
+ .errstr = "Invalid R0, cannot return 1",
+ .result = REJECT,
+},
+{
+ "lsm return value: positive allowed, return one",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "getprocattr",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: positive allowed, return two",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "getprocattr",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: only one allowed, return one",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "audit_rule_match",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: only one allowed, return two",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "audit_rule_match",
+ .expected_attach_type = BPF_LSM_MAC,
+ .errstr = "Invalid R0, cannot return > 1",
+ .result = REJECT,
+},
+{
+ "lsm return value: negative not allowed, return -EPERM",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, -EPERM),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "ismaclabel",
+ .expected_attach_type = BPF_LSM_MAC,
+ .errstr = "Invalid R0, cannot return < 0",
+ .result = REJECT,
+},
+{
+ "lsm return value: negative not allowed, return zero",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "ismaclabel",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
+{
+ "lsm return value: negative not allowed, return one",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_LSM,
+ .kfunc = "ismaclabel",
+ .expected_attach_type = BPF_LSM_MAC,
+ .result = ACCEPT,
+},
--
2.25.1
On Wed, Dec 7, 2022 at 9:25 AM Roberto Sassu
<[email protected]> wrote:
>
> From: Roberto Sassu <[email protected]>
>
> BPF LSM needs a reliable source of information to determine if the return
> value given by eBPF programs is acceptable or not. At the moment, choosing
> either the 64 bit or the 32 bit one does not seem to be an option
> (selftests fail).
>
> If we choose the 64 bit one, the following happens.
>
> 14: 61 10 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
> 15: 74 00 00 00 15 00 00 00 w0 >>= 21
> 16: 54 00 00 00 01 00 00 00 w0 &= 1
> 17: 04 00 00 00 ff ff ff ff w0 += -1
>
> This is the last part of test_deny_namespace. After #16, the register
> values are:
>
> smin_value = 0x0, smax_value = 0x1,
> s32_min_value = 0x0, s32_max_value = 0x1,
>
> After #17, they become:
>
> smin_value = 0x0, smax_value = 0xffffffff,
> s32_min_value = 0xffffffff, s32_max_value = 0x0
>
> where only the 32 bit values are correct.
>
> If we choose the 32 bit ones, the following happens.
>
> 0000000000000000 <check_access>:
> 0: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
> 1: 79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8)
> 2: 67 00 00 00 3e 00 00 00 r0 <<= 62
> 3: c7 00 00 00 3f 00 00 00 r0 s>>= 63
>
> This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
> case, 64 bit register values should be used (for the 32 bit ones, there is
> no precise information from the verifier).
>
> As the examples above suggest that which register values to use depends on
> the specific case, mark ALU32 operations in bpf_reg_state structure, so
> that BPF LSM can choose the proper ones.
I have a hard time understanding what is the problem you're
trying to solve and what is the proposed fix.
The patch is trying to remember the bitness of the last
operation, but what for?
The registers are 64-bit. There are 32-bit operations,
but they always update the upper 32-bits of the register.
reg_bounds_sync() updates 32 and 64 bit bounds regardless
whether the previous operation was on 32 or 64 bit.
It seems you're trying to hack around something that breaks
patch 3 which also looks fishy.
Please explain the problem first with a concrete example.
On Sat, 2022-12-10 at 18:28 -0800, Alexei Starovoitov wrote:
> On Wed, Dec 7, 2022 at 9:25 AM Roberto Sassu
> <[email protected]> wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > BPF LSM needs a reliable source of information to determine if the return
> > value given by eBPF programs is acceptable or not. At the moment, choosing
> > either the 64 bit or the 32 bit one does not seem to be an option
> > (selftests fail).
> >
> > If we choose the 64 bit one, the following happens.
> >
> > 14: 61 10 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
> > 15: 74 00 00 00 15 00 00 00 w0 >>= 21
> > 16: 54 00 00 00 01 00 00 00 w0 &= 1
> > 17: 04 00 00 00 ff ff ff ff w0 += -1
> >
> > This is the last part of test_deny_namespace. After #16, the register
> > values are:
> >
> > smin_value = 0x0, smax_value = 0x1,
> > s32_min_value = 0x0, s32_max_value = 0x1,
> >
> > After #17, they become:
> >
> > smin_value = 0x0, smax_value = 0xffffffff,
> > s32_min_value = 0xffffffff, s32_max_value = 0x0
> >
> > where only the 32 bit values are correct.
> >
> > If we choose the 32 bit ones, the following happens.
> >
> > 0000000000000000 <check_access>:
> > 0: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
> > 1: 79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8)
> > 2: 67 00 00 00 3e 00 00 00 r0 <<= 62
> > 3: c7 00 00 00 3f 00 00 00 r0 s>>= 63
> >
> > This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
> > case, 64 bit register values should be used (for the 32 bit ones, there is
> > no precise information from the verifier).
> >
> > As the examples above suggest that which register values to use depends on
> > the specific case, mark ALU32 operations in bpf_reg_state structure, so
> > that BPF LSM can choose the proper ones.
>
> I have a hard time understanding what is the problem you're
> trying to solve and what is the proposed fix.
The problem is allowing BPF LSM programs to return positive values when
LSM hooks expect zero or negative values. Those values could be
converted to a pointer, and escape the IS_ERR() check.
The challenge is to ensure that the verifier prediction of R0 is
accurate, so that the eBPF program is not unnecessarily rejected.
> The patch is trying to remember the bitness of the last
> operation, but what for?
> The registers are 64-bit. There are 32-bit operations,
> but they always update the upper 32-bits of the register.
> reg_bounds_sync() updates 32 and 64 bit bounds regardless
> whether the previous operation was on 32 or 64 bit.
Ok, yes. I also thought that using the 64 bit register should be ok,
but selftests fail.
Regarding your comment, I have not seen reg_bounds_sync() for the case
R = imm.
> It seems you're trying to hack around something that breaks
> patch 3 which also looks fishy.
I thought it was a good idea that changes in the LSM infrastructure are
automatically reflected in the boundaries that BPF LSM should enforce.
If not, I'm open to new ideas. If we should use BTF ID sets, I'm fine
with it.
> Please explain the problem first with a concrete example.
Ok, I have a simple one:
$ llvm-objdump -d test_bpf_cookie.bpf.o
0000000000000000 <test_int_hook>:
[...]
8: 85 00 00 00 0e 00 00 00 call 14
9: b4 06 00 00 ff ff ff ff w6 = -1
10: 5e 08 07 00 00 00 00 00 if w8 != w0 goto +7 <LBB11_3>
11: bf 71 00 00 00 00 00 00 r1 = r7
12: 85 00 00 00 ae 00 00 00 call 174
13: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
15: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
16: 4f 02 00 00 00 00 00 00 r2 |= r0
17: 7b 21 00 00 00 00 00 00 *(u64 *)(r1 + 0) = r2
smin_value = 0xffffffff, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
This is what I see at the time the BPF LSM check should be done.
How this should be properly handled?
Thanks
Roberto
On Mon, Dec 12, 2022 at 4:45 AM Roberto Sassu
<[email protected]> wrote:
>
> On Sat, 2022-12-10 at 18:28 -0800, Alexei Starovoitov wrote:
> > On Wed, Dec 7, 2022 at 9:25 AM Roberto Sassu
> > <[email protected]> wrote:
> > > From: Roberto Sassu <[email protected]>
> > >
> > > BPF LSM needs a reliable source of information to determine if the return
> > > value given by eBPF programs is acceptable or not. At the moment, choosing
> > > either the 64 bit or the 32 bit one does not seem to be an option
> > > (selftests fail).
> > >
> > > If we choose the 64 bit one, the following happens.
> > >
> > > 14: 61 10 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
> > > 15: 74 00 00 00 15 00 00 00 w0 >>= 21
> > > 16: 54 00 00 00 01 00 00 00 w0 &= 1
> > > 17: 04 00 00 00 ff ff ff ff w0 += -1
> > >
> > > This is the last part of test_deny_namespace. After #16, the register
> > > values are:
> > >
> > > smin_value = 0x0, smax_value = 0x1,
> > > s32_min_value = 0x0, s32_max_value = 0x1,
> > >
> > > After #17, they become:
> > >
> > > smin_value = 0x0, smax_value = 0xffffffff,
> > > s32_min_value = 0xffffffff, s32_max_value = 0x0
> > >
> > > where only the 32 bit values are correct.
> > >
> > > If we choose the 32 bit ones, the following happens.
> > >
> > > 0000000000000000 <check_access>:
> > > 0: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
> > > 1: 79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8)
> > > 2: 67 00 00 00 3e 00 00 00 r0 <<= 62
> > > 3: c7 00 00 00 3f 00 00 00 r0 s>>= 63
> > >
> > > This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
> > > case, 64 bit register values should be used (for the 32 bit ones, there is
> > > no precise information from the verifier).
> > >
> > > As the examples above suggest that which register values to use depends on
> > > the specific case, mark ALU32 operations in bpf_reg_state structure, so
> > > that BPF LSM can choose the proper ones.
> >
> > I have a hard time understanding what is the problem you're
> > trying to solve and what is the proposed fix.
>
> The problem is allowing BPF LSM programs to return positive values when
> LSM hooks expect zero or negative values. Those values could be
> converted to a pointer, and escape the IS_ERR() check.
The bigger goal is clear.
> The challenge is to ensure that the verifier prediction of R0 is
> accurate, so that the eBPF program is not unnecessarily rejected.
There is a code in the verifier already that checks ret values.
lsm restrictions should fit right in.
> > The patch is trying to remember the bitness of the last
> > operation, but what for?
> > The registers are 64-bit. There are 32-bit operations,
> > but they always update the upper 32-bits of the register.
> > reg_bounds_sync() updates 32 and 64 bit bounds regardless
> > whether the previous operation was on 32 or 64 bit.
>
> Ok, yes. I also thought that using the 64 bit register should be ok,
> but selftests fail.
maybe selftests are buggy?
they fail with patch 3 alone without patch 2 ?
please explain exactly the problem.
> Regarding your comment, I have not seen reg_bounds_sync() for the case
> R = imm.
because it's unnecessary there.
> > It seems you're trying to hack around something that breaks
> > patch 3 which also looks fishy.
>
> I thought it was a good idea that changes in the LSM infrastructure are
> automatically reflected in the boundaries that BPF LSM should enforce.
That's fine. Encoding restrictions in lsm_hook_defs.h
is the cleanest approach.
> If not, I'm open to new ideas. If we should use BTF ID sets, I'm fine
> with it.
>
> > Please explain the problem first with a concrete example.
>
> Ok, I have a simple one:
>
> $ llvm-objdump -d test_bpf_cookie.bpf.o
>
> 0000000000000000 <test_int_hook>:
>
> [...]
>
> 8: 85 00 00 00 0e 00 00 00 call 14
> 9: b4 06 00 00 ff ff ff ff w6 = -1
> 10: 5e 08 07 00 00 00 00 00 if w8 != w0 goto +7 <LBB11_3>
> 11: bf 71 00 00 00 00 00 00 r1 = r7
> 12: 85 00 00 00 ae 00 00 00 call 174
> 13: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
> 15: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
> 16: 4f 02 00 00 00 00 00 00 r2 |= r0
> 17: 7b 21 00 00 00 00 00 00 *(u64 *)(r1 + 0) = r2
>
> smin_value = 0xffffffff, smax_value = 0xffffffff,
> s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
and this applies where?
what reg are you talking about?
Where is the issue?
> This is what I see at the time the BPF LSM check should be done.
>
> How this should be properly handled?
The patch 3 should be fine alone. I don't see a need for patch 2 yet.
On Mon, 2022-12-12 at 09:04 -0800, Alexei Starovoitov wrote:
> On Mon, Dec 12, 2022 at 4:45 AM Roberto Sassu
> <[email protected]> wrote:
> > On Sat, 2022-12-10 at 18:28 -0800, Alexei Starovoitov wrote:
> > > On Wed, Dec 7, 2022 at 9:25 AM Roberto Sassu
> > > <[email protected]> wrote:
> > > > From: Roberto Sassu <[email protected]>
> > > >
> > > > BPF LSM needs a reliable source of information to determine if the return
> > > > value given by eBPF programs is acceptable or not. At the moment, choosing
> > > > either the 64 bit or the 32 bit one does not seem to be an option
> > > > (selftests fail).
> > > >
> > > > If we choose the 64 bit one, the following happens.
> > > >
> > > > 14: 61 10 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
> > > > 15: 74 00 00 00 15 00 00 00 w0 >>= 21
> > > > 16: 54 00 00 00 01 00 00 00 w0 &= 1
> > > > 17: 04 00 00 00 ff ff ff ff w0 += -1
> > > >
> > > > This is the last part of test_deny_namespace. After #16, the register
> > > > values are:
> > > >
> > > > smin_value = 0x0, smax_value = 0x1,
> > > > s32_min_value = 0x0, s32_max_value = 0x1,
> > > >
> > > > After #17, they become:
> > > >
> > > > smin_value = 0x0, smax_value = 0xffffffff,
> > > > s32_min_value = 0xffffffff, s32_max_value = 0x0
> > > >
> > > > where only the 32 bit values are correct.
> > > >
> > > > If we choose the 32 bit ones, the following happens.
> > > >
> > > > 0000000000000000 <check_access>:
> > > > 0: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
> > > > 1: 79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8)
> > > > 2: 67 00 00 00 3e 00 00 00 r0 <<= 62
> > > > 3: c7 00 00 00 3f 00 00 00 r0 s>>= 63
> > > >
> > > > This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
> > > > case, 64 bit register values should be used (for the 32 bit ones, there is
> > > > no precise information from the verifier).
> > > >
> > > > As the examples above suggest that which register values to use depends on
> > > > the specific case, mark ALU32 operations in bpf_reg_state structure, so
> > > > that BPF LSM can choose the proper ones.
> > >
> > > I have a hard time understanding what is the problem you're
> > > trying to solve and what is the proposed fix.
> >
> > The problem is allowing BPF LSM programs to return positive values when
> > LSM hooks expect zero or negative values. Those values could be
> > converted to a pointer, and escape the IS_ERR() check.
>
> The bigger goal is clear.
>
> > The challenge is to ensure that the verifier prediction of R0 is
> > accurate, so that the eBPF program is not unnecessarily rejected.
>
> There is a code in the verifier already that checks ret values.
> lsm restrictions should fit right in.
>
> > > The patch is trying to remember the bitness of the last
> > > operation, but what for?
> > > The registers are 64-bit. There are 32-bit operations,
> > > but they always update the upper 32-bits of the register.
> > > reg_bounds_sync() updates 32 and 64 bit bounds regardless
> > > whether the previous operation was on 32 or 64 bit.
> >
> > Ok, yes. I also thought that using the 64 bit register should be ok,
> > but selftests fail.
>
> maybe selftests are buggy?
> they fail with patch 3 alone without patch 2 ?
> please explain exactly the problem.
Ok, I let it run getting what the verifier provides (smin/smax).
smin_value = 0xffffffff, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
Invalid R0, cannot return > 1
#10 bpf_cookie:FAIL
smin_value = 0x0, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0x0,
Invalid R0, cannot return 1
#58/1 deny_namespace/unpriv_userns_create_no_bpf:FAIL
#58 deny_namespace:FAIL
smin_value = 0x0, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0x0,
Invalid R0, cannot return 1
#100 libbpf_get_fd_by_id_opts:FAIL
smin_value = 0xfffffffe, smax_value = 0xfffffffe,
s32_min_value = 0xfffffffe, s32_max_value = 0xfffffffe,
#114 lookup_key:FAIL
smin_value = 0xffffffff, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
Invalid R0, cannot return > 1
#210 test_ima:FAIL
smin_value = 0xffffffff, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
Invalid R0, cannot return > 1
#211 test_local_storage:FAIL
smin_value = 0xffffffff, smax_value = 0xffffffff,
s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
Invalid R0, cannot return > 1
#212 test_lsm:FAIL
As you can see, these tests fail because smin or smax are positive
values.
I kept the selftest patches. In test_lsm, for example, ret is a
parameter, populated by previous eBPF programs. In this case, I added
an additional check to explicitly reject positive values.
> > Regarding your comment, I have not seen reg_bounds_sync() for the case
> > R = imm.
>
> because it's unnecessary there.
__mark_reg_known(regs + insn->dst_reg,
(u32)insn->imm);
This prevents smin/smax from being negative. But I know that this was
patched by Jann Horn. Remembering the endianness of the operation,
makes it clear what register value you should use.
> > > It seems you're trying to hack around something that breaks
> > > patch 3 which also looks fishy.
> >
> > I thought it was a good idea that changes in the LSM infrastructure are
> > automatically reflected in the boundaries that BPF LSM should enforce.
>
> That's fine. Encoding restrictions in lsm_hook_defs.h
> is the cleanest approach.
>
> > If not, I'm open to new ideas. If we should use BTF ID sets, I'm fine
> > with it.
> >
> > > Please explain the problem first with a concrete example.
> >
> > Ok, I have a simple one:
> >
> > $ llvm-objdump -d test_bpf_cookie.bpf.o
> >
> > 0000000000000000 <test_int_hook>:
> >
> > [...]
> >
> > 8: 85 00 00 00 0e 00 00 00 call 14
> > 9: b4 06 00 00 ff ff ff ff w6 = -1
> > 10: 5e 08 07 00 00 00 00 00 if w8 != w0 goto +7 <LBB11_3>
> > 11: bf 71 00 00 00 00 00 00 r1 = r7
> > 12: 85 00 00 00 ae 00 00 00 call 174
> > 13: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0 ll
> > 15: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
> > 16: 4f 02 00 00 00 00 00 00 r2 |= r0
> > 17: 7b 21 00 00 00 00 00 00 *(u64 *)(r1 + 0) = r2
> >
> > smin_value = 0xffffffff, smax_value = 0xffffffff,
> > s32_min_value = 0xffffffff, s32_max_value = 0xffffffff,
>
> and this applies where?
This is in check_return_code(), for BPF_PROG_TYPE_LSM.
> what reg are you talking about?
R0.
> Where is the issue?
s32_min_value/s32_max_value are the values we should get.
Roberto