[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 12 Dec 2022 19:10:01 +0100
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...gle.com>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
Florent Revest <revest@...omium.org>,
Brendan Jackman <jackmanb@...omium.org>,
Mykola Lysenko <mykolal@...com>,
Paul Moore <paul@...l-moore.com>,
James Morris <jmorris@...ei.org>,
"Serge E . Hallyn" <serge@...lyn.com>,
Shuah Khan <shuah@...nel.org>, bpf <bpf@...r.kernel.org>,
LSM List <linux-security-module@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [RFC][PATCH v2 2/7] bpf: Mark ALU32 operations in bpf_reg_state
structure
On Mon, 2022-12-12 at 09:04 -0800, Alexei Starovoitov wrote:
> On Mon, Dec 12, 2022 at 4:45 AM Roberto Sassu
> <roberto.sassu@...weicloud.com> wrote:
> > On Sat, 2022-12-10 at 18:28 -0800, Alexei Starovoitov wrote:
> > > On Wed, Dec 7, 2022 at 9:25 AM Roberto Sassu
> > > <roberto.sassu@...weicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@...wei.com>
> > > >
> > > > 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
Powered by blists - more mailing lists