On Mon, Dec 12, 2022 at 4:45 AM Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote: > > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > > > > > 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.
