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@xxxxxxxxxxx>
> Signed-off-by: Onkarnath <onkarnath.1@xxxxxxxxxxx>
> Signed-off-by: Maninder Singh <maninder1.s@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxx

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