Date:   Tue, 19 Dec 2017 09:53:59 +0100
From:   Thomas-Mich Richter <tmricht@...ux.vnet.ibm.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Hendrik Brueckner <brueckner@...ux.vnet.ibm.com>,
        "Tobin C. Harding" <me@...in.cc>
Subject: Re: [PATCH] trace/uprobes: fix output issue with address
 randomization

On 12/18/2017 07:16 PM, Linus Torvalds wrote:
> On Mon, Dec 18, 2017 at 9:59 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
>>
>> Hmm, since the scrambling of %p is to prevent kernel addresses from
>> leaking, I wonder if it would be OK to make it only scramble the address
>> if the address is a kernel address. It should be fine to print user
>> space addresses unscrambled. Shouldn't it?
> 
> So %p itself shouldn't have logic like that, because some of those
> addresses can be sensitive even if they aren't strictly kernel
> addresses.
> 
> For example, anything that prints out sensitive physical addresses
> wouldn't look like a kernel virtual address, but it could still expose
> very sensitive data.
> 
> So that check would have to be done by the user of %p, not by %p itself.
> 
> So generally, the fix for "oops %p hashing now breaks xyz" should
> generally be to make sure that the protections are correct, and then
> it can be turned into %px (when it can't just be removed).
> 
> In this particular case, we already have the magical case of "don't
> print (null)", and I think _that_ the case that could be just extended
> to say "don't print sensitive data" and then the %p can be changed
> into a %px.
> 
> So one approach would be to simply checking (at _open_ time, not at IO
> time! That was one of the things that I absolutely detested about %pK
> - getting that fundamentally wrong) whether the opener could write a
> kernel address to the file, and if the opener has those permissions,
> then it obviously can read the values too.
> 
> But in this case I would suggest just making "uprobe_events" be 0600
> rather than 0644. Why the hell should a normal user be able to read
> whatever events that root has set?
> 
> Same goes for  "uprobe_profile". Is it really sensible that anybody
> can read that file? It sounds insane to me. Why should a random
> regular user be able to see what probes are installed?
> 
> Honestly, very few of the "let's just change %p to %px" has been
> correct. There is pretty much _always_ some permission problem or
> other that says "hell no, that data should not have been so widely
> available before".
> 
> The only time a pure %p -> %px is warranted is likely for oops reports
> etc. We did have a couple of those.
> 
>                 Linus
> 

As Steven Rostedt already pointed out, the 2 file systems debugfs and
tracefs are accessible for root only:

[root@...lp76 ~]# mount | egrep 'debug|trace'
debugfs on /sys/kernel/debug type debugfs (rw,relatime)
tracefs on /sys/kernel/debug/tracing type tracefs (rw,relatime)
[root@...lp76 ~]# ls -ld /sys/kernel/debug/ /sys/kernel/debug/tracing/
drwx------ 14 root root 0 Dec 18 11:34 /sys/kernel/debug/
drwx------  7 root root 0 Dec 18 11:34 /sys/kernel/debug/tracing/
[root@...lp76 ~]# 

Surely the file permissions for uprobe_events and uprobe_profile
in directory /sys/kernel/debug/tracing 
can be changed to be accessible by owner (root) only, but does this really
help?
I have looked at all the other files in this directory and almost
all of them have read permissions for group and others.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Powered by blists - more mailing lists