On Thu, Aug 17, 2023 at 11:31 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Aug 16, 2023 at 7:31 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> >
> > On Thu, Aug 17, 2023 at 9:54 AM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Mon, Aug 14, 2023 at 7:46 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, Aug 15, 2023 at 8:28 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 8/14/23 7:33 AM, Yafang Shao wrote:
> > > > > > Add a new bpf_current_capable kfunc to check whether the current task
> > > > > > has a specific capability. In our use case, we will use it in a lsm bpf
> > > > > > program to help identify if the user operation is permitted.
> > > > > >
> > > > > > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > > > > > ---
> > > > > > kernel/bpf/helpers.c | 6 ++++++
> > > > > > 1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > > > index eb91cae..bbee7ea 100644
> > > > > > --- a/kernel/bpf/helpers.c
> > > > > > +++ b/kernel/bpf/helpers.c
> > > > > > @@ -2429,6 +2429,11 @@ __bpf_kfunc void bpf_rcu_read_unlock(void)
> > > > > > rcu_read_unlock();
> > > > > > }
> > > > > >
> > > > > > +__bpf_kfunc bool bpf_current_capable(int cap)
> > > > > > +{
> > > > > > + return has_capability(current, cap);
> > > > > > +}
> > > > >
> > > > > Since you are testing against 'current' capabilities, I assume
> > > > > that the context should be process. Otherwise, you are testing
> > > > > against random task which does not make much sense.
> > > >
> > > > It is in the process context.
> > > >
> > > > >
> > > > > Since you are testing against 'current' cap, and if the capability
> > > > > for that task is stable, you do not need this kfunc.
> > > > > You can test cap in user space and pass it into the bpf program.
> > > > >
> > > > > But if the cap for your process may change in the middle of
> > > > > run, then you indeed need bpf prog to test capability in real time.
> > > > > Is this your use case and could you describe in in more detail?
> > > >
> > > > After we convert the capability of our networking bpf program from
> > > > CAP_SYS_ADMIN to CAP_BPF+CAP_NET_ADMIN to enhance the security, we
> > > > encountered the "pointer comparison prohibited" error, because
> > > > allow_ptr_leaks is enabled only when CAP_PERFMON is set. However, if
> > > > we enable the CAP_PERFMON for the networking bpf program, it can run
> > > > tracing bpf prog, perf_event bpf prog and etc, that is not expected by
> > > > us.
> > > >
> > > > Hence we are planning to use a lsm bpf program to disallow it from
> > > > running other bpf programs. In our lsm bpf program we will check the
> > > > capability of processes, if the process has cap_net_admin, cap_bpf and
> > > > cap_perfmon but don't have cap_sys_admin we will refuse it to run
> > > > tracing and perf_event bpf program. While if a process has cap_bpf
> > > > and cap_perfmon but doesn't have cap_net_admin, that said it is a bpf
> > > > program which wants to run trace bpf, we will allow it.
> > > >
> > > > We can't use lsm_cgroup because it is supported on cgroup2 only, while
> > > > we're still using cgroup1.
> > > >
> > > > Another possible solution is enable allow_ptr_leaks for cap_net_admin
> > > > as well, but after I checked the commit which introduces the cap_bpf
> > > > and cap_perfmon [1], I think we wouldn't like to do it.
> > >
> > > Sorry. None of these options are acceptable.
> > >
> > > The idea of introducing a bpf_current_capable() kfunc just to work
> > > around a deficiency in the verifier is not sound.
> >
> > So what should we do then?
> > Just enabling the cap_perfmon for it? That does not sound as well ...
>
> Yonghong already pointed out upthread that
> comparison of two packet pointers is not a pointer leak.
> See this code:
> } else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg],
> this_branch, other_branch) &&
> is_pointer_value(env, insn->dst_reg)) {
> verbose(env, "R%d pointer comparison prohibited\n",
> insn->dst_reg);
> return -EACCES;
> }
>
> It's not clear why it doesn't address your case.
It can address the issue.
It seems we should do the code change below.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0b9da95..c66dc61 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13819,6 +13819,18 @@ static int check_cond_jmp_op(struct
bpf_verifier_env *env,
return -EINVAL;
}
+ other_branch = push_stack(env, *insn_idx + insn->off + 1, *insn_idx,
+ false);
+ if (!other_branch)
+ return -EFAULT;
+
+ /* check src2 operand */
+ err = check_reg_arg(env, insn->dst_reg, SRC_OP);
+ if (err)
+ return err;
+
+ dst_reg = ®s[insn->dst_reg];
+
if (BPF_SRC(insn->code) == BPF_X) {
if (insn->imm != 0) {
verbose(env, "BPF_JMP/JMP32 uses reserved fields\n");
@@ -13830,7 +13842,9 @@ static int check_cond_jmp_op(struct
bpf_verifier_env *env,
if (err)
return err;
- if (is_pointer_value(env, insn->src_reg)) {
+ if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg],
+ this_branch, other_branch) &&
+ is_pointer_value(env, insn->src_reg)) {
verbose(env, "R%d pointer comparison prohibited\n",
insn->src_reg);
return -EACCES;
@@ -13843,12 +13857,6 @@ static int check_cond_jmp_op(struct
bpf_verifier_env *env,
}
}
- /* check src2 operand */
- err = check_reg_arg(env, insn->dst_reg, SRC_OP);
- if (err)
- return err;
-
- dst_reg = ®s[insn->dst_reg];
is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32;
if (BPF_SRC(insn->code) == BPF_K) {
@@ -13920,10 +13928,6 @@ static int check_cond_jmp_op(struct
bpf_verifier_env *env,
return 0;
}
- other_branch = push_stack(env, *insn_idx + insn->off + 1, *insn_idx,
- false);
- if (!other_branch)
- return -EFAULT;
other_branch_regs = other_branch->frame[other_branch->curframe]->regs;
/* detect if we are comparing against a constant value so we can adjust
--
Regards
Yafang
