[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 6 Jun 2013 12:13:24 +0200
From: Stephane Eranian <eranian@...gle.com>
To: Stephane Eranian <eranian@...gle.com>, Peter Zijlstra <a.p.zijlstra@...llo.nl>,
"ak@...ux.intel.com" <ak@...ux.intel.com>, security@...nel.org,
Marcus Meissner <meissner@...e.de>, OSS Security List <oss-security@...ts.openwall.com>
Subject: Re: CVE Request: More perf security fixes
On Thu, Jun 6, 2013 at 12:03 PM, Petr Matousek <pmatouse@...hat.com> wrote:
> On Thu, Jun 06, 2013 at 10:16:39AM +0200, Stephane Eranian wrote:
>> On Wed, Jun 5, 2013 at 9:23 PM, Petr Matousek <pmatouse@...hat.com> wrote:
>> > On Wed, Jun 05, 2013 at 03:53:52PM +0200, Stephane Eranian wrote:
>> >> On Wed, Jun 5, 2013 at 3:35 PM, Petr Matousek <pmatouse@...hat.com> wrote:
>> >> > On Wed, Jun 05, 2013 at 03:02:53PM +0200, Peter Zijlstra wrote:
>> >> >> On Wed, Jun 05, 2013 at 02:38:56PM +0200, Petr Matousek wrote:
>> >> >> > On Wed, Jun 05, 2013 at 02:15:59PM +0200, Peter Zijlstra wrote:
>> >> >> > > On Wed, Jun 05, 2013 at 02:10:54PM +0200, Petr Matousek wrote:
>> >> >> > > > Hello, Peter.
>> >> >> > > >
>> >> >> > > > On Tue, Jun 04, 2013 at 05:53:16PM +0200, Marcus Meissner wrote:
>> >> >> > > > > 1. Info leak (?) via PERF_SAMPLE_BRANCH_KERNEL
>> >> >> > > > >
>> >> >> > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7cc23cd6c0c7d7f4bee057607e7ce01568925717
>> >> >> > > > >
>> >> >> > > > > commit 7cc23cd6c0c7d7f4bee057607e7ce01568925717
>> >> >> > > > > Author: Peter Zijlstra <a.p.zijlstra@...llo.nl>
>> >> >> > > > > Date: Fri May 3 14:11:25 2013 +0200
>> >> >> > > > >
>> >> >> > > > > perf/x86/intel/lbr: Demand proper privileges for PERF_SAMPLE_BRANCH_KERNEL
>> >> >> > > > >
>> >> >> > > > > We should always have proper privileges when requesting kernel
>> >> >> > > > > data.
>> >> >> > > > >
>> >> >> > > > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
>> >> >> > > > > Cc: <stable@...nel.org>
>> >> >> > > > > Cc: Andi Kleen <ak@...ux.intel.com>
>> >> >> > > > > Cc: eranian@...gle.com
>> >> >> > > > > Link: http://lkml.kernel.org/r/20130503121256.230745028@chello.nl
>> >> >> > > > > [ Fix build error reported by fengguang.wu@...el.com, propagate error code back. ]
>> >> >> > > > > Signed-off-by: Ingo Molnar <mingo@...nel.org>
>> >> >> > > > > Link: http://lkml.kernel.org/n/tip-v0x9ky3ahzr6nm3c6ilwrili@git.kernel.org
>> >> >> > > >
>> >> >> > > > There is similar check in perf_copy_attr() which is called from
>> >> >> > > > perf_event_open syscall --
>> >> >> > > >
>> >> >> > > > /* kernel level capture: check permissions */
>> >> >> > > > if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
>> >> >> > > > && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> >> >> > > > return -EACCES;
>> >> >> > > >
>> >> >> > > > It seems to me that it covers PERF_SAMPLE_BRANCH_KERNEL as well. Am I
>> >> >> > > > missing something?
>> >> >> > > >
>> >> >> > >
>> >> >> > > I overlooked it, also its slightly broken. See the discussion at:
>> >> >> > > https://lkml.org/lkml/2013/5/21/166
>> >> >> >
>> >> >> > Got it, thanks for the pointer. So it is safe to say there never was a
>> >> >> > leak in this case (and thus no security issue worth CVE)?
>> >> >>
>> >> >> There was a leak, notice how Stephane's patch did a
>> >> >> s/PERF_SAMPLE_BRANCH_PERM_PLM/PERF_SAMPLE_BRANCH_KERNEL/
>> >> >
>> >> > PERF_SAMPLE_BRANCH_PERM_PLM is a superset of PERF_SAMPLE_BRANCH_KERNEL:
>> >> >
>> >> > #define PERF_SAMPLE_BRANCH_PERM_PLM \
>> >> > (PERF_SAMPLE_BRANCH_KERNEL |\
>> >> > PERF_SAMPLE_BRANCH_HV)
>> >> >
>> >> >
>> >> >> but also places
>> >> >> the check _after_ we propagate the event PLM levels in the case none
>> >> >> were LBR specific.
>> >> >
>> >> > Assuming the leak does occur only when PERF_SAMPLE_BRANCH_KERNEL is set,
>> >> > that does not matter:
>> >> >
>> >> > /* kernel level capture: check permissions */
>> >> > if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
>> >> > && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> >> > return -EACCES;
>> >> >
>> >> > ^^^ this assures proper permission check if PERF_SAMPLE_BRANCH_KERNEL
>> >> > is explicitly set
>> >> >
>> >> >
>> >> > /* propagate priv level, when not set for branch */
>> >> > if (!(mask & PERF_SAMPLE_BRANCH_PLM_ALL)) {
>> >> >
>> >> > /* exclude_kernel checked on syscall entry */
>> >> > if (!attr->exclude_kernel)
>> >> > mask |= PERF_SAMPLE_BRANCH_KERNEL;
>> >> >
>> >> > And following check in perf_event_open syscall assures the permission
>> >> > are right for (!(mask & PERF_SAMPLE_BRANCH_PLM_ALL)) code:
>> >> >
>> >> > if (!attr.exclude_kernel) {
>> >> > if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> >> > return -EACCES;
>> >> > }
>> >> >
>> >> Yes, your analysis is correct. If the branch has not explicit priv
>> >> level mask, then
>> >> it is inherited from the event branches are requested from.
>> >
>> > I am sorry to re-iterate the question, but does that mean that even
>> > before your and Peter's changes, it was not possible to set
>> > PERF_SAMPLE_BRANCH_KERNEL without passing "perf_paranoid_kernel() &&
>> > !capable(CAP_SYS_ADMIN" check either in perf_copy_attr or
>> > perf_event_open (attr.exclude_kernel check)?
>> >
>> > Did your patch change anything at all or it was just refactoring?
>> >
>> Before:
>> /* kernel level capture: check permissions */
>> if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
>> && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> return -EACCES;
>>
>> 1. If you were coming in with explicit branch priv level set
>> to BRANCH_KERNEL, it would check against paranoid() + cap()
>> 2. If you were coming in with explicit branch priv level set
>> to BRANCH_HV, it would check against paranoid() + cap()
>> 3. If you were coming in with explicit branch priv level set
>> to BRANCH_USER, nothing would happen
>>
>> That's all because PERM_PLM = KERNEL | HV
>> So I think it was okay.
>>
>> In the new, code:
>> /* kernel level capture: check permissions */
>> if ((mask & PERF_SAMPLE_BRANCH_KERNEL)
>> && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> return -EACCES;
>>
>> We only check for BRANCH_KERNEL, and not BRANCH_HV.
>> I think we need to fix that, my bad. So we need to use
>> PERF_SAMPLE_BRANCH_PERM_PLM again here.
>> I will send a patch ASAP. I got confused about the macro
>> name, sorry. Thanks for insisting.
>
> Actually I think your latest patch (PERF_SAMPLE_BRANCH_KERNEL ->
> PERF_SAMPLE_BRANCH_PERM_PLM) on top of the permission check move after
> the privilege level propagation in case no explicit privilege level is
> specified improved things.
>
Yes, exactly. This was not a a cleanup patch. It also fixed the
problem with BRANCH_HV
not being checked against paranoid setting.
> Before, PERF_SAMPLE_BRANCH_HV could be set without the privilege check
> by the privilege level propagation code in perf_copy_attr(), because
> there is no explicit check for !attr.exclude_hv similar to
>
> if (!attr.exclude_kernel) {
> if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> return -EACCES;
> }
>
> in perf_event_open. This was not the case of PERF_SAMPLE_BRANCH_KERNEL,
> because !attr.exclude_kernel permission check was always there.
>
> Thanks Stephane!
>
> --
> Petr Matousek / Red Hat Security Response Team
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
