Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hi,
On 03.10.2018 20:01, Jann Horn wrote:
> On Mon, Oct 1, 2018 at 10:53 PM Alexey Budankov
> wrote:
>> 3. Every time an event for ${PMU} is created over perf_event_open():
>>a) the calling thread's euid is checked to belong to ${PMU}_users group
>> and if it does then the event's fd is allocated;
>>b) then traditional checks against perf_event_pranoid content are applied;
>>c) if the file doesn't exist the access is governed by global setting
>> at /proc/sys/kernel/perf_even_paranoid;
>
> You'll also have to make sure that this thing in kernel/events/core.c
> doesn't have any bad effect:
>
> /*
> * Special case software events and allow them to be part of
> * any hardware group.
> */
>
> As in, make sure that you can't smuggle in arbitrary software events
> by attaching them to a whitelisted hardware event.
Yes, makes sense. Please see and comment below.
>
>> Security analysis for uncore IMC, QPI/UPI, PCIe PMUs is still required
>> to be enabled for fine grain control.
>
> And you can't whitelist anything that permits using sampling events
> with arbitrary sample_type.
>
It appears that there is a dependency on the significance of data that PMUs
captures
for later analysis. Currently there are following options for data being
captured
(please correct or extend if something is missing from the list below):
1) Monitored process details:
- system information on a process as a container (of threads, memory data
and
IDs (e.g. open fds) from process specific namespaces and etc.);
- system information on threads as containers (of execution context details);
2) Execution context details:
- memory addresses;
- memory data;
- calculation results;
- calculation state in HW;
3) Monitored process and execution context telemetry data, used for building
various performance metrics and can come from:
- user mode code and OS kernel;
- various parts of HW e.g. core, uncore, peripheral and etc.
Group 2) is the potential leakage source of sensitive process data so if a PMU,
at some mode, samples execution context details then the PMU, working in that
mode,
is the subject for *access* and *scope* control.
On the other hand if captured data contain only the monitored process details
and/or associated execution telemetry, there is probably no sensitive data
leakage
thru that captured data.
For example, if cpu PMU samples PC addresses overtime, e.g. for providing
hotspots-by-function profile, then this requires to be controlled as from
access as
from scope perspective, because PC addresses is execution context details that
can contain sensitive data.
However, if cpu PMU does counting of some metric value, or if software PMU
reads
value of thread active time from the OS, possibly overtime, for later building
some
rating profile, or reading of some HW counter value without attribution to any
execution context details, that is probably not that risky as in the case of
PC address sampling.
Uncore PMUs e.g. memory controller (IMC), interconnect (QPI/UPI) and peripheral
(PCIe)
currently only read counters values that are captured system wide by HW, and
provide
no attribution to any specific execution context details, thus, sensitive
process data.
Based on that,
A) paranoid knob is required for a PMU if it can capture data from group 2)
B) paranoid knob limits scope of capturing sensitive data:
-3 - *scope* is defined by some high level setting
-2 - disabled - no allowed *scope*
-1 - no restrictions - max *scope*
0 - system wide
1 - process user and kernel space
2 - process user space only
C) paranoid knob has to be checked every time the PMU is going to start
capturing sensitive data to avoid capturing beyond the allowed scope.
PMU *access* semantics is derived from fs ACLs and could look like this:
r - read PMU architectural and configuration details, read PMU *access* settings
w - modify PMU *access* settings
x - modify PMU configuration and collect data
So levels of *access* to PMU could look like this:
root=rwx, ${PMU}_users=r-x, other=r--.
Possible examples of *scope* control settings could look like this:
1) system wide user+kernel mode CPU sampling with context switches
and uncore counting:
/proc/sys/kernel/perf_event_paranoid (-2, 2): 0
SW.paranoid (-3, 2):(root=rwx, SW_users=r-x,other=r--): -3
CPU.paranoid (-3, 2):(root=rwx,CPU_users=r-x,other=r--): -3
IMC.paranoid (-3,-1):(root=rwx,IMC_users=r-x,other=r--): -3
UPI.paranoid (-3,-1):(root=rwx,UPI_users=r-x,other=r--): -3
PCI.paranoid (-3,-1):(root=rwx,PCI_users=r-x,other=r--): -3
2) per-process CPU sampling with context switches and uncore counting:
/proc/sys/kernel/perf_event_paranoid (-2, 2): 1|2
SW.paranoid (-3, 2):(root=rwx, SW_users=r-x,other=r--): -3
CPU.paranoid (-3, 2):(root=rwx,CPU_users=r-x,other=r--): -3
IMC.paranoid
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hi,
On 03.10.2018 20:01, Jann Horn wrote:
> On Mon, Oct 1, 2018 at 10:53 PM Alexey Budankov
> wrote:
>> 3. Every time an event for ${PMU} is created over perf_event_open():
>>a) the calling thread's euid is checked to belong to ${PMU}_users group
>> and if it does then the event's fd is allocated;
>>b) then traditional checks against perf_event_pranoid content are applied;
>>c) if the file doesn't exist the access is governed by global setting
>> at /proc/sys/kernel/perf_even_paranoid;
>
> You'll also have to make sure that this thing in kernel/events/core.c
> doesn't have any bad effect:
>
> /*
> * Special case software events and allow them to be part of
> * any hardware group.
> */
>
> As in, make sure that you can't smuggle in arbitrary software events
> by attaching them to a whitelisted hardware event.
Yes, makes sense. Please see and comment below.
>
>> Security analysis for uncore IMC, QPI/UPI, PCIe PMUs is still required
>> to be enabled for fine grain control.
>
> And you can't whitelist anything that permits using sampling events
> with arbitrary sample_type.
>
It appears that there is a dependency on the significance of data that PMUs
captures
for later analysis. Currently there are following options for data being
captured
(please correct or extend if something is missing from the list below):
1) Monitored process details:
- system information on a process as a container (of threads, memory data
and
IDs (e.g. open fds) from process specific namespaces and etc.);
- system information on threads as containers (of execution context details);
2) Execution context details:
- memory addresses;
- memory data;
- calculation results;
- calculation state in HW;
3) Monitored process and execution context telemetry data, used for building
various performance metrics and can come from:
- user mode code and OS kernel;
- various parts of HW e.g. core, uncore, peripheral and etc.
Group 2) is the potential leakage source of sensitive process data so if a PMU,
at some mode, samples execution context details then the PMU, working in that
mode,
is the subject for *access* and *scope* control.
On the other hand if captured data contain only the monitored process details
and/or associated execution telemetry, there is probably no sensitive data
leakage
thru that captured data.
For example, if cpu PMU samples PC addresses overtime, e.g. for providing
hotspots-by-function profile, then this requires to be controlled as from
access as
from scope perspective, because PC addresses is execution context details that
can contain sensitive data.
However, if cpu PMU does counting of some metric value, or if software PMU
reads
value of thread active time from the OS, possibly overtime, for later building
some
rating profile, or reading of some HW counter value without attribution to any
execution context details, that is probably not that risky as in the case of
PC address sampling.
Uncore PMUs e.g. memory controller (IMC), interconnect (QPI/UPI) and peripheral
(PCIe)
currently only read counters values that are captured system wide by HW, and
provide
no attribution to any specific execution context details, thus, sensitive
process data.
Based on that,
A) paranoid knob is required for a PMU if it can capture data from group 2)
B) paranoid knob limits scope of capturing sensitive data:
-3 - *scope* is defined by some high level setting
-2 - disabled - no allowed *scope*
-1 - no restrictions - max *scope*
0 - system wide
1 - process user and kernel space
2 - process user space only
C) paranoid knob has to be checked every time the PMU is going to start
capturing sensitive data to avoid capturing beyond the allowed scope.
PMU *access* semantics is derived from fs ACLs and could look like this:
r - read PMU architectural and configuration details, read PMU *access* settings
w - modify PMU *access* settings
x - modify PMU configuration and collect data
So levels of *access* to PMU could look like this:
root=rwx, ${PMU}_users=r-x, other=r--.
Possible examples of *scope* control settings could look like this:
1) system wide user+kernel mode CPU sampling with context switches
and uncore counting:
/proc/sys/kernel/perf_event_paranoid (-2, 2): 0
SW.paranoid (-3, 2):(root=rwx, SW_users=r-x,other=r--): -3
CPU.paranoid (-3, 2):(root=rwx,CPU_users=r-x,other=r--): -3
IMC.paranoid (-3,-1):(root=rwx,IMC_users=r-x,other=r--): -3
UPI.paranoid (-3,-1):(root=rwx,UPI_users=r-x,other=r--): -3
PCI.paranoid (-3,-1):(root=rwx,PCI_users=r-x,other=r--): -3
2) per-process CPU sampling with context switches and uncore counting:
/proc/sys/kernel/perf_event_paranoid (-2, 2): 1|2
SW.paranoid (-3, 2):(root=rwx, SW_users=r-x,other=r--): -3
CPU.paranoid (-3, 2):(root=rwx,CPU_users=r-x,other=r--): -3
IMC.paranoid
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Mon, Oct 1, 2018 at 10:53 PM Alexey Budankov
wrote:
> On 01.10.2018 19:11, Thomas Gleixner wrote:
> > Peter and I discussed that and we came up with the idea that the file
> > descriptor is not even required, i.e. you could make it backward
> > compatible.
> >
> > perf_event_open() knows which PMU is associated with the event the caller
> > tries to open. So perf_event_open() can try to access/open the special per
> > PMU file on behalf of the caller. That should get the same security
> > treatment like a regular open() from user space. If that succeeds, access
> > is granted.
> >
> > The magic file could still be writeable for root to give general
> > restrictions aside of the file based ones similar to what you are
> > proposing.
>
> Let me wrap up all the requirements and ideas that have been captured so far.
>
> 1. A file [1] is added so that it can belong to a group of users allowed to
> use ${PMU},
>something like this:
>
> ls -alh /sys/bus/event_source/devices/${PMU}/caps/
> total 0
> drwxr-xr-x 2 root root0 Oct 1 20:36 .
> drwxr-xr-x 6 root root0 Oct 1 20:36 ..
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 branches
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 max_precise
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 pmu_name
> -rw-r--r-- root ${PMU}_users paranoid<===
>
>Modifications of file content are allowed to those who can
>modify /proc/sys/kernel/perf_event_paranoid setting.
>
> 2. Semantics and content of the introduced paranoid file is
>similar to /proc/sys/kernel/perf_even_paranoid [2]:
>
>The perf_event_paranoid file can be set to restrict access
>to the performance counters.
>
>2 allow only user-space measurements (default since Linux 4.6).
>1 allow both kernel and user measurements (default before Linux 4.6).
>0 allow access to CPU-specific data but not raw trace‐point samples.
> -1 no restrictions.
>
>The existence of the perf_event_paranoid file is the official method
>for determining if a kernel supports perf_event_open().
>
> 3. Every time an event for ${PMU} is created over perf_event_open():
>a) the calling thread's euid is checked to belong to ${PMU}_users group
> and if it does then the event's fd is allocated;
>b) then traditional checks against perf_event_pranoid content are applied;
>c) if the file doesn't exist the access is governed by global setting
> at /proc/sys/kernel/perf_even_paranoid;
You'll also have to make sure that this thing in kernel/events/core.c
doesn't have any bad effect:
/*
* Special case software events and allow them to be part of
* any hardware group.
*/
As in, make sure that you can't smuggle in arbitrary software events
by attaching them to a whitelisted hardware event.
> 4. Documentation/admin-guide/perf-security.rst file is introduced that:
>a) contains general explanation for fine grained access control;
>b) contains a section with guidance about scope and risk for each PMU
> which is enabled for fine grained access control;
>c) file is extended when more PMUs are enabled for fine grain control;
>
> >
> > The analysis and documentation requirements still remain of course.
>
> Security analysis for uncore IMC, QPI/UPI, PCIe PMUs is still required
> to be enabled for fine grain control.
And you can't whitelist anything that permits using sampling events
with arbitrary sample_type.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Mon, Oct 1, 2018 at 10:53 PM Alexey Budankov
wrote:
> On 01.10.2018 19:11, Thomas Gleixner wrote:
> > Peter and I discussed that and we came up with the idea that the file
> > descriptor is not even required, i.e. you could make it backward
> > compatible.
> >
> > perf_event_open() knows which PMU is associated with the event the caller
> > tries to open. So perf_event_open() can try to access/open the special per
> > PMU file on behalf of the caller. That should get the same security
> > treatment like a regular open() from user space. If that succeeds, access
> > is granted.
> >
> > The magic file could still be writeable for root to give general
> > restrictions aside of the file based ones similar to what you are
> > proposing.
>
> Let me wrap up all the requirements and ideas that have been captured so far.
>
> 1. A file [1] is added so that it can belong to a group of users allowed to
> use ${PMU},
>something like this:
>
> ls -alh /sys/bus/event_source/devices/${PMU}/caps/
> total 0
> drwxr-xr-x 2 root root0 Oct 1 20:36 .
> drwxr-xr-x 6 root root0 Oct 1 20:36 ..
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 branches
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 max_precise
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 pmu_name
> -rw-r--r-- root ${PMU}_users paranoid<===
>
>Modifications of file content are allowed to those who can
>modify /proc/sys/kernel/perf_event_paranoid setting.
>
> 2. Semantics and content of the introduced paranoid file is
>similar to /proc/sys/kernel/perf_even_paranoid [2]:
>
>The perf_event_paranoid file can be set to restrict access
>to the performance counters.
>
>2 allow only user-space measurements (default since Linux 4.6).
>1 allow both kernel and user measurements (default before Linux 4.6).
>0 allow access to CPU-specific data but not raw trace‐point samples.
> -1 no restrictions.
>
>The existence of the perf_event_paranoid file is the official method
>for determining if a kernel supports perf_event_open().
>
> 3. Every time an event for ${PMU} is created over perf_event_open():
>a) the calling thread's euid is checked to belong to ${PMU}_users group
> and if it does then the event's fd is allocated;
>b) then traditional checks against perf_event_pranoid content are applied;
>c) if the file doesn't exist the access is governed by global setting
> at /proc/sys/kernel/perf_even_paranoid;
You'll also have to make sure that this thing in kernel/events/core.c
doesn't have any bad effect:
/*
* Special case software events and allow them to be part of
* any hardware group.
*/
As in, make sure that you can't smuggle in arbitrary software events
by attaching them to a whitelisted hardware event.
> 4. Documentation/admin-guide/perf-security.rst file is introduced that:
>a) contains general explanation for fine grained access control;
>b) contains a section with guidance about scope and risk for each PMU
> which is enabled for fine grained access control;
>c) file is extended when more PMUs are enabled for fine grain control;
>
> >
> > The analysis and documentation requirements still remain of course.
>
> Security analysis for uncore IMC, QPI/UPI, PCIe PMUs is still required
> to be enabled for fine grain control.
And you can't whitelist anything that permits using sampling events
with arbitrary sample_type.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello, On 02.10.2018 9:40, Thomas Gleixner wrote: > > Not only the user group, it really should do the full security checks which > are done on open(). I expect it is already implemented by some internal kernel API so that it could be reused. > >>b) then traditional checks against perf_event_pranoid content are applied; > > Hmm, not sure about that because that might be conflicting. Well, possible contradictions could be converged to some reasonable point during technical review stage. Current perf_event_paranoid semantics is still required for PMUs that are governed by global setting at /proc/sys/kernel/perf_event_paranoid. >> 4. Documentation/admin-guide/perf-security.rst file is introduced that: > > 0) Better documentation of /proc/sys/kernel/perf_even_paranoid Exactly. perf_event_open man7 [1] requires update as well, however this is not a part of kernel source tree so these docs changes are to be mailed TO: mtk.manpa...@gmail.com and CC: linux-...@vger.kernel.org. Thanks, Alexey [1] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello, On 02.10.2018 9:40, Thomas Gleixner wrote: > > Not only the user group, it really should do the full security checks which > are done on open(). I expect it is already implemented by some internal kernel API so that it could be reused. > >>b) then traditional checks against perf_event_pranoid content are applied; > > Hmm, not sure about that because that might be conflicting. Well, possible contradictions could be converged to some reasonable point during technical review stage. Current perf_event_paranoid semantics is still required for PMUs that are governed by global setting at /proc/sys/kernel/perf_event_paranoid. >> 4. Documentation/admin-guide/perf-security.rst file is introduced that: > > 0) Better documentation of /proc/sys/kernel/perf_even_paranoid Exactly. perf_event_open man7 [1] requires update as well, however this is not a part of kernel source tree so these docs changes are to be mailed TO: mtk.manpa...@gmail.com and CC: linux-...@vger.kernel.org. Thanks, Alexey [1] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Alexey,
On Mon, 1 Oct 2018, Alexey Budankov wrote:
> > perf_event_open() knows which PMU is associated with the event the caller
> > tries to open. So perf_event_open() can try to access/open the special per
> > PMU file on behalf of the caller. That should get the same security
> > treatment like a regular open() from user space. If that succeeds, access
> > is granted.
> >
> > The magic file could still be writeable for root to give general
> > restrictions aside of the file based ones similar to what you are
> > proposing.
>
> Let me wrap up all the requirements and ideas that have been captured so far.
>
> 1. A file [1] is added so that it can belong to a group of users allowed to
> use ${PMU},
>something like this:
>
> ls -alh /sys/bus/event_source/devices/${PMU}/caps/
> total 0
> drwxr-xr-x 2 root root0 Oct 1 20:36 .
> drwxr-xr-x 6 root root0 Oct 1 20:36 ..
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 branches
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 max_precise
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 pmu_name
> -rw-r--r-- root ${PMU}_users paranoid<===
Right, though I personaly prefer something like 'access_control' as file
name, but that's bike shed painting realm.
>Modifications of file content are allowed to those who can
>modify /proc/sys/kernel/perf_event_paranoid setting.
>
> 2. Semantics and content of the introduced paranoid file is
>similar to /proc/sys/kernel/perf_even_paranoid [2]:
>
>The perf_event_paranoid file can be set to restrict access
>to the performance counters.
>
>2 allow only user-space measurements (default since Linux 4.6).
>1 allow both kernel and user measurements (default before Linux 4.6).
>0 allow access to CPU-specific data but not raw trace‐point samples.
> -1 no restrictions.
>
>The existence of the perf_event_paranoid file is the official method
>for determining if a kernel supports perf_event_open().
>
> 3. Every time an event for ${PMU} is created over perf_event_open():
>a) the calling thread's euid is checked to belong to ${PMU}_users group
> and if it does then the event's fd is allocated;
Not only the user group, it really should do the full security checks which
are done on open().
>b) then traditional checks against perf_event_pranoid content are applied;
Hmm, not sure about that because that might be conflicting.
>c) if the file doesn't exist the access is governed by global setting
> at /proc/sys/kernel/perf_even_paranoid;
Correct.
> 4. Documentation/admin-guide/perf-security.rst file is introduced that:
0) Better documentation of /proc/sys/kernel/perf_even_paranoid
>a) contains general explanation for fine grained access control;
>b) contains a section with guidance about scope and risk for each PMU
> which is enabled for fine grained access control;
>c) file is extended when more PMUs are enabled for fine grain control;
Thanks,
tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Alexey,
On Mon, 1 Oct 2018, Alexey Budankov wrote:
> > perf_event_open() knows which PMU is associated with the event the caller
> > tries to open. So perf_event_open() can try to access/open the special per
> > PMU file on behalf of the caller. That should get the same security
> > treatment like a regular open() from user space. If that succeeds, access
> > is granted.
> >
> > The magic file could still be writeable for root to give general
> > restrictions aside of the file based ones similar to what you are
> > proposing.
>
> Let me wrap up all the requirements and ideas that have been captured so far.
>
> 1. A file [1] is added so that it can belong to a group of users allowed to
> use ${PMU},
>something like this:
>
> ls -alh /sys/bus/event_source/devices/${PMU}/caps/
> total 0
> drwxr-xr-x 2 root root0 Oct 1 20:36 .
> drwxr-xr-x 6 root root0 Oct 1 20:36 ..
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 branches
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 max_precise
> -r--r--r-- 1 root root 4.0K Oct 1 20:36 pmu_name
> -rw-r--r-- root ${PMU}_users paranoid<===
Right, though I personaly prefer something like 'access_control' as file
name, but that's bike shed painting realm.
>Modifications of file content are allowed to those who can
>modify /proc/sys/kernel/perf_event_paranoid setting.
>
> 2. Semantics and content of the introduced paranoid file is
>similar to /proc/sys/kernel/perf_even_paranoid [2]:
>
>The perf_event_paranoid file can be set to restrict access
>to the performance counters.
>
>2 allow only user-space measurements (default since Linux 4.6).
>1 allow both kernel and user measurements (default before Linux 4.6).
>0 allow access to CPU-specific data but not raw trace‐point samples.
> -1 no restrictions.
>
>The existence of the perf_event_paranoid file is the official method
>for determining if a kernel supports perf_event_open().
>
> 3. Every time an event for ${PMU} is created over perf_event_open():
>a) the calling thread's euid is checked to belong to ${PMU}_users group
> and if it does then the event's fd is allocated;
Not only the user group, it really should do the full security checks which
are done on open().
>b) then traditional checks against perf_event_pranoid content are applied;
Hmm, not sure about that because that might be conflicting.
>c) if the file doesn't exist the access is governed by global setting
> at /proc/sys/kernel/perf_even_paranoid;
Correct.
> 4. Documentation/admin-guide/perf-security.rst file is introduced that:
0) Better documentation of /proc/sys/kernel/perf_even_paranoid
>a) contains general explanation for fine grained access control;
>b) contains a section with guidance about scope and risk for each PMU
> which is enabled for fine grained access control;
>c) file is extended when more PMUs are enabled for fine grain control;
Thanks,
tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello,
On 01.10.2018 19:11, Thomas Gleixner wrote:
> Peter and I discussed that and we came up with the idea that the file
> descriptor is not even required, i.e. you could make it backward
> compatible.
>
> perf_event_open() knows which PMU is associated with the event the caller
> tries to open. So perf_event_open() can try to access/open the special per
> PMU file on behalf of the caller. That should get the same security
> treatment like a regular open() from user space. If that succeeds, access
> is granted.
>
> The magic file could still be writeable for root to give general
> restrictions aside of the file based ones similar to what you are
> proposing.
Let me wrap up all the requirements and ideas that have been captured so far.
1. A file [1] is added so that it can belong to a group of users allowed to use
${PMU},
something like this:
ls -alh /sys/bus/event_source/devices/${PMU}/caps/
total 0
drwxr-xr-x 2 root root0 Oct 1 20:36 .
drwxr-xr-x 6 root root0 Oct 1 20:36 ..
-r--r--r-- 1 root root 4.0K Oct 1 20:36 branches
-r--r--r-- 1 root root 4.0K Oct 1 20:36 max_precise
-r--r--r-- 1 root root 4.0K Oct 1 20:36 pmu_name
-rw-r--r-- root ${PMU}_users paranoid<===
Modifications of file content are allowed to those who can
modify /proc/sys/kernel/perf_event_paranoid setting.
2. Semantics and content of the introduced paranoid file is
similar to /proc/sys/kernel/perf_even_paranoid [2]:
The perf_event_paranoid file can be set to restrict access
to the performance counters.
2 allow only user-space measurements (default since Linux 4.6).
1 allow both kernel and user measurements (default before Linux 4.6).
0 allow access to CPU-specific data but not raw trace‐point samples.
-1 no restrictions.
The existence of the perf_event_paranoid file is the official method
for determining if a kernel supports perf_event_open().
3. Every time an event for ${PMU} is created over perf_event_open():
a) the calling thread's euid is checked to belong to ${PMU}_users group
and if it does then the event's fd is allocated;
b) then traditional checks against perf_event_pranoid content are applied;
c) if the file doesn't exist the access is governed by global setting
at /proc/sys/kernel/perf_even_paranoid;
4. Documentation/admin-guide/perf-security.rst file is introduced that:
a) contains general explanation for fine grained access control;
b) contains a section with guidance about scope and risk for each PMU
which is enabled for fine grained access control;
c) file is extended when more PMUs are enabled for fine grain control;
>
> The analysis and documentation requirements still remain of course.
Security analysis for uncore IMC, QPI/UPI, PCIe PMUs is still required
to be enabled for fine grain control.
Thanks,
Alexey
[1] https://patchwork.kernel.org/patch/9249919/#19714087
[2] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello,
On 01.10.2018 19:11, Thomas Gleixner wrote:
> Peter and I discussed that and we came up with the idea that the file
> descriptor is not even required, i.e. you could make it backward
> compatible.
>
> perf_event_open() knows which PMU is associated with the event the caller
> tries to open. So perf_event_open() can try to access/open the special per
> PMU file on behalf of the caller. That should get the same security
> treatment like a regular open() from user space. If that succeeds, access
> is granted.
>
> The magic file could still be writeable for root to give general
> restrictions aside of the file based ones similar to what you are
> proposing.
Let me wrap up all the requirements and ideas that have been captured so far.
1. A file [1] is added so that it can belong to a group of users allowed to use
${PMU},
something like this:
ls -alh /sys/bus/event_source/devices/${PMU}/caps/
total 0
drwxr-xr-x 2 root root0 Oct 1 20:36 .
drwxr-xr-x 6 root root0 Oct 1 20:36 ..
-r--r--r-- 1 root root 4.0K Oct 1 20:36 branches
-r--r--r-- 1 root root 4.0K Oct 1 20:36 max_precise
-r--r--r-- 1 root root 4.0K Oct 1 20:36 pmu_name
-rw-r--r-- root ${PMU}_users paranoid<===
Modifications of file content are allowed to those who can
modify /proc/sys/kernel/perf_event_paranoid setting.
2. Semantics and content of the introduced paranoid file is
similar to /proc/sys/kernel/perf_even_paranoid [2]:
The perf_event_paranoid file can be set to restrict access
to the performance counters.
2 allow only user-space measurements (default since Linux 4.6).
1 allow both kernel and user measurements (default before Linux 4.6).
0 allow access to CPU-specific data but not raw trace‐point samples.
-1 no restrictions.
The existence of the perf_event_paranoid file is the official method
for determining if a kernel supports perf_event_open().
3. Every time an event for ${PMU} is created over perf_event_open():
a) the calling thread's euid is checked to belong to ${PMU}_users group
and if it does then the event's fd is allocated;
b) then traditional checks against perf_event_pranoid content are applied;
c) if the file doesn't exist the access is governed by global setting
at /proc/sys/kernel/perf_even_paranoid;
4. Documentation/admin-guide/perf-security.rst file is introduced that:
a) contains general explanation for fine grained access control;
b) contains a section with guidance about scope and risk for each PMU
which is enabled for fine grained access control;
c) file is extended when more PMUs are enabled for fine grain control;
>
> The analysis and documentation requirements still remain of course.
Security analysis for uncore IMC, QPI/UPI, PCIe PMUs is still required
to be enabled for fine grain control.
Thanks,
Alexey
[1] https://patchwork.kernel.org/patch/9249919/#19714087
[2] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Mon, Oct 1, 2018 at 6:12 PM Thomas Gleixner wrote: > From a design POV, Jann's idea to have a per PMU special file which you > need to open for getting access is way better than the extra knobs. It > allows to use all existing security mechanisms to be used. (That was Mark's idea, not mine, I just agree with his idea a lot.)
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Mon, Oct 1, 2018 at 6:12 PM Thomas Gleixner wrote: > From a design POV, Jann's idea to have a per PMU special file which you > need to open for getting access is way better than the extra knobs. It > allows to use all existing security mechanisms to be used. (That was Mark's idea, not mine, I just agree with his idea a lot.)
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Alexey, On Mon, 1 Oct 2018, Alexey Budankov wrote: > > On Fri, Sep 28, 2018 at 11:22:37PM +0200, Jann Horn wrote: > >> > >> Is that true? IIRC if you want to use the perf tools after a kernel > >> update, you have to install a new version of perf anyway, no? > > There are usages in production where perf_event_open() syscall > accompanied with read(), mmap() etc. is embedded into application > on per-thread basis and is used for self monitoring and dynamic > execution tuning. > > There are also other Perf tools around that, for example, are > statically linked and then used as on Linux as on Android. > > Backward compatibility does matter in these cases. Well, it's nothing fundamentally new, that new features require changes to applications, libraries etc. It's nice if it can be avoided of course. >From a design POV, Jann's idea to have a per PMU special file which you need to open for getting access is way better than the extra knobs. It allows to use all existing security mechanisms to be used. Peter and I discussed that and we came up with the idea that the file descriptor is not even required, i.e. you could make it backward compatible. perf_event_open() knows which PMU is associated with the event the caller tries to open. So perf_event_open() can try to access/open the special per PMU file on behalf of the caller. That should get the same security treatment like a regular open() from user space. If that succeeds, access is granted. The magic file could still be writeable for root to give general restrictions aside of the file based ones similar to what you are proposing. The analysis and documentation requirements still remain of course. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Alexey, On Mon, 1 Oct 2018, Alexey Budankov wrote: > > On Fri, Sep 28, 2018 at 11:22:37PM +0200, Jann Horn wrote: > >> > >> Is that true? IIRC if you want to use the perf tools after a kernel > >> update, you have to install a new version of perf anyway, no? > > There are usages in production where perf_event_open() syscall > accompanied with read(), mmap() etc. is embedded into application > on per-thread basis and is used for self monitoring and dynamic > execution tuning. > > There are also other Perf tools around that, for example, are > statically linked and then used as on Linux as on Android. > > Backward compatibility does matter in these cases. Well, it's nothing fundamentally new, that new features require changes to applications, libraries etc. It's nice if it can be avoided of course. >From a design POV, Jann's idea to have a per PMU special file which you need to open for getting access is way better than the extra knobs. It allows to use all existing security mechanisms to be used. Peter and I discussed that and we came up with the idea that the file descriptor is not even required, i.e. you could make it backward compatible. perf_event_open() knows which PMU is associated with the event the caller tries to open. So perf_event_open() can try to access/open the special per PMU file on behalf of the caller. That should get the same security treatment like a regular open() from user space. If that succeeds, access is granted. The magic file could still be writeable for root to give general restrictions aside of the file based ones similar to what you are proposing. The analysis and documentation requirements still remain of course. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello Jann and Kees, On 29.09.2018 1:02, Jann Horn wrote: > Ah, I guess the answer is "0", since you want to see data about what > other users are doing. > > Does the i915 PMU expose sampling events, counting events, or both? > The thing about sampling events is that they AFAIK always let the user > pick arbitrary data to collect - like register contents, or userspace > stack memory -, and independent of the performance counter being > monitored, this kind of access should not be permitted to other > contexts. (But it might be that I misunderstand how perf works - I'm > not super familiar with its API.) > Currently *core* paranoid >= 1 (per-process mode) prevents simultaneous sampling on CPU events (perf record) and reading of uncore HW counters (perf stat -I), because uncore counters count system wide and that is allowed only when *core* paranoid <= 0. Uncore counts collected simultaneously with CPU event samples can be correlated using timestamps taken from some common system clock e.g. CLOCK_MONOTONIC_RAW. Could it be secure enough to still allow reading of system wide uncore HW counters when sampling of CPU events is limited to specific processes by *core* paranoid >= 1? Thanks, Alexey
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello Jann and Kees, On 29.09.2018 1:02, Jann Horn wrote: > Ah, I guess the answer is "0", since you want to see data about what > other users are doing. > > Does the i915 PMU expose sampling events, counting events, or both? > The thing about sampling events is that they AFAIK always let the user > pick arbitrary data to collect - like register contents, or userspace > stack memory -, and independent of the performance counter being > monitored, this kind of access should not be permitted to other > contexts. (But it might be that I misunderstand how perf works - I'm > not super familiar with its API.) > Currently *core* paranoid >= 1 (per-process mode) prevents simultaneous sampling on CPU events (perf record) and reading of uncore HW counters (perf stat -I), because uncore counters count system wide and that is allowed only when *core* paranoid <= 0. Uncore counts collected simultaneously with CPU event samples can be correlated using timestamps taken from some common system clock e.g. CLOCK_MONOTONIC_RAW. Could it be secure enough to still allow reading of system wide uncore HW counters when sampling of CPU events is limited to specific processes by *core* paranoid >= 1? Thanks, Alexey
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello Jann, > On Fri, Sep 28, 2018 at 11:22:37PM +0200, Jann Horn wrote: >> >> >> Is that true? IIRC if you want to use the perf tools after a kernel >> update, you have to install a new version of perf anyway, no? There are usages in production where perf_event_open() syscall accompanied with read(), mmap() etc. is embedded into application on per-thread basis and is used for self monitoring and dynamic execution tuning. There are also other Perf tools around that, for example, are statically linked and then used as on Linux as on Android. Backward compatibility does matter in these cases. Thanks, Alexey
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello Jann, > On Fri, Sep 28, 2018 at 11:22:37PM +0200, Jann Horn wrote: >> >> >> Is that true? IIRC if you want to use the perf tools after a kernel >> update, you have to install a new version of perf anyway, no? There are usages in production where perf_event_open() syscall accompanied with read(), mmap() etc. is embedded into application on per-thread basis and is used for self monitoring and dynamic execution tuning. There are also other Perf tools around that, for example, are statically linked and then used as on Linux as on Android. Backward compatibility does matter in these cases. Thanks, Alexey
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello, On 28.09.2018 21:20, Thomas Gleixner wrote: > Start with something like Documentation/admin-guide/perf-security.rst or > whatever name fits better and add a proper documentation for the existing > knob. With the infrastructure for fine grained access control add the > general explanation for fine grained access control. With each PMU which > opt's in for the knob, add a section with guidance about scope and risk for > this particular one. Sounds like a plan. Thanks! BR, Alexey > > Thanks, > > tglx > > >
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello, On 28.09.2018 21:20, Thomas Gleixner wrote: > Start with something like Documentation/admin-guide/perf-security.rst or > whatever name fits better and add a proper documentation for the existing > knob. With the infrastructure for fine grained access control add the > general explanation for fine grained access control. With each PMU which > opt's in for the knob, add a section with guidance about scope and risk for > this particular one. Sounds like a plan. Thanks! BR, Alexey > > Thanks, > > tglx > > >
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, 28 Sep 2018, Andi Kleen wrote: > > > This new file descriptor argument doesn't exist today so it would > > > need to create a new system call with more arguments > > > > Is that true? The first argument is a pointer to a struct that > > contains its own size, so it can be expanded without an ABI break. I > > don't see any reason why you couldn't cram more stuff in there. > > You're right we could put the fd into the perf_event, but the following is > still true: > > > > Obviously we would need to keep the old system call around > > > for compability, so you would need to worry about this > > > interaction in any case! > > > > > > So tying it together doesn't make any sense, because > > > the problem has to be solved separately anyways. And why so? You can keep the original functionality around with the existing restrictions without breaking any existing user space. That existing functionality does not require new knobs. It stays as is. So if you want to use the enhanced version with per PMU permissions based on file descriptors you need a new version of perf. That's nothing new, if the kernel adds new features to any syscall, then you need new tools, new libraries etc. The only guarantee the kernel makes is not to break existing user space, but there is no guarantee that you can utilize new features with existing userspace. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, 28 Sep 2018, Andi Kleen wrote: > > > This new file descriptor argument doesn't exist today so it would > > > need to create a new system call with more arguments > > > > Is that true? The first argument is a pointer to a struct that > > contains its own size, so it can be expanded without an ABI break. I > > don't see any reason why you couldn't cram more stuff in there. > > You're right we could put the fd into the perf_event, but the following is > still true: > > > > Obviously we would need to keep the old system call around > > > for compability, so you would need to worry about this > > > interaction in any case! > > > > > > So tying it together doesn't make any sense, because > > > the problem has to be solved separately anyways. And why so? You can keep the original functionality around with the existing restrictions without breaking any existing user space. That existing functionality does not require new knobs. It stays as is. So if you want to use the enhanced version with per PMU permissions based on file descriptors you need a new version of perf. That's nothing new, if the kernel adds new features to any syscall, then you need new tools, new libraries etc. The only guarantee the kernel makes is not to break existing user space, but there is no guarantee that you can utilize new features with existing userspace. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, 28 Sep 2018, Andi Kleen wrote: > So there isn't a single answer, and that is why it is important > that this if configurable. I said clearly that I'm not opposed against making it configurable. But because there is no single answer, it's even more important to have proper documentation. And that's all I'm asking for aside of making it opt-in instead of a wholesale expose everything approach. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, 28 Sep 2018, Andi Kleen wrote: > So there isn't a single answer, and that is why it is important > that this if configurable. I said clearly that I'm not opposed against making it configurable. But because there is no single answer, it's even more important to have proper documentation. And that's all I'm asking for aside of making it opt-in instead of a wholesale expose everything approach. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 5:12 PM Jann Horn wrote: > > On Fri, Sep 28, 2018 at 3:22 PM Tvrtko Ursulin > wrote: > > On 28/09/2018 11:26, Thomas Gleixner wrote: > > > On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: > > >> For situations where sysadmins might want to allow different level of > > >> access control for different PMUs, we start creating per-PMU > > >> perf_event_paranoid controls in sysfs. > > >> > > >> These work in equivalent fashion as the existing perf_event_paranoid > > >> sysctl, which now becomes the parent control for each PMU. > > >> > > >> On PMU registration the global/parent value will be inherited by each > > >> PMU, > > >> as it will be propagated to all registered PMUs when the sysctl is > > >> updated. > > >> > > >> At any later point individual PMU access controls, located in > > >> /device//perf_event_paranoid, can be adjusted to achieve > > >> fine grained access control. > > >> > > >> Discussion from previous posting: > > >> https://lkml.org/lkml/2018/5/21/156 > > > > > > This is really not helpful. The cover letter and the change logs should > > > contain a summary of that discussion and a proper justification of the > > > proposed change. Just saying 'sysadmins might want to allow' is not useful > > > at all, it's yet another 'I want a pony' thing. > > > > Okay, for the next round I will expand the cover letter with at least > > one concrete example on how it is usable and summarize the discussion a bit. > > > > > I read through the previous thread and there was a clear request to > > > involve > > > security people into this. Especially those who are deeply involved with > > > hardware side channels. I don't see anyone Cc'ed on the whole series. > > > > Who would you recommend I add? Because I really don't know.. > > > > > For the record, I'm not buying the handwavy 'more noise' argument at > > > all. It wants a proper analysis and we need to come up with criteria which > > > PMUs can be exposed at all. > > > > > > All of this want's a proper documentation clearly explaining the risks and > > > scope of these knobs per PMU. Just throwing magic knobs at sysadmins and > > > then saying 'its their problem to figure it out' is not acceptable. > > > > Presumably you see adding fine grained control as diminishing the > > overall security rather than raising it? Could you explain why? Because > > incompetent sysadmin will turn it off for some PMU, while without having > > the fine-grained control they wouldn't turn it off globally? > > > > This feature was requested by the exact opposite concern, that in order > > to access the i915 PMU, one has to compromise the security of the entire > > system by allowing access to *all* PMU's. > > > > Making this ability fine-grained sounds like a logical solution for > > solving this weakening of security controls. > > > > Concrete example was that on video transcoding farms users want to > > monitor the utilization of GPU engines (like CPU cores) and they can do > > that via the i915 PMU. But for that to work today they have to dial down > > the global perf_event_paranoid setting. Obvious improvement was to allow > > them to only dial down the i915.perf_event_paranoid setting. As such, > > for this specific use case at least, the security is increased. > > Which paranoia level would be used for the i915.perf_event_paranoid > setting in such a case? Ah, I guess the answer is "0", since you want to see data about what other users are doing. Does the i915 PMU expose sampling events, counting events, or both? The thing about sampling events is that they AFAIK always let the user pick arbitrary data to collect - like register contents, or userspace stack memory -, and independent of the performance counter being monitored, this kind of access should not be permitted to other contexts. (But it might be that I misunderstand how perf works - I'm not super familiar with its API.)
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 5:12 PM Jann Horn wrote: > > On Fri, Sep 28, 2018 at 3:22 PM Tvrtko Ursulin > wrote: > > On 28/09/2018 11:26, Thomas Gleixner wrote: > > > On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: > > >> For situations where sysadmins might want to allow different level of > > >> access control for different PMUs, we start creating per-PMU > > >> perf_event_paranoid controls in sysfs. > > >> > > >> These work in equivalent fashion as the existing perf_event_paranoid > > >> sysctl, which now becomes the parent control for each PMU. > > >> > > >> On PMU registration the global/parent value will be inherited by each > > >> PMU, > > >> as it will be propagated to all registered PMUs when the sysctl is > > >> updated. > > >> > > >> At any later point individual PMU access controls, located in > > >> /device//perf_event_paranoid, can be adjusted to achieve > > >> fine grained access control. > > >> > > >> Discussion from previous posting: > > >> https://lkml.org/lkml/2018/5/21/156 > > > > > > This is really not helpful. The cover letter and the change logs should > > > contain a summary of that discussion and a proper justification of the > > > proposed change. Just saying 'sysadmins might want to allow' is not useful > > > at all, it's yet another 'I want a pony' thing. > > > > Okay, for the next round I will expand the cover letter with at least > > one concrete example on how it is usable and summarize the discussion a bit. > > > > > I read through the previous thread and there was a clear request to > > > involve > > > security people into this. Especially those who are deeply involved with > > > hardware side channels. I don't see anyone Cc'ed on the whole series. > > > > Who would you recommend I add? Because I really don't know.. > > > > > For the record, I'm not buying the handwavy 'more noise' argument at > > > all. It wants a proper analysis and we need to come up with criteria which > > > PMUs can be exposed at all. > > > > > > All of this want's a proper documentation clearly explaining the risks and > > > scope of these knobs per PMU. Just throwing magic knobs at sysadmins and > > > then saying 'its their problem to figure it out' is not acceptable. > > > > Presumably you see adding fine grained control as diminishing the > > overall security rather than raising it? Could you explain why? Because > > incompetent sysadmin will turn it off for some PMU, while without having > > the fine-grained control they wouldn't turn it off globally? > > > > This feature was requested by the exact opposite concern, that in order > > to access the i915 PMU, one has to compromise the security of the entire > > system by allowing access to *all* PMU's. > > > > Making this ability fine-grained sounds like a logical solution for > > solving this weakening of security controls. > > > > Concrete example was that on video transcoding farms users want to > > monitor the utilization of GPU engines (like CPU cores) and they can do > > that via the i915 PMU. But for that to work today they have to dial down > > the global perf_event_paranoid setting. Obvious improvement was to allow > > them to only dial down the i915.perf_event_paranoid setting. As such, > > for this specific use case at least, the security is increased. > > Which paranoia level would be used for the i915.perf_event_paranoid > setting in such a case? Ah, I guess the answer is "0", since you want to see data about what other users are doing. Does the i915 PMU expose sampling events, counting events, or both? The thing about sampling events is that they AFAIK always let the user pick arbitrary data to collect - like register contents, or userspace stack memory -, and independent of the performance counter being monitored, this kind of access should not be permitted to other contexts. (But it might be that I misunderstand how perf works - I'm not super familiar with its API.)
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 11:22:37PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 10:59 PM Andi Kleen wrote: > > > > This new file descriptor argument doesn't exist today so it would > > > > need to create a new system call with more arguments > > > > > > Is that true? The first argument is a pointer to a struct that > > > contains its own size, so it can be expanded without an ABI break. I > > > don't see any reason why you couldn't cram more stuff in there. > > > > You're right we could put the fd into the perf_event, but the following is > > still true: > > > > > > Obviously we would need to keep the old system call around > > > > for compability, so you would need to worry about this > > > > interaction in any case! > > > Is that true? IIRC if you want to use the perf tools after a kernel > update, you have to install a new version of perf anyway, no? Not at all. perf is fully ABI compatible. Yes Ubuntu/Debian make you do it, but there is no reason for it other than their ignorance. Other sane distributions don't. Usually the first step when I'm forced to use one of those machine is to remove the useless wrapper and call the perf binary directly. -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 11:22:37PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 10:59 PM Andi Kleen wrote: > > > > This new file descriptor argument doesn't exist today so it would > > > > need to create a new system call with more arguments > > > > > > Is that true? The first argument is a pointer to a struct that > > > contains its own size, so it can be expanded without an ABI break. I > > > don't see any reason why you couldn't cram more stuff in there. > > > > You're right we could put the fd into the perf_event, but the following is > > still true: > > > > > > Obviously we would need to keep the old system call around > > > > for compability, so you would need to worry about this > > > > interaction in any case! > > > Is that true? IIRC if you want to use the perf tools after a kernel > update, you have to install a new version of perf anyway, no? Not at all. perf is fully ABI compatible. Yes Ubuntu/Debian make you do it, but there is no reason for it other than their ignorance. Other sane distributions don't. Usually the first step when I'm forced to use one of those machine is to remove the useless wrapper and call the perf binary directly. -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 10:59 PM Andi Kleen wrote: > > > This new file descriptor argument doesn't exist today so it would > > > need to create a new system call with more arguments > > > > Is that true? The first argument is a pointer to a struct that > > contains its own size, so it can be expanded without an ABI break. I > > don't see any reason why you couldn't cram more stuff in there. > > You're right we could put the fd into the perf_event, but the following is > still true: > > > > Obviously we would need to keep the old system call around > > > for compability, so you would need to worry about this > > > interaction in any case! Is that true? IIRC if you want to use the perf tools after a kernel update, you have to install a new version of perf anyway, no? I think after I run a kernel update, when I run "perf top", it just refuses to start and tells me to go install a newer version. Would the users of perf_event_open() that want to monitor this graphics stuff normally keep working after a kernel version bump? I realize that the kernel is very much against breaking userspace interfaces, but if userspace has already decided to break itself after every update, we might as well take advantage of that... > > > So tying it together doesn't make any sense, because > > > the problem has to be solved separately anyways.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 10:59 PM Andi Kleen wrote: > > > This new file descriptor argument doesn't exist today so it would > > > need to create a new system call with more arguments > > > > Is that true? The first argument is a pointer to a struct that > > contains its own size, so it can be expanded without an ABI break. I > > don't see any reason why you couldn't cram more stuff in there. > > You're right we could put the fd into the perf_event, but the following is > still true: > > > > Obviously we would need to keep the old system call around > > > for compability, so you would need to worry about this > > > interaction in any case! Is that true? IIRC if you want to use the perf tools after a kernel update, you have to install a new version of perf anyway, no? I think after I run a kernel update, when I run "perf top", it just refuses to start and tells me to go install a newer version. Would the users of perf_event_open() that want to monitor this graphics stuff normally keep working after a kernel version bump? I realize that the kernel is very much against breaking userspace interfaces, but if userspace has already decided to break itself after every update, we might as well take advantage of that... > > > So tying it together doesn't make any sense, because > > > the problem has to be solved separately anyways.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
> > This new file descriptor argument doesn't exist today so it would > > need to create a new system call with more arguments > > Is that true? The first argument is a pointer to a struct that > contains its own size, so it can be expanded without an ABI break. I > don't see any reason why you couldn't cram more stuff in there. You're right we could put the fd into the perf_event, but the following is still true: > > Obviously we would need to keep the old system call around > > for compability, so you would need to worry about this > > interaction in any case! > > > > So tying it together doesn't make any sense, because > > the problem has to be solved separately anyways. -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
> > This new file descriptor argument doesn't exist today so it would > > need to create a new system call with more arguments > > Is that true? The first argument is a pointer to a struct that > contains its own size, so it can be expanded without an ABI break. I > don't see any reason why you couldn't cram more stuff in there. You're right we could put the fd into the perf_event, but the following is still true: > > Obviously we would need to keep the old system call around > > for compability, so you would need to worry about this > > interaction in any case! > > > > So tying it together doesn't make any sense, because > > the problem has to be solved separately anyways. -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 10:49 PM Andi Kleen wrote:
>
> On Fri, Sep 28, 2018 at 06:40:17PM +0100, Mark Rutland wrote:
> > On Fri, Sep 28, 2018 at 10:23:40AM -0700, Andi Kleen wrote:
> > > > There's also been prior discussion on these feature in other contexts
> > > > (e.g. android expoits resulting from out-of-tree drivers). It would be
> > > > nice to see those considered.
> > > >
> > > > IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> > > > finer granularity of control such that we could limit PMU access to
> > > > specific users -- e.g. disallow arbitrary android apps from poking *any*
> > > > PMU, while allowing some more trusted apps/users to uses *some* specific
> > > > PMUs.
> > > >
> > > > e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> > > > this via the usual fs ACLs, and pass the fd to perf_event_open()
> > > > somehow. A valid fd would act as a capability, taking precedence over
> > > > perf_event_paranoid.
> > >
> > > That sounds like an orthogonal feature. I don't think the original
> > > patchkit would need to be hold up for this. It would be something
> > > in addition.
> >
> > I have to say that I disagree -- these controls will have to interact
> > somehow, and the fewer of them we have, the less complexity we'll have
> > to deal with longer-term.
>
> You're proposing to completely redesign perf_event_open.
And I think it would be a very good redesign. :) I love things that
use file descriptors to represent capabilities.
> This new file descriptor argument doesn't exist today so it would
> need to create a new system call with more arguments
Is that true? The first argument is a pointer to a struct that
contains its own size, so it can be expanded without an ABI break. I
don't see any reason why you couldn't cram more stuff in there.
> (and BTW it would be more than the normal 6 argument limit
> we have, so actually it couldn't even be a standard sycall)
>
> Obviously we would need to keep the old system call around
> for compability, so you would need to worry about this
> interaction in any case!
>
> So tying it together doesn't make any sense, because
> the problem has to be solved separately anyways.
>
> >
> > > BTW can't you already do that with the syscall filter? I assume
> > > the Android sandboxes already use that. Just forbid perf_event_open
> > > for the apps.
> >
> > Note that this was about providing access to *some* PMUs in some cases.
> >
> > IIUC, if that can be done today via a syscall filter, the same is true
> > of per-pmu paranoid settings.
>
> The difference is that the Android sandboxes likely already doing this
> and have all the infrastructure, and it's just another rule.
>
> Requiring syscall filters just to use the PMU on xn system
> that otherwise doesn't need them would be very odd.
>
> -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 10:49 PM Andi Kleen wrote:
>
> On Fri, Sep 28, 2018 at 06:40:17PM +0100, Mark Rutland wrote:
> > On Fri, Sep 28, 2018 at 10:23:40AM -0700, Andi Kleen wrote:
> > > > There's also been prior discussion on these feature in other contexts
> > > > (e.g. android expoits resulting from out-of-tree drivers). It would be
> > > > nice to see those considered.
> > > >
> > > > IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> > > > finer granularity of control such that we could limit PMU access to
> > > > specific users -- e.g. disallow arbitrary android apps from poking *any*
> > > > PMU, while allowing some more trusted apps/users to uses *some* specific
> > > > PMUs.
> > > >
> > > > e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> > > > this via the usual fs ACLs, and pass the fd to perf_event_open()
> > > > somehow. A valid fd would act as a capability, taking precedence over
> > > > perf_event_paranoid.
> > >
> > > That sounds like an orthogonal feature. I don't think the original
> > > patchkit would need to be hold up for this. It would be something
> > > in addition.
> >
> > I have to say that I disagree -- these controls will have to interact
> > somehow, and the fewer of them we have, the less complexity we'll have
> > to deal with longer-term.
>
> You're proposing to completely redesign perf_event_open.
And I think it would be a very good redesign. :) I love things that
use file descriptors to represent capabilities.
> This new file descriptor argument doesn't exist today so it would
> need to create a new system call with more arguments
Is that true? The first argument is a pointer to a struct that
contains its own size, so it can be expanded without an ABI break. I
don't see any reason why you couldn't cram more stuff in there.
> (and BTW it would be more than the normal 6 argument limit
> we have, so actually it couldn't even be a standard sycall)
>
> Obviously we would need to keep the old system call around
> for compability, so you would need to worry about this
> interaction in any case!
>
> So tying it together doesn't make any sense, because
> the problem has to be solved separately anyways.
>
> >
> > > BTW can't you already do that with the syscall filter? I assume
> > > the Android sandboxes already use that. Just forbid perf_event_open
> > > for the apps.
> >
> > Note that this was about providing access to *some* PMUs in some cases.
> >
> > IIUC, if that can be done today via a syscall filter, the same is true
> > of per-pmu paranoid settings.
>
> The difference is that the Android sandboxes likely already doing this
> and have all the infrastructure, and it's just another rule.
>
> Requiring syscall filters just to use the PMU on xn system
> that otherwise doesn't need them would be very odd.
>
> -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 06:40:17PM +0100, Mark Rutland wrote:
> On Fri, Sep 28, 2018 at 10:23:40AM -0700, Andi Kleen wrote:
> > > There's also been prior discussion on these feature in other contexts
> > > (e.g. android expoits resulting from out-of-tree drivers). It would be
> > > nice to see those considered.
> > >
> > > IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> > > finer granularity of control such that we could limit PMU access to
> > > specific users -- e.g. disallow arbitrary android apps from poking *any*
> > > PMU, while allowing some more trusted apps/users to uses *some* specific
> > > PMUs.
> > >
> > > e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> > > this via the usual fs ACLs, and pass the fd to perf_event_open()
> > > somehow. A valid fd would act as a capability, taking precedence over
> > > perf_event_paranoid.
> >
> > That sounds like an orthogonal feature. I don't think the original
> > patchkit would need to be hold up for this. It would be something
> > in addition.
>
> I have to say that I disagree -- these controls will have to interact
> somehow, and the fewer of them we have, the less complexity we'll have
> to deal with longer-term.
You're proposing to completely redesign perf_event_open.
This new file descriptor argument doesn't exist today so it would
need to create a new system call with more arguments
(and BTW it would be more than the normal 6 argument limit
we have, so actually it couldn't even be a standard sycall)
Obviously we would need to keep the old system call around
for compability, so you would need to worry about this
interaction in any case!
So tying it together doesn't make any sense, because
the problem has to be solved separately anyways.
>
> > BTW can't you already do that with the syscall filter? I assume
> > the Android sandboxes already use that. Just forbid perf_event_open
> > for the apps.
>
> Note that this was about providing access to *some* PMUs in some cases.
>
> IIUC, if that can be done today via a syscall filter, the same is true
> of per-pmu paranoid settings.
The difference is that the Android sandboxes likely already doing this
and have all the infrastructure, and it's just another rule.
Requiring syscall filters just to use the PMU on xn system
that otherwise doesn't need them would be very odd.
-Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 06:40:17PM +0100, Mark Rutland wrote:
> On Fri, Sep 28, 2018 at 10:23:40AM -0700, Andi Kleen wrote:
> > > There's also been prior discussion on these feature in other contexts
> > > (e.g. android expoits resulting from out-of-tree drivers). It would be
> > > nice to see those considered.
> > >
> > > IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> > > finer granularity of control such that we could limit PMU access to
> > > specific users -- e.g. disallow arbitrary android apps from poking *any*
> > > PMU, while allowing some more trusted apps/users to uses *some* specific
> > > PMUs.
> > >
> > > e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> > > this via the usual fs ACLs, and pass the fd to perf_event_open()
> > > somehow. A valid fd would act as a capability, taking precedence over
> > > perf_event_paranoid.
> >
> > That sounds like an orthogonal feature. I don't think the original
> > patchkit would need to be hold up for this. It would be something
> > in addition.
>
> I have to say that I disagree -- these controls will have to interact
> somehow, and the fewer of them we have, the less complexity we'll have
> to deal with longer-term.
You're proposing to completely redesign perf_event_open.
This new file descriptor argument doesn't exist today so it would
need to create a new system call with more arguments
(and BTW it would be more than the normal 6 argument limit
we have, so actually it couldn't even be a standard sycall)
Obviously we would need to keep the old system call around
for compability, so you would need to worry about this
interaction in any case!
So tying it together doesn't make any sense, because
the problem has to be solved separately anyways.
>
> > BTW can't you already do that with the syscall filter? I assume
> > the Android sandboxes already use that. Just forbid perf_event_open
> > for the apps.
>
> Note that this was about providing access to *some* PMUs in some cases.
>
> IIUC, if that can be done today via a syscall filter, the same is true
> of per-pmu paranoid settings.
The difference is that the Android sandboxes likely already doing this
and have all the infrastructure, and it's just another rule.
Requiring syscall filters just to use the PMU on xn system
that otherwise doesn't need them would be very odd.
-Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
> Right now we have a single knob, which is poorly documented and that should > be fixed first. But some googling gives you the information that allowing > unprivilegded access is a security risk. So the security focussed sysadmin Ah only if google could simply answer all our questions! > will deny access to the PMUs no matter what. It's not like there is or isn't a security risk and that you can say that it is or it isn't in a global way. Essentially these are channels of information. The channels always exist in form of timing variances for any shared resource (like shared caches or shared memory/IO/interconnect bandwidth) that can be measured. Perfmon counters make the channels generally less noisy, but they do not cause them. To really close them completely you would need to avoid sharing anything, or not allowing to measure time, neither of which is practical short of an air gap. There are reasonable assesments you can make either way and the answers will be different based on your requirements. There isn't a single answer that works for everyone. There are cases where it isn't a problem at all. If you don't have multiple users on the system your tolerance should be extremely high. For users who have multiple users there can be different tradeoffs. So there isn't a single answer, and that is why it is important that this if configurable. -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
> Right now we have a single knob, which is poorly documented and that should > be fixed first. But some googling gives you the information that allowing > unprivilegded access is a security risk. So the security focussed sysadmin Ah only if google could simply answer all our questions! > will deny access to the PMUs no matter what. It's not like there is or isn't a security risk and that you can say that it is or it isn't in a global way. Essentially these are channels of information. The channels always exist in form of timing variances for any shared resource (like shared caches or shared memory/IO/interconnect bandwidth) that can be measured. Perfmon counters make the channels generally less noisy, but they do not cause them. To really close them completely you would need to avoid sharing anything, or not allowing to measure time, neither of which is practical short of an air gap. There are reasonable assesments you can make either way and the answers will be different based on your requirements. There isn't a single answer that works for everyone. There are cases where it isn't a problem at all. If you don't have multiple users on the system your tolerance should be extremely high. For users who have multiple users there can be different tradeoffs. So there isn't a single answer, and that is why it is important that this if configurable. -Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Alexey, On Fri, 28 Sep 2018, Alexey Budankov wrote: > On 28.09.2018 17:02, Thomas Gleixner wrote: > > But this does also require a proper analysis and documentation why it is > > not a security risk to expose the i915 PMU or what potential security > > issues this can create, so that the competent sysadmin can make a > > judgement. > > > > And the same is required for all other PMUs which can be enabled in the > > same way for unprivileged access. And we might as well come to the > > conclusion via analysis that for some PMUs unpriviledged access is just not > > a good idea and exclude them. I surely know a few which qualify for > > exclusion, so the right approach is to provide this knob only when the risk > > is analyzed and documented and the PMU has been flagged as candidate for > > unpriviledged exposure. I.e. opt in and not opt out. > > If you ask me then, IMHO, unprivileged access to CBOX pmu looks unsafe > and is now governed by traditional *core* perf_event_paranoid setting. > > But *core* paranoid >= 1 (per-process mode) prevents simultaneous perf > record sampling and perf stat -I reading from IMC, UPI, PCIe and other > uncore counters. > > This kind of monitoring could make process performance observability > thru Perf subsystem more flexible and better tailored for cloud and > cluster environments. However it requires fine-tuning control > capabilities in order to still keep system as secure as possible. > > Could i915, IMC, UPI, PCIe pmus be safe enough to be governed by > a separate perf_event_paranoid settings? Just to make it clear. I'm not against separate settings at all. But I'm against adding knobs for every PMU the kernel supports wholesale without analysis and documentation just because we can and somebody wants it. Right now we have a single knob, which is poorly documented and that should be fixed first. But some googling gives you the information that allowing unprivilegded access is a security risk. So the security focussed sysadmin will deny access to the PMUs no matter what. Now we add more knobs without documentation what the exposure and risk of each PMU is. The proposed patch set does this wholesale for every PMU supported by the kernel. So the GPU user will ask the sysadmin to allow him access. How can he make an informed decision? If he grants it then the next user comes around and wants it for something else arguing that the other got it for the GPU already. How can he make an informed decision about that one? We provide the knobs, so it's also our responsibility towards our users to give them the information about their usage and scope. And every single PMU knob has a different scope. The documentation of the gazillion of knobs in /proc and /sysfs is not really brilliant, but we should really not continue this bad practice especially not when these knobs have potentially security relevant implcations. Yes, I know, writing documentation is work, but it's valuable and is appreciated by our users. To make this doable and not blocked by requiring every PMU to be analyzed and documented at once, I suggested to make this opt-in. Do analysis for a given PMU, decide whether it should be exposed at all. If so, document it proper and flip the bit. That way this can be done gradually as the need arises and we can exclude the riskier ones completely. I don't think this is an unreasonable request as it does not require the i915 folks to look at PMUs they are not familiar with and does not get blocked by waiting on every PMU wizard on the planet to have time. Start with something like Documentation/admin-guide/perf-security.rst or whatever name fits better and add a proper documentation for the existing knob. With the infrastructure for fine grained access control add the general explanation for fine grained access control. With each PMU which opt's in for the knob, add a section with guidance about scope and risk for this particular one. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Alexey, On Fri, 28 Sep 2018, Alexey Budankov wrote: > On 28.09.2018 17:02, Thomas Gleixner wrote: > > But this does also require a proper analysis and documentation why it is > > not a security risk to expose the i915 PMU or what potential security > > issues this can create, so that the competent sysadmin can make a > > judgement. > > > > And the same is required for all other PMUs which can be enabled in the > > same way for unprivileged access. And we might as well come to the > > conclusion via analysis that for some PMUs unpriviledged access is just not > > a good idea and exclude them. I surely know a few which qualify for > > exclusion, so the right approach is to provide this knob only when the risk > > is analyzed and documented and the PMU has been flagged as candidate for > > unpriviledged exposure. I.e. opt in and not opt out. > > If you ask me then, IMHO, unprivileged access to CBOX pmu looks unsafe > and is now governed by traditional *core* perf_event_paranoid setting. > > But *core* paranoid >= 1 (per-process mode) prevents simultaneous perf > record sampling and perf stat -I reading from IMC, UPI, PCIe and other > uncore counters. > > This kind of monitoring could make process performance observability > thru Perf subsystem more flexible and better tailored for cloud and > cluster environments. However it requires fine-tuning control > capabilities in order to still keep system as secure as possible. > > Could i915, IMC, UPI, PCIe pmus be safe enough to be governed by > a separate perf_event_paranoid settings? Just to make it clear. I'm not against separate settings at all. But I'm against adding knobs for every PMU the kernel supports wholesale without analysis and documentation just because we can and somebody wants it. Right now we have a single knob, which is poorly documented and that should be fixed first. But some googling gives you the information that allowing unprivilegded access is a security risk. So the security focussed sysadmin will deny access to the PMUs no matter what. Now we add more knobs without documentation what the exposure and risk of each PMU is. The proposed patch set does this wholesale for every PMU supported by the kernel. So the GPU user will ask the sysadmin to allow him access. How can he make an informed decision? If he grants it then the next user comes around and wants it for something else arguing that the other got it for the GPU already. How can he make an informed decision about that one? We provide the knobs, so it's also our responsibility towards our users to give them the information about their usage and scope. And every single PMU knob has a different scope. The documentation of the gazillion of knobs in /proc and /sysfs is not really brilliant, but we should really not continue this bad practice especially not when these knobs have potentially security relevant implcations. Yes, I know, writing documentation is work, but it's valuable and is appreciated by our users. To make this doable and not blocked by requiring every PMU to be analyzed and documented at once, I suggested to make this opt-in. Do analysis for a given PMU, decide whether it should be exposed at all. If so, document it proper and flip the bit. That way this can be done gradually as the need arises and we can exclude the riskier ones completely. I don't think this is an unreasonable request as it does not require the i915 folks to look at PMUs they are not familiar with and does not get blocked by waiting on every PMU wizard on the planet to have time. Start with something like Documentation/admin-guide/perf-security.rst or whatever name fits better and add a proper documentation for the existing knob. With the infrastructure for fine grained access control add the general explanation for fine grained access control. With each PMU which opt's in for the knob, add a section with guidance about scope and risk for this particular one. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 10:23:40AM -0700, Andi Kleen wrote:
> > There's also been prior discussion on these feature in other contexts
> > (e.g. android expoits resulting from out-of-tree drivers). It would be
> > nice to see those considered.
> >
> > IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> > finer granularity of control such that we could limit PMU access to
> > specific users -- e.g. disallow arbitrary android apps from poking *any*
> > PMU, while allowing some more trusted apps/users to uses *some* specific
> > PMUs.
> >
> > e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> > this via the usual fs ACLs, and pass the fd to perf_event_open()
> > somehow. A valid fd would act as a capability, taking precedence over
> > perf_event_paranoid.
>
> That sounds like an orthogonal feature. I don't think the original
> patchkit would need to be hold up for this. It would be something
> in addition.
I have to say that I disagree -- these controls will have to interact
somehow, and the fewer of them we have, the less complexity we'll have
to deal with longer-term.
> BTW can't you already do that with the syscall filter? I assume
> the Android sandboxes already use that. Just forbid perf_event_open
> for the apps.
Note that this was about providing access to *some* PMUs in some cases.
IIUC, if that can be done today via a syscall filter, the same is true
of per-pmu paranoid settings.
Thanks,
Mark.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 10:23:40AM -0700, Andi Kleen wrote:
> > There's also been prior discussion on these feature in other contexts
> > (e.g. android expoits resulting from out-of-tree drivers). It would be
> > nice to see those considered.
> >
> > IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> > finer granularity of control such that we could limit PMU access to
> > specific users -- e.g. disallow arbitrary android apps from poking *any*
> > PMU, while allowing some more trusted apps/users to uses *some* specific
> > PMUs.
> >
> > e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> > this via the usual fs ACLs, and pass the fd to perf_event_open()
> > somehow. A valid fd would act as a capability, taking precedence over
> > perf_event_paranoid.
>
> That sounds like an orthogonal feature. I don't think the original
> patchkit would need to be hold up for this. It would be something
> in addition.
I have to say that I disagree -- these controls will have to interact
somehow, and the fewer of them we have, the less complexity we'll have
to deal with longer-term.
> BTW can't you already do that with the syscall filter? I assume
> the Android sandboxes already use that. Just forbid perf_event_open
> for the apps.
Note that this was about providing access to *some* PMUs in some cases.
IIUC, if that can be done today via a syscall filter, the same is true
of per-pmu paranoid settings.
Thanks,
Mark.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
> There's also been prior discussion on these feature in other contexts
> (e.g. android expoits resulting from out-of-tree drivers). It would be
> nice to see those considered.
>
> IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> finer granularity of control such that we could limit PMU access to
> specific users -- e.g. disallow arbitrary android apps from poking *any*
> PMU, while allowing some more trusted apps/users to uses *some* specific
> PMUs.
>
> e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> this via the usual fs ACLs, and pass the fd to perf_event_open()
> somehow. A valid fd would act as a capability, taking precedence over
> perf_event_paranoid.
That sounds like an orthogonal feature. I don't think the original
patchkit would need to be hold up for this. It would be something
in addition.
BTW can't you already do that with the syscall filter? I assume
the Android sandboxes already use that. Just forbid perf_event_open
for the apps.
-Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
> There's also been prior discussion on these feature in other contexts
> (e.g. android expoits resulting from out-of-tree drivers). It would be
> nice to see those considered.
>
> IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
> finer granularity of control such that we could limit PMU access to
> specific users -- e.g. disallow arbitrary android apps from poking *any*
> PMU, while allowing some more trusted apps/users to uses *some* specific
> PMUs.
>
> e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
> this via the usual fs ACLs, and pass the fd to perf_event_open()
> somehow. A valid fd would act as a capability, taking precedence over
> perf_event_paranoid.
That sounds like an orthogonal feature. I don't think the original
patchkit would need to be hold up for this. It would be something
in addition.
BTW can't you already do that with the syscall filter? I assume
the Android sandboxes already use that. Just forbid perf_event_open
for the apps.
-Andi
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 12:26:52PM +0200, Thomas Gleixner wrote:
> On Wed, 19 Sep 2018, Tvrtko Ursulin wrote:
>
> It would be very helpful if you cc all involved people on the cover letter
> instead of just cc'ing your own pile of email addresses. CC'ed now.
>
> > For situations where sysadmins might want to allow different level of
> > access control for different PMUs, we start creating per-PMU
> > perf_event_paranoid controls in sysfs.
> >
> > These work in equivalent fashion as the existing perf_event_paranoid
> > sysctl, which now becomes the parent control for each PMU.
> >
> > On PMU registration the global/parent value will be inherited by each PMU,
> > as it will be propagated to all registered PMUs when the sysctl is
> > updated.
> >
> > At any later point individual PMU access controls, located in
> > /device//perf_event_paranoid, can be adjusted to achieve
> > fine grained access control.
> >
> > Discussion from previous posting:
> > https://lkml.org/lkml/2018/5/21/156
>
> This is really not helpful. The cover letter and the change logs should
> contain a summary of that discussion and a proper justification of the
> proposed change. Just saying 'sysadmins might want to allow' is not useful
> at all, it's yet another 'I want a pony' thing.
>
> I read through the previous thread and there was a clear request to involve
> security people into this. Especially those who are deeply involved with
> hardware side channels. I don't see anyone Cc'ed on the whole series.
>
> For the record, I'm not buying the handwavy 'more noise' argument at
> all. It wants a proper analysis and we need to come up with criteria which
> PMUs can be exposed at all.
>
> All of this want's a proper documentation clearly explaining the risks and
> scope of these knobs per PMU. Just throwing magic knobs at sysadmins and
> then saying 'its their problem to figure it out' is not acceptable.
There's also been prior discussion on these feature in other contexts
(e.g. android expoits resulting from out-of-tree drivers). It would be
nice to see those considered.
IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
finer granularity of control such that we could limit PMU access to
specific users -- e.g. disallow arbitrary android apps from poking *any*
PMU, while allowing some more trusted apps/users to uses *some* specific
PMUs.
e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
this via the usual fs ACLs, and pass the fd to perf_event_open()
somehow. A valid fd would act as a capability, taking precedence over
perf_event_paranoid.
Thanks,
Mark.
[1] https://patchwork.kernel.org/patch/9249919/
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 12:26:52PM +0200, Thomas Gleixner wrote:
> On Wed, 19 Sep 2018, Tvrtko Ursulin wrote:
>
> It would be very helpful if you cc all involved people on the cover letter
> instead of just cc'ing your own pile of email addresses. CC'ed now.
>
> > For situations where sysadmins might want to allow different level of
> > access control for different PMUs, we start creating per-PMU
> > perf_event_paranoid controls in sysfs.
> >
> > These work in equivalent fashion as the existing perf_event_paranoid
> > sysctl, which now becomes the parent control for each PMU.
> >
> > On PMU registration the global/parent value will be inherited by each PMU,
> > as it will be propagated to all registered PMUs when the sysctl is
> > updated.
> >
> > At any later point individual PMU access controls, located in
> > /device//perf_event_paranoid, can be adjusted to achieve
> > fine grained access control.
> >
> > Discussion from previous posting:
> > https://lkml.org/lkml/2018/5/21/156
>
> This is really not helpful. The cover letter and the change logs should
> contain a summary of that discussion and a proper justification of the
> proposed change. Just saying 'sysadmins might want to allow' is not useful
> at all, it's yet another 'I want a pony' thing.
>
> I read through the previous thread and there was a clear request to involve
> security people into this. Especially those who are deeply involved with
> hardware side channels. I don't see anyone Cc'ed on the whole series.
>
> For the record, I'm not buying the handwavy 'more noise' argument at
> all. It wants a proper analysis and we need to come up with criteria which
> PMUs can be exposed at all.
>
> All of this want's a proper documentation clearly explaining the risks and
> scope of these knobs per PMU. Just throwing magic knobs at sysadmins and
> then saying 'its their problem to figure it out' is not acceptable.
There's also been prior discussion on these feature in other contexts
(e.g. android expoits resulting from out-of-tree drivers). It would be
nice to see those considered.
IIRC The conclusion from prior discussions (e.g. [1]) was that we wanted
finer granularity of control such that we could limit PMU access to
specific users -- e.g. disallow arbitrary android apps from poking *any*
PMU, while allowing some more trusted apps/users to uses *some* specific
PMUs.
e.g. we could add /sys/bus/event_source/devices/${PMU}/device, protect
this via the usual fs ACLs, and pass the fd to perf_event_open()
somehow. A valid fd would act as a capability, taking precedence over
perf_event_paranoid.
Thanks,
Mark.
[1] https://patchwork.kernel.org/patch/9249919/
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello, On 28.09.2018 17:02, Thomas Gleixner wrote: > Tvrtko, > > On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: >> On 28/09/2018 11:26, Thomas Gleixner wrote: >>> On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: >>> >>> It would be very helpful if you cc all involved people on the cover letter >>> instead of just cc'ing your own pile of email addresses. CC'ed now. >> >> I accept it was by bad to miss adding Cc's on the cover letter, but my own >> email addresses hopefully should not bother you. It is simply a question of >> what I have in .gitconfig vs what I forgot to do manually. > > The keyword in the above sentence is 'just'. You can add as many of yours > as you want as long as everybody else is cc'ed. > >>> I read through the previous thread and there was a clear request to involve >>> security people into this. Especially those who are deeply involved with >>> hardware side channels. I don't see anyone Cc'ed on the whole series. >> >> Who would you recommend I add? Because I really don't know.. > > Sure, and because you don't know you didn't bother to ask around and > ignored the review request. > > I already added Kees and Jann. Please look for the SECCOMP folks in > MAINTAINERS. Thomas, thanks a lot for involving the folks! > >>> For the record, I'm not buying the handwavy 'more noise' argument at >>> all. It wants a proper analysis and we need to come up with criteria which >>> PMUs can be exposed at all. >>> >>> All of this want's a proper documentation clearly explaining the risks and >>> scope of these knobs per PMU. Just throwing magic knobs at sysadmins and >>> then saying 'its their problem to figure it out' is not acceptable. >> >> Presumably you see adding fine grained control as diminishing the overall >> security rather than raising it? Could you explain why? Because incompetent >> sysadmin will turn it off for some PMU, while without having the fine-grained >> control they wouldn't turn it off globally? > > I did not say at all that this might be diminishing security. And the > argumentation with 'incompetent sysadmins' is just the wrong attitude. > > What I was asking for is proper documentation and this proper documentation > is meant for _competent_ sysadmins. > > That documentation has to clearly describe what kind of information is > accessible and what potential side effects security wise this might > have. You cannot expect that even competent sysadmins know offhand what > which PMU might expose. And telling them 'Use Google' is just not the right > thing to do. > > If you can't explain and document it, then providing the knob is just > fulfilling somebodys 'I want a pony' request.> >> This feature was requested by the exact opposite concern, that in order to >> access the i915 PMU, one has to compromise the security of the entire system >> by allowing access to *all* PMU's. >> >> Making this ability fine-grained sounds like a logical solution for solving >> this weakening of security controls. > > Sure, and this wants to be documented in the cover letter and the > changelogs. > > But this does also require a proper analysis and documentation why it is > not a security risk to expose the i915 PMU or what potential security > issues this can create, so that the competent sysadmin can make a > judgement. > > And the same is required for all other PMUs which can be enabled in the > same way for unprivileged access. And we might as well come to the > conclusion via analysis that for some PMUs unpriviledged access is just not > a good idea and exclude them. I surely know a few which qualify for > exclusion, so the right approach is to provide this knob only when the risk > is analyzed and documented and the PMU has been flagged as candidate for > unpriviledged exposure. I.e. opt in and not opt out. If you ask me then, IMHO, unprivileged access to CBOX pmu looks unsafe and is now governed by traditional *core* perf_event_paranoid setting. But *core* paranoid >= 1 (per-process mode) prevents simultaneous perf record sampling and perf stat -I reading from IMC, UPI, PCIe and other uncore counters. This kind of monitoring could make process performance observability thru Perf subsystem more flexible and better tailored for cloud and cluster environments. However it requires fine-tuning control capabilities in order to still keep system as secure as possible. Could i915, IMC, UPI, PCIe pmus be safe enough to be governed by a separate perf_event_paranoid settings? Thanks! Alexey > > Thanks, > > tglx >
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hello, On 28.09.2018 17:02, Thomas Gleixner wrote: > Tvrtko, > > On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: >> On 28/09/2018 11:26, Thomas Gleixner wrote: >>> On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: >>> >>> It would be very helpful if you cc all involved people on the cover letter >>> instead of just cc'ing your own pile of email addresses. CC'ed now. >> >> I accept it was by bad to miss adding Cc's on the cover letter, but my own >> email addresses hopefully should not bother you. It is simply a question of >> what I have in .gitconfig vs what I forgot to do manually. > > The keyword in the above sentence is 'just'. You can add as many of yours > as you want as long as everybody else is cc'ed. > >>> I read through the previous thread and there was a clear request to involve >>> security people into this. Especially those who are deeply involved with >>> hardware side channels. I don't see anyone Cc'ed on the whole series. >> >> Who would you recommend I add? Because I really don't know.. > > Sure, and because you don't know you didn't bother to ask around and > ignored the review request. > > I already added Kees and Jann. Please look for the SECCOMP folks in > MAINTAINERS. Thomas, thanks a lot for involving the folks! > >>> For the record, I'm not buying the handwavy 'more noise' argument at >>> all. It wants a proper analysis and we need to come up with criteria which >>> PMUs can be exposed at all. >>> >>> All of this want's a proper documentation clearly explaining the risks and >>> scope of these knobs per PMU. Just throwing magic knobs at sysadmins and >>> then saying 'its their problem to figure it out' is not acceptable. >> >> Presumably you see adding fine grained control as diminishing the overall >> security rather than raising it? Could you explain why? Because incompetent >> sysadmin will turn it off for some PMU, while without having the fine-grained >> control they wouldn't turn it off globally? > > I did not say at all that this might be diminishing security. And the > argumentation with 'incompetent sysadmins' is just the wrong attitude. > > What I was asking for is proper documentation and this proper documentation > is meant for _competent_ sysadmins. > > That documentation has to clearly describe what kind of information is > accessible and what potential side effects security wise this might > have. You cannot expect that even competent sysadmins know offhand what > which PMU might expose. And telling them 'Use Google' is just not the right > thing to do. > > If you can't explain and document it, then providing the knob is just > fulfilling somebodys 'I want a pony' request.> >> This feature was requested by the exact opposite concern, that in order to >> access the i915 PMU, one has to compromise the security of the entire system >> by allowing access to *all* PMU's. >> >> Making this ability fine-grained sounds like a logical solution for solving >> this weakening of security controls. > > Sure, and this wants to be documented in the cover letter and the > changelogs. > > But this does also require a proper analysis and documentation why it is > not a security risk to expose the i915 PMU or what potential security > issues this can create, so that the competent sysadmin can make a > judgement. > > And the same is required for all other PMUs which can be enabled in the > same way for unprivileged access. And we might as well come to the > conclusion via analysis that for some PMUs unpriviledged access is just not > a good idea and exclude them. I surely know a few which qualify for > exclusion, so the right approach is to provide this knob only when the risk > is analyzed and documented and the PMU has been flagged as candidate for > unpriviledged exposure. I.e. opt in and not opt out. If you ask me then, IMHO, unprivileged access to CBOX pmu looks unsafe and is now governed by traditional *core* perf_event_paranoid setting. But *core* paranoid >= 1 (per-process mode) prevents simultaneous perf record sampling and perf stat -I reading from IMC, UPI, PCIe and other uncore counters. This kind of monitoring could make process performance observability thru Perf subsystem more flexible and better tailored for cloud and cluster environments. However it requires fine-tuning control capabilities in order to still keep system as secure as possible. Could i915, IMC, UPI, PCIe pmus be safe enough to be governed by a separate perf_event_paranoid settings? Thanks! Alexey > > Thanks, > > tglx >
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Tvrtko, On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: > On 28/09/2018 15:02, Thomas Gleixner wrote: > > > Presumably you see adding fine grained control as diminishing the overall > > > security rather than raising it? Could you explain why? Because > > > incompetent > > > sysadmin will turn it off for some PMU, while without having the > > > fine-grained > > > control they wouldn't turn it off globally? > > > > I did not say at all that this might be diminishing security. And the > > argumentation with 'incompetent sysadmins' is just the wrong attitude. > > Wrong attitude what? I was trying to guess your reasoning (cues in > "presumably" and a lot of question marks) since it wasn't clear to me why is > your position what it is. Guessing my reasonings has nothing to do with you mentioning incompentent sysadmins. > > What I was asking for is proper documentation and this proper documentation > > is meant for _competent_ sysadmins. > > > > That documentation has to clearly describe what kind of information is > > accessible and what potential side effects security wise this might > > have. You cannot expect that even competent sysadmins know offhand what > > which PMU might expose. And telling them 'Use Google' is just not the right > > thing to do. > > I did not mention Google. I did not say that you mentioned google. But what is a sysadmin supposed to do when there is no documentation aside of using google? And not having documentation is basically the same thing as telling them to use google. > > If you can't explain and document it, then providing the knob is just > > fulfilling somebodys 'I want a pony' request. > > Well it's not a pony, it is mechanism to avoid having to turn off all > security. We can hopefully discuss it without ponies. If you want to make a pettifogger contest out of this discussion, then we can stop right here. I explained it technically why just adding a knob without further explanation and analysis is not acceptable. > > And the same is required for all other PMUs which can be enabled in the > > same way for unprivileged access. And we might as well come to the > > conclusion via analysis that for some PMUs unpriviledged access is just not > > a good idea and exclude them. I surely know a few which qualify for > > exclusion, so the right approach is to provide this knob only when the risk > > is analyzed and documented and the PMU has been flagged as candidate for > > unpriviledged exposure. I.e. opt in and not opt out. > > I am happy to work on the mechanics of achieving this once the security guys > and all PMU owners get involved. Even though I am not convinced the bar to > allow fine-grained control should be evaluating all possible PMUs*, but if the > security folks agree that is the case it is fine by me. Making the knob opt in per PMU does not need all PMU owners to be involved. It allows to add the opt in flag on a case by case basis. > *) The part of my reply you did not quote explains how the fine-grained > control improves security in existing deployments. The documentation I added > refers to the existing perf_event_paranoid documentation for explanation of > security concerns involved. Which is not much in itself. But essentially we > both have a PMU and a knob already. I don't see why adding the same knob > per-PMU needs much more stringent criteria to be accepted. But as said, that's > for security people to decide. The fact, that the existing knob is poorly documented does make an excuse for adding more knobs without documentation. Quite the contrary, if we notice that the existing knob lacks proper documentation, then we should fix that first. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Tvrtko, On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: > On 28/09/2018 15:02, Thomas Gleixner wrote: > > > Presumably you see adding fine grained control as diminishing the overall > > > security rather than raising it? Could you explain why? Because > > > incompetent > > > sysadmin will turn it off for some PMU, while without having the > > > fine-grained > > > control they wouldn't turn it off globally? > > > > I did not say at all that this might be diminishing security. And the > > argumentation with 'incompetent sysadmins' is just the wrong attitude. > > Wrong attitude what? I was trying to guess your reasoning (cues in > "presumably" and a lot of question marks) since it wasn't clear to me why is > your position what it is. Guessing my reasonings has nothing to do with you mentioning incompentent sysadmins. > > What I was asking for is proper documentation and this proper documentation > > is meant for _competent_ sysadmins. > > > > That documentation has to clearly describe what kind of information is > > accessible and what potential side effects security wise this might > > have. You cannot expect that even competent sysadmins know offhand what > > which PMU might expose. And telling them 'Use Google' is just not the right > > thing to do. > > I did not mention Google. I did not say that you mentioned google. But what is a sysadmin supposed to do when there is no documentation aside of using google? And not having documentation is basically the same thing as telling them to use google. > > If you can't explain and document it, then providing the knob is just > > fulfilling somebodys 'I want a pony' request. > > Well it's not a pony, it is mechanism to avoid having to turn off all > security. We can hopefully discuss it without ponies. If you want to make a pettifogger contest out of this discussion, then we can stop right here. I explained it technically why just adding a knob without further explanation and analysis is not acceptable. > > And the same is required for all other PMUs which can be enabled in the > > same way for unprivileged access. And we might as well come to the > > conclusion via analysis that for some PMUs unpriviledged access is just not > > a good idea and exclude them. I surely know a few which qualify for > > exclusion, so the right approach is to provide this knob only when the risk > > is analyzed and documented and the PMU has been flagged as candidate for > > unpriviledged exposure. I.e. opt in and not opt out. > > I am happy to work on the mechanics of achieving this once the security guys > and all PMU owners get involved. Even though I am not convinced the bar to > allow fine-grained control should be evaluating all possible PMUs*, but if the > security folks agree that is the case it is fine by me. Making the knob opt in per PMU does not need all PMU owners to be involved. It allows to add the opt in flag on a case by case basis. > *) The part of my reply you did not quote explains how the fine-grained > control improves security in existing deployments. The documentation I added > refers to the existing perf_event_paranoid documentation for explanation of > security concerns involved. Which is not much in itself. But essentially we > both have a PMU and a knob already. I don't see why adding the same knob > per-PMU needs much more stringent criteria to be accepted. But as said, that's > for security people to decide. The fact, that the existing knob is poorly documented does make an excuse for adding more knobs without documentation. Quite the contrary, if we notice that the existing knob lacks proper documentation, then we should fix that first. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 3:22 PM Tvrtko Ursulin wrote: > On 28/09/2018 11:26, Thomas Gleixner wrote: > > On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: > >> For situations where sysadmins might want to allow different level of > >> access control for different PMUs, we start creating per-PMU > >> perf_event_paranoid controls in sysfs. > >> > >> These work in equivalent fashion as the existing perf_event_paranoid > >> sysctl, which now becomes the parent control for each PMU. > >> > >> On PMU registration the global/parent value will be inherited by each PMU, > >> as it will be propagated to all registered PMUs when the sysctl is > >> updated. > >> > >> At any later point individual PMU access controls, located in > >> /device//perf_event_paranoid, can be adjusted to achieve > >> fine grained access control. > >> > >> Discussion from previous posting: > >> https://lkml.org/lkml/2018/5/21/156 > > > > This is really not helpful. The cover letter and the change logs should > > contain a summary of that discussion and a proper justification of the > > proposed change. Just saying 'sysadmins might want to allow' is not useful > > at all, it's yet another 'I want a pony' thing. > > Okay, for the next round I will expand the cover letter with at least > one concrete example on how it is usable and summarize the discussion a bit. > > > I read through the previous thread and there was a clear request to involve > > security people into this. Especially those who are deeply involved with > > hardware side channels. I don't see anyone Cc'ed on the whole series. > > Who would you recommend I add? Because I really don't know.. > > > For the record, I'm not buying the handwavy 'more noise' argument at > > all. It wants a proper analysis and we need to come up with criteria which > > PMUs can be exposed at all. > > > > All of this want's a proper documentation clearly explaining the risks and > > scope of these knobs per PMU. Just throwing magic knobs at sysadmins and > > then saying 'its their problem to figure it out' is not acceptable. > > Presumably you see adding fine grained control as diminishing the > overall security rather than raising it? Could you explain why? Because > incompetent sysadmin will turn it off for some PMU, while without having > the fine-grained control they wouldn't turn it off globally? > > This feature was requested by the exact opposite concern, that in order > to access the i915 PMU, one has to compromise the security of the entire > system by allowing access to *all* PMU's. > > Making this ability fine-grained sounds like a logical solution for > solving this weakening of security controls. > > Concrete example was that on video transcoding farms users want to > monitor the utilization of GPU engines (like CPU cores) and they can do > that via the i915 PMU. But for that to work today they have to dial down > the global perf_event_paranoid setting. Obvious improvement was to allow > them to only dial down the i915.perf_event_paranoid setting. As such, > for this specific use case at least, the security is increased. Which paranoia level would be used for the i915.perf_event_paranoid setting in such a case? Perhaps also CC kernel-harden...@lists.openwall.com on the next version.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On Fri, Sep 28, 2018 at 3:22 PM Tvrtko Ursulin wrote: > On 28/09/2018 11:26, Thomas Gleixner wrote: > > On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: > >> For situations where sysadmins might want to allow different level of > >> access control for different PMUs, we start creating per-PMU > >> perf_event_paranoid controls in sysfs. > >> > >> These work in equivalent fashion as the existing perf_event_paranoid > >> sysctl, which now becomes the parent control for each PMU. > >> > >> On PMU registration the global/parent value will be inherited by each PMU, > >> as it will be propagated to all registered PMUs when the sysctl is > >> updated. > >> > >> At any later point individual PMU access controls, located in > >> /device//perf_event_paranoid, can be adjusted to achieve > >> fine grained access control. > >> > >> Discussion from previous posting: > >> https://lkml.org/lkml/2018/5/21/156 > > > > This is really not helpful. The cover letter and the change logs should > > contain a summary of that discussion and a proper justification of the > > proposed change. Just saying 'sysadmins might want to allow' is not useful > > at all, it's yet another 'I want a pony' thing. > > Okay, for the next round I will expand the cover letter with at least > one concrete example on how it is usable and summarize the discussion a bit. > > > I read through the previous thread and there was a clear request to involve > > security people into this. Especially those who are deeply involved with > > hardware side channels. I don't see anyone Cc'ed on the whole series. > > Who would you recommend I add? Because I really don't know.. > > > For the record, I'm not buying the handwavy 'more noise' argument at > > all. It wants a proper analysis and we need to come up with criteria which > > PMUs can be exposed at all. > > > > All of this want's a proper documentation clearly explaining the risks and > > scope of these knobs per PMU. Just throwing magic knobs at sysadmins and > > then saying 'its their problem to figure it out' is not acceptable. > > Presumably you see adding fine grained control as diminishing the > overall security rather than raising it? Could you explain why? Because > incompetent sysadmin will turn it off for some PMU, while without having > the fine-grained control they wouldn't turn it off globally? > > This feature was requested by the exact opposite concern, that in order > to access the i915 PMU, one has to compromise the security of the entire > system by allowing access to *all* PMU's. > > Making this ability fine-grained sounds like a logical solution for > solving this weakening of security controls. > > Concrete example was that on video transcoding farms users want to > monitor the utilization of GPU engines (like CPU cores) and they can do > that via the i915 PMU. But for that to work today they have to dial down > the global perf_event_paranoid setting. Obvious improvement was to allow > them to only dial down the i915.perf_event_paranoid setting. As such, > for this specific use case at least, the security is increased. Which paranoia level would be used for the i915.perf_event_paranoid setting in such a case? Perhaps also CC kernel-harden...@lists.openwall.com on the next version.
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On 28/09/2018 15:02, Thomas Gleixner wrote: Tvrtko, On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: On 28/09/2018 11:26, Thomas Gleixner wrote: On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: It would be very helpful if you cc all involved people on the cover letter instead of just cc'ing your own pile of email addresses. CC'ed now. I accept it was by bad to miss adding Cc's on the cover letter, but my own email addresses hopefully should not bother you. It is simply a question of what I have in .gitconfig vs what I forgot to do manually. The keyword in the above sentence is 'just'. You can add as many of yours as you want as long as everybody else is cc'ed. Sure, but you also used the word "pile" and I would argue that made the rest of your sentence, after and including "instead", sound like it not only bothers you I forgot to Cc people on the cover letter, but it also bothers you I included a "pile" of my own addresses. If that wasn't your intention in the slightest then I apologise for misreading it. I read through the previous thread and there was a clear request to involve security people into this. Especially those who are deeply involved with hardware side channels. I don't see anyone Cc'ed on the whole series. Who would you recommend I add? Because I really don't know.. Sure, and because you don't know you didn't bother to ask around and ignored the review request. No, not because of that. You are assuming my actions and motivations and constructing a story. "did not bother" = negative connotations "ignored" = negative connotations Note instead the time lapse between this and previous posting of the series, and if you want to assume something, assume things can get missed and forgotten without intent or malice. I already added Kees and Jann. Please look for the SECCOMP folks in MAINTAINERS. Thanks! For the record, I'm not buying the handwavy 'more noise' argument at all. It wants a proper analysis and we need to come up with criteria which PMUs can be exposed at all. All of this want's a proper documentation clearly explaining the risks and scope of these knobs per PMU. Just throwing magic knobs at sysadmins and then saying 'its their problem to figure it out' is not acceptable. Presumably you see adding fine grained control as diminishing the overall security rather than raising it? Could you explain why? Because incompetent sysadmin will turn it off for some PMU, while without having the fine-grained control they wouldn't turn it off globally? I did not say at all that this might be diminishing security. And the argumentation with 'incompetent sysadmins' is just the wrong attitude. Wrong attitude what? I was trying to guess your reasoning (cues in "presumably" and a lot of question marks) since it wasn't clear to me why is your position what it is. What I was asking for is proper documentation and this proper documentation is meant for _competent_ sysadmins. That documentation has to clearly describe what kind of information is accessible and what potential side effects security wise this might have. You cannot expect that even competent sysadmins know offhand what which PMU might expose. And telling them 'Use Google' is just not the right thing to do. I did not mention Google. If you can't explain and document it, then providing the knob is just fulfilling somebodys 'I want a pony' request. Well it's not a pony, it is mechanism to avoid having to turn off all security. We can hopefully discuss it without ponies. This feature was requested by the exact opposite concern, that in order to access the i915 PMU, one has to compromise the security of the entire system by allowing access to *all* PMU's. Making this ability fine-grained sounds like a logical solution for solving this weakening of security controls. Sure, and this wants to be documented in the cover letter and the changelogs. But this does also require a proper analysis and documentation why it is not a security risk to expose the i915 PMU or what potential security issues this can create, so that the competent sysadmin can make a judgement. And the same is required for all other PMUs which can be enabled in the same way for unprivileged access. And we might as well come to the conclusion via analysis that for some PMUs unpriviledged access is just not a good idea and exclude them. I surely know a few which qualify for exclusion, so the right approach is to provide this knob only when the risk is analyzed and documented and the PMU has been flagged as candidate for unpriviledged exposure. I.e. opt in and not opt out. I am happy to work on the mechanics of achieving this once the security guys and all PMU owners get involved. Even though I am not convinced the bar to allow fine-grained control should be evaluating all possible PMUs*, but if the security folks agree that is the case it is fine by me. Regards, Tvrtko *) The part of my reply you did not quote explains how the
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
On 28/09/2018 15:02, Thomas Gleixner wrote: Tvrtko, On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: On 28/09/2018 11:26, Thomas Gleixner wrote: On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: It would be very helpful if you cc all involved people on the cover letter instead of just cc'ing your own pile of email addresses. CC'ed now. I accept it was by bad to miss adding Cc's on the cover letter, but my own email addresses hopefully should not bother you. It is simply a question of what I have in .gitconfig vs what I forgot to do manually. The keyword in the above sentence is 'just'. You can add as many of yours as you want as long as everybody else is cc'ed. Sure, but you also used the word "pile" and I would argue that made the rest of your sentence, after and including "instead", sound like it not only bothers you I forgot to Cc people on the cover letter, but it also bothers you I included a "pile" of my own addresses. If that wasn't your intention in the slightest then I apologise for misreading it. I read through the previous thread and there was a clear request to involve security people into this. Especially those who are deeply involved with hardware side channels. I don't see anyone Cc'ed on the whole series. Who would you recommend I add? Because I really don't know.. Sure, and because you don't know you didn't bother to ask around and ignored the review request. No, not because of that. You are assuming my actions and motivations and constructing a story. "did not bother" = negative connotations "ignored" = negative connotations Note instead the time lapse between this and previous posting of the series, and if you want to assume something, assume things can get missed and forgotten without intent or malice. I already added Kees and Jann. Please look for the SECCOMP folks in MAINTAINERS. Thanks! For the record, I'm not buying the handwavy 'more noise' argument at all. It wants a proper analysis and we need to come up with criteria which PMUs can be exposed at all. All of this want's a proper documentation clearly explaining the risks and scope of these knobs per PMU. Just throwing magic knobs at sysadmins and then saying 'its their problem to figure it out' is not acceptable. Presumably you see adding fine grained control as diminishing the overall security rather than raising it? Could you explain why? Because incompetent sysadmin will turn it off for some PMU, while without having the fine-grained control they wouldn't turn it off globally? I did not say at all that this might be diminishing security. And the argumentation with 'incompetent sysadmins' is just the wrong attitude. Wrong attitude what? I was trying to guess your reasoning (cues in "presumably" and a lot of question marks) since it wasn't clear to me why is your position what it is. What I was asking for is proper documentation and this proper documentation is meant for _competent_ sysadmins. That documentation has to clearly describe what kind of information is accessible and what potential side effects security wise this might have. You cannot expect that even competent sysadmins know offhand what which PMU might expose. And telling them 'Use Google' is just not the right thing to do. I did not mention Google. If you can't explain and document it, then providing the knob is just fulfilling somebodys 'I want a pony' request. Well it's not a pony, it is mechanism to avoid having to turn off all security. We can hopefully discuss it without ponies. This feature was requested by the exact opposite concern, that in order to access the i915 PMU, one has to compromise the security of the entire system by allowing access to *all* PMU's. Making this ability fine-grained sounds like a logical solution for solving this weakening of security controls. Sure, and this wants to be documented in the cover letter and the changelogs. But this does also require a proper analysis and documentation why it is not a security risk to expose the i915 PMU or what potential security issues this can create, so that the competent sysadmin can make a judgement. And the same is required for all other PMUs which can be enabled in the same way for unprivileged access. And we might as well come to the conclusion via analysis that for some PMUs unpriviledged access is just not a good idea and exclude them. I surely know a few which qualify for exclusion, so the right approach is to provide this knob only when the risk is analyzed and documented and the PMU has been flagged as candidate for unpriviledged exposure. I.e. opt in and not opt out. I am happy to work on the mechanics of achieving this once the security guys and all PMU owners get involved. Even though I am not convinced the bar to allow fine-grained control should be evaluating all possible PMUs*, but if the security folks agree that is the case it is fine by me. Regards, Tvrtko *) The part of my reply you did not quote explains how the
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Tvrtko, On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: > On 28/09/2018 11:26, Thomas Gleixner wrote: > > On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: > > > > It would be very helpful if you cc all involved people on the cover letter > > instead of just cc'ing your own pile of email addresses. CC'ed now. > > I accept it was by bad to miss adding Cc's on the cover letter, but my own > email addresses hopefully should not bother you. It is simply a question of > what I have in .gitconfig vs what I forgot to do manually. The keyword in the above sentence is 'just'. You can add as many of yours as you want as long as everybody else is cc'ed. > > I read through the previous thread and there was a clear request to involve > > security people into this. Especially those who are deeply involved with > > hardware side channels. I don't see anyone Cc'ed on the whole series. > > Who would you recommend I add? Because I really don't know.. Sure, and because you don't know you didn't bother to ask around and ignored the review request. I already added Kees and Jann. Please look for the SECCOMP folks in MAINTAINERS. > > For the record, I'm not buying the handwavy 'more noise' argument at > > all. It wants a proper analysis and we need to come up with criteria which > > PMUs can be exposed at all. > > > > All of this want's a proper documentation clearly explaining the risks and > > scope of these knobs per PMU. Just throwing magic knobs at sysadmins and > > then saying 'its their problem to figure it out' is not acceptable. > > Presumably you see adding fine grained control as diminishing the overall > security rather than raising it? Could you explain why? Because incompetent > sysadmin will turn it off for some PMU, while without having the fine-grained > control they wouldn't turn it off globally? I did not say at all that this might be diminishing security. And the argumentation with 'incompetent sysadmins' is just the wrong attitude. What I was asking for is proper documentation and this proper documentation is meant for _competent_ sysadmins. That documentation has to clearly describe what kind of information is accessible and what potential side effects security wise this might have. You cannot expect that even competent sysadmins know offhand what which PMU might expose. And telling them 'Use Google' is just not the right thing to do. If you can't explain and document it, then providing the knob is just fulfilling somebodys 'I want a pony' request. > This feature was requested by the exact opposite concern, that in order to > access the i915 PMU, one has to compromise the security of the entire system > by allowing access to *all* PMU's. > > Making this ability fine-grained sounds like a logical solution for solving > this weakening of security controls. Sure, and this wants to be documented in the cover letter and the changelogs. But this does also require a proper analysis and documentation why it is not a security risk to expose the i915 PMU or what potential security issues this can create, so that the competent sysadmin can make a judgement. And the same is required for all other PMUs which can be enabled in the same way for unprivileged access. And we might as well come to the conclusion via analysis that for some PMUs unpriviledged access is just not a good idea and exclude them. I surely know a few which qualify for exclusion, so the right approach is to provide this knob only when the risk is analyzed and documented and the PMU has been flagged as candidate for unpriviledged exposure. I.e. opt in and not opt out. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Tvrtko, On Fri, 28 Sep 2018, Tvrtko Ursulin wrote: > On 28/09/2018 11:26, Thomas Gleixner wrote: > > On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: > > > > It would be very helpful if you cc all involved people on the cover letter > > instead of just cc'ing your own pile of email addresses. CC'ed now. > > I accept it was by bad to miss adding Cc's on the cover letter, but my own > email addresses hopefully should not bother you. It is simply a question of > what I have in .gitconfig vs what I forgot to do manually. The keyword in the above sentence is 'just'. You can add as many of yours as you want as long as everybody else is cc'ed. > > I read through the previous thread and there was a clear request to involve > > security people into this. Especially those who are deeply involved with > > hardware side channels. I don't see anyone Cc'ed on the whole series. > > Who would you recommend I add? Because I really don't know.. Sure, and because you don't know you didn't bother to ask around and ignored the review request. I already added Kees and Jann. Please look for the SECCOMP folks in MAINTAINERS. > > For the record, I'm not buying the handwavy 'more noise' argument at > > all. It wants a proper analysis and we need to come up with criteria which > > PMUs can be exposed at all. > > > > All of this want's a proper documentation clearly explaining the risks and > > scope of these knobs per PMU. Just throwing magic knobs at sysadmins and > > then saying 'its their problem to figure it out' is not acceptable. > > Presumably you see adding fine grained control as diminishing the overall > security rather than raising it? Could you explain why? Because incompetent > sysadmin will turn it off for some PMU, while without having the fine-grained > control they wouldn't turn it off globally? I did not say at all that this might be diminishing security. And the argumentation with 'incompetent sysadmins' is just the wrong attitude. What I was asking for is proper documentation and this proper documentation is meant for _competent_ sysadmins. That documentation has to clearly describe what kind of information is accessible and what potential side effects security wise this might have. You cannot expect that even competent sysadmins know offhand what which PMU might expose. And telling them 'Use Google' is just not the right thing to do. If you can't explain and document it, then providing the knob is just fulfilling somebodys 'I want a pony' request. > This feature was requested by the exact opposite concern, that in order to > access the i915 PMU, one has to compromise the security of the entire system > by allowing access to *all* PMU's. > > Making this ability fine-grained sounds like a logical solution for solving > this weakening of security controls. Sure, and this wants to be documented in the cover letter and the changelogs. But this does also require a proper analysis and documentation why it is not a security risk to expose the i915 PMU or what potential security issues this can create, so that the competent sysadmin can make a judgement. And the same is required for all other PMUs which can be enabled in the same way for unprivileged access. And we might as well come to the conclusion via analysis that for some PMUs unpriviledged access is just not a good idea and exclude them. I surely know a few which qualify for exclusion, so the right approach is to provide this knob only when the risk is analyzed and documented and the PMU has been flagged as candidate for unpriviledged exposure. I.e. opt in and not opt out. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hi, On 28/09/2018 11:26, Thomas Gleixner wrote: Tvrtko, On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: It would be very helpful if you cc all involved people on the cover letter instead of just cc'ing your own pile of email addresses. CC'ed now. I accept it was by bad to miss adding Cc's on the cover letter, but my own email addresses hopefully should not bother you. It is simply a question of what I have in .gitconfig vs what I forgot to do manually. For situations where sysadmins might want to allow different level of access control for different PMUs, we start creating per-PMU perf_event_paranoid controls in sysfs. These work in equivalent fashion as the existing perf_event_paranoid sysctl, which now becomes the parent control for each PMU. On PMU registration the global/parent value will be inherited by each PMU, as it will be propagated to all registered PMUs when the sysctl is updated. At any later point individual PMU access controls, located in /device//perf_event_paranoid, can be adjusted to achieve fine grained access control. Discussion from previous posting: https://lkml.org/lkml/2018/5/21/156 This is really not helpful. The cover letter and the change logs should contain a summary of that discussion and a proper justification of the proposed change. Just saying 'sysadmins might want to allow' is not useful at all, it's yet another 'I want a pony' thing. Okay, for the next round I will expand the cover letter with at least one concrete example on how it is usable and summarize the discussion a bit. I read through the previous thread and there was a clear request to involve security people into this. Especially those who are deeply involved with hardware side channels. I don't see anyone Cc'ed on the whole series. Who would you recommend I add? Because I really don't know.. For the record, I'm not buying the handwavy 'more noise' argument at all. It wants a proper analysis and we need to come up with criteria which PMUs can be exposed at all. All of this want's a proper documentation clearly explaining the risks and scope of these knobs per PMU. Just throwing magic knobs at sysadmins and then saying 'its their problem to figure it out' is not acceptable. Presumably you see adding fine grained control as diminishing the overall security rather than raising it? Could you explain why? Because incompetent sysadmin will turn it off for some PMU, while without having the fine-grained control they wouldn't turn it off globally? This feature was requested by the exact opposite concern, that in order to access the i915 PMU, one has to compromise the security of the entire system by allowing access to *all* PMU's. Making this ability fine-grained sounds like a logical solution for solving this weakening of security controls. Concrete example was that on video transcoding farms users want to monitor the utilization of GPU engines (like CPU cores) and they can do that via the i915 PMU. But for that to work today they have to dial down the global perf_event_paranoid setting. Obvious improvement was to allow them to only dial down the i915.perf_event_paranoid setting. As such, for this specific use case at least, the security is increased. Regards, Tvrtko
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Hi, On 28/09/2018 11:26, Thomas Gleixner wrote: Tvrtko, On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: It would be very helpful if you cc all involved people on the cover letter instead of just cc'ing your own pile of email addresses. CC'ed now. I accept it was by bad to miss adding Cc's on the cover letter, but my own email addresses hopefully should not bother you. It is simply a question of what I have in .gitconfig vs what I forgot to do manually. For situations where sysadmins might want to allow different level of access control for different PMUs, we start creating per-PMU perf_event_paranoid controls in sysfs. These work in equivalent fashion as the existing perf_event_paranoid sysctl, which now becomes the parent control for each PMU. On PMU registration the global/parent value will be inherited by each PMU, as it will be propagated to all registered PMUs when the sysctl is updated. At any later point individual PMU access controls, located in /device//perf_event_paranoid, can be adjusted to achieve fine grained access control. Discussion from previous posting: https://lkml.org/lkml/2018/5/21/156 This is really not helpful. The cover letter and the change logs should contain a summary of that discussion and a proper justification of the proposed change. Just saying 'sysadmins might want to allow' is not useful at all, it's yet another 'I want a pony' thing. Okay, for the next round I will expand the cover letter with at least one concrete example on how it is usable and summarize the discussion a bit. I read through the previous thread and there was a clear request to involve security people into this. Especially those who are deeply involved with hardware side channels. I don't see anyone Cc'ed on the whole series. Who would you recommend I add? Because I really don't know.. For the record, I'm not buying the handwavy 'more noise' argument at all. It wants a proper analysis and we need to come up with criteria which PMUs can be exposed at all. All of this want's a proper documentation clearly explaining the risks and scope of these knobs per PMU. Just throwing magic knobs at sysadmins and then saying 'its their problem to figure it out' is not acceptable. Presumably you see adding fine grained control as diminishing the overall security rather than raising it? Could you explain why? Because incompetent sysadmin will turn it off for some PMU, while without having the fine-grained control they wouldn't turn it off globally? This feature was requested by the exact opposite concern, that in order to access the i915 PMU, one has to compromise the security of the entire system by allowing access to *all* PMU's. Making this ability fine-grained sounds like a logical solution for solving this weakening of security controls. Concrete example was that on video transcoding farms users want to monitor the utilization of GPU engines (like CPU cores) and they can do that via the i915 PMU. But for that to work today they have to dial down the global perf_event_paranoid setting. Obvious improvement was to allow them to only dial down the i915.perf_event_paranoid setting. As such, for this specific use case at least, the security is increased. Regards, Tvrtko
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Tvrtko, On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: It would be very helpful if you cc all involved people on the cover letter instead of just cc'ing your own pile of email addresses. CC'ed now. > For situations where sysadmins might want to allow different level of > access control for different PMUs, we start creating per-PMU > perf_event_paranoid controls in sysfs. > > These work in equivalent fashion as the existing perf_event_paranoid > sysctl, which now becomes the parent control for each PMU. > > On PMU registration the global/parent value will be inherited by each PMU, > as it will be propagated to all registered PMUs when the sysctl is > updated. > > At any later point individual PMU access controls, located in > /device//perf_event_paranoid, can be adjusted to achieve > fine grained access control. > > Discussion from previous posting: > https://lkml.org/lkml/2018/5/21/156 This is really not helpful. The cover letter and the change logs should contain a summary of that discussion and a proper justification of the proposed change. Just saying 'sysadmins might want to allow' is not useful at all, it's yet another 'I want a pony' thing. I read through the previous thread and there was a clear request to involve security people into this. Especially those who are deeply involved with hardware side channels. I don't see anyone Cc'ed on the whole series. For the record, I'm not buying the handwavy 'more noise' argument at all. It wants a proper analysis and we need to come up with criteria which PMUs can be exposed at all. All of this want's a proper documentation clearly explaining the risks and scope of these knobs per PMU. Just throwing magic knobs at sysadmins and then saying 'its their problem to figure it out' is not acceptable. Thanks, tglx
Re: [RFC 0/5] perf: Per PMU access controls (paranoid setting)
Tvrtko, On Wed, 19 Sep 2018, Tvrtko Ursulin wrote: It would be very helpful if you cc all involved people on the cover letter instead of just cc'ing your own pile of email addresses. CC'ed now. > For situations where sysadmins might want to allow different level of > access control for different PMUs, we start creating per-PMU > perf_event_paranoid controls in sysfs. > > These work in equivalent fashion as the existing perf_event_paranoid > sysctl, which now becomes the parent control for each PMU. > > On PMU registration the global/parent value will be inherited by each PMU, > as it will be propagated to all registered PMUs when the sysctl is > updated. > > At any later point individual PMU access controls, located in > /device//perf_event_paranoid, can be adjusted to achieve > fine grained access control. > > Discussion from previous posting: > https://lkml.org/lkml/2018/5/21/156 This is really not helpful. The cover letter and the change logs should contain a summary of that discussion and a proper justification of the proposed change. Just saying 'sysadmins might want to allow' is not useful at all, it's yet another 'I want a pony' thing. I read through the previous thread and there was a clear request to involve security people into this. Especially those who are deeply involved with hardware side channels. I don't see anyone Cc'ed on the whole series. For the record, I'm not buying the handwavy 'more noise' argument at all. It wants a proper analysis and we need to come up with criteria which PMUs can be exposed at all. All of this want's a proper documentation clearly explaining the risks and scope of these knobs per PMU. Just throwing magic knobs at sysadmins and then saying 'its their problem to figure it out' is not acceptable. Thanks, tglx
