Home page logo
/

oss-sec logo oss-sec mailing list archives

Re: CVE Request: More perf security fixes
From: Stephane Eranian <eranian () google com>
Date: Thu, 6 Jun 2013 12:13:24 +0200

On Thu, Jun 6, 2013 at 12:03 PM, Petr Matousek <pmatouse () redhat 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 () 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.

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


  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]
AlienVault