Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Andrii Nakryiko]

On Tue, Jun 6, 2023 at 8:46 PM Maninder Singh <maninder1.s@xxxxxxxxxxx> 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