Linaro / OpenCSD

CoreSight trace stream decoder developed openly
https://github.com/Linaro/opencsd/wiki
Other
143 stars 54 forks source link

Invalid index check while processing Waypoint Update packet for PTM #48

Closed vimalraj-rajasekharan closed 2 years ago

vimalraj-rajasekharan commented 2 years ago

As per the PFT protocol, a Waypoint update packet can have up to 5 bytes carrying the waypoint address, but in the source code, the index value is limited to 4 bytes, in function TrcPktProcPtm::pktWPointUpdate().

mikel-armbb commented 2 years ago

Source code in TrcPktProcPtm::pktWPointUpdate() handles 4 bytes then the 5th in:

   else
            {
                // 5th address byte - determine ISA from this.

Please provide a more specific example of why you think this code does not work

vimalraj-rajasekharan commented 2 years ago

I tried decoding a trace raw file from Cortex A9 (PFTv1.0) and the trace was not properly decoded. I debugged into the issue and found that Waypoint packets were not properly decoded. It looks like the address bytes of the WPUpdate packets are indexed from 1 to 5, and not 0 to 4. But the index checks done here are based on 0-4 range.

In function TrcPktProcPtm::pktWPointUpdate(),

        if(readByte(currByte))
        {
            byteIdx = m_currPacketData.size() - 1;  // Address byte index starts at 1
            if(!m_gotAddrBytes)
            {
                if(byteIdx < 4)                     // This check skips last address byte
                {
                    // address bytes  1 - 4;
                    // ISA stays the same
                    if((currByte & 0x80) == 0x00)
                    {

After making below change, decoding worked fine:

--- a/decoder/source/ptm/trc_pkt_proc_ptm.cpp
+++ b/decoder/source/ptm/trc_pkt_proc_ptm.cpp
@@ -571,7 +571,7 @@ void TrcPktProcPtm::pktWPointUpdate()
             byteIdx = m_currPacketData.size() - 1;
             if(!m_gotAddrBytes)
             {
-                if(byteIdx < 4)
+                if(byteIdx <= 4)
                 {
                     // address bytes  1 - 4;
                     // ISA stays the same

Could you please have a look at this?

mikel-armbb commented 2 years ago

Fixed in 1.3.3