Message-ID: <379ee6c3-9de1-f552-406d-11fd8216c96b@iogearbox.net>
Date:   Fri, 24 Jan 2020 22:20:13 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Martin Lau <kafai@...com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [Potential Spoof] [PATCH bpf-next] libbpf: improve handling of
 failed CO-RE relocations

On 1/24/20 9:12 PM, Martin Lau wrote:
> On Fri, Jan 24, 2020 at 10:30:12AM -0800, Andrii Nakryiko wrote:
>> On Thu, Jan 23, 2020 at 11:21 PM Martin Lau <kafai@...com> wrote:
>>> On Thu, Jan 23, 2020 at 09:38:37PM -0800, Andrii Nakryiko wrote:
>>>> Previously, if libbpf failed to resolve CO-RE relocation for some
>>>> instructions, it would either return error immediately, or, if
>>>> .relaxed_core_relocs option was set, would replace relocatable offset/imm part
>>>> of an instruction with a bogus value (-1). Neither approach is good, because
>>>> there are many possible scenarios where relocation is expected to fail (e.g.,
>>>> when some field knowingly can be missing on specific kernel versions). On the
>>>> other hand, replacing offset with invalid one can hide programmer errors, if
>>>> this relocation failue wasn't anticipated.
>>>>
>>>> This patch deprecates .relaxed_core_relocs option and changes the approach to
>>>> always replacing instruction, for which relocation failed, with invalid BPF
>>>> helper call instruction. For cases where this is expected, BPF program should
>>>> already ensure that that instruction is unreachable, in which case this
>>>> invalid instruction is going to be silently ignored. But if instruction wasn't
>>>> guarded, BPF program will be rejected at verification step with verifier log
>>>> pointing precisely to the place in assembly where the problem is.
>>>>
>>>> Signed-off-by: Andrii Nakryiko <andriin@...com>
>>>> ---
>>>>   tools/lib/bpf/libbpf.c | 95 +++++++++++++++++++++++++-----------------
>>>>   tools/lib/bpf/libbpf.h |  6 ++-
>>>>   2 files changed, 61 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index ae34b681ae82..39f1b7633a7c 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -345,7 +345,6 @@ struct bpf_object {
>>>>
>>>>        bool loaded;
>>>>        bool has_pseudo_calls;
>>>> -     bool relaxed_core_relocs;
>>>>
>>>>        /*
>>>>         * Information when doing elf related work. Only valid if fd
>>>> @@ -4238,25 +4237,38 @@ static int bpf_core_calc_field_relo(const struct bpf_program *prog,
>>>>    */
>>>>   static int bpf_core_reloc_insn(struct bpf_program *prog,
>>>>                               const struct bpf_field_reloc *relo,
>>>> +                            int relo_idx,
>>>>                               const struct bpf_core_spec *local_spec,
>>>>                               const struct bpf_core_spec *targ_spec)
>>>>   {
>>>> -     bool failed = false, validate = true;
>>>>        __u32 orig_val, new_val;
>>>>        struct bpf_insn *insn;
>>>> +     bool validate = true;
>>>>        int insn_idx, err;
>>>>        __u8 class;
>>>>
>>>>        if (relo->insn_off % sizeof(struct bpf_insn))
>>>>                return -EINVAL;
>>>>        insn_idx = relo->insn_off / sizeof(struct bpf_insn);
>>>> +     insn = &prog->insns[insn_idx];
>>>> +     class = BPF_CLASS(insn->code);
>>>>
>>>>        if (relo->kind == BPF_FIELD_EXISTS) {
>>>>                orig_val = 1; /* can't generate EXISTS relo w/o local field */
>>>>                new_val = targ_spec ? 1 : 0;
>>>>        } else if (!targ_spec) {
>>>> -             failed = true;
>>>> -             new_val = (__u32)-1;
>>>> +             pr_debug("prog '%s': relo #%d: substituting insn #%d w/ invalid insn\n",
>>>> +                      bpf_program__title(prog, false), relo_idx, insn_idx);
>>>> +             insn->code = BPF_JMP | BPF_CALL;
>>>> +             insn->dst_reg = 0;
>>>> +             insn->src_reg = 0;
>>>> +             insn->off = 0;
>>>> +             /* if this instruction is reachable (not a dead code),
>>>> +              * verifier will complain with the following message:
>>>> +              * invalid func unknown#195896080
>>>> +              */
>>>> +             insn->imm = 195896080; /* => 0xbad2310 => "bad relo" */
>>> Should this value become a binded contract in uapi/bpf.h so
>>> that the verifier can print a more meaningful name than "unknown#195896080"?
>>
>> It feels a bit premature to fix this in kernel. It's one of many ways
>> we can do this, e.g., alternative would be using invalid opcode
>> altogether. It's not yet clear what's the best way to report this from
>> kernel. Maybe in the future verifier will have some better way to
>> pinpoint where and what problem there is in user's program through
>> some more structured approach than current free-form log.
>>
>> So what I'm trying to say is that we should probably get a bit more
>> experience using these features first and understand what
>> kernel/userspace interface should be for reporting issues like this,
>> before setting anything in stone in verifier. For now, this
>> "unknown#195896080" should be a pretty unique search term :)
> Sure.  I think this value will never be used for real in the life time.
> I was mostly worry this message will be confusing.  May be the loader
> could be improved to catch this and interpret it in a more meaningful
> way.

Agree with both of you that we might want to find a better error reporting
mechanism here in future, but can be done on top of this. Applied, thanks!

Powered by blists - more mailing lists