From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
Jiri Olsa <jolsa@kernel.org>,
Florent Revest <revest@chromium.org>,
Brendan Jackman <jackmanb@chromium.org>,
Mykola Lysenko <mykolal@fb.com>, Paul Moore <paul@paul-moore.com>,
James Morris <jmorris@namei.org>,
"Serge E . Hallyn" <serge@hallyn.com>,
Shuah Khan <shuah@kernel.org>, bpf <bpf@vger.kernel.org>,
LSM List <linux-security-module@vger.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Roberto Sassu <roberto.sassu@huawei.com>
Subject: Re: [RFC][PATCH v2 2/7] bpf: Mark ALU32 operations in bpf_reg_state structure
Date: Mon, 12 Dec 2022 09:04:49 -0800 [thread overview]
Message-ID: <CAADnVQJedxOdWnDfLcK-o5SiKK-2Qkw-b=-kiP_8rryF3MgtpQ@mail.gmail.com> (raw)
In-Reply-To: <17749b60bcffdc05ce0343199c14ef3cf2d54010.camel@huaweicloud.com>
On Mon, Dec 12, 2022 at 4:45 AM Roberto Sassu
<roberto.sassu@huaweicloud.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@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.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.
> 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.
next prev parent reply other threads:[~2022-12-12 17:05 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-07 17:24 [RFC][PATCH v2 0/7] bpf-lsm: Check return values of security modules Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 1/7] bpf: Remove superfluous btf_id_set_contains() declaration Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 2/7] bpf: Mark ALU32 operations in bpf_reg_state structure Roberto Sassu
2022-12-11 2:28 ` Alexei Starovoitov
2022-12-12 12:44 ` Roberto Sassu
2022-12-12 17:04 ` Alexei Starovoitov [this message]
2022-12-12 18:10 ` Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 3/7] lsm: Redefine LSM_HOOK() macro to add return value flags as argument Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 4/7] bpf-lsm: Enforce return value limitations on security modules Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 5/7] selftests/bpf: Check if return values of LSM programs are allowed Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 6/7] selftests/bpf: Prevent positive ret values in test_lsm and verify_pkcs7_sig Roberto Sassu
2022-12-07 17:24 ` [RFC][PATCH v2 7/7] selftests/bpf: Change return value in test_libbpf_get_fd_by_id_opts.c Roberto Sassu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAADnVQJedxOdWnDfLcK-o5SiKK-2Qkw-b=-kiP_8rryF3MgtpQ@mail.gmail.com' \
--to=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=jackmanb@chromium.org \
--cc=jmorris@namei.org \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=mykolal@fb.com \
--cc=paul@paul-moore.com \
--cc=revest@chromium.org \
--cc=roberto.sassu@huawei.com \
--cc=roberto.sassu@huaweicloud.com \
--cc=sdf@google.com \
--cc=serge@hallyn.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).