linux-kernel - Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 28 Apr 2018 01:10:56 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Ananth N Mavinakayanahalli <ananth@...ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jon Medhurst <tixy@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Arnd Bergmann <arnd@...db.de>,
        David Howells <dhowells@...hat.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        "Tobin C . Harding" <me@...in.cc>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        akpm@...ux-foundation.org, acme@...nel.org, rostedt@...dmis.org,
        brueckner@...ux.vnet.ibm.com, schwidefsky@...ibm.com,
        stable@...r.kernel.org
Subject: Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as
 kallsyms does

On Fri, 27 Apr 2018 09:14:17 +0200
Ingo Molnar <mingo@...nel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@...nel.org> wrote:
> 
> > +	/*
> > +	 * As long as kallsyms shows the address, kprobes blacklist also
> > +	 * show it, Or, it shows null address and symbol.
> > +	 */
> 
> Please _read_ the comments you write!
> 
> In which universe does a capitalized 'Or' make sense, even if we ignore the 
> various other spelling mistakes?

It's a typo. I mean "show it. Or, it shows..." anyway,

> 
> Also, that sentence is unnecessarily complex, just say this:
> 
> > +	/*
> > +	 * If /proc/kallsyms is not showing kernel addresses then we won't show 
> > +      * them here either:
> > +	 */

OK, look good to me.

> 
> But I'm unhappy about the messy typing and the messy code flow:
> 
> +       void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;
> 
> +       /*
> +        * As long as kallsyms shows the address, kprobes blacklist also
> +        * show it, Or, it shows null address and symbol.
> +        */
> +       if (!kallsyms_show_value())
> +               start = end = NULL;
> +
> +       seq_printf(m, "0x%px-0x%px\t%ps\n", start, end,
> +                  (void *)ent->start_addr);
> 
> 
> All three 'void *' type casts here are due to the bad type choices here:
> 
> struct kprobe_blacklist_entry {
>         struct list_head list;
>         unsigned long start_addr;
>         unsigned long end_addr;
> };
> 
> The natural type of ->start_addr and ->end_addr is 'void *', AFAICS this would 
> remove some other type casts from the kprobes code as well, such as from the 
> arch_deref_entry_point()...

Would you really think we should handle all the address with 'void *'?
IOW, are there any policy that we handle the generic address by 'void *'
or 'unsigned long'?
For example, other address checker like kernel_text_address(),
module_text_address(), and ftrace_location() receive 'unsigned long'.
(only jump_label_text_reserved() using 'void *')

> 
> But the whole code flow introduced by this patch is messy as hell as well.
> Why cannot this do the obvious thing:
> 
> 	if (!kallsyms_show_value())
> 		seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL, ent->start_addr);
> 	else
> 		seq_printf(m, "0x%px-0x%px\t%ps\n", ent->start_addr, ent->end_addr, ent->start_addr);
> 
> ?

Both are OK to me. I just didn't want to repeat the printk format string there.

> 
> This variant eliminates the unnecessary complication over local variables and 
> makes it abundantly clear what gets printed and how.

Agreed, it may shorten the patch.

> ( Note that the kprobe_blacklist_entry type cleanup should still be done, 
>   regardless of code flow choices. )
> 
> Thanks,
> 
> 	Ingo

Thank you,


-- 
Masami Hiramatsu <mhiramat@...nel.org>