linux-kernel - Re: [PATCHv2 2/4] coresight: tmc-etf: Fix NULL ptr dereference in tmc_enable_etf_sink

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <060def4c-ced9-74c5-8e73-d8aedfdbc107@arm.com>
Date:   Fri, 23 Oct 2020 11:49:29 +0100
From:   Suzuki Poulose <suzuki.poulose@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        Mike Leach <mike.leach@...aro.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>, coresight@...ts.linaro.org,
        Stephen Boyd <swboyd@...omium.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCHv2 2/4] coresight: tmc-etf: Fix NULL ptr dereference in
 tmc_enable_etf_sink_perf()

On 10/23/20 10:23 AM, Peter Zijlstra wrote:
> On Fri, Oct 23, 2020 at 09:49:53AM +0100, Suzuki Poulose wrote:
>> On 10/23/20 8:39 AM, Peter Zijlstra wrote:
> 
>>> So then I don't understand the !->owner issue, that only happens when
>>> the task dies, which cannot be concurrent with event creation. Are you
>>
>> Part of the patch from Sai, fixes this by avoiding the dereferencing
>> after event creation (by caching it). But the kernel events needs
>> fixing.
>>
>> One follow up question on the !->owner issue. Given the ->owner is
>> dying, does it prevent events from being scheduled ? Or is there a delay
>> between that and eventually stopping the events. In this case, we hit
>> the issue when :
>>
>> A					  A or B ?
>>
>> event_start()
>>    ...					event->owner = NULL
>>
>>   READ_ONCE(event->owner);
>>
>> Is this expected ?
> 
> Yeah, teardown is a bit of an effort. Also, you can pass an fd over a
> unix socket to another process, so this isn't something you can rely on
> in any case.
> 
> The perf tool doesn't do it, but the kernel infra should be able to deal
> with someone doing a perf-deamon of sorts, where you can request a perf
> event and recieve a fd from it.
> 
> Imagine the fun ;-)
> 
>>> As for the kernel events.. why do you care about the actual task_struct
>>> * in there? I see you're using it to grab the task-pid, but how is that
>>> useful?
>>
>> Correct, kernel events are something that the driver didn't account for.
>> May be we could handle this case with a "special pid" and simply
>> disallow sharing (which is fine I believe, given there are not grouping
>> for the kernel created events).
> 
> Why do you need a pid in the first place? Can't you use the "task_struct
> *" as a value?

We could. But, without a refcount on the task pointer, that could be
tricky, even though we don't dereference it. In the same situation,
if the tsk owner dies and is freed and is reallocated to a new perf 
session task but with different PID, we could be mixing things up again
?

Special pid here could be -1.