RTEMS | libmisc/stackchk: Add stack smashing protection (!277)

[Gedare Bloom (@gedare)]

Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277 was reviewed by Gedare Bloom

--
  
Gedare Bloom started a new discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117280

 > + * 
 > + * @note: This function is invoked if a stack overflow is detected
 > + *        if GCC flag -fstack-protection is enabled.

Typo: `-fstack-protector`

--
  
Gedare Bloom started a new discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117281

 > + *        if GCC flag -fstack-protection is enabled.
 > + */
 > +void __stack_chk_fail(void);

Is there a way to check if the `-fstack-protector` has been enabled? It might be worth only defining this support if it's been actually used.

--
  
Gedare Bloom started a new discussion on cpukit/include/rtems/confdefs/extensions.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117282

 >  
 > +#ifndef CONFIGURE_STACK_CHECKER_GUARD_VALUE
 > +  #define CONFIGURE_STACK_CHECKER_GUARD_VALUE 0xDEADBEEF

Doesn't `-fstack-protector` create it's own canaries? Why do we need this?

--
  
Gedare Bloom started a new discussion on cpukit/libmisc/shell/main_stackuse.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117283

 >   *  On-Line Applications Research Corporation (OAR).
 > - *
 > + * 

Don't change random white space.

--
  
Gedare Bloom commented on a discussion on cpukit/libmisc/shell/main_stackuse.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117284

 >  #include <rtems/shell.h>
 >  #include <rtems/score/threadimpl.h>
 > +#include <rtems/score/basedefs.h>

These attributes aren't used in this file.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/stackchk05.scn: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117285

 > +*** TEST STACKCHK05 ***

This isn't the test output.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk06/config.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117286

 > +
 > +#define CONFIGURE_INIT
 > +#include "../stackchk05/system.h"

This isn't exactly great. It creates a dependency between tests. As I wrote above you probably need to change some of the shared code anyway.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk06/config.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117287

 > +
 > +/* configuration information */
 > +#define CONFIGURE_STACK_CHECKER_ENABLED

Should the `-fstack-protector` also work without this CONFIGURE enabled? If so, there should be a test for it.

With this CONFIGURE enabled, you have to check the `printk` output to be sure that `__stack_chk_fail` was called. This does not seem to be ideal. I think you should rewrite the fatal extension handler in this test to be a failure path for the test, and put the `TEST_END` directly here in `__stack_chk_fail`. You can just not provide a fatal extension handler in this test.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117288

 > +  strcpy(buffer_short, long_string);
 > +  
 > +  printk("Stack Overflow invoked\n");

I would prefer to have nothing printed for a successful test.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117289

 > +  invoke_stack_overflows();
 > +
 > +  rtems_task_exit();

Add a comment that this is a failure path for this test.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk06/config.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117290

 > +{
 > +  // Custom handling for stack overflow
 > +  printk("Custom stack overflow handler invoked\n");

If you modify the test to remove the fatal extension handler, then you don't need to print here.

--
  
Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/system.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117291

 > +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
 > +
 > +#define TASK_STACK_SIZE (RTEMS_MINIMUM_STACK_SIZE*3)

This test doesn't need all these extra configuration options. That said, I'm surprised this test works considering the additional stack size that is allocated. The stack checker should only detect a blown stack that goes beyond the task's stack size.


-- 
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277
You're receiving this email because of your account on gitlab.rtems.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20241224/66bb2281/attachment-0001.htm>