Report on failing tests with thread stack protection and their resolution.
Joel Sherrill
joel at rtems.org
Sat Jan 23 15:59:25 UTC 2021
On Sat, Jan 23, 2021, 12:28 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
>
> On Mon, Dec 7, 2020 at 8:00 AM Utkarsh Rai <utkarsh.rai60 at gmail.com>
> wrote:
>
>>
>>
>> On Thu, Dec 3, 2020 at 10:22 PM Gedare Bloom <gedare at rtems.org> wrote:
>>
>>>
>>>
>>> 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?
>>>
>>
>> While iterating over threads, if the visitor returns true the iteration
>> stops.
>>
>>
>>>
>>>
>>>> +}
>>>> +
>>>> +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')?
>>>
>>
>> No, it is not necessary. I will make the changes.
>>
>>
>>>
>>>
>>>> +#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
>>>
>>>
>
> Hello,
> Sorry for the long break but my university examinations had put me off
> track. A quick review of work done till now -
> The tests where inter-stack access takes place were identified (the test
> report is attached in this thread) and the concerning mechanisms were made
> thread-stack protection compatible. The current method of disabling memory
> protection for these
> regions uses _Thread_Iterate() which is bad in terms of performance, I
> have modified it by disabling the memory protection solely for
> 'the_thread'. The issue that remains is of User extension iterators, in
> particular of nested iterators. My idea is to disable memory protection in
> places during iteration where inter-stack access takes place. The problem
> is determining the region for which memory protection needs to be disabled.
> One way to get around this would be by disabling memory protection for all
> the stacks, in a blanket-based manner. The other possibility is to access
> the last blocked thread and disable memory protection just for the stack of
> this thread (as inter-stack access of the previously blocked thread takes
> place during iteration). How to get access to the queue of blocked threads?
> And is this method feasible?
>
Is it possible to identify the reasons that threads fail this check? I'm
suspicious there are a few places where disabling checks entirely for the
scope of an RTEMS method is needed. Running the stack usage report would
seem to be a case for that.
But other cases might be writing method results like a message buffer on a
stack would need to get access to a specific task just long enough to write
the result.
Random cases in applications can't be generally solved but RTEMS itself is
a finite set of code. Identifying the reasons RTEMS needs access and
handling those might eliminate most of these. Fix one case and multiple
tests may pass.
Also important to know when these cases occur in RTEMS.
Just a thought.
--joel
_______________________________________________
> 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/20210123/7ad9a579/attachment-0001.html>
More information about the devel
mailing list