Re: [RFC PATCH bpf-next 1/2] bpf: BTF support for __ksym externs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Andrii,

Thanks for taking a look at this. You comments are clear, I will fix them in v2.

> Also, in the next version, please split kernel part and libbpf part
> into separate patches.
>

Got it. Will do.

> I don't think that's the right approach. It can't be the best effort.
> It's actually pretty clear when a user wants a BTF-based variable with
> ability to do direct memory access vs __ksym address that we have
> right now: variable type info. In your patch you are only looking up
> variable by name, but it needs to be more elaborate logic:
>
> 1. if variable type is `extern void` -- do what we do today (no BTF required)
> 2. if the variable type is anything but `extern void`, then find that
> variable in BTF. If no BTF or variable is not found -- hard error with
> detailed enough message about what we expected to find in kernel BTF.
> 3. If such a variable is found in the kernel, then might be a good
> idea to additionally check type compatibility (e.g., struct/union
> should match struct/union, int should match int, typedefs should get
> resolved to underlying type, etc). I don't think deep comparison of
> structs is right, though, due to CO-RE, so just high-level
> compatibility checks to prevent the most obvious mistakes.
>

Ack.

> >
> > Also note since we need to carry the ksym's address (64bits) as well as
> > its btf_id (32bits), pseudo_btf_id uses ld_imm64's both imm and off
> > fields.
>
> For BTF-enabled ksyms, libbpf doesn't need to provide symbol address,
> kernel will find it and substitute it, so BTF ID is the only
> parameter. Thus it can just go into the imm field (and simplify
> ldimm64 validation logic a bit).
>

Ack.

> >  /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative
> >   * offset to another bpf function
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3c1efc9d08fd..3c925957b9b6 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -7131,15 +7131,29 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >                 verbose(env, "invalid BPF_LD_IMM insn\n");
> >                 return -EINVAL;
> >         }
> > +       err = check_reg_arg(env, insn->dst_reg, DST_OP);
> > +       if (err)
> > +               return err;
> > +
> > +       /*
> > +        * BPF_PSEUDO_BTF_ID insn's off fields carry the ksym's btf_id, so its
> > +        * handling has to come before the reserved field check.
> > +        */
> > +       if (insn->src_reg == BPF_PSEUDO_BTF_ID) {
> > +               u32 id = ((u32)(insn + 1)->off << 16) | (u32)insn->off;
> > +               const struct btf_type *t = btf_type_by_id(btf_vmlinux, id);
> > +
>
> This is the kernel, we should be paranoid and assume the hackers want
> to do bad things. So check t for NULL. Check that it's actually a
> BTF_KIND_VAR. Check the name, find ksym addr, etc.
>

Ack.



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux