[PATCH 1/2] kallsyms: add kallsyms_show_value definition in all cases

[PATCH 1/2] kallsyms: add kallsyms_show_value definition in all cases

[Maninder Singh]

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:

/ # insmod crash.ko
/ # lsmod
crash 12288 0 - Live 0x0000000000000000 (O)

After change (making definition generic)
============
/ # lsmod
crash 12288 0 - Live 0xffff800000ec0000 (O)
/ # cat /proc/modules
crash 12288 0 - Live 0xffff800000ec0000 (O)
/ #

BPF code has high dependency on kallsyms,
so bpf_dump_raw_ok check is not changed.

Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
BPF logic is not changed and seems kprobe will work, rest kprobe
maintainer can comment if it can cause issue. Tested for modules data.
https://lkml.org/lkml/2022/4/13/326

 include/linux/filter.h   |  7 ++++++
 include/linux/kallsyms.h | 10 +++-----
 kernel/Makefile          |  2 +-
 kernel/kallsyms.c        | 35 ---------------------------
 kernel/kallsyms_tiny.c   | 51 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 43 deletions(-)
 create mode 100644 kernel/kallsyms_tiny.c

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;
+	}
+}
-- 
2.17.1

[PATCH 2/2] kallsyms: move sprint_module_info to kallsyms_tiny.c

[Maninder Singh]

As previous patch makes new file for generic kallsyms
(always compilable), move sprint_module_info module to
new file kallsyms_tiny.c

no functional change with this commit

Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 include/linux/kallsyms.h | 11 +++++++++
 kernel/kallsyms_tiny.c   | 47 +++++++++++++++++++++++++++++++++++
 lib/vsprintf.c           | 53 ----------------------------------------
 3 files changed, 58 insertions(+), 53 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index c5e63a217404..95a2f4ade996 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -27,6 +27,17 @@ struct module;
 /* How and when do we show kallsyms values? */
 extern bool kallsyms_show_value(const struct cred *cred);
 
+#if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES)
+extern int sprint_module_info(char *buf, unsigned long value,
+				int modbuildid, int backtrace, int symbol);
+#else
+static inline int sprint_module_info(char *buf, unsigned long value,
+				int modbuildid, int backtrace, int symbol)
+{
+	return 0;
+}
+#endif
+
 static inline int is_kernel_text(unsigned long addr)
 {
 	if (__is_kernel_text(addr))
diff --git a/kernel/kallsyms_tiny.c b/kernel/kallsyms_tiny.c
index 96ad06836126..8ed9fdd7d9f7 100644
--- a/kernel/kallsyms_tiny.c
+++ b/kernel/kallsyms_tiny.c
@@ -49,3 +49,50 @@ bool kallsyms_show_value(const struct cred *cred)
 		return false;
 	}
 }
+
+#if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES)
+int sprint_module_info(char *buf, unsigned long value,
+			     int modbuildid, int backtrace, int symbol)
+{
+	struct module *mod;
+	unsigned long offset;
+	void *base;
+	char *modname;
+	int len;
+	const unsigned char *buildid = NULL;
+	bool add_offset;
+
+	if (is_ksym_addr(value))
+		return 0;
+
+	if (backtrace || symbol)
+		add_offset = true;
+	else
+		add_offset = false;
+
+	preempt_disable();
+	mod = __module_address(value);
+	if (mod) {
+		modname = mod->name;
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
+		if (modbuildid)
+			buildid = mod->build_id;
+#endif
+		if (add_offset) {
+			base = mod->core_layout.base;
+			offset = value - (unsigned long)base;
+		}
+	}
+	preempt_enable();
+	if (!mod)
+		return 0;
+
+	/* address belongs to module */
+	if (add_offset)
+		len = sprintf(buf, "0x%p+0x%lx", base, offset);
+	else
+		len = sprintf(buf, "0x%lx", value);
+
+	return len + fill_name_build_id(buf, modname, modbuildid, buildid, len);
+}
+#endif
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 799fccca4a2d..983fdb02543c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -999,59 +999,6 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev,
 }
 #endif
 
-#if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES)
-static int sprint_module_info(char *buf, unsigned long value,
-			     int modbuildid, int backtrace, int symbol)
-{
-	struct module *mod;
-	unsigned long offset;
-	void *base;
-	char *modname;
-	int len;
-	const unsigned char *buildid = NULL;
-	bool add_offset;
-
-	if (is_ksym_addr(value))
-		return 0;
-
-	if (backtrace || symbol)
-		add_offset = true;
-	else
-		add_offset = false;
-
-	preempt_disable();
-	mod = __module_address(value);
-	if (mod) {
-		modname = mod->name;
-#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
-		if (modbuildid)
-			buildid = mod->build_id;
-#endif
-		if (add_offset) {
-			base = mod->core_layout.base;
-			offset = value - (unsigned long)base;
-		}
-	}
-	preempt_enable();
-	if (!mod)
-		return 0;
-
-	/* address belongs to module */
-	if (add_offset)
-		len = sprintf(buf, "0x%p+0x%lx", base, offset);
-	else
-		len = sprintf(buf, "0x%lx", value);
-
-	return len + fill_name_build_id(buf, modname, modbuildid, buildid, len);
-}
-#else
-static inline int sprint_module_info(char *buf, unsigned long value,
-			     int modbuildid, int backtrace, int symbol)
-{
-	return 0;
-}
-#endif
-
 static noinline_for_stack
 char *symbol_string(char *buf, char *end, void *ptr,
 		    struct printf_spec spec, const char *fmt)
-- 
2.17.1

Re: [PATCH 1/2] kallsyms: add kallsyms_show_value definition in all cases

[Kees Cook]

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

-- 
Kees Cook

RE: [PATCH 1/2] kallsyms: add kallsyms_show_value definition in all cases

[Maninder Singh]

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

Re: [PATCH 1/2] kallsyms: add kallsyms_show_value definition in all cases

[andriy.shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko