Skip to content

opencsd: etm4: reject TRACE_INFO with speculation depth above MAXSPEC - #93

Open
EdwinFairchild wants to merge 1 commit into
Linaro:masterfrom
EdwinFairchild:etm4-trace-info-spec-depth
Open

opencsd: etm4: reject TRACE_INFO with speculation depth above MAXSPEC#93
EdwinFairchild wants to merge 1 commit into
Linaro:masterfrom
EdwinFairchild:etm4-trace-info-spec-depth

Conversation

@EdwinFairchild

Copy link
Copy Markdown

What happens

TrcPktDecodeEtmV4I::doTraceInfoPacket() reads the current speculation depth from the TRACE_INFO packet and hands it to createUnseenUncommitedP0Elem(), which allocates one TrcStackElem per unit of depth. The value is never checked, so a corrupt or mis-synchronised TRACE_INFO goes straight into heap allocations.

Any trace with gaps or idle bytes will eventually resync on garbage and parse a TRACE_INFO that is not one. We hit it on an ETMv4 stream from a Cortex-M33, a core with no speculation at all (TRCIDR8.MAXSPEC = 0): one bad packet reported a depth of 44,025,981 and the decoder allocated 44 million elements, about 1.3 GB, for it. A 25 MB capture with a handful of these committed 58 GB before the machine ran out of memory. A malloc-tracing build put the whole amount under EtmV4P0Stack::createUnseenUncommitedP0Elem called from doTraceInfoPacket.

The change

A valid TRACE_INFO cannot report more outstanding speculative elements than the implementation supports, and the decoder already has that limit in m_max_spec_depth (TRCIDR8.MAXSPEC from the config).

  • doTraceInfoPacket() compares m_curr_spec_depth with m_max_spec_depth before creating anything. A larger value logs OCSD_ERR_BAD_DECODE_PKT with the packet index and the decoder goes back to WAIT_SYNC, the same treatment other bad packets get, instead of returning OCSD_RESP_FATAL_SYS_ERR.
  • doTraceInfoPacket() returns ocsd_err_t instead of bool, so the caller can tell a rejected packet (OCSD_ERR_BAD_DECODE_PKT) from a real allocation failure (OCSD_ERR_MEM), which stays fatal as before.

Testing

Built on Linux (gcc 13) and Windows (MinGW UCRT64) from v1.8.3 plus this change:

  • the 25 MB Cortex-M33 capture above now decodes in under a second with a peak of about 0.1 GB;
  • good traces are unchanged: a clean capture and a lossy one (about a third of the bytes missing) produce exactly the same decode output as unpatched 1.8.3.

I have no Cortex-A trace to hand, but a valid depth on a speculating core is within MAXSPEC by definition, so the check does not fire on good data.

doTraceInfoPacket() creates one unseen uncommitted P0 element for every
unit of the speculation depth carried by the TRACE_INFO packet, without
checking the value. A corrupt or mis-synchronised TRACE_INFO therefore
turns straight into heap allocations: on a Cortex-M33 stream
(TRCIDR8.MAXSPEC = 0) one such packet reported a depth of 44,025,981 and
the decoder allocated 44 million elements, about 1.3 GB, for a packet it
should not have accepted. A 25 MB capture containing a few of them
committed 58 GB.

A valid TRACE_INFO cannot report more outstanding speculative elements
than TRCIDR8.MAXSPEC, which the decoder already holds in
m_max_spec_depth. Check the depth against it before creating anything: a
larger value is logged as OCSD_ERR_BAD_DECODE_PKT at the packet index and
the decoder returns to WAIT_SYNC, as it does for other bad packets,
instead of failing the decode. doTraceInfoPacket() now returns ocsd_err_t
so the caller can tell a rejected packet from an allocation failure, which
stays fatal.

Signed-off-by: Eddie <eddie@edwinfairchild.com>
@mikel-armbb

Copy link
Copy Markdown
Contributor

This does seem a reasonable check - certainly there is no need to create a whole bunch of unnecessary objects for corrupt trace.

However - if you do have corrupt trace, then I have to doubt the value of continuing to decode. Even apparently decodable trace may also be corrupt - so the accuracy cannot be relied upon.

For the next release I shall certainly include the validation check - but reserve judgement for now on whether we should abort completely.

If you do want to test against A class trace - and indeed trace with a non-zero spec depth, then run the python regression script in the tests directory, Run the full suite once before your change, then run again after - the script can be told to compare the two test runs directly

If you are in a position to add a new test that would also be appreciated.

Thanks

Mike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants