Report on failing tests with thread stack protection and their resolution.
Gedare Bloom
gedare at rtems.org
Thu Dec 3 16:52:45 UTC 2020
On Wed, Dec 2, 2020 at 5:53 PM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
> Hello,
> As discussed in this
> <https://lists.rtems.org/pipermail/devel/2020-November/063341.html> thread,
> I have compiled a list of the tests that deal with inter stack
> communication and fail with the thread stack protection option. Most of
> these tests pass when, as Sebastian suggested and had provided a
> wonderful example, I disable memory protection at places where contents of
> different thread stacks are accessed by the current thread. There are a few
> tests that still fail due to inter-stack access in the application code
> itself.
>
> The changes I have made are -
>
> diff --git a/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> b/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> index c176d4b8c5..a45b175395 100644
> --- a/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> +++ b/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> @@ -1,15 +1,49 @@
> #include <bsp/arm-cp15-start.h>
> #include <rtems/score/memoryprotection.h>
> +#include <rtems/score/threadimpl.h>
> #include <libcpu/arm-cp15.h>
>
> +bool set_memory_flags(Thread_Control* thread, void* arg)
> +{
> + uintptr_t begin;
> + uintptr_t end;
> + uint32_t flags;
> + rtems_interrupt_level irq_level;
> + Thread_Control *executing;
> +
> + executing = _Thread_Executing;
> +
> + if(thread != executing) {
>
This is not concurrency-safe. By time the check happens, or following code
happens, the thread could become executing.
> +
> + flags = *( uint32_t *)( arg );
> + begin = thread->Start.Initial_stack.area;
> + end = begin + thread->Start.Initial_stack.size;
> +
> + rtems_interrupt_disable(irq_level);
> + arm_cp15_set_translation_table_entries(begin, end, flags);
> + rtems_interrupt_enable(irq_level);
> + }
> +
> + return false;
>
why -- what does the return value mean?
> +}
> +
> +rtems_status_code _Memory_protection_Enable( void )
> +{
> + uint32_t access_flags;
> +
> + access_flags = translate_flags( RTEMS_NO_ACCESS );
> +
> + _Thread_Iterate( set_memory_flags, &access_flags );
> +
> + return RTEMS_SUCCESSFUL; // check the return values for iterating
> function and current method.
> +}
> +
> +rtems_status_code _Memory_protection_Disable( void )
> +{
> + uint32_t access_flags;
> +
> + access_flags = translate_flags( RTEMS_READ_WRITE );
> +
> + _Thread_Iterate( set_memory_flags, &access_flags );
> +
> + return RTEMS_SUCCESSFUL;
> }
> \ No newline at end of file
> diff --git a/cpukit/include/rtems/score/coremsgimpl.h
> b/cpukit/include/rtems/score/coremsgimpl.h
> index e598dce96a..3719a3d3c8 100644
> --- a/cpukit/include/rtems/score/coremsgimpl.h
> +++ b/cpukit/include/rtems/score/coremsgimpl.h
> @@ -27,6 +27,10 @@
> #include <rtems/score/threaddispatch.h>
> #include <rtems/score/threadqimpl.h>
>
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + #include <rtems/score/memoryprotection.h>
> +#endif
> +
> #include <limits.h>
> #include <string.h>
>
> @@ -586,7 +590,9 @@ RTEMS_INLINE_ROUTINE Thread_Control
> *_CORE_message_queue_Dequeue_receiver(
> if ( the_thread == NULL ) {
> return NULL;
> }
> -
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + _Memory_protection_Disable();
>
I wonder if it is necessary to disable all protection, or can you just
disable the protection applied to 'the_thread' (or maybe to 'executing')?
> +#endif
> *(size_t *) the_thread->Wait.return_argument = size;
> the_thread->Wait.count = (uint32_t) submit_type;
>
> @@ -595,6 +601,9 @@ RTEMS_INLINE_ROUTINE Thread_Control
> *_CORE_message_queue_Dequeue_receiver(
> the_thread->Wait.return_argument_second.mutable_object,
> size
> );
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + _Memory_protection_Enable();
> +#endif
>
> _Thread_queue_Extract_critical(
> &the_message_queue->Wait_queue.Queue,
>
> diff --git a/cpukit/posix/src/psignalunblockthread.c
> b/cpukit/posix/src/psignalunblockthread.c
> index 80a0f33a09..e0f8468de6 100644
> --- a/cpukit/posix/src/psignalunblockthread.c
> +++ b/cpukit/posix/src/psignalunblockthread.c
> @@ -24,6 +24,9 @@
> #include <signal.h>
>
> #include <rtems/score/isr.h>
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> +#include <rtems/score/memoryprotection.h>
> +#endif
> #include <rtems/score/threadimpl.h>
> #include <rtems/score/threadqimpl.h>
> #include <rtems/score/watchdogimpl.h>
> @@ -205,6 +208,10 @@ bool _POSIX_signals_Unblock_thread(
>
> the_info = (siginfo_t *) the_thread->Wait.return_argument;
>
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> +_Memory_protection_Disable();
> +#endif
> +
> if ( !info ) {
> the_info->si_signo = signo;
> the_info->si_code = SI_USER;
> @@ -212,6 +219,9 @@ bool _POSIX_signals_Unblock_thread(
> } else {
> *the_info = *info;
> }
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> +_Memory_protection_Enable();
> +#endif
>
> _Thread_queue_Extract_with_proxy( the_thread );
> return _POSIX_signals_Unblock_thread_done( the_thread, api, true );
> diff --git a/cpukit/rtems/src/eventsurrender.c
> b/cpukit/rtems/src/eventsurrender.c
> index 49f77d2663..5de62ec292 100644
> --- a/cpukit/rtems/src/eventsurrender.c
> +++ b/cpukit/rtems/src/eventsurrender.c
> @@ -23,6 +23,10 @@
> #include <rtems/score/threadimpl.h>
> #include <rtems/score/watchdogimpl.h>
>
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + #include <rtems/score/memoryprotection.h>
> +#endif
> +
> static void _Event_Satisfy(
> Thread_Control *the_thread,
> Event_Control *event,
> @@ -31,7 +35,13 @@ static void _Event_Satisfy(
> )
> {
> event->pending_events = _Event_sets_Clear( pending_events,
> seized_events );
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + _Memory_protection_Disable();
> +#endif
> *(rtems_event_set *) the_thread->Wait.return_argument = seized_events;
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + _Memory_protection_Enable();
> +#endif
> }
>
> static bool _Event_Is_blocking_on_event(
> diff --git a/cpukit/rtems/src/regionprocessqueue.c
> b/cpukit/rtems/src/regionprocessqueue.c
> index 4adaf66674..29b078a38c 100644
> --- a/cpukit/rtems/src/regionprocessqueue.c
> +++ b/cpukit/rtems/src/regionprocessqueue.c
> @@ -22,6 +22,10 @@
> #include <rtems/score/status.h>
> #include <rtems/score/threadqimpl.h>
>
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + #include <rtems/score/memoryprotection.h>
> +#endif
> +
> void _Region_Process_queue(
> Region_Control *the_region
> )
> @@ -63,8 +67,13 @@ void _Region_Process_queue(
>
> if ( the_segment == NULL )
> break;
> -
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + _Memory_protection_Disable();
> +#endif
> *(void **)the_thread->Wait.return_argument = the_segment;
> +#if defined RTEMS_THREAD_STACK_PROTECTION
> + _Memory_protection_Enable();
> +#endif
> _Thread_queue_Extract( the_thread );
> the_thread->Wait.return_code = STATUS_SUCCESSFUL;
> }
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20201203/6eb57f90/attachment.html>
More information about the devel
mailing list