Out of bound Read & Write found in TraceFmtDcdImpl::unpackFrame()
Closed this issue · 2 comments
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_datais 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;
--
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.
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