Re: [PATCH v3] scripts: add leaking_addresses.pl

Commit Message

Tobin Harding Nov. 8, 2017, 3:24 a.m. UTC

On Mon, Nov 06, 2017 at 09:27:09AM -0800, Linus Torvalds wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <[e-mail]> wrote:
> > Currently we are leaking addresses from the kernel to user space. This
> > script is an attempt to find some of those leakages. Script parses
> > `dmesg` output and /proc and /sys files for hex strings that look like
> > kernel addresses.
> 
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
> 
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"

If we assume kptr_restrict is totally broken. Running this (with
kptr_restrict==0 and outputting a summary) hints that plugging all these
leaks is not an insurmountable problem. Perhaps we do not need to hash
%p by default? If we add %pX (or what ever) that hashes the address then
using the script to find the problems we can migrate to %pX as needed.

This approach relies on a concrete concensus (do we have those ;) as to
how and when we should print pointers. I just replied to you on that
topic in another thread. This raises the issue that this hashing
discussion is crazy fragmented and hard for people to follow (CC lists
are different on a few of the threads also), is it correct protocol to
patch Documentation with the concensus we have so far?

---
 Documentation/leaking_addresses.rst | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/leaking_addresses.rst

Patch

diff --git a/Documentation/leaking_addresses.rst b/Documentation/leaking_addresses.rst
new file mode 100644
index 000000000000..737a1b76b3f7
--- /dev/null
+++ b/Documentation/leaking_addresses.rst
@@ -0,0 +1,33 @@ 
+Leaking Kernel Addresses
+========================
+
+If we show kernel addresses to user space bad things can happen.
+
+Work in Progress
+----------------
+
+- Create a tool to scan the kernel for leaking addresses.
+  - Partially done, see scripts/leaking_addresses.pl
+- Provide some sort of pointer hashing (i.e unique identifier).
+- Move away from kptr_restrict (and %pK).
+- Fix leaks on a case by case basis.
+
+WTF, just tell me how to print a pointer
+----------------------------------------
+
+Essentially you must consider _carefully_ who the needs to see the address and why.
+
+- If it is for profiling guard with perf_event_paranoid.
+- If the file is intended for root-only, then guard via file permissions.
+- If a unique identifier will suffice use hashing specifier (still to do).
+
+- If you really need the address ...    
+
+Ideas
+-----
+
+- Add a printk specifier that prints conditionally based on
+  perf_event_paranoid? 
+
+- Add seq_printf_sanitize() that only shows addresses to root (based on the
+  permissions of the process that opens the file)?