Re: CVE Request: More perf security fixes

[Petr Matousek <pmatouse () redhat com>]

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 () redhat 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 () redhat 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 () 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
> > > > > >
> > > > > > 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.

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