Subject: Re: [PATCH] trace/uprobes: fix output issue with address
 randomization
To: Linus Torvalds <torvalds@linux-foundation.org>,
        Steven Rostedt <rostedt@goodmis.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Heiko Carstens <heiko.carstens@de.ibm.com>,
        Martin Schwidefsky <schwidefsky@de.ibm.com>,
        Hendrik Brueckner <brueckner@linux.vnet.ibm.com>,
        "Tobin C. Harding" <me@tobin.cc>
References: <20171218153225.12192-1-tmricht@linux.vnet.ibm.com>
 <20171218125940.117d7696@gandalf.local.home>
 <CA+55aFzPVBHvpB4YP_h6fT_m85RJzf-1N4c47KdLdypigNSs8A@mail.gmail.com>
From: Thomas-Mich Richter <tmricht@linux.vnet.ibm.com>
Organization: IBM LTC
Date: Tue, 19 Dec 2017 09:53:59 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <CA+55aFzPVBHvpB4YP_h6fT_m85RJzf-1N4c47KdLdypigNSs8A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-IE
Content-Transfer-Encoding: 8bit
Message-Id: <8a5a604c-1406-9bc8-a7fa-c355ad24d167@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3336
Lines: 76

On 12/18/2017 07:16 PM, Linus Torvalds wrote:
> On Mon, Dec 18, 2017 at 9:59 AM, Steven Rostedt <rostedt@goodmis.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@s35lp76 ~]# mount | egrep 'debug|trace'
debugfs on /sys/kernel/debug type debugfs (rw,relatime)
tracefs on /sys/kernel/debug/tracing type tracefs (rw,relatime)
[root@s35lp76 ~]# 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@s35lp76 ~]# 

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