• State Resolved
  • Locked Locked
  • Replies 10 replies
  • Answers 1 answer
  • Subscribers 84 subscribers
  • Views 339 views
  • Users 0 members are here
Support feedback
Options
Options
Related

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

TMS320F28379D: the generated cinit copy table wrong, causing buffer overflow.

Martin Otzen
Intellectual 912 points
Part Number: TMS320F28379D
Other Parts Discussed in Thread: SYSCONFIG, , C2000WARE

Tool/software:

I defined a variable

static uint16_t myvar = 0;

I needed this variable to be in the shared ram (RAM_GS)

So I added a user section in Sysconfig named  LOCAL_RAM_GS, giving this line in the CMD file

 LOCAL_RAM_GS {  }      >  RAM_GS

and added a pragma to the variable declaration, so it looks like this.

#pragma DATA_SECTION( myvar, "LOCAL_RAM_GS" )
static bq_size_t myvar = 0;

All fine and good until I started to get ITRAP exceptions.

I used the debugger to find that the first byte of a ramfunction in another module had been overwritten with a zero (ITRAP0).

My findings:

The linker allocates 1 byte for this variable, acording to these snippets from the map file

GLOBAL SYMBOLS: SORTED ALPHABETICALLY BY Name 

page  address   name                                                            
----  -------   ----                                                            
0     00011774  foo_readData                                   00091204
0     00011770  foo_sendData                                   00091200
0     00088c0d  bar_sendData   


zoo.obj

Run/Load            
Value     Binding   Name (Section)
--------  --------  ---------------
0009017d  global    Isr_IpcFlag0 (.text:retain)
0001176f  local     LOCAL_RAM_GS (LOCAL_RAM_GS)
00015980  local     other_var (.bss)
...
0001176f  local     myvar (LOCAL_RAM_GS)


LINKER GENERATED COPY TABLES

__TI_cinit_table @ 0009432c records: 6, size/record: 4, table size: 24
	.data: load addr=00093e10, load size=00000500 bytes, run addr=00015992, run size=00002d61 bytes, compression=lzss
	LOCAL_RAM_GS: load addr=00094316, load size=00000005 bytes, run addr=0001176f, run size=00000001 bytes, compression=copy
    ...
binit @ 00094348 records: 4, size/record: 6, table size: 26
    ...
	FOO_OBJ_TO_RAM: load addr=00091200, load size=0000010a, run addr=00011770, run size=0000010a, compression=none
	...

So `myvar` should be placed at 0x0001176f and have length 1

and `foo_sendData` should be placed at 0x00011770

The reason why `foo_sendData` is being overwritten I found in the actual copy table read from memory using the debugger.

looking at the load address i find

switching to 32 bit, as that is how `__TI_auto_init` reads it

first word is the idx for `TI_Handler_Table` and this idx points to the `__TI_decompress_none`

Second word is the count, passed to `memcpy` by `__TI_decompress_none`. And here I find a "2", and not a 1 as indicated in the map file.

This means that `__TI_decompress_none`->`memcpy` will write to address 0x0001176f and 0x00011770

0x00011770 being the first byte of the foo_sendData function.

I tried changing the config of the user section., but in Sysconfig I can only find an option to enable .align(2), and as I understand the linker manual i need to use .palign(2), to ensure enough memory is allocated for this case.

In a very short term solution, I see that if I set the .align(2) attribute it will place the `myvar` with a hole after it. But that is probably only the case because the chuck coming after `myvar`is also aligned. In other words, this is a very temporary solution.

Please advise.

Edit:

Almost forgot tool versions used:

Code Composer Studio Version: 12.8.1.00005

        device="TMS320F28379D"
        toolChain="TI"
        toolVersion="22.6.0.LTS"
        products="sysconfig:1.19.0;c2000ware_software_package:5.01.00.01"
        outputType="executable"
        outputFormat="ELF"
Edit2:
I have tested with latest compiler 22.6.2, same result.
  • 0 in reply to Martin Otzen
    TI__Intellectual 880 points

    Hi Martin,

    Apologies for the delay. This thread has been assigned to the subject expert. Please allow us some more time to check and get back.

    Thanks and Regards,

    Sudeshna

  • 0 in reply to Sudeshna Chaudhuri
    TI__Guru**** 251200 points

    Unfortunately, you have experienced a known issue EXT_EP-10875.  Visit that link for the details and a workaround.  You will see that the fix is not available in any current release of the C28x compiler.  A different workaround is described in this forum post.

    Thanks and regards,

    -George

  • 0 in reply to George Mock
    Intellectual 912 points

    Hi

    I have looked deeper into this. And neither of the workarounds will work for me.

    1. Using the  `--cinit_compression=rle,lzss` option has no effect the Linker still uses the copy/none handler for the small section. (btw. the linker do not accept `--cinit_compression=rle,lzss` as suggested, it is either `--cinit_compression=lzss` or `--cinit_compression=rle`)

    2. From the forum post "Initializing the affected variable at main". That's a quite naive suggestion, as it would mean that for every change to any part of the source I would have to check what the linker has now placed after my small sector.... Besides In the case that triggered my investigation, it was a ram function that was overwritten.

  • 0 in reply to Martin Otzen
    TI__Guru**** 251200 points

    I apologize.  For your situation, the workaround mentioned in this forum post needs a few changes.

    Everywhere you currently apply #pragma DATA_SECTION, change it to use the noinit attribute instead.  For example:

    __attribute__((noinit))
static bq_size_t myvar;

    The part about initializing all these variables in _system_post_cinit is similar.  If all of these variables are in the same source file, then you can continue to keep them static, as long as you also implement _system_post_cinit in the same file.  If these variables are both static and in different files, then you have to change them to global.

    In your linker command file change ...

    Martin Otzen said:
     LOCAL_RAM_GS {  }      >  RAM_GS

    to ...

        .TI.noinit > RAM_GS

    Every variable with the noinit attribute is in an input section named .TI.noinit.  This code creates an output section named .TI.noinit.  It is made up of all the input sections named .TI.noinit.  It is allocated to the memory range RAM_GS.

    Martin Otzen said:
    it would mean that for every change to any part of the source I would have to check what the linker has now placed after my small sector

    After implementing this workaround, checks like that are not needed.

    Thanks and regards,

    -George

  • 0 in reply to Martin Otzen
    TI__Guru**** 251200 points

    Even though it solves your immediate problem, I am not comfortable with how you use palign(2).  It causes the output section LOCAL_RAM_GS to be bigger, and thus the broken initialization overwrites memory that is never used.  But it still occurs.  The workaround I describe prevents the broken initialization from occurring at all.  

    Thanks and regards,

    -George

  • +1 in reply to George Mock
    Intellectual 912 points

    I found a better solution.

    From the assembler/linker manual

    8.5.5.2.6 Alignment With Padding
    As with align, you can tell the linker to place an output section at an address that falls on an n-byte boundary,
    where n is a power of 2, by using the palign keyword. In addition, palign ensures that the size of the section is a
    multiple of its placement alignment restrictions, padding the section size up to such a boundary, as needed.

    Using this I can force the linker to minimum allocate 2 bytes of memory, so it will match the broken copytable.

    Only issue to that solution is that this option not supported by SysConfig that we use for the generation of the CMD file.

    Sysconfig can only be setup to generate the align option.

    My workaround for that (that becomes the workaround to make a workaround :-)... )

    Is to just add an .cmd file to the project, as CCS automatically pics that up and add to the generated one from SysConfig.

    In that .cmd file I can manually define the LOCAL:GS_RAM setion.

    SECTIONS
{
    LOCAL_RAM_GS {  }    >  RAM_GS, PALIGN(2)
}

    Recommendation

    If this solution holds up, I strongly recommend that you update the SysConfig tool to support the paling option. And configure it to warn if it is missing, as it already warns if flash is not setup to minimum align of 8.

    I also strongly recommend, that a warning or minimum a remark is added if the CMD file contains a small section that is not paligned to min. 2.

    And the doc is updated about this.

    All the above assuming that the fix for the linker itself is still far away.

    Some observations.

    I noticed that when I used the --fill option to the linker, the extra write that the copy operation does, contains this fill value. This indicates that it is not just a random zero that was written, but intentionally the fill value. This makes me think that the part of the linker that generates the actual copy table see the one byte section as being padded just as when i use paling(2). I wonder if there is a mismatch inside the linker created by the switch from charSize=8 to charSize=16.

    align(2) could potentially be used as a workaround too, if and only if, it is ensured that all sections has an alignment of minimum 2.

  • 0 in reply to George Mock
    Intellectual 912 points
    George Mock said:
      It causes the output section LOCAL_RAM_GS to be bigger, and thus the broken initialization overwrites memory that is never used.

    That is more or less the definition of what padding is supposed to do.

    From the same manual

    If the linker adds padding to an initialized output section then the padding space is also initialized. By default,
    padding space is filled with a value of 0 (zero). However, if a fill value is specified for the output section then any
    padding for the section is also filled with that fill value.

    As noted in my observations, the extra write done by the copy operation, is using the given fill value. So the copy operation is infact padding.

    And the padding in it self is not the problem, actually there are several places where it is padding automatically. If I look in the latest map file i find this snippet  "--HOLE-- [fill =" 6 times, that means that the linker padded a "hole" in the memory-map 6 times.

    The real issue was that the part of the linker that creates the memory-map, was not considering padding for the LOCAL_RAM_GS section, and thus the copy table and the memory-map did not match, and we have the risk of a buffer overflow.

    So by giving this option I do not change what the copy operation does, or how the copy table is generated. But it does ensure that both ends of the linker agrees on this, and makes the linker create a memory-map that matches the copy table.

    George Mock said:

    The part about initializing all these variables in _system_post_cinit is similar.  If all of these variables are in the same source file, then you can continue to keep them static, as long as you also implement _system_post_cinit in the same file.  If these variables are both static and in different files, then you have to change them to global.

    We make testable, reusable and readable software modules, so we don't use global variables....

    If I just wanted to quick fix this single case, and leave the next developer to unknowingly hit this problem again later. I could just have removed the explicit  initialization of my static variable.

    This code makes the variable be initialized by copy in cinit.

    #pragma DATA_SECTION( myvar, "LOCAL_RAM_GS" )
static bq_size_t myvar = 0;

    This code makes the variable be initialized by zero_init in cinit, and the zero_init do not suffer from this bug.

    #pragma DATA_SECTION( myvar, "LOCAL_RAM_GS" )
static bq_size_t myvar;

    Even though the `= 0` initializer for a static variable is redundant in C, I often put it there to help readability of the code.

    As a sidenote. I am a little surprised that the compiler/linker do not recognize the first code example as a zero init.

  • 0 in reply to Martin Otzen
    Intellectual 912 points

    Did expect some reply to this, either telling me why I am wrong or confirming this solution.

  • 0 in reply to Martin Otzen
    TI__Guru**** 251200 points
    Martin Otzen said:
    telling me why I am wrong or confirming this solution.

    Sorry, but I can do neither.  I have already explained why I think my suggested workaround is better.  You disagree, and want to use a different workaround.  I agree your workaround is good enough for your specific situation.  And I think that is a reasonable place to leave this thread.

    Thanks and regards,

    -George