Out of bound Read & Write found in TraceFmtDcdImpl::unpackFrame()

[yabinc]

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

Issue Description

Out of bound found in libopencsd_decoder due to absense of bound check in TraceFmtDcdImpl::unpackFrame() in trc_frame_deformatter.cpp.

It may cause security vulnerability by reading and writing invalid memory address.

external/OpenCSD/decoder/source/trc_frame_deformatter_impl.h

• m_out_data is a member variable of class TraceFmtDcdImpl, which is a 7-length array type.

 46 typedef struct _out_chan_data {
 47     ocsd_trc_index_t index;      //!< trace source index for start of these bytes
 48     uint8_t id;             //!< Id for these bytes
 49     uint8_t data[15];       //!< frame data bytes for this ID
 50     uint32_t valid;         //!< Valid data bytes.
 51     uint32_t used;          //!< Data bytes output (used by attached processor).
 52 } out_chan_data;

...

158     out_chan_data m_out_data[7];

external/OpenCSD/decoder/source/trc_frame_deformatter.cpp

(line 613) The local variable m_out_data_idx is initialized to 0.

(line 623) According to the loop condition, this loop is executed 7 times.

(line 646) So m_out_data_idx can be up to 7, since the m_out_data_idx++; command can be executed 7 times in this loop.

If the value of m_out_data_idx is 7, then all m_out_data[m_out_data_idx] accesses are out of bound.

604 bool TraceFmtDcdImpl::unpackFrame()
605 {
606     // unpack cannot fail as never called on incomplete frame.
607     uint8_t frameFlagBit = 0x1;
608     uint8_t newSrcID = OCSD_BAD_CS_SRC_ID;
609     bool PrevIDandIDChange = false;
610     uint64_t noneDataBytes = 0;
611
612     // init output processing
613     m_out_data_idx = 0;
614     m_out_processed = 0;
615
616     // set up first out data packet...
617     m_out_data[m_out_data_idx].id = m_curr_src_ID;
618     m_out_data[m_out_data_idx].valid = 0;
619     m_out_data[m_out_data_idx].index =  m_trc_curr_idx_sof;
620     m_out_data[m_out_data_idx].used = 0;
621
622     // work on byte pairs - bytes 0 - 13.
623     for(int i = 0; i < 14; i+=2)
624     {
625         PrevIDandIDChange = false;
626
627         // it's an ID + data
628         if(m_ex_frm_data[i] & 0x1)
629         {
630             newSrcID = (m_ex_frm_data[i] >> 1) & 0x7f;
631             if(newSrcID != m_curr_src_ID)   // ID change
632             {
633                 PrevIDandIDChange = ((frameFlagBit & m_ex_frm_data[15]) != 0);
634
635                 // following byte for old id?
636                 if(PrevIDandIDChange)
637                     // 2nd byte always data
638                     m_out_data[m_out_data_idx].data[m_out_data[m_out_data_idx].valid++] = m_ex_frm_data[i+1];
639
640                 // change ID
641                 m_curr_src_ID = newSrcID;
642
643                 // if we already have data in this buffer
644                 if(m_out_data[m_out_data_idx].valid > 0)
645                 {
646                     m_out_data_idx++; // move to next buffer
647                     m_out_data[m_out_data_idx].valid = 0;  <- OOB Write!
648                     m_out_data[m_out_data_idx].used = 0;  <- OOB Write!
649                     m_out_data[m_out_data_idx].index = m_trc_curr_idx_sof + i;  <- OOB Write!
650                 }
651
652                 // set new ID on buffer
653                 m_out_data[m_out_data_idx].id = m_curr_src_ID;  <- OOB Write!
654
655                 /// TBD - ID indexing in here.
656             }
657             noneDataBytes++;
658         }
659         else
660         // it's just data
661         {
662             m_out_data[m_out_data_idx].data[m_out_data[m_out_data_idx].valid++] = m_ex_frm_data[i] | ((frameFlagBit & m_ex_frm_data[15]) ? 0x1 : 0x0);
663         }
664
665         // 2nd byte always data
666         if(!PrevIDandIDChange) // output only if we didn't for an ID change + prev ID.
667             m_out_data[m_out_data_idx].data[m_out_data[m_out_data_idx].valid++] = m_ex_frm_data[i+1];  <- OOB Read & Write!
668
669         frameFlagBit <<= 1;
670     }
671
672     // unpack byte 14;
673
674     // it's an ID
675     if(m_ex_frm_data[14] & 0x1)
676     {
677         // no matter if change or not, no associated data in byte 15 anyway so just set.
678         m_curr_src_ID = (m_ex_frm_data[14] >> 1) & 0x7f;
679         noneDataBytes++;
680     }
681     // it's data
682     else
683     {
684         m_out_data[m_out_data_idx].data[m_out_data[m_out_data_idx].valid++] = m_ex_frm_data[14] | ((frameFlagBit & m_ex_frm_data[15]) ? 0x1 : 0x0);  <- OOB Read & Write!
685     }
686     m_ex_frm_n_bytes = 0;   // mark frame as empty;
687
688     noneDataBytes++;    // byte 15 is always non-data.
689     addToFrameStats(noneDataBytes); // update the non data byte stats.
690     return true;
691 }

suggest fix:

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

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

diff --git a/decoder/source/trc_frame_deformatter.cpp b/decoder/source/trc_frame_deformatter.cpp
index dc12e3f..fc7a76c 100644
--- a/decoder/source/trc_frame_deformatter.cpp
+++ b/decoder/source/trc_frame_deformatter.cpp
@@ -643,6 +643,10 @@ bool TraceFmtDcdImpl::unpackFrame()
                 // if we already have data in this buffer
                 if(m_out_data[m_out_data_idx].valid > 0)
                 {
+                    if (m_out_data_idx == 6)
+                    {
+                        return false;
+                    }
                     m_out_data_idx++; // move to next buffer
                     m_out_data[m_out_data_idx].valid = 0;
                     m_out_data[m_out_data_idx].used = 0;
--

[mikel-armbb]

Not sure it is actually possible to build a system with trace hardware that could generate an input sequence with 7 ID changes in a single frame, but in the event the correct response is to not increment the index larger than 6 and skip the last initialisation.

Will look at this in more detail in due course.

[mikel-armbb]

There is a case if 7 ID changes occur in a frame, each with following data assigned to the previous ID, the final ID/Data byte being data, where 8 buffers are required.

Unlikely to be possible in real hardware but fix applied to ensure robustness of the library

Releases in version 1.3.3