linux-kernel - Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken

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: <27912fc6-8419-4828-82a7-dacde5b4a759@linaro.org>
Date: Fri, 9 Aug 2024 16:19:54 +0100
From: James Clark <james.clark@...aro.org>
To: Mike Leach <mike.leach@...aro.org>
Cc: Leo Yan <leo.yan@....com>,
 Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>,
 scclevenger@...amperecomputing.com, acme@...hat.com,
 coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, darren@...amperecomputing.com,
 james.clark@....com, suzuki.poulose@....com, Al.Grant@....com
Subject: Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm
 if address continuity is broken



On 09/08/2024 3:13 pm, Mike Leach wrote:
> Hi James
> 
> On Thu, 8 Aug 2024 at 10:32, James Clark <james.clark@...aro.org> wrote:
>>
>>
>>
>> On 07/08/2024 5:48 pm, Leo Yan wrote:
>>> Hi all,
>>>
>>> On 8/7/2024 3:53 PM, James Clark wrote:
>>>
>>> A minor suggestion: if the discussion is too long, please delete the
>>> irrelevant message ;)
>>>
>>> [...]
>>>
>>>>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>>> @@ -257,6 +257,11 @@ def process_event(param_dict):
>>>>>                    print("Stop address 0x%x is out of range [ 0x%x .. 0x%x
>>>>> ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso))
>>>>>                    return
>>>>>
>>>>> +       if (stop_addr < start_addr):
>>>>> +               if (options.verbose == True):
>>>>> +                       print("Packet Dropped, Discontinuity detected
>>>>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr,
>>>>> dso))
>>>>> +               return
>>>>> +
>>>>
>>>> I suppose my only concern with this is that it hides real errors and
>>>> Perf shouldn't be outputting samples that go backwards. Considering that
>>>> fixing this in OpenCSD and Perf has a much wider benefit I think that
>>>> should be the ultimate goal. I'm putting this on my todo list for now
>>>> (including Steve's merging idea).
>>>
>>> In the perf's util/cs-etm.c file, it handles DISCONTINUITY with:
>>>
>>>      case CS_ETM_DISCONTINUITY:
>>>           /*
>>>            * The trace is discontinuous, if the previous packet is
>>>            * instruction packet, set flag PERF_IP_FLAG_TRACE_END
>>>            * for previous packet.
>>>            */
>>>           if (prev_packet->sample_type == CS_ETM_RANGE)
>>>                   prev_packet->flags |= PERF_IP_FLAG_BRANCH |
>>>                                         PERF_IP_FLAG_TRACE_END;
>>>
>>> I am wandering if OpenCSD has passed the correct info so Perf decoder can
>>> detect the discontinuity. If yes, then the flag 'PERF_IP_FLAG_TRACE_END' will
>>> be set (it is a general flag in branch sample), then we can consider use it in
>>> the python script to handle discontinuous data.
>>
>> No OpenCSD isn't passing the correct info here. Higher up in the thread
>> I suggested an OpenCSD patch that makes it detect the error earlier and
>> fixes the issue. It also needs to output a discontinuity when the
>> address goes backwards. So two fixes and then the script works without
>> modifications.
>>
> 
> Which address is going backwards here? - OpenCSD generates trace
> ranges only by walking forwards from the last known address till it
> hits a branch. Unless this wraps round 0x000000 this will never result
> in a backwards address as far as I can see.
> Do you have an example dump with OpenCSD outputting a range packet
> with backwards addresses?
> 
> Mike
> 
The example I have I think is something like this:

1.  Start address / trace on
2.  E
3.  Output range
     ...
4.  Periodic address update
     ...
5.  E
6.  Output range

If decode has gone wrong (but undetectably) between steps 1 and 3. Then 
the next steps still output a second range based on the last periodic 
address received. (I think it might not necessarily have to be a 
periodic address but could also be indirect address packet?). Perf 
converts the ranges into branch samples by taking the end of the first 
range and beginning of the second range. Then the disassembly script 
converts those samples into ranges again by taking the source and 
destination of the last two branch samples.

The original issue that Ganapat saw was that the periodic address causes 
OpenCSD to put the source address of the second range somewhere before 
the first one, even though it didn't output a branch or discontinuity 
that would explain how it got there.

But yes you're right the ranges themselves always go forwards from the 
point of view of their own start and end addresses.

I thought it might be possible for OpenCSD to check against the last 
range output? Although I wasn't sure if maybe it's actually valid to do 
a backwards jump like that without the trace on/off packets with address 
filtering or something?

The root cause is still the incorrect image, but I think this check 
along with the other direct branch check should make it pretty difficult 
for people to make the mistake.