linux-kernel - Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4Bzb4B9FxMnf3t81D22FWkciLOvwDPLY0BbEPGGe7R5QPrg@mail.gmail.com>
Date:   Wed, 7 Jun 2023 15:19:22 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     maninder1.s@...sung.com
Cc:     "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "martin.lau@...ux.dev" <martin.lau@...ux.dev>,
        "song@...nel.org" <song@...nel.org>, "yhs@...com" <yhs@...com>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "sdf@...gle.com" <sdf@...gle.com>,
        "haoluo@...gle.com" <haoluo@...gle.com>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "thunder.leizhen@...wei.com" <thunder.leizhen@...wei.com>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "boqun.feng@...il.com" <boqun.feng@...il.com>,
        "vincenzopalazzodev@...il.com" <vincenzopalazzodev@...il.com>,
        "ojeda@...nel.org" <ojeda@...nel.org>,
        "jgross@...e.com" <jgross@...e.com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "michael.christie@...cle.com" <michael.christie@...cle.com>,
        "samitolvanen@...gle.com" <samitolvanen@...gle.com>,
        "glider@...gle.com" <glider@...gle.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "stephen.s.brennan@...cle.com" <stephen.s.brennan@...cle.com>,
        "alan.maguire@...cle.com" <alan.maguire@...cle.com>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Onkarnath <onkarnath.1@...sung.com>
Subject: Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

On Tue, Jun 6, 2023 at 8:46 PM Maninder Singh <maninder1.s@...sung.com> wrote:
>
> Hi Andrii Nakryiko,
>
> >>
> >> bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
> >> have a false definition for the !CONFIG_KALLSYMS case. But we'll
> >> soon expand on kallsyms_show_value() and so to make the code
> >> easier to follow just provide a direct !CONFIG_KALLSYMS definition
> >> for bpf_dump_raw_ok() as well.
> >
> > I'm sorry, I'm failing to follow the exact reasoning about
> > simplification. It seems simpler to have
> >
> > static inline bool kallsyms_show_value(const struct cred *cred)
> > {
> >     return false;
> > }
> >
> > and control it from kallsyms-related internal header, rather than
> > adding CONFIG_KALLSYMS ifdef-ery to include/linux/filter.h and
> > redefining that `return false` decision. What if in the future we
> > decide that if !CONFIG_KALLSYMS it's ok to show raw addresses, now
> > we'll have to remember to update it in two places.
> >
> > Unless I'm missing some other complications?
> >
>
> Patch 3/3 does the same, it extends functionality of kallsyms_show_value()
> in case of  !CONFIG_KALLSYMS.
>
> All other users likes modules code, kprobe codes are using this API
> for sanity/permission, and then prints the address like below:
>
> static int kprobe_blacklist_seq_show(struct seq_file *m, void *v)
> {
> ...
>         if (!kallsyms_show_value(m->file->f_cred))
>                 seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL,
>                            (void *)ent->start_addr);
>         else
>                 seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
>                            (void *)ent->end_addr, (void *)ent->start_addr);
> ..
> }
>
> so there will be no issues if we move kallsyms_show_value() out of KALLSYMS dependency.
> and these codes will work in case of !KALLSYMS also.
>
> but BPF code logic was complex and seems this API was used as checking for whether KALLSYMS is
> enabled or not as per comment in bpf_dump_raw_ok():
>
> /*
>  * Reconstruction of call-sites is dependent on kallsyms,
>  * thus make dump the same restriction.
>  */
>
> also as per below code:
> (we were not sure whether BPF will work or not with patch 3/3 because of no expertise on BPF code,
> so we keep the behaviour same)

I think bpf_dump_raw_ok() is purely about checking whether it's ok to
return unobfuscated kernel addresses to user/BPF program. So it feels
like it should be ok to just rely on kallsyms_show_value() and not
special case here. If some of the code relies on actually having
CONFIG_KALLSYMS and related functionality, it should be properly
guarded already (or should enforce `SELECT KALLSYMS` in Kconfig).

>
>        if (ulen) {
>                 if (bpf_dump_raw_ok(file->f_cred)) {
>                         unsigned long ksym_addr;
>                         u64 __user *user_ksyms;
>                         u32 i;
>
>                         /* copy the address of the kernel symbol
>                          * corresponding to each function
>                          */
>                         ulen = min_t(u32, info.nr_jited_ksyms, ulen);
>                         user_ksyms = u64_to_user_ptr(info.jited_ksyms);
>                         if (prog->aux->func_cnt) {
>                                 for (i = 0; i < ulen; i++) {
>    ...
>    }
>
> earlier conversation for this change:
> https://lkml.org/lkml/2022/4/13/326
>
> here Petr CC'ed BPF maintainers to know their opinion whether BPF code can work with patch 3/3,
> if not then we need this patch.
>
> Thanks,
> Maninder Singh