| Message ID | 20220511080657.3996053-1-maninder1.s@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [1/2] kallsyms: add kallsyms_show_value definition in all cases | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-PR | fail | merge-conflict |
| netdev/tree_selection | success | Not a local patch |
On Wed, May 11, 2022 at 01:36:56PM +0530, Maninder Singh wrote: > kallsyms_show_value return false if KALLSYMS is disabled, > but its usage is done by module.c also. > Thus when KALLSYMS is disabled, system will not print module > load address: Eek, I hadn't see the other changes this depends on. I think those changes need to be reworked first. Notably in the other patch, this is no good: /* address belongs to module */ if (add_offset) len = sprintf(buf, "0x%p+0x%lx", base, offset); else len = sprintf(buf, "0x%lx", value); This is printing raw kernel addresses with no hashing, as far as I can tell. That's not okay at all. Once that other patch gets fixed, this one then can be revisited. And just on naming: "kallsyms_tiny" is a weird name: it's just "ksyms" -- there's no "all". :)
Hi, > On Wed, May 11, 2022 at 01:36:56PM +0530, Maninder Singh wrote: > > kallsyms_show_value return false if KALLSYMS is disabled, > > but its usage is done by module.c also. > > Thus when KALLSYMS is disabled, system will not print module > > load address: > > Eek, I hadn't see the other changes this depends on. I think those > changes need to be reworked first. Notably in the other patch, this is > no good: > > /* address belongs to module */ > if (add_offset) > len = sprintf(buf, "0x%p+0x%lx", base, offset); > else > len = sprintf(buf, "0x%lx", value); > > This is printing raw kernel addresses with no hashing, as far as I can > tell. That's not okay at all. > yes same was suggested by Petr also, because earlier we were printing base address also as raw address. https://lkml.org/lkml/2022/2/28/847 but then modified approach to print base address as hash when we are going to show offset of module address, but when we print complete address then we thought of keeping it same as it was: original: [12.487424] ps 0xffff800000eb008c with patch: [9.624152] ps 0xffff800001bd008c [crash] But if its has to be hashed, will fix that also. > Once that other patch gets fixed, this one then can be revisited. > I will check detailed comments on that also > And just on naming: "kallsyms_tiny" is a weird name: it's just "ksyms" > -- there's no "all". :) Ok :) Will name it as knosyms.c (if it seems ok). Thanks, Maninder Singh
On Thu, May 12, 2022 at 09:16:50AM +0530, Maninder Singh wrote: > > On Wed, May 11, 2022 at 01:36:56PM +0530, Maninder Singh wrote: ... > > This is printing raw kernel addresses with no hashing, as far as I can > > tell. That's not okay at all. > > yes same was suggested by Petr also, because earlier we were printing base address also as raw address. > > https://lkml.org/lkml/2022/2/28/847 > > but then modified approach to print base address as hash when we are going to show offset of module address, > but when we print complete address then we thought of keeping it same as it was: > > original: > [12.487424] ps 0xffff800000eb008c > with patch: > [9.624152] ps 0xffff800001bd008c [crash] > > But if its has to be hashed, will fix that also. In such case it should be a separate change since it will be the one that changes behaviour.
diff --git a/include/linux/filter.h b/include/linux/filter.h index ed0c0ff42ad5..98c4365e726d 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -951,6 +951,7 @@ bool bpf_jit_needs_zext(void); bool bpf_jit_supports_kfunc_call(void); bool bpf_helper_changes_pkt_data(void *func); +#ifdef CONFIG_KALLSYMS static inline bool bpf_dump_raw_ok(const struct cred *cred) { /* Reconstruction of call-sites is dependent on kallsyms, @@ -958,6 +959,12 @@ static inline bool bpf_dump_raw_ok(const struct cred *cred) */ 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); diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index c24fa627ab6e..c5e63a217404 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -24,6 +24,9 @@ struct cred; struct module; +/* How and when do we show kallsyms values? */ +extern bool kallsyms_show_value(const struct cred *cred); + static inline int is_kernel_text(unsigned long addr) { if (__is_kernel_text(addr)) @@ -95,8 +98,6 @@ extern int sprint_kallsym_common(char *buffer, unsigned long address, int build_ int lookup_symbol_name(unsigned long addr, char *symname); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); -/* How and when do we show kallsyms values? */ -extern bool kallsyms_show_value(const struct cred *cred); #else /* !CONFIG_KALLSYMS */ @@ -160,11 +161,6 @@ static inline int lookup_symbol_attrs(unsigned long addr, unsigned long *size, u return -ERANGE; } -static inline bool kallsyms_show_value(const struct cred *cred) -{ - return false; -} - #endif /*CONFIG_KALLSYMS*/ static inline void print_ip_sym(const char *loglvl, unsigned long ip) diff --git a/kernel/Makefile b/kernel/Makefile index 318789c728d3..844ed3df95f6 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -10,7 +10,7 @@ obj-y = fork.o exec_domain.o panic.o \ extable.o params.o \ kthread.o sys_ni.o nsproxy.o \ notifier.o ksysfs.o cred.o reboot.o \ - async.o range.o smpboot.o ucount.o regset.o + async.o range.o smpboot.o ucount.o regset.o kallsyms_tiny.o obj-$(CONFIG_USERMODE_DRIVER) += usermode_driver.o obj-$(CONFIG_MODULES) += kmod.o diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 9a0d6cfca619..d818048cb9f7 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -819,41 +819,6 @@ static const struct seq_operations kallsyms_op = { .show = s_show }; -static inline int kallsyms_for_perf(void) -{ -#ifdef CONFIG_PERF_EVENTS - extern int sysctl_perf_event_paranoid; - if (sysctl_perf_event_paranoid <= 1) - return 1; -#endif - return 0; -} - -/* - * We show kallsyms information even to normal users if we've enabled - * kernel profiling and are explicitly not paranoid (so kptr_restrict - * is clear, and sysctl_perf_event_paranoid isn't set). - * - * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to - * block even that). - */ -bool kallsyms_show_value(const struct cred *cred) -{ - switch (kptr_restrict) { - case 0: - if (kallsyms_for_perf()) - return true; - fallthrough; - case 1: - if (security_capable(cred, &init_user_ns, CAP_SYSLOG, - CAP_OPT_NOAUDIT) == 0) - return true; - fallthrough; - default: - return false; - } -} - static int kallsyms_open(struct inode *inode, struct file *file) { /* diff --git a/kernel/kallsyms_tiny.c b/kernel/kallsyms_tiny.c new file mode 100644 index 000000000000..96ad06836126 --- /dev/null +++ b/kernel/kallsyms_tiny.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2022 Samsung Electronics Co., Ltd + * + * Author: + * Maninder singh <maninder1.s@samsung.com> + * Onkarnath <onkarnath.1@samsung.com> + * + * A split of kernel/kallsyms.c + * Contains few generic definitions independent of CONFIG_KALLSYMS + * or defined under !CONFIG_KALLSYMS. + */ + +#include <linux/kallsyms.h> +#include <linux/security.h> + +static inline int kallsyms_for_perf(void) +{ +#ifdef CONFIG_PERF_EVENTS + extern int sysctl_perf_event_paranoid; + + if (sysctl_perf_event_paranoid <= 1) + return 1; +#endif + return 0; +} + +/* + * We show kallsyms information even to normal users if we've enabled + * kernel profiling and are explicitly not paranoid (so kptr_restrict + * is clear, and sysctl_perf_event_paranoid isn't set). + * + * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to + * block even that). + */ +bool kallsyms_show_value(const struct cred *cred) +{ + switch (kptr_restrict) { + case 0: + if (kallsyms_for_perf()) + return true; + fallthrough; + case 1: + if (security_capable(cred, &init_user_ns, CAP_SYSLOG, + CAP_OPT_NOAUDIT) == 0) + return true; + fallthrough; + default: + return false; + } +}