Re: CVE Request: More perf security fixes

[Petr Matousek <pmatouse () redhat com>]

On Wed, Jun 05, 2013 at 02:30:13PM +0200, Stephane Eranian wrote:
> Hi,
>
>
> On Wed, Jun 5, 2013 at 2:15 PM, Peter Zijlstra <peterz () infradead org> 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 () chello 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 () chello nl>
> > > >     Cc: <stable () kernel org>
> > > >     Cc: Andi Kleen <ak () linux intel com>
> > > >     Cc: eranian () google com
> > > >     Link: http://lkml.kernel.org/r/20130503121256.230745028 () chello nl
> > > >     [ Fix build error reported by fengguang.wu () intel com, propagate error code back. ]
> > > >     Signed-off-by: Ingo Molnar <mingo () kernel 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
> Yes, there was a typo in the constant. Was checking the wrong bits.

Before we were checking PERF_SAMPLE_BRANCH_PERM_PLM bits.
PERF_SAMPLE_BRANCH_PERM_PLM is defined as 

#define PERF_SAMPLE_BRANCH_PERM_PLM \
        (PERF_SAMPLE_BRANCH_KERNEL |\
         PERF_SAMPLE_BRANCH_HV)

Can you please explain why that was considered as wrong bits? Shouldn't
we also check for PERF_SAMPLE_BRANCH_HV now? I admit that it has now
more sense that before and it will actually work after you reorganized
and moved the check the way you did, but ain't we leaking some useful
privileged info by not checking for PERF_SAMPLE_BRANCH_PERM_PLM but only
for PERF_SAMPLE_BRANCH_KERNEL?

-- 
Petr Matousek / Red Hat Security Response Team