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 <[email protected]>
> Signed-off-by: Onkarnath <[email protected]>
> Signed-off-by: Maninder Singh <[email protected]>
> ---
> 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/[email protected]
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
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/[email protected]
>
> 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
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/[email protected]
> >
> > 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
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/[email protected]
> >
> > 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
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 <[email protected]> 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/[email protected]
> > >
> > > 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
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