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

Commit Message

Maninder Singh June 6, 2023, 4:28 a.m. UTC

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.

Co-developed-by: Onkarnath <[e-mail]>
Signed-off-by: Onkarnath <[e-mail]>
Signed-off-by: Maninder Singh <[e-mail]>
Reviewed-by: Luis Chamberlain <[e-mail]>
---
 include/linux/filter.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Zhen Lei June 6, 2023, 11:26 a.m. UTC | #1

On 2023/6/6 12:28, Maninder Singh wrote:
> 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.

Reviewed-by: Zhen Lei <[e-mail]>

> 
> Co-developed-by: Onkarnath <[e-mail]>
> Signed-off-by: Onkarnath <[e-mail]>
> Signed-off-by: Maninder Singh <[e-mail]>
> Reviewed-by: Luis Chamberlain <[e-mail]>
> ---
>  include/linux/filter.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index bbce89937fde..1f237a3bb11a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -923,13 +923,21 @@ bool bpf_jit_supports_kfunc_call(void);
>  bool bpf_jit_supports_far_kfunc_call(void);
>  bool bpf_helper_changes_pkt_data(void *func);
>  
> +/*
> + * Reconstruction of call-sites is dependent on kallsyms,
> + * thus make dump the same restriction.
> + */
> +#ifdef CONFIG_KALLSYMS
>  static inline bool bpf_dump_raw_ok(const struct cred *cred)
>  {
> -	/* Reconstruction of call-sites is dependent on kallsyms,
> -	 * thus make dump the same restriction.
> -	 */
>  	return kallsyms_show_value(cred);
>  }
> +#else
> +static inline bool bpf_dump_raw_ok(const struct cred *cred)
> +{
> +	return false;
> +}
> +#endif
>  
>  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>  				       const struct bpf_insn *patch, u32 len);
>

Andrii Nakryiko June 6, 2023, 5:08 p.m. UTC | #2

On Mon, Jun 5, 2023 at 9:28 PM Maninder Singh <[e-mail]> wrote:
>
> 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?

>
> Co-developed-by: Onkarnath <[e-mail]>
> Signed-off-by: Onkarnath <[e-mail]>
> Signed-off-by: Maninder Singh <[e-mail]>
> Reviewed-by: Luis Chamberlain <[e-mail]>
> ---
>  include/linux/filter.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index bbce89937fde..1f237a3bb11a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -923,13 +923,21 @@ bool bpf_jit_supports_kfunc_call(void);
>  bool bpf_jit_supports_far_kfunc_call(void);
>  bool bpf_helper_changes_pkt_data(void *func);
>
> +/*
> + * Reconstruction of call-sites is dependent on kallsyms,
> + * thus make dump the same restriction.
> + */
> +#ifdef CONFIG_KALLSYMS
>  static inline bool bpf_dump_raw_ok(const struct cred *cred)
>  {
> -       /* Reconstruction of call-sites is dependent on kallsyms,
> -        * thus make dump the same restriction.
> -        */
>         return kallsyms_show_value(cred);
>  }
> +#else
> +static inline bool bpf_dump_raw_ok(const struct cred *cred)
> +{
> +       return false;
> +}
> +#endif
>
>  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>                                        const struct bpf_insn *patch, u32 len);
> --
> 2.17.1
>

Maninder Singh June 7, 2023, 3:40 a.m. UTC | #3

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)

       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

Andrii Nakryiko June 7, 2023, 10:19 p.m. UTC | #4

On Tue, Jun 6, 2023 at 8:46 PM Maninder Singh <[e-mail]> 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

Maninder Singh June 8, 2023, 3:10 a.m. UTC | #5

Hi,

> > 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).

Thanks for your confirmation, I will resend patches without bpf change.

Thanks,
Maninder Singh

Patch

diff --git a/include/linux/filter.h b/include/linux/filter.h
index bbce89937fde..1f237a3bb11a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -923,13 +923,21 @@  bool bpf_jit_supports_kfunc_call(void);
 bool bpf_jit_supports_far_kfunc_call(void);
 bool bpf_helper_changes_pkt_data(void *func);
 
+/*
+ * Reconstruction of call-sites is dependent on kallsyms,
+ * thus make dump the same restriction.
+ */
+#ifdef CONFIG_KALLSYMS
 static inline bool bpf_dump_raw_ok(const struct cred *cred)
 {
-	/* Reconstruction of call-sites is dependent on kallsyms,
-	 * thus make dump the same restriction.
-	 */
 	return kallsyms_show_value(cred);
 }
+#else
+static inline bool bpf_dump_raw_ok(const struct cred *cred)
+{
+	return false;
+}
+#endif
 
 struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 				       const struct bpf_insn *patch, u32 len);