Messages in this thread | | | Subject | Re: [PoC][PATCH] bpf: Call return value check function in the JITed code | From | Roberto Sassu <> | Date | Wed, 30 Nov 2022 14:52:38 +0100 |
| |
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@huaweicloud.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@huaweicloud.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
| |