linux-kernel - Re: [PATCH] perf/x86: Consider pinned events for group validation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 20 Aug 2019 13:13:36 -0400
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     mingo@...hat.com, acme@...nel.org, linux-kernel@...r.kernel.org,
        eranian@...gle.com, ak@...ux.intel.com
Subject: Re: [PATCH] perf/x86: Consider pinned events for group validation

>>>> +	/*
>>>> +	 * The new group must can be scheduled
>>>> +	 * together with current pinned events.
>>>> +	 * Otherwise, it will never get a chance
>>>> +	 * to be scheduled later.
>>>
>>> That's wrapped short; also I don't think it is sufficient; what if you
>>> happen to have a pinned event on CPU1 (and not others) and happen to run
>>> validation for a new CPU1 event on CPUn ?
>>>
>>
>> The patch doesn't support this case.
> 
> Which makes the whole thing even more random.

Maybe we can use the cpuc on event->cpu. That could help a little here.
cpuc = per_cpu_ptr(&cpu_hw_events, event->cpu >= 0 ? event->cpu : 
raw_smp_processor_id());

> 
>> It is mentioned in the description.
>> The patch doesn't intend to catch all possible cases that cannot be
>> scheduled. I think it's impossible to catch all cases.
>> We only want to improve the validate_group() a little bit to catch some
>> common cases, e.g. NMI watchdog interacting with group.
>>
>>> Also; per that same; it is broken, you're accessing the cpu-local cpuc
>>> without serialization.
>>
>> Do you mean accessing all cpuc serially?
>> We only check the cpuc on current CPU here. It doesn't intend to access
>> other cpuc.
> 
> There's nothing preventing the cpuc you're looking at changing while
> you're looking at it. Heck, afaict it is possible to UaF here. Nothing
> prevents the events you're looking at from going away and getting freed.

You are right.
I think we can add a lock to prevent the event_list[] in x86_pmu_add() 
and x86_pmu_del().


Thanks,
Kan