|
Message-ID: <20171108030601.GA18478@eros> Date: Wed, 8 Nov 2017 14:06:01 +1100 From: "Tobin C. Harding" <me@...in.cc> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: Kees Cook <keescook@...omium.org>, "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>, Andy Lutomirski <luto@...nel.org>, Joe Perches <joe@...ches.com>, Network Development <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>, "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, "Jason A. Donenfeld" <Jason@...c4.com>, Theodore Ts'o <tytso@....edu>, Paolo Bonzini <pbonzini@...hat.com>, Tycho Andersen <tycho@...ker.com>, "Roberts, William C" <william.c.roberts@...el.com>, Tejun Heo <tj@...nel.org>, Jordan Glover <Golden_Miller83@...tonmail.ch>, Greg KH <gregkh@...uxfoundation.org>, Petr Mladek <pmladek@...e.com>, Ian Campbell <ijc@...lion.org.uk>, Sergey Senozhatsky <sergey.senozhatsky@...il.com>, Catalin Marinas <catalin.marinas@....com>, Will Deacon <wilal.deacon@....com>, Steven Rostedt <rostedt@...dmis.org>, Chris Fries <cfries@...gle.com>, Dave Weinstein <olorin@...gle.com>, Daniel Micay <danielmicay@...il.com>, Djalal Harouni <tixxdz@...il.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v3] scripts: add leaking_addresses.pl On Tue, Nov 07, 2017 at 01:44:01PM -0800, Linus Torvalds wrote: > On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@...omium.org> wrote: > > > > Linus, what do you have in mind for the root-only "yes we really need > > the actual address output" exceptions? > > I am convinced that absolutely none of them should use '%pK'. > > So far we have actually never seen a valid case wher %pK was really > the right thing to do. > > > For example, right now /sys/kernel/debug/kernel_page_tables > > (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x. > > So I think it could continue to use %x, and just make sure the whole > file is root-only. > > And that is why %pK is so wrong. It's almost never really about root. > > Look at /proc/kallsyms, for example. There it's mainly about kernel > profiles (although there certainly have been other uses historically, > and maybe some of them remain) - which we have another flag for > entirely that is very much specifically about kernel profiles. > > > Looking other places that stand out, it seems like > > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of > > %p usage. It's unclear to me if a hash is sufficient for meaningful > > debugging there? > > Maybe not, but that is also _so_ esoteric that I suspect the right fix > is to just make it root-only readable. > > I've never used it, we should check with people who have. I get the > feeling that this is purely for PeterZ debugging. > > The very first commit that introduced that code actually has a > > (FIXME: should go into debugfs) > > so I suspect it never should have been user-readable to begin with. I > guess it makes some things easier, but it really is *very* different > from things like profiling. > > Profiling you often *cannot* do as root - some things you profile > really shouldn't be run as root, and might even refuse to do so. So > requiring you to be root just to get a kernel profile is very bad. > > But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal. > > And I really suspect that's true of a _lot_ of these %p things that > really want a pointer. It's not that they really want %pK, it's that > they shouldn't have been visible to regular users in the first place. > > Things that *do* want a pointer and should be visible to regular users > are things like oops messages etc, but even there we obviously > generally want to use %pS/%pF when possible (but generally %x when not > - things like register contents etc that *may* contain pointers). This is opt-in, it means asking developers to do the right thing every time they think they need a pointer. If we hash %p as soon as someone has been bitten by it they will start using %x and sooner or later they will use %x exclusively (suggesting using %x for _really_ necessary addresses will only hasten the process). If the only real benefit of hashing %p is to clean up kruft why don't we add %pX and do tree wide substitution of all %p to %pX. People that get broken will change it back to %p and we will have half a chance of finding new abusers of %p down the track. If there is not a 'one size fits all' solution, as we have seen with kptr_restrict, then should we not be trying to make it easier for people to do the right thing _and_ easier for us to catch it when we do the wrong thing? There is already almost 30 000 users of %{x,X}, surely we don't want to promote more kernel address printing to be mixed in with all of that? > And if they really are visible to users - because you want to > cross-correlate them for things like netstat - I think the hashing is > the right thing to do both for root and for regular users. > > > Seems like these three from dmesg could be removed? > > > > [ 0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576 > > arch/x86/realmode/init.c > > > > [ 0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944 > > r8192 d30512 u524288 > > mm/percpu.c > > > > [ 0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB) > > mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff] > > lib/swiotlb.c > > Yes, I think the solution for a lot of the random device discovery > messages etc is to just remove them. They were likely useful when the > code was new and untested, and just stayed around afterwards. Is anyone actually going to do this? If the hashing gets done then these messages are not a risk, are _real_ kernel developers going to bother cleaning this up? Surely newbies are not going to get much love either if they submit patches that '[PATCH] remove unnecessary printk message'. thanks, Tobin.
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.