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

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

[Maninder Singh]

kallsyms_show_value return false if KALLSYMS is disabled,
but it's used in 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)
/ #

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.c        | 35 -----------------------------------
 lib/vsprintf.c           | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index e5ad6e31697d..efabb8c18492 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))
@@ -93,9 +96,6 @@ extern int sprint_backtrace_build_id(char *buffer, unsigned long address);
 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 */
 
 static inline unsigned long kallsyms_lookup_name(const char *name)
@@ -158,11 +158,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/kallsyms.c b/kernel/kallsyms.c
index e8d2262ef2d2..71ef15ba20c7 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -818,41 +818,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/lib/vsprintf.c b/lib/vsprintf.c
index 49ef55ffabd7..4bc96a4f3a00 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -870,6 +870,42 @@ static char *default_pointer(char *buf, char *end, const void *ptr,
 
 int kptr_restrict __read_mostly;
 
+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 noinline_for_stack
 char *restricted_pointer(char *buf, char *end, const void *ptr,
 			 struct printf_spec spec)
-- 
2.17.1

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

[Petr Mladek]

On Wed 2022-04-13 11:23:05, Maninder Singh wrote:
> kallsyms_show_value return false if KALLSYMS is disabled,
> but it's used in 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)
> / #
> 
> 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.c        | 35 -----------------------------------
>  lib/vsprintf.c           | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 43 deletions(-)
> 
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index e5ad6e31697d..efabb8c18492 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))
> @@ -93,9 +96,6 @@ extern int sprint_backtrace_build_id(char *buffer, unsigned long address);
>  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 */
>  
>  static inline unsigned long kallsyms_lookup_name(const char *name)
> @@ -158,11 +158,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;
> -}

With the patch, kallsyms_show_value() might return true even when
CONFIG_KALLYSMS are disabled. Did you check all users of this API
that they could handle this?

For example, the comment in bpf_dump_raw_ok() suggests that
it might require more kallsyms functionality.

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);
}

You should definitely add into CC people from affected subsystems.
I do not see:

    + Kees Cook who added/updated most of the callers
    + BPF people that might require even more kallsyms functionality
    + kprobe people that using it as well


> -
>  #endif /*CONFIG_KALLSYMS*/
>  
>  static inline void print_ip_sym(const char *loglvl, unsigned long ip)
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index e8d2262ef2d2..71ef15ba20c7 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -818,41 +818,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/lib/vsprintf.c b/lib/vsprintf.c
> index 49ef55ffabd7..4bc96a4f3a00 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -870,6 +870,42 @@ static char *default_pointer(char *buf, char *end, const void *ptr,
>  
>  int kptr_restrict __read_mostly;
>  
> +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;
> +	}
> +}

It is really weird that the function is declared in kallsyms.h
and implemented in vsprintf.c.

What about splitting kallsyms.c into two source files where one
would be always compiled? It would be usable also for the
spring function introduced by
https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com

It might be something like kallsyms_full.c and/or kallsyms_tiny.c.

Best Regards,
Petr

>  static noinline_for_stack
>  char *restricted_pointer(char *buf, char *end, const void *ptr,
>  			 struct printf_spec spec)
> -- 
> 2.17.1

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

[Maninder Singh]

[-- Attachment #1: Type: text/plain, Size: 2952 bytes --]

Hi,

> 
> With the patch, kallsyms_show_value() might return true even when
> CONFIG_KALLYSMS are disabled. Did you check all users of this API
> that they could handle this?
> 
> For example, the comment in bpf_dump_raw_ok() suggests that
> it might require more kallsyms functionality.
>

Ok, we will check these other callers also, if it causes negative impact at other places.

> 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);
> }
> 
> You should definitely add into CC people from affected subsystems.
> I do not see:
> 
>     + Kees Cook who added/updated most of the callers
>     + BPF people that might require even more kallsyms functionality
>     + kprobe people that using it as well
>

Yes, I thought maintainer.pl has shown all people, but seems caller function owners
also need to be added, I will add  in next revision.



> > +/*
> > + * 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;
> > +	}
> > +}
> 
> It is really weird that the function is declared in kallsyms.h
> and implemented in vsprintf.c.
>

Yes it does not look good.
Initially we thought to make it as static inline function in kallsyms.h only.
But as function is little big and it will increase code size, so we added
definition in vsprintf.c, because its alwyas compilable code and also it has
some wrapper APIs for kallsyms.
 
But as you suggested to make a new file, it will be good.

> What about splitting kallsyms.c into two source files where one
> would be always compiled? It would be usable also for the
> spring function introduced by
> https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com
> 
> It might be something like kallsyms_full.c and/or kallsyms_tiny.c.
>

This patch is not taken by Luis yet in module-tetsing branch.
So what will be the best approach to make new version of this patch ?

A) to make new file kallsyms_tiny.c and add only one definition in that file
and when above patch of spring functions is merged in next we can move definitions
to new file

or

B) we send patch to Luis's branch of module-testing with moving definition(of earlier patch)
to new file and current patch also.

Thanks,
Maninder Singh

[-- Attachment #2: rcptInfo.txt --]
[-- Type: application/octet-stream, Size: 2004 bytes --]


   =================================================================================================================================
      Subject    : Re: [PATCH 1/1] kallsyms: add kallsyms_show_value definition in all cases
      From       : null
      Sent Date  : 2022-04-13 14:54  GMT+5:30
   =================================================================================================================================
                  Name                Type          Job Title                       Dept.                               Company                
   =================================================================================================================================
      Maninder Singh                 TO                                    System S/W Group /SRI-Delhi               Samsung Electronics 
      mcgrof@kernel.org              CC
      rostedt@goodmis.org            CC
      senozhatsky@chromium.org       CC
      andriy.shevchenko@linux.i...   CC
      akpm@linux-foundation.org      CC
      wangkefeng.wang@huawei.com     CC
      wedsonaf@google.com            CC
      boqun.feng@gmail.com           CC
      christophe.leroy@csgroup.eu    CC
      swboyd@chromium.org            CC
      ojeda@kernel.org               CC
      ast@kernel.org                 CC
      gary@garyguo.net               CC
      mbenes@suse.cz                 CC
      ndesaulniers@google.com        CC
      void@manifault.com             CC
      jolsa@kernel.org               CC
      linux-kernel@vger.kernel.org   CC
      Vaneet Narang                  CC                                    System S/W Group /SRI-Delhi               Samsung Electronics
      Onkarnath                      CC                                    System S/W Group /SRI-Delhi               Samsung Electronics
   =================================================================================================================================

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

[Petr Mladek]

On Mon 2022-04-18 10:04:42, Maninder Singh wrote:
> > It is really weird that the function is declared in kallsyms.h
> > and implemented in vsprintf.c.
> >
> 
> Yes it does not look good.
> Initially we thought to make it as static inline function in kallsyms.h only.
> But as function is little big and it will increase code size, so we added
> definition in vsprintf.c, because its alwyas compilable code and also it has
> some wrapper APIs for kallsyms.
>  
> But as you suggested to make a new file, it will be good.
> 
> > What about splitting kallsyms.c into two source files where one
> > would be always compiled? It would be usable also for the
> > spring function introduced by
> > https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com
> > 
> > It might be something like kallsyms_full.c and/or kallsyms_tiny.c.
> >
> 
> This patch is not taken by Luis yet in module-tetsing branch.
> So what will be the best approach to make new version of this patch ?

I am not sure about Luis' plans and if the patch applies on top of
the current module.c split patchset (linux-next).


> A) to make new file kallsyms_tiny.c and add only one definition in that file
> and when above patch of spring functions is merged in next we can move definitions
> to new file
>
> or
> 
> B) we send patch to Luis's branch of module-testing with moving definition(of earlier patch)
> to new file and current patch also.

I wonder how many conflicts it might cause. The patchset splitting
module.c actually does not touch kernel/kallsyms.c.

Anyway, I would probably prepare it on top of linux-next.

Best Regards,
Petr

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

[Luis Chamberlain]

On Mon, Apr 18, 2022 at 10:04:42AM +0530, Maninder Singh wrote:
> > What about splitting kallsyms.c into two source files where one
> > would be always compiled? It would be usable also for the
> > spring function introduced by
> > https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com
> > 
> > It might be something like kallsyms_full.c and/or kallsyms_tiny.c.
> >
> 
> This patch is not taken by Luis yet in module-tetsing branch.

Please resend that patch, as I'm happy to route it in through
modules-next [0]. I use modules-testing *prior* to pushing to
modules-next, if 0-day does not complain after about a few days
then I push to modules-next and this gets merged to linux-next.

You can send that patch and then this one and include in the subject
something like:

[PATCH modules-next 1/1]

Vimal, I'd like your review of these patches too. And after those
we can see where your change fits / if it is still needed.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=modules-next

  Luis

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

[Vimal Agrawal]

Hi Maninder,

in sprint_module_info(), we need to take care of mod->init_layout
addresses as well.
Please test the patch against  init symbols.

Vimal

On Wed, Apr 20, 2022 at 8:51 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Apr 18, 2022 at 10:04:42AM +0530, Maninder Singh wrote:
> > > What about splitting kallsyms.c into two source files where one
> > > would be always compiled? It would be usable also for the
> > > spring function introduced by
> > > https://lore.kernel.org/r/20220323164742.2984281-1-maninder1.s@samsung.com
> > >
> > > It might be something like kallsyms_full.c and/or kallsyms_tiny.c.
> > >
> >
> > This patch is not taken by Luis yet in module-tetsing branch.
>
> Please resend that patch, as I'm happy to route it in through
> modules-next [0]. I use modules-testing *prior* to pushing to
> modules-next, if 0-day does not complain after about a few days
> then I push to modules-next and this gets merged to linux-next.
>
> You can send that patch and then this one and include in the subject
> something like:
>
> [PATCH modules-next 1/1]
>
> Vimal, I'd like your review of these patches too. And after those
> we can see where your change fits / if it is still needed.
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=modules-next
>
>   Luis

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

[Maninder Singh]

Hi Vimal,

> Hi Maninder,
> 
> in sprint_module_info(), we need to take care of mod->init_layout
> addresses as well.
> Please test the patch against  init symbols.
> 
> Vimal

Same I updated in below thread.

https://lkml.org/lkml/2022/3/22/56

Can you check if anything else needs to be fixed or updated.

(because of office mail system, reply thread was made a new thread rather than same patch conversation)

Thanks,
Maninder Singh