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 <[email protected]>
Signed-off-by: Onkarnath <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
Reviewed-by: Luis Chamberlain <[email protected]>
---
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
On Mon, Jun 5, 2023 at 9:28 PM Maninder Singh <[email protected]> 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 <[email protected]>
> Signed-off-by: Onkarnath <[email protected]>
> Signed-off-by: Maninder Singh <[email protected]>
> Reviewed-by: Luis Chamberlain <[email protected]>
> ---
> 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
>
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
On Tue, Jun 6, 2023 at 8:46 PM Maninder Singh <[email protected]> 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
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