Stack overflow check improvements

Report here problems in any of ChibiOS components. This forum is NOT for support.
dismirlian
Posts: 65
Joined: Fri Dec 20, 2013 3:59 pm
Location: Buenos Aires, Argentina
Has thanked: 1 time
Been thanked: 15 times

Stack overflow check improvements

Postby dismirlian » Tue Apr 20, 2021 4:07 pm

Hi all,

The ARM-v7m code for stack overflow detection does the following:

Code: Select all

    #define port_switch(ntp, otp) do {                                      \
      struct port_intctx *r13 = (struct port_intctx *)__get_PSP();          \
      if ((stkalign_t *)(r13 - 1) < (otp)->wabase) {                        \
        chSysHalt("stack overflow");                                        \
      }                                                                     \
      __port_switch(ntp, otp);                                              \
    } while (0)


It can be seen that, when a thread is switched out, the switch code check the current value of the PSP, and compares it with the base of the thread's stack. If it's lower, then a stack overflow occurred.

The problem with this method is that we can't detect if the stack was corrupted and then the PSP returned to the correct reserved area before the context switch. Maybe a better approach would be to add a guard (single word) to the stack, and check its value. As a proof of concept, I did:

Code: Select all

    #define port_switch(ntp, otp) do {                                      \
      if ((*(const uint8_t *)((otp)->wabase) != CH_DBG_STACK_FILL_VALUE) {  \
        chSysHalt("stack overflow");                                        \
      }                                                                     \
      __port_switch(ntp, otp);                                              \
    } while (0)


With this modification in place ChibiOS detected a stack corruption that was previously unnoticed.

Thanks,
Diego.

User avatar
Giovanni
Site Admin
Posts: 13591
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 878 times
Been thanked: 752 times
Contact:

Re: Stack overflow check improvements

Postby Giovanni » Tue Apr 20, 2021 5:15 pm

Hi,

It is a good idea, perhaps combine both because a stack could skip the guard word without changing it.

Note that there is also an hard check using the MPU on the armv7-m port.

Giovanni

dismirlian
Posts: 65
Joined: Fri Dec 20, 2013 3:59 pm
Location: Buenos Aires, Argentina
Has thanked: 1 time
Been thanked: 15 times

Re: Stack overflow check improvements

Postby dismirlian » Tue Apr 20, 2021 5:42 pm

Hi Giovanni, combining both is a good idea.

Yes, I've turned on PORT_ENABLE_GUARD_PAGES. It didn't work at first, but that was because I didn't turn on the MPU manually (mpuEnable(MPU_CTRL_PRIVDEFENA_Msk)).

Some thoughts:
- Would it be possible for ChibiOS to turn on the MPU automatically if PORT_ENABLE_GUARD_PAGES is TRUE?
- For MCUs that have a MPU, would it be sensible to default to PORT_ENABLE_GUARD_PAGES = TRUE when CH_DBG_ENABLE_STACK_CHECK is TRUE? I know the unhandled exception would be less friendly for developers, but the runtime cost for stack checking would be zero.

Thanks,
Diego.

User avatar
Giovanni
Site Admin
Posts: 13591
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 878 times
Been thanked: 752 times
Contact:

Re: Stack overflow check improvements

Postby Giovanni » Tue Apr 20, 2021 6:46 pm

dismirlian wrote:Yes, I've turned on PORT_ENABLE_GUARD_PAGES. It didn't work at first, but that was because I didn't turn on the MPU manually (mpuEnable(MPU_CTRL_PRIVDEFENA_Msk)).


This is a regression in trunk and 20.x, it is supposed to enable it, it was OK in 19.x.

I am moving this topic in "bug reports" so it is not forgotten...

Giovanni

dismirlian
Posts: 65
Joined: Fri Dec 20, 2013 3:59 pm
Location: Buenos Aires, Argentina
Has thanked: 1 time
Been thanked: 15 times

Re: Stack overflow check improvements

Postby dismirlian » Wed Apr 21, 2021 12:42 am

Great, thanks Giovanni.

steved
Posts: 772
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 10 times
Been thanked: 122 times

Re: Stack overflow check improvements

Postby steved » Wed Apr 21, 2021 4:13 pm

Giovanni wrote:This is a regression in trunk and 20.x, it is supposed to enable it, it was OK in 19.x.
Giovanni

Possible "tidy-up" while you're there - the F7 and H7 ports define and use MPU_CTRL_PRIVDEFENA, which has the same value as MPU_CTRL_PRIVDEFENA_Msk (defined in CMSIS files); although the first is type 'UL' and the second is type 'L'.

dismirlian
Posts: 65
Joined: Fri Dec 20, 2013 3:59 pm
Location: Buenos Aires, Argentina
Has thanked: 1 time
Been thanked: 15 times

Re: Stack overflow check improvements

Postby dismirlian » Mon May 03, 2021 11:59 pm

As a minor related problem, the ChibiOS eclipse plugin incorrectly reports free stack if PORT_ENABLE_GUARD_PAGES is enabled (it counts the guard bytes as "available").

User avatar
Giovanni
Site Admin
Posts: 13591
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 878 times
Been thanked: 752 times
Contact:

Re: Stack overflow check improvements

Postby Giovanni » Tue May 04, 2021 6:02 am

dismirlian wrote:As a minor related problem, the ChibiOS eclipse plugin incorrectly reports free stack if PORT_ENABLE_GUARD_PAGES is enabled (it counts the guard bytes as "available").


Yes, currently there is no way, for the debugger plugin, to know that the MPU is in use. The plugin is not CPU-architecture-aware.

Giovanni

dismirlian
Posts: 65
Joined: Fri Dec 20, 2013 3:59 pm
Location: Buenos Aires, Argentina
Has thanked: 1 time
Been thanked: 15 times

Re: Stack overflow check improvements

Postby dismirlian » Tue May 04, 2021 11:47 am

Yes, but what if wabase is set to tdp->wbase + PORT_GUARD_PAGE_SIZE?

Would this have any unwanted side-effect?

User avatar
Giovanni
Site Admin
Posts: 13591
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 878 times
Been thanked: 752 times
Contact:

Re: Stack overflow check improvements

Postby Giovanni » Tue May 04, 2021 12:52 pm

dismirlian wrote:Yes, but what if wabase is set to tdp->wbase + PORT_GUARD_PAGE_SIZE?

Would this have any unwanted side-effect?


Yes, several actually, dynamic treads for example.

Giovanni


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 4 guests