[Intel-gfx] [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

[Gupta, Sourab]

On Wed, 2015-08-05 at 10:30 +0000, Chris Wilson wrote:
> On Wed, Aug 05, 2015 at 10:18:50AM +0000, Gupta, Sourab wrote:
> > On Wed, 2015-08-05 at 10:03 +0000, Chris Wilson wrote:
> > > On Wed, Aug 05, 2015 at 11:25:44AM +0530, sourab.gupta at intel.com wrote:
> > > > From: Sourab Gupta <sourab.gupta at intel.com>
> > > > 
> > > > This patch adds support for retrieving MMIO register values alongwith
> > > > timestamps and forwarding them to userspace through perf.
> > > > The userspace can request upto 8 MMIO register values to be dumped.
> > > > The addresses of upto 8 MMIO registers can be passed through perf attr
> > > > config. The registers are checked against a whitelist before passing them
> > > > on. The commands to dump the values of these MMIO registers are then
> > > > inserted into the ring alongwith commands to dump the timestamps.
> > > 
> > > The values reported to userspace are deltas across batches right? We
> > > don't expose the global value to an unprivileged user? It would be nice
> > > to clarify that in perf_init so that the reviewer is aware that
> > > the issue of unprivileged information leak is addressed (or at least
> > > reminded that the register values do not leak!).
> > > -Chris
> > > 
> > Hi Chris,
> > Two things here:
> > 1) Only root is allowed to call event_init for gen pmu. This restriction
> > is there in event_init. (The thought behind this restriction being that
> > we are profiling data across contexts here, so a process wishing to
> > listen to global activity happening in system across all contexts ought
> > to have root priviliges). Is this thought process correct? Should we be
> > supporting non-root users too?
> 
> That is not clear in this patch, so you need to address such concerns at
> least in the changelog, and preferrably with a reminder in the
> whitelist (that these register reads are safe because they are being
> done from a privileged context only - we then have a red flag in case we
> lower it).
> 
> What is the privilege check you are using here exactly?

In the current patch set, during the gen pmu event_init, I'm checking
for root access using the below check:
+	if (!capable(CAP_SYS_ADMIN))
+			return -EACCES;

> 
> For gen pmu, I want it user accessible. How long does it take to execute
> my batches is a common developer query. We may even be able to make
> anonymised information freely available ala top (per-process GPU usage,
> memory usage, though cgroups/namespacing rules probably apply here).
> 

So, aiui the privilige access should be controlled as below:
- For gen pmu, no need to restrict only to root processes. This would
imply that user processes would now be able to gather timestamps for all
the batches (no unpriviliged information leak since timestamps are
inherently anonymised) ..

- For the collection of mmio register data, we have following options:
    - If it is a root process, allow access (is whitelist check
necessary in this case?).
    - If not root, one option is to disallow mmio register dump(probably
not a preferable option?).
    - If not root, second option is to allow mmio dump (after checking
against the whitelist?). In this case, do we send the mmio register
values as they exist or do we anonymise them?. Since my impression was
that perf is expected to simply return the dump of mmio registers
requested, and throw an access error in case of unpriviliged operation.
And if required, how do we anonymise the mmio data?

can you let me know your opinion here wrt the above points. And the
mechanism to anonymise the mmio data.

> > 2) Being already a root, do we need to worry about the unauthorized mmio
> > access while exposing these mmio values through the interface?
> 
> Yes. See above, the information here can be anonymised and useful for
> user processes exactly like TIMESTAMP.
>  
> > In the current patches, the full mmio register value is dumped to be
> > passed on to userspace (no deltas across batches), provided the register
> > is there in the whitelist. Does the question of unpriviliged information
> > leak arise here(the user being root)?
> 
> Not for root.
> -Chris
>