Out of bound Read found in TraceFmtDcdImpl::extractFrame()

[yabinc]

It's reported by kj2648@gmail.com in Android. Forward it to upstream:

Issue Description

Out of bound Read found in libopencsd_decoder due to bound check fail in TraceFmtDcdImpl::extractFrame() in trc_frame_deformatter.cpp.

It may cause security vulnerability by reading invalid memory address in memcpy.

external/OpenCSD/decoder/source/trc_frame_deformatter.cpp

(line 491) This is a conditional statement that checks the end of input data.

(line 496) It is always assumed that there is a complete frame if the condition is not satisfied, but sometimes it is not.

(line 500) OOB Read occurs in memcpy() when (m_in_block_processed + f_sync_bytes) + OCSD_DFRMTR_FRAME_SIZE > m_in_block_size.

459 bool TraceFmtDcdImpl::extractFrame()
460 {
...
491         if((m_in_block_processed+f_sync_bytes) == m_in_block_size)
492         {
493             m_ex_frm_n_bytes = 0;
494             cont_process = false;   // end of input data.
495         }
496                 else
497                 {
498                         // always a complete frame.
499                         m_ex_frm_n_bytes = OCSD_DFRMTR_FRAME_SIZE;
500                         memcpy(m_ex_frm_data, m_in_block_base + m_in_block_processed + f_sync_bytes, m_ex_frm_n_bytes);  <- OOB Read!
501                         m_trc_curr_idx_sof = m_trc_curr_idx + f_sync_bytes;
502                         ex_bytes = OCSD_DFRMTR_FRAME_SIZE;
503         }
504     }

...

592     // update the processed count for the buffer
593     m_in_block_processed += total_processed;  <- m_in_block_processed is added up to the end of input data.

Suggest fix:

0001-Fix-Out-of-Bounds-in-TraceFmtDcdImpl-extractFrame.patch

---
 decoder/source/trc_frame_deformatter.cpp | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/decoder/source/trc_frame_deformatter.cpp b/decoder/source/trc_frame_deformatter.cpp
index dc12e3f..9f6811e 100644
--- a/decoder/source/trc_frame_deformatter.cpp
+++ b/decoder/source/trc_frame_deformatter.cpp
@@ -488,18 +488,18 @@ bool TraceFmtDcdImpl::extractFrame()
             }
         }

-        if((m_in_block_processed+f_sync_bytes) == m_in_block_size)
+        if((m_in_block_processed + f_sync_bytes) + OCSD_DFRMTR_FRAME_SIZE > m_in_block_size)
         {
             m_ex_frm_n_bytes = 0;
             cont_process = false;   // end of input data.
         }
-               else
-               {
-                       // always a complete frame.
-                       m_ex_frm_n_bytes = OCSD_DFRMTR_FRAME_SIZE;
-                       memcpy(m_ex_frm_data, m_in_block_base + m_in_block_processed + f_sync_bytes, m_ex_frm_n_bytes);
-                       m_trc_curr_idx_sof = m_trc_curr_idx + f_sync_bytes;
-                       ex_bytes = OCSD_DFRMTR_FRAME_SIZE;
+                   else
+                   {
+            // always a complete frame.
+            m_ex_frm_n_bytes = OCSD_DFRMTR_FRAME_SIZE;
+            memcpy(m_ex_frm_data, m_in_block_base + m_in_block_processed + f_sync_bytes, m_ex_frm_n_bytes);
+            m_trc_curr_idx_sof = m_trc_curr_idx + f_sync_bytes;
+            ex_bytes = OCSD_DFRMTR_FRAME_SIZE;
         }
     }
     else
--

[mikel-armbb]

Code in TraceFmtDcdImpl::processTraceData() ensures that data passed into the block guarded by

// memory aligned sources are always multiples of frames, aligned to start. if( m_cfgFlags & OCSD_DFRMTR_FRAME_MEM_ALIGN) {

which includes line 491 etc ensures that the incoming data block is always full frame i.e. 16 byte aligned. This is based on the input flags which initialise the decoder in the library. So the complete frame predicate remains true and the code is safe.

if you believe you have a sequence / data input file that breaks this while correctly setting decoder initialisation flags then please provide it here. Otherwise I do not beleive this change is necessary.

[yabinc]

How much do we trust that the ETM data are in good format, considering all the buffer overflows and data overwritten that can happen along the path? In my experience of using OpenCSD in Android, it's not uncommon for OpenCSD to report a decoding error and we have to continue with the rest data. The code is in https://cs.android.com/android/platform/superproject/+/master:system/extras/simpleperf/ETMDecoder.cpp;l=737;bpv=1 So I feel it's not a bad idea for OpenCSD to be preventive about bad ETM data.

To reproduce it, kj2648@gmail.com provided below code, which can detect the problem with ASAN. However, I don't know if it's from real ETM data. I will ask them.

include "common/ocsd_dcd_tree.h"

include

int main() { const uint8_t buf[] = { 0xff, 0xff, 0xff, 0x7f, 0x30, 0xff, 0x53, 0x54, 0x4d, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0, 0x36, 0xff, 0xb1, 0xff, 0x36, 0x36, 0x36, 0x36, 0x36, 0x2b, 0x36, 0x36, 0x3a, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0, 0, 0x2c, 0, 0, 0, 0x32, 0x1, 0, }; size_t buf_size = sizeof(buf) / sizeof(buf[0]);

auto pCfg = std::make_unique(); auto pDecoder = std::unique_ptr( DecodeTree::CreateDecodeTree(OCSD_TRC_SRC_FRAME_FORMATTED, 52)); assert(pDecoder->createDecoder("ETE", OCSD_CREATE_FLG_FULL_DECODER, &*pCfg)); assert(pDecoder->createMemAccMapper() == OCSD_OK);

uint32_t out; pDecoder->TraceDataIn(OCSD_OP_DATA, 0, buf_size, buf, &out); return 0; }

[mikel-armbb]

The overflows / ETM data is irrelevant at this stage of decode. The TraceFmtDcd object takes in data in CoreSight 16 byte frame format, generated by the formatters in ETR, ETF or TPIU, and de-multiplexes it into the correct Trace ID streams. No packet or full decode happens at this stage. TPIU can add in FSYNCs into the stream, & on some systems using continuous mode trace from TPIU then HSYNCs can occur (M class primarily).

The formatters will always produce 16 byte frames. TPIU output - which must be captured by an external capture device will as mentioned sometimes pad / add in sync points to the frames. ETR / ETF never uses these sync elements as the memory they used is always 16 byte aligned, which is why we set up the decoder for memory aligned formatted data for these sources,

[mikel-armbb]

The actual problem occurs in checkForResetFSyncPatterns(), which should reject any sequences of FSYNC that is not a multiple of 4 FSYNCS. It does not, thereby mis-aligning the frame data - in the routine that assumes an integer multiple of16 byte aligned frames - i.e. no partial frames.

FSYNCs never appear in frames generated by ETF / ETR and traced into system memory, so this pattern is used by perf as a marker within the perf.data trace buffer as end of a particular capture.

The data block you supply above is invalid data for the decoder - and it should cause it to throw an error after the first four bytes (a single FSYNC). This was not happening.

The library has been fixed to correctly reject illegal FSYNC patterns.

Will be released in version 1.3.3

[yabinc]

Thanks for the fix!