lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <30f658418386dd55aef5d109a52b7a32c4678648.camel@huaweicloud.com>
Date:   Wed, 30 Nov 2022 14:52:38 +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>,
        Paul Moore <paul@...l-moore.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>, bpf <bpf@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PoC][PATCH] bpf: Call return value check function in the JITed
 code

On Wed, 2022-11-16 at 09:55 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> <roberto.sassu@...weicloud.com> wrote:
> > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> > > <roberto.sassu@...weicloud.com> wrote:
> > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > > > +{
> > > > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > > > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > > > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > > > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > > > +               return false;
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > > >   * function where a BPF program can be attached.
> > > >   */
> > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> > > >  #include <linux/lsm_hook_defs.h>
> > > >  #undef LSM_HOOK
> > > > 
> > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > > > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > > > +{                                              \
> > > > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > > > +}
> > > > +
> > > > +#include <linux/lsm_hook_defs.h>
> > > > +#undef LSM_HOOK
> > > > +
> > > 
> > > because lsm hooks is mess of undocumented return values your
> > > "solution" is to add hundreds of noninline functions
> > > and hack the call into them in JITs ?!
> > 
> > I revisited the documentation and checked each LSM hook one by one.
> > Hopefully, I completed it correctly, but I would review again (others
> > are also welcome to do it).
> > 
> > Not sure if there is a more efficient way. Do you have any idea?
> > Maybe we find a way to use only one check function (by reusing the
> > address of the attachment point?).
> > 
> > Regarding the JIT approach, I didn't find a reliable solution for using
> > just the verifier. As I wrote to you, there could be the case where the
> > range can include positive values, despite the possible return values
> > are zero and -EACCES.
> 
> Didn't you find that there are only 12 or so odd return cases.
> Maybe refactor some of them to something that the verifier can enforce
> and denylist the rest ?

Ok, went back to trying to enforce the return value on the verifier
side, assuming that for now we consider hooks that return zero or a
negative value.

I wanted to see if at least we are able to enforce that.

The biggest problem is which value of the register I should use, the 64
bit one or the 32 bit one?

We can have a look at test_libbpf_get_fd_by_id_opts. The default flavor
gives:

0000000000000000 <check_access>:
       0:	b4 00 00 00 00 00 00 00	w0 = 0
       1:	79 12 00 00 00 00 00 00	r2 = *(u64 *)(r1 + 0)
       2:	18 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00	r3 = 0 ll
       4:	5d 32 05 00 00 00 00 00	if r2 != r3 goto +5 <LBB0_3>
       5:	79 11 08 00 00 00 00 00	r1 = *(u64 *)(r1 + 8)
       6:	57 01 00 00 02 00 00 00	r1 &= 2
       7:	b4 00 00 00 00 00 00 00	w0 = 0
       8:	15 01 01 00 00 00 00 00	if r1 == 0 goto +1 <LBB0_3>
       9:	b4 00 00 00 f3 ff ff ff	w0 = -13

smin_value = 0xfffffff3, smax_value = 0xfffffff3,
s32_min_value = 0xfffffff3, s32_max_value = 0xfffffff3,

I think it is because of this, in check_alu_op():

if (BPF_CLASS(insn->code) == BPF_ALU64) {
	__mark_reg_known(regs + insn->dst_reg,
			 insn->imm);
} else {
	__mark_reg_known(regs + insn->dst_reg,
			 (u32)insn->imm);
	}
}

So, here you have to use the 32 bit values. But, if you use the
no_alu32 flavor:

0000000000000000 <check_access>:
       0:	b7 00 00 00 00 00 00 00	r0 = 0
       1:	79 12 00 00 00 00 00 00	r2 = *(u64 *)(r1 + 0)
       2:	18 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00	r3 = 0 ll
       4:	5d 32 04 00 00 00 00 00	if r2 != r3 goto +4 <LBB0_2>
       5:	79 10 08 00 00 00 00 00	r0 = *(u64 *)(r1 + 8)
       6:	67 00 00 00 3e 00 00 00	r0 <<= 62
       7:	c7 00 00 00 3f 00 00 00	r0 s>>= 63

smin_value = 0xffffffffffffffff, smax_value = 0x0,
s32_min_value = 0x80000000, s32_max_value = 0x7fffffff,

       8:	57 00 00 00 f3 ff ff ff	r0 &= -13

smin_value = 0xfffffffffffffff3, smax_value = 0x7fffffffffffffff,
s32_min_value = 0x80000000, s32_max_value = 0x7ffffff3,

I would have hoped to see:

smin_value = 0xfffffffffffffff3, smax_value = 0x0,

but it doesn't because of this, in scalar_min_max_and():

if (dst_reg->smin_value < 0 || smin_val < 0) {
	/* Lose signed bounds when ANDing negative numbers,
	 * ain't nobody got time for that.
	 */
	dst_reg->smin_value = S64_MIN;
	dst_reg->smax_value = S64_MAX;

Could we do an AND, if src_reg is known?

And what would be the right register value to use?

Thanks

Roberto

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ